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: Rarely reporting too long frame delays #4278

Merged
merged 2 commits into from
Aug 14, 2024

Conversation

philipphofmann
Copy link
Member

📜 Description

Fix a race condition in the SentryFramesTracker and SentryDelayedFramesTracker that sometimes leads to frame delay durations longer than the queried interval of start and end time. This is fixed by moving the previousFrameSystemTimestamp down to the SentryDelayedFramesTracker so we can adequately synchronize it without acquiring extra locks on the main thread.

💡 Motivation and Context

Came up while working on GH-3492.

💚 How did you test it?

Unit test.

📝 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

Fix a race condition in the SentryFramesTracker and
SentryDelayedFramesTracker that sometimes leads to frame delay durations
longer than the queried interval of start and end time. This is fixed by
moving the previousFrameSystemTimestamp down to the
SentryDelayedFramesTracker so we can adequately synchronize it without
acquiring extra locks on the main thread.
Copy link

codecov bot commented Aug 13, 2024

Codecov Report

Attention: Patch coverage is 97.22222% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.547%. Comparing base (464117d) to head (dcbe5eb).

Files Patch % Lines
Sources/Sentry/SentryDelayedFramesTracker.m 90.909% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4278       +/-   ##
=============================================
- Coverage   91.612%   91.547%   -0.065%     
=============================================
  Files          616       615        -1     
  Lines        50000     49939       -61     
  Branches     18091     18012       -79     
=============================================
- Hits         45806     45718       -88     
- Misses        4103      4128       +25     
- Partials        91        93        +2     
Files Coverage Δ
SentryTestUtils/TestDisplayLinkWrapper.swift 95.890% <100.000%> (+0.176%) ⬆️
Sources/Sentry/SentryFramesTracker.m 99.354% <100.000%> (+0.653%) ⬆️
...ance/FramesTracking/SentryFramesTrackerTests.swift 99.551% <100.000%> (+0.018%) ⬆️
Sources/Sentry/SentryDelayedFramesTracker.m 99.009% <90.909%> (-0.991%) ⬇️

... and 24 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 464117d...dcbe5eb. Read the comment docs.

@philipphofmann philipphofmann mentioned this pull request Aug 13, 2024
7 tasks
Copy link

github-actions bot commented Aug 14, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1231.16 ms 1245.45 ms 14.28 ms
Size 21.58 KiB 705.65 KiB 684.07 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
f1c36e0 1215.18 ms 1223.62 ms 8.43 ms
939cd63 1222.53 ms 1250.76 ms 28.23 ms
2a894d5 1202.07 ms 1227.66 ms 25.59 ms
65f104b 1229.87 ms 1247.69 ms 17.82 ms
46bb3fe 1221.53 ms 1234.24 ms 12.71 ms
aeec206 1229.27 ms 1253.70 ms 24.43 ms
31ac438 1244.29 ms 1264.06 ms 19.78 ms
10ee2ce 1250.90 ms 1258.57 ms 7.67 ms
eb41178 1228.06 ms 1248.37 ms 20.31 ms
25bcc50 1240.47 ms 1254.70 ms 14.23 ms

App size

Revision Plain With Sentry Diff
f1c36e0 21.58 KiB 670.40 KiB 648.81 KiB
939cd63 21.58 KiB 424.35 KiB 402.76 KiB
2a894d5 21.58 KiB 414.57 KiB 392.99 KiB
65f104b 21.58 KiB 625.82 KiB 604.24 KiB
46bb3fe 21.58 KiB 542.19 KiB 520.61 KiB
aeec206 20.76 KiB 434.88 KiB 414.12 KiB
31ac438 20.76 KiB 393.36 KiB 372.60 KiB
10ee2ce 20.76 KiB 427.77 KiB 407.00 KiB
eb41178 21.58 KiB 544.86 KiB 523.28 KiB
25bcc50 20.76 KiB 427.22 KiB 406.46 KiB

Previous results on branch: fix/too-long-frame-delays

Startup times

Revision Plain With Sentry Diff
f8d77f9 1227.02 ms 1250.57 ms 23.55 ms

App size

Revision Plain With Sentry Diff
f8d77f9 21.58 KiB 705.65 KiB 684.07 KiB

@philipphofmann philipphofmann merged commit a4ffa76 into main Aug 14, 2024
61 of 65 checks passed
@philipphofmann philipphofmann deleted the fix/too-long-frame-delays branch August 14, 2024 09: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.

2 participants