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

fix: Guard FramesTracker start and stop #4224

Merged
merged 2 commits into from
Aug 1, 2024

Conversation

philipphofmann
Copy link
Member

📜 Description

Profiling starts the frames tracker if it's not running, but the frames tracker again subscribes to the didBecomeActive and willResignActive notifications. To fix this, the start operation is a NoOp now if the frames tracker has already started. The same applies to stop.

💡 Motivation and Context

This came up while investigating long TTID spans for a customer.

💚 How did you test it?

Unit tests.

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

Profiling starts the frames tracker if it's not running, but the frames
tracker again subscribes to the didBecomeActive and willResignActive
notifications. To fix this, the start operation is a NoOp now if the
frames tracker has already started. The same applies to stop.
Copy link

codecov bot commented Aug 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.375%. Comparing base (61249fb) to head (e4c5e03).

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4224       +/-   ##
=============================================
- Coverage   91.350%   90.375%   -0.976%     
=============================================
  Files          610       610               
  Lines        48963     48811      -152     
  Branches     17596     17330      -266     
=============================================
- Hits         44728     44113      -615     
- Misses        4142      4605      +463     
  Partials        93        93               
Files Coverage Δ
SentryTestUtils/TestDisplayLinkWrapper.swift 95.161% <100.000%> (-0.077%) ⬇️
...tryTestUtils/TestNSNotificationCenterWrapper.swift 90.909% <100.000%> (+9.090%) ⬆️
Sources/Sentry/SentryFramesTracker.m 98.692% <100.000%> (-0.623%) ⬇️
...ance/FramesTracking/SentryFramesTrackerTests.swift 99.530% <100.000%> (+0.015%) ⬆️

... and 101 files with indirect coverage changes


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 61249fb...e4c5e03. Read the comment docs.

Copy link

github-actions bot commented Aug 1, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1223.00 ms 1230.06 ms 7.06 ms
Size 21.58 KiB 694.58 KiB 673.00 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
d10ae0c 1250.02 ms 1253.74 ms 3.72 ms
f25febb 1224.69 ms 1247.38 ms 22.68 ms
e324230 1225.84 ms 1250.40 ms 24.57 ms
d3abae0 1200.36 ms 1224.22 ms 23.87 ms
16450b8 1196.33 ms 1217.26 ms 20.93 ms
7cd187e 1229.02 ms 1233.06 ms 4.04 ms
3297d6e 1206.80 ms 1213.86 ms 7.06 ms
4af7581 1204.67 ms 1241.33 ms 36.66 ms
f1a6589 1253.67 ms 1269.06 ms 15.39 ms
7bc3c0d 1212.35 ms 1228.94 ms 16.59 ms

App size

Revision Plain With Sentry Diff
d10ae0c 20.76 KiB 419.86 KiB 399.10 KiB
f25febb 21.58 KiB 414.92 KiB 393.34 KiB
e324230 22.85 KiB 408.87 KiB 386.02 KiB
d3abae0 20.76 KiB 434.92 KiB 414.16 KiB
16450b8 22.85 KiB 410.98 KiB 388.13 KiB
7cd187e 20.76 KiB 401.65 KiB 380.89 KiB
3297d6e 21.58 KiB 418.44 KiB 396.86 KiB
4af7581 22.84 KiB 401.62 KiB 378.78 KiB
f1a6589 22.85 KiB 408.87 KiB 386.02 KiB
7bc3c0d 20.76 KiB 427.35 KiB 406.59 KiB

Previous results on branch: fix/frames-tracker-start-stop

Startup times

Revision Plain With Sentry Diff
a221130 1224.69 ms 1240.04 ms 15.35 ms

App size

Revision Plain With Sentry Diff
a221130 21.58 KiB 694.58 KiB 672.99 KiB

@philipphofmann philipphofmann merged commit cc31630 into main Aug 1, 2024
53 of 57 checks passed
@philipphofmann philipphofmann deleted the fix/frames-tracker-start-stop branch August 1, 2024 14:51
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.

2 participants