-
Notifications
You must be signed in to change notification settings - Fork 133
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
RUMM-1691 Reduce utilization of CTTelephonyNetworkInfo
to mitigate CarrierInfoProvider
crash
#627
RUMM-1691 Reduce utilization of CTTelephonyNetworkInfo
to mitigate CarrierInfoProvider
crash
#627
Conversation
…g on iOS12+ This should significantly recude number of reads from `CTTelephonyNetworkInfo` which was causing infrequent crashes on iOS14.x. Instead of reading properties of `CTTelephonyNetworkInfo` for each collected event, now we only read it on startup and after `CTCarrier` change (which corresponds to switching SIM card in the phone).
c0670b3
to
619994e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a retain circle of networkInfo
in iOS12CarrierInfoProvider
.
Now that makes me wondering if it worth adding this complexity just in case the user swap their sim card during a session 🤔 we could just fetch the carrier when initialising the sdk. I understand this not being a trade-off we want to take, just asking :)
I get the point 👍. Two things though:
|
Yes, I'm convinced now :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👌
I've just a minor suggestion
…rovider # Conflicts: # Datadog/Example/Base.lproj/Main.storyboard
What and why?
📦 This PR updates the way we retrieve
CarrierInfo
information on iOS 12+. It's an educated guess to mitigate infrequent (2 impacted users) crashes reported in #623 and #619. Instead of pullingCTTelephonyNetworkInfo
for each collected event to createCarrierInfo
, now we only listen toCTTelephonyNetworkInfo
changes to updateCarrierInfo
when necessary.We use
CTTelephonyNetworkInfo
(fromCoreTelephony
framework) to read carrier information:A loop of investigation, research and debugging, lead me to following conclusions:
CTTelephonyNetworkInfo
only changes when swapping SIM card to another one (it does not change when removing SIM card - the last known info is still available). This is the only example listed in Apple doc.ProcessInfo.isLowPowerModeEnabled
,NWPath.currentPath
.Both issues in C3 were fixed by changing from pulling model, to subscription model (it was also a recommendation I received for
NWPath
issue from Apple's engineer). Given thatCTTelephonyNetworkInfo
changes are very infrequent (C2) it makes no sense to pull this value and computeDatadog.CarrierInfo
for each event we collect. Instead, we can listen toCTTelephonyNetworkInfo
changes and updateDatadog.CarrierInfo
only when necessary.I can't confirm if this fixes the crash, because I didn't manage to reproduce it even when stress testing our SDK with
~500logs/s
sent from iPhone X and swapping between different SIM cards during the test.How?
Now, we use
serviceSubscriberCellularProvidersDidUpdateNotifier
available on iOS 12+ to updateCarrierInfo
. Initial value is read when SDK starts.The logic for iOS 11 remains the same with the difference that now we also read initial value for
CarrierInfo
when starting the SDK. Although there is an API for listening toCTTelephonyNetworkInfo
updates on iOS 11, it is deprecated (ref.:CTRadioAccessTechnologyDidChange
) and because we have no reports for iOS 11, I decided to not change existing logic.Last, I added simple utility to stress test Logging feature in Example app:
Review checklist