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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jan 13, 2025
+58 −18
Merged

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

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5a6156d
fix: Add API 29+ support while keeping backward compatibility with mi…
orioonyx Jan 8, 2025
24f194c
Merge branch 'main' of https://github.com/orioonyx/opentelemetry-andr…
orioonyx Jan 9, 2025
6179531
refactor: Simplify and align code in SimpleNetworkDetector
orioonyx Jan 9, 2025
9fd2c4a
Merge branch 'main' of https://github.com/orioonyx/opentelemetry-andr…
orioonyx Jan 11, 2025
2474fff
refactor: improve network detection for modern and legacy APIs
orioonyx Jan 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 38 additions & 4 deletions ...va/io/opentelemetry/android/internal/services/network/detector/SimpleNetworkDetector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
import static io.opentelemetry.android.internal.services.network.CurrentNetworkProvider.UNKNOWN_NETWORK;

import android.net.ConnectivityManager;
import android.net.Network;
import android.net.NetworkCapabilities;
import android.net.NetworkInfo;
import io.opentelemetry.android.common.internal.features.networkattributes.data.CurrentNetwork;
import io.opentelemetry.android.common.internal.features.networkattributes.data.NetworkState;
Expand All @@ -25,17 +27,49 @@ class SimpleNetworkDetector implements NetworkDetector {

@Override
public CurrentNetwork detectCurrentNetwork() {
// For API 29 and above, use modern APIs
if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.Q) {
Network network = connectivityManager.getActiveNetwork();
if (network == null) {
return NO_NETWORK;
}

NetworkCapabilities capabilities = connectivityManager.getNetworkCapabilities(network);
if (capabilities == null) {
return UNKNOWN_NETWORK;
}

// 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! 😊

.build();
} else if (capabilities.hasTransport(NetworkCapabilities.TRANSPORT_WIFI)) {
return CurrentNetwork.builder(NetworkState.TRANSPORT_WIFI).subType("").build();
} else if (capabilities.hasTransport(NetworkCapabilities.TRANSPORT_VPN)) {
return CurrentNetwork.builder(NetworkState.TRANSPORT_VPN).subType("").build();
} else if (capabilities.hasTransport(NetworkCapabilities.TRANSPORT_ETHERNET)) {
return CurrentNetwork.builder(NetworkState.TRANSPORT_WIRED).subType("").build();
}

// Default to UNKNOWN_NETWORK for other types
return UNKNOWN_NETWORK;
}

// For API 28 and below, use deprecated methods
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!

if (activeNetwork == null) {
return NO_NETWORK;
}
switch (activeNetwork.getType()) {
case ConnectivityManager.TYPE_MOBILE: // Deprecated in API 28

// Determine network type using TYPE_* constants
switch (activeNetwork.getType()) { // Deprecated in API 28
case ConnectivityManager.TYPE_MOBILE:
return CurrentNetwork.builder(NetworkState.TRANSPORT_CELLULAR)
.subType(activeNetwork.getSubtypeName())
.build();
case ConnectivityManager.TYPE_WIFI: // Deprecated in API 28
case ConnectivityManager.TYPE_WIFI:
return CurrentNetwork.builder(NetworkState.TRANSPORT_WIFI)
.subType(activeNetwork.getSubtypeName())
.build();
Expand All @@ -48,7 +82,7 @@ public CurrentNetwork detectCurrentNetwork() {
.subType(activeNetwork.getSubtypeName())
.build();
}
// there is an active network, but it doesn't fall into the neat buckets above
// Return UNKNOWN_NETWORK if type does not match predefined cases
return UNKNOWN_NETWORK;
}
}
Loading