Conversation
Refine device.connection_type from cellular to cellular_4g or cellular_5g when the network technology is known. From Android 12 the generation comes from the display info listener, which needs no permission. On older versions it is read from TelephonyManager only when the hosting app already holds READ_PHONE_STATE or READ_BASIC_PHONE_STATE, which the SDK does not declare itself. Fixes #3886
Contributor
|
Contributor
Author
|
@cursor review |
📲 Install BuildsAndroid
|
The display info listener was started before the network callback was registered. Unregistering is skipped while there is no network callback, so a failed registration left the listener running for the rest of the process, including in the background. Start it only after the network callback registered, and guard the check-then-act in register and unregister with a lock so concurrent callers cannot register a second listener that is never stored.
Contributor
Author
|
@cursor review |
alwx
marked this pull request as ready for review
September 23, 2026 10:51
alwx
requested review from
0xadam-brown,
adinauer,
markushi,
romtsn and
runningcode
as code owners
September 23, 2026 10:51
device.connection_type is deprecated in favour of network.connection.type, and the conventions already have network.connection.effective_type for the generation, so the technology is no longer folded into the connection type. Device gains connection_effective_type with 2g, 3g, 4g or 5g, mirroring how connection_type aliases network.connection.type.
Contributor
Author
|
@cursor review |
Reading the connection type and the network technology separately let a connectivity change land between them, so an event could report a wifi connection with a cellular technology. Both now come from one accessor that derives them from a single cache read.
Contributor
Author
|
@cursor review |
The NSA mmWave override network type fell through to the network type, which is LTE for NSA, so a 5G mmWave connection reported 4g. Unregistering also dropped the callback reference before it was unregistered, orphaning a still-registered callback when the telephony manager was gone.
Contributor
Author
|
@cursor review |
Monitoring stops when the app goes to the background, and from API 31 on the display info listener is the only source, so clearing the cached generation left every background event without a technology while the connection type still resolved.
Contributor
Author
|
@cursor review |
The SDK executor rejects work once it is shut down, and its queue check doesn't cover that, so a display info change arriving after Sentry.close threw RejectedExecutionException on a thread owned by the telephony framework.
Contributor
Author
|
@cursor review |
The rejecting executor is shared through the options, and teardown closes the provider through it, so leaving it rejecting skipped clearing the static connectivity manager for later tests.
Contributor
Author
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 5a32881. Configure here.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📜 Description
Following the network attribute conventions, where
device.connection_typeis deprecated in favour ofnetwork.connection.typeand backfilled from it, the connection type keeps its documented values and the cellular generation gets its own attribute:connection_typenetwork.connection.typewifi,ethernet,cellular(unchanged)connection_effective_typenetwork.connection.effective_type2g,3g,4g,5gDevicegainsconnection_effective_type, which is a public API addition (see the regeneratedsentry.api). Relay has no typednetworkcontext yet, so the value rides in the device context under the analogous leaf name - worth confirming that placement.The technology comes from two sources, in
CellularNetworkTechnologyProvider:TelephonyCallback.DisplayInfoListener, which requires nopermission. The listener is registered and unregistered alongside the existing network callback
in
AndroidConnectionStatusProvider, so nothing extra stays alive in the background, and readingthe value never blocks on telephony.
TelephonyDisplayInfo.getOverrideNetworkType()takesprecedence over
getNetworkType(), so a device showing 5G to the user is also reported as 5G.TelephonyManager.getDataNetworkType()(API 24+), called only when thehosting app already holds
READ_PHONE_STATEorREAD_BASIC_PHONE_STATE. Perthis comment the
SDK does not declare those permissions itself; apps without them simply get no technology.
Two things worth a second opinion:
network.connection.effective_typewith bare4g/5g, not folded into the connection type.NetworkBreadcrumbsIntegrationuses the staticgetConnectionType(NetworkCapabilities), which is unchanged. It has no access to the telephonystate, and its
isSimilarde-duplication compares the type, so a technology flapping between 4Gand 5G would produce breadcrumb noise. Happy to extend it in a follow-up if wanted.
On exception handling: the new code catches
SecurityException,IllegalStateExceptionandUnsupportedOperationExceptionexplicitly rather thanThrowable. Those are the documentedfailures for telephony registration on devices without telephony hardware or for processes that are
not allowed to listen.
💡 Motivation and Context
Original request: getsentry/team-mobile#150,
Linear project SDK-2033.
💚 How did you test it?
New
CellularNetworkTechnologyProviderTestcovers theNETWORK_TYPE_*to generation mapping, theoverride-type precedence, the permission gate below API 31, the API 24 floor for
getDataNetworkType(), registering and unregistering the display info listener, and theno-telephony and
SecurityExceptionpaths.AndroidConnectionStatusProviderTestgained cases forthe refined
cellular_5gvalue, for falling back to plaincellularwithout permission, and forleaving
wifiuntouched.Verified with the full
:sentry-android-core:testReleaseUnitTestsuite,:sentry-android-core:lintRelease(which caught the missing API 24 guard on
getDataNetworkType()), and./gradlew spotlessApply apiDump— the latter produced no
.apichanges, since the new class is@ApiStatus.Internal.📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
device.connection_typeis not documented in thedevice context docs, so the
granular values should be added there. Hybrid SDKs pick the value up from the Android device context
without changes.