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

feat: track network connectivity changes with breadcrumbs #3232

Merged
merged 14 commits into from
Sep 20, 2023

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented Aug 18, 2023

📜 Description

Monitor reachability of https://sentry.io in SentryBreadcrumbTracker and have its delegate record a breadcrumb every time connectivity changes, recording its description in the breadcrumb data.

Currently calling this breadcrumb type "connectivity", which we'd need to add to https://develop.sentry.dev/sdk/event-payloads/breadcrumbs/#breadcrumb-types. This is subject to change, as is the breadcrumb category of "device.connectivity".

Refactors:

  • access SentryReachability via SentryDependencyContainer
  • expose description constants eg SentryConnectivityCellular

💡 Motivation and Context

For #3127

💚 How did you test it?

New test case in SentryBreadcrumbTrackerTests.swift.

@github-actions
Copy link

github-actions bot commented Aug 18, 2023

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 1c1690e

@codecov
Copy link

codecov bot commented Aug 18, 2023

Codecov Report

Merging #3232 (7fa081e) into main (be2977c) will increase coverage by 0.089%.
Report is 1 commits behind head on main.
The diff coverage is 100.000%.

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3232       +/-   ##
=============================================
+ Coverage   89.128%   89.217%   +0.089%     
=============================================
  Files          502       501        -1     
  Lines        54251     54022      -229     
  Branches     19475     19200      -275     
=============================================
- Hits         48353     48197      -156     
- Misses        4919      4968       +49     
+ Partials       979       857      -122     
Files Changed Coverage
Sources/Sentry/SentryReachability.m ø
Sources/Sentry/SentryBreadcrumbTracker.m 100.000%
Sources/Sentry/SentryDependencyContainer.m 100.000%
...ons/Breadcrumbs/SentryBreadcrumbTrackerTests.swift 100.000%
...s/SentryTests/Networking/SentryReachabilityTests.m 100.000%
...entryTests/Networking/TestSentryReachability.swift 100.000%

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be2977c...7fa081e. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Aug 18, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1242.59 ms 1253.88 ms 11.29 ms
Size 22.85 KiB 408.73 KiB 385.88 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
881a955 1230.98 ms 1246.22 ms 15.24 ms
3e7aa41 1214.02 ms 1238.84 ms 24.82 ms
98752f3 1240.61 ms 1259.80 ms 19.18 ms
289c0b8 1245.63 ms 1253.76 ms 8.13 ms
b066509 1246.14 ms 1272.42 ms 26.28 ms
fdfe96b 1227.90 ms 1242.56 ms 14.66 ms
dbc67d2 1239.49 ms 1248.88 ms 9.39 ms
4259afd 1222.12 ms 1249.74 ms 27.62 ms
dacf894 1223.96 ms 1250.41 ms 26.45 ms
24b4df1 1240.98 ms 1253.98 ms 13.00 ms

App size

Revision Plain With Sentry Diff
881a955 22.85 KiB 407.63 KiB 384.79 KiB
3e7aa41 20.76 KiB 432.31 KiB 411.55 KiB
98752f3 20.76 KiB 435.09 KiB 414.33 KiB
289c0b8 22.85 KiB 407.67 KiB 384.82 KiB
b066509 22.84 KiB 403.13 KiB 380.29 KiB
fdfe96b 20.76 KiB 419.70 KiB 398.95 KiB
dbc67d2 20.76 KiB 427.74 KiB 406.98 KiB
4259afd 20.76 KiB 419.70 KiB 398.94 KiB
dacf894 20.76 KiB 426.34 KiB 405.58 KiB
24b4df1 20.76 KiB 425.77 KiB 405.01 KiB

Previous results on branch: armcknight/feat/3127-network-change-breadcrumbs

Startup times

Revision Plain With Sentry Diff
41c525b 1218.84 ms 1226.62 ms 7.79 ms
d63a3a5 1257.60 ms 1271.42 ms 13.82 ms
5afbde7 1261.22 ms 1264.67 ms 3.45 ms
a93728b 1240.27 ms 1252.06 ms 11.79 ms
dc541cc 1243.61 ms 1256.54 ms 12.93 ms
6dd4832 1234.71 ms 1256.18 ms 21.47 ms
f94c893 1244.84 ms 1261.40 ms 16.56 ms

App size

Revision Plain With Sentry Diff
41c525b 22.85 KiB 407.46 KiB 384.61 KiB
d63a3a5 22.85 KiB 408.72 KiB 385.88 KiB
5afbde7 22.84 KiB 403.75 KiB 380.90 KiB
a93728b 22.85 KiB 407.75 KiB 384.90 KiB
dc541cc 22.85 KiB 407.88 KiB 385.03 KiB
6dd4832 22.85 KiB 407.92 KiB 385.07 KiB
f94c893 22.85 KiB 407.46 KiB 384.61 KiB

@stefanosiano
Copy link
Member

stefanosiano commented Aug 18, 2023

On Android the breadcrumbs have:
type: system
category: network.event
data["action"]: one of NETWORK_AVAILABLE, NETWORK_LOST, NETWORK_CAPABILITIES_CHANGED
Except for the action, i guess we can align on type and category, so they are the same on Android and Cocoa (and flutter and rn as they rely on Android and Cocoa).

We already collect other breadcrumbs under the system type, coming from the system, like temperature, phone state and other generic system events.
I'm updating the meta issue with this info.
I'm not sure if develop docs are to be updated, as these really affect only mobile SDKs, but perhaps it's a good idea anyway, since i didn't find the system type documented anywhere, either.

@armcknight
Copy link
Member Author

Thanks @stefanosiano for the info! Let me know anything I can do to help update docs, even if you just need a review ✨

@armcknight
Copy link
Member Author

armcknight commented Aug 18, 2023

I just noticed that much of the relevant implementation of SentryReachability are managed by static state, so instantiating another one and calling -[monitorURL:usingCallback:] will erase the previous monitoring setup by our internal networking stack. This will require more refactoring.

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

Code wise looks good, but CI is broken

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

LGTM

@armcknight
Copy link
Member Author

@brustolin I pushed a new commit that refactors a couple things:

  • allows multiple observers to watch reachability changes
  • internalize the URL used to monitor network state
  • don't ignore the first reachability callback, as it contains useful information (like, in the happy path, that the network is reachable as of the time monitoring started)
  • simplify several flows, including rearranging code in reachability functions to use early exits

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

All good with the changes.

@armcknight armcknight merged commit 97797b4 into main Sep 20, 2023
@armcknight armcknight deleted the armcknight/feat/3127-network-change-breadcrumbs branch September 20, 2023 19:13
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.

4 participants