Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(network): modernize simplenetworkdetector with updated android apis #736

Conversation

orioonyx
Copy link
Contributor

@orioonyx orioonyx commented Jan 9, 2025

Related issue

This pull request resolves issue #735.

Summary of changes

This PR updates the SimpleNetworkDetector class to replace deprecated Android APIs with their modern counterparts while ensuring backward compatibility:

  1. For API 29 and above:
    • Replaced getActiveNetworkInfo() with getActiveNetwork() and getNetworkCapabilities().
    • Used NetworkCapabilities.TRANSPORT_* constants to determine network type.
  2. For API 28 and below:
    • Retained the existing logic using getActiveNetworkInfo() and TYPE_* constants to support older devices.

Additional improvements include:

  • Added null checks to handle cases where modern APIs might return null.
  • Simplified and aligned the code for better readability.

Why these changes are important

  • Aligning with modern Android APIs ensures compatibility with Android 10 and newer versions.
  • Maintaining backward compatibility supports older Android versions, benefiting a broader range of users.
  • Improving code readability and structure enhances maintainability for future contributors.

Acknowledgment

Thank you for providing this opportunity to contribute to the project. I have done my best to follow the contribution guidelines and ensure the changes adhere to project standards.

If there are any further improvements or adjustments required, please let me know. I will be more than happy to address any feedback promptly.

Thank you for your time and consideration!

@orioonyx orioonyx requested a review from a team as a code owner January 9, 2025 12:42
Copy link

linux-foundation-easycla bot commented Jan 9, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

// Determine network type based on transport capabilities
if (capabilities.hasTransport(NetworkCapabilities.TRANSPORT_CELLULAR)) {
return CurrentNetwork.builder(NetworkState.TRANSPORT_CELLULAR)
.subType("") // Additional details can be added
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not possible to do activeNetwork.getSubtypeName() like the older APIs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the question!

Unfortunately, getSubtypeName() is not available in the newer APIs like NetworkCapabilities. The newer APIs focus on transport-level details (e.g., cellular, WiFi) rather than providing specific subtype information like "LTE" or "HSPA."

Thank you for your review and feedback! 😊

Copy link
Member

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left a question otherwise LGTM

Comment on lines 60 to 61
NetworkInfo activeNetwork =
connectivityManager.getActiveNetworkInfo(); // Deprecated in API 29
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that this exists means that we will still get a build-time warning, right?

I think that if you factor out the body of the above if(api_29) block into a method, you could then also factor out the lower half of this method into a new method that we could suppress the warnings for. Just an idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for your valuable feedback and suggestions! 😊

I've addressed your comments by:

  • Adding @RequiresApi(api = Build.VERSION_CODES.Q) to the detectUsingModernApi method to align with the API requirements for modern implementations.
  • Improving code readability by refactoring the buildCurrentNetwork calls to adhere to the project's formatting standards.
  • Running spotlessApply to ensure the code meets the required formatting rules.

Please let me know if there's anything else you'd like me to adjust or further refine. I'm more than happy to make additional changes based on your feedback.

Thank you again for taking the time to review my PR and for providing such helpful guidance!

Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution. I think this is a step forward, but I made one suggestion about refactoring.

I think once this lands, we can convert it to kotlin and leverage pattern matching to make it even nicer. 👍🏻

Copy link
Contributor

@LikeTheSalad LikeTheSalad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@LikeTheSalad LikeTheSalad merged commit 887f352 into open-telemetry:main Jan 13, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modernize SimpleNetworkDetector with updated Android network APIs
4 participants