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: Long-lasting TTID/TTFD spans #4225

Merged
merged 2 commits into from
Aug 2, 2024
Merged

fix: Long-lasting TTID/TTFD spans #4225

merged 2 commits into from
Aug 2, 2024

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Aug 1, 2024

📜 Description

Presenting a UIViewController when going to the background could lead to long-lasting TTID/TTFD spans. This is fixed now by not creating TTID/TTFD spans when the FramesTracker isn't running.

Docs PR: getsentry/sentry-docs#10960.

💡 Motivation and Context

A customer reported to get long lasting TTID/TTFD spans for UIViewControllers presented when the didEnterBackgroundNotification is posted. They do this due to privacy reasons.

💚 How did you test it?

Added a small sample to present an UIViewController when the didEnterBackgroundNotification is posted and could sometimes reproduce the problem. This PR fixes that problem. I didn't add the sample as a UITest, because it isn't reliable and this PR addresses an edge case. Furthermore, I added 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

Presenting a UIViewController when going to the background could lead to
long-lasting TTID/TTFD spans. This is fixed now by not creating
TTID/TTFD spans when the FramesTracker isn't running.
Copy link

codecov bot commented Aug 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.421%. Comparing base (cc31630) to head (cfc1e09).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4225       +/-   ##
=============================================
- Coverage   91.429%   91.421%   -0.008%     
=============================================
  Files          611       611               
  Lines        49088     49125       +37     
  Branches     17726     17743       +17     
=============================================
+ Hits         44881     44911       +30     
- Misses        4114      4121        +7     
  Partials        93        93               
Files Coverage Δ
Sources/Sentry/SentryTimeToDisplayTracker.m 100.000% <100.000%> (ø)
.../Sentry/SentryUIViewControllerPerformanceTracker.m 99.033% <100.000%> (+0.009%) ⬆️
...iewController/SentryTimeToDisplayTrackerTest.swift 100.000% <100.000%> (ø)
...entryUIViewControllerPerformanceTrackerTests.swift 97.977% <100.000%> (+0.095%) ⬆️

... 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 cc31630...cfc1e09. Read the comment docs.

Copy link

github-actions bot commented Aug 1, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1233.54 ms 1253.06 ms 19.52 ms
Size 21.58 KiB 694.62 KiB 673.04 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
f25febb 1238.80 ms 1245.35 ms 6.56 ms
e324230 1252.69 ms 1274.56 ms 21.87 ms
0001a09 1223.90 ms 1249.56 ms 25.66 ms
8cd6cfa 1229.88 ms 1251.45 ms 21.57 ms
25e65a5 1249.08 ms 1277.78 ms 28.70 ms
7cd187e 1243.56 ms 1250.20 ms 6.64 ms
3297d6e 1228.57 ms 1255.04 ms 26.47 ms
3d3a411 1233.37 ms 1246.44 ms 13.07 ms
4af7581 1214.55 ms 1237.10 ms 22.55 ms
0589699 1230.14 ms 1245.18 ms 15.04 ms

App size

Revision Plain With Sentry Diff
f25febb 21.58 KiB 414.92 KiB 393.34 KiB
e324230 22.85 KiB 408.87 KiB 386.02 KiB
0001a09 20.76 KiB 433.19 KiB 412.43 KiB
8cd6cfa 21.58 KiB 571.85 KiB 550.26 KiB
25e65a5 22.85 KiB 403.19 KiB 380.34 KiB
7cd187e 20.76 KiB 401.65 KiB 380.89 KiB
3297d6e 21.58 KiB 418.45 KiB 396.86 KiB
3d3a411 21.58 KiB 418.58 KiB 396.99 KiB
4af7581 22.84 KiB 401.62 KiB 378.77 KiB
0589699 21.58 KiB 656.60 KiB 635.02 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.

Pretty straight forward. I believe this will fix the issue.

Just one suggestion but all looks good.

@philipphofmann philipphofmann merged commit 649d940 into main Aug 2, 2024
64 of 68 checks passed
@philipphofmann philipphofmann deleted the fix/long-ttid-spans branch August 2, 2024 07:32
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.

3 participants