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

RUMM-1691 Reduce utilization of CTTelephonyNetworkInfo to mitigate CarrierInfoProvider crash #627

Merged
merged 5 commits into from Oct 11, 2021

Conversation

ncreated
Copy link
Collaborator

@ncreated ncreated commented Oct 7, 2021

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 pulling CTTelephonyNetworkInfo for each collected event to create CarrierInfo, now we only listen to CTTelephonyNetworkInfo changes to update CarrierInfo when necessary.

We use CTTelephonyNetworkInfo (from CoreTelephony framework) to read carrier information:

Screenshot 2021-10-07 at 12 46 30

A loop of investigation, research and debugging, lead me to following conclusions:

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 that CTTelephonyNetworkInfo changes are very infrequent (C2) it makes no sense to pull this value and compute Datadog.CarrierInfo for each event we collect. Instead, we can listen to CTTelephonyNetworkInfo changes and update Datadog.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.

Screenshot 2021-10-06 at 18 27 04

How?

Now, we use serviceSubscriberCellularProvidersDidUpdateNotifier available on iOS 12+ to update CarrierInfo. 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 to CTTelephonyNetworkInfo 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:

Screenshot 2021-10-07 at 12 42 58

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference

…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).
@ncreated ncreated self-assigned this Oct 7, 2021
@ncreated ncreated force-pushed the ncreated/RUMM-1691-crash-in-carrier-info-provider branch from c0670b3 to 619994e Compare October 7, 2021 10:41
@ncreated ncreated marked this pull request as ready for review October 7, 2021 11:15
@ncreated ncreated requested a review from a team as a code owner October 7, 2021 11:15
@ncreated ncreated marked this pull request as draft October 7, 2021 11:25
@ncreated ncreated marked this pull request as ready for review October 7, 2021 11:47
Copy link
Member

@maxep maxep left a 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 :)

Sources/Datadog/Core/System/CarrierInfoProvider.swift Outdated Show resolved Hide resolved
@ncreated
Copy link
Collaborator Author

ncreated commented Oct 8, 2021

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:

  • With this PR, we now "fetch the carrier when initialising the SDK" (vs. pulling it all the time before). The notification block added in this PR only fires when the SIM card is swapped, so I don't consider this a big complexity. IMO this is the minimal complexity we can add to support this case.
  • About swapping SIM card being an "edge case" - this is an assumption that I'd rather avoid making. Our mission is to be the go-to place for troubleshooting and sometimes having a trace of "error happens after SIM card is replaced" could be priceless.

@maxep
Copy link
Member

maxep commented Oct 8, 2021

Yes, I'm convinced now :)

@ncreated ncreated requested a review from maxep October 8, 2021 14:05
Copy link
Member

@maxep maxep left a 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
@ncreated ncreated merged commit d095948 into master Oct 11, 2021
@ncreated ncreated deleted the ncreated/RUMM-1691-crash-in-carrier-info-provider branch October 11, 2021 12:52
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.

None yet

3 participants