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

ref: Add FramesDelayResult #4279

Merged
merged 4 commits into from
Aug 14, 2024
Merged

ref: Add FramesDelayResult #4279

merged 4 commits into from
Aug 14, 2024

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Aug 13, 2024

📜 Description

Add SentryFramesDelayResult containing framesContributingToDelayCount, which is the count for the frames that contributed to the frames delay count.

This PR is based on #4278.

#skip-changelog

💡 Motivation and Context

This is required for GH-3492.

💚 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

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.
Add SentryFramesDelayResult containing framesContributingToDelayCount,
which is the count for the frames that contributed to the frames delay
count. This is required for GH-3492.
Copy link

codecov bot commented Aug 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.611%. Comparing base (a4ffa76) to head (ba5cd71).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4279       +/-   ##
=============================================
+ Coverage   91.600%   91.611%   +0.010%     
=============================================
  Files          616       617        +1     
  Lines        50027     50044       +17     
  Branches     18094     18095        +1     
=============================================
+ Hits         45825     45846       +21     
+ Misses        4110      4105        -5     
- Partials        92        93        +1     
Files Coverage Δ
Sources/Sentry/SentryDelayedFramesTracker.m 99.056% <100.000%> (+0.046%) ⬆️
Sources/Sentry/SentryFramesTracker.m 99.354% <ø> (ø)
Sources/Sentry/SentrySpan.m 99.363% <100.000%> (+0.004%) ⬆️
Sources/Sentry/SentryTracer.m 96.721% <100.000%> (+0.007%) ⬆️
...tions/FramesTracking/SentryFramesDelayResult.swift 100.000% <100.000%> (ø)
...ance/FramesTracking/SentryFramesTrackerTests.swift 99.558% <100.000%> (+0.006%) ⬆️

... and 4 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 a4ffa76...ba5cd71. Read the comment docs.

Copy link

github-actions bot commented Aug 13, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1220.65 ms 1245.80 ms 25.15 ms
Size 21.58 KiB 706.13 KiB 684.55 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
98a8c16 1206.40 ms 1232.14 ms 25.74 ms
fdb003d 1214.51 ms 1230.49 ms 15.98 ms
5f8ee7a 1249.48 ms 1252.20 ms 2.72 ms
7e757f4 1229.31 ms 1252.00 ms 22.69 ms
01a28a9 1200.78 ms 1227.90 ms 27.12 ms
f0737f6 1220.43 ms 1236.44 ms 16.01 ms
2a894d5 1211.02 ms 1236.72 ms 25.70 ms
e145ca1 1207.19 ms 1230.67 ms 23.48 ms
b066509 1246.14 ms 1272.42 ms 26.28 ms
289c0b8 1245.63 ms 1253.76 ms 8.13 ms

App size

Revision Plain With Sentry Diff
98a8c16 20.76 KiB 431.00 KiB 410.24 KiB
fdb003d 22.85 KiB 414.11 KiB 391.26 KiB
5f8ee7a 22.85 KiB 411.93 KiB 389.08 KiB
7e757f4 21.58 KiB 682.40 KiB 660.82 KiB
01a28a9 22.85 KiB 405.39 KiB 382.54 KiB
f0737f6 20.76 KiB 437.12 KiB 416.36 KiB
2a894d5 21.58 KiB 414.57 KiB 392.99 KiB
e145ca1 21.58 KiB 669.70 KiB 648.12 KiB
b066509 22.84 KiB 403.13 KiB 380.29 KiB
289c0b8 22.85 KiB 407.67 KiB 384.82 KiB

Previous results on branch: ref/add-frames-delay-result

Startup times

Revision Plain With Sentry Diff
96b212d 1246.51 ms 1266.74 ms 20.23 ms

App size

Revision Plain With Sentry Diff
96b212d 21.58 KiB 706.14 KiB 684.56 KiB

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.

I guess you're planing to use framesContributingToDelayCount in another PR, so all good.

Base automatically changed from fix/too-long-frame-delays to main August 14, 2024 09:52
@philipphofmann philipphofmann merged commit df2835d into main Aug 14, 2024
65 checks passed
@philipphofmann philipphofmann deleted the ref/add-frames-delay-result branch August 14, 2024 11:03
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