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: Finish TTFD when binding transaction to scope #4526

Merged

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Nov 12, 2024

📜 Description

Finish the TTFD span before creating a new transaction and binding it to the scope in SentryUIViewControllerPerformanceTracker when the user doesn't call reportFullyDisplayed.

💡 Motivation and Context

Fixes GH-4513

💚 How did you test it?

Unit tests and simulator.

📝 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

Finish the TTID/TTFD spans when a new UIViewController gets displayed,
instead of waiting for the 30 seconds timeout.
Copy link

codecov bot commented Nov 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.538%. Comparing base (d9f518a) to head (402b971).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4526       +/-   ##
=============================================
- Coverage   91.566%   91.538%   -0.029%     
=============================================
  Files          615       614        -1     
  Lines        69701     69750       +49     
  Branches     24967     24883       -84     
=============================================
+ Hits         63823     63848       +25     
- Misses        5787      5809       +22     
- Partials        91        93        +2     
Files with missing lines Coverage Δ
Sources/Sentry/SentryPerformanceTracker.m 100.000% <100.000%> (ø)
Sources/Sentry/SentryTimeToDisplayTracker.m 100.000% <100.000%> (ø)
.../Sentry/SentryUIViewControllerPerformanceTracker.m 99.267% <100.000%> (+0.008%) ⬆️
...iewController/SentryTimeToDisplayTrackerTest.swift 100.000% <100.000%> (ø)
...entryUIViewControllerPerformanceTrackerTests.swift 99.012% <100.000%> (+0.149%) ⬆️

... and 37 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 d9f518a...402b971. Read the comment docs.

Copy link

github-actions bot commented Nov 12, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1242.73 ms 1263.96 ms 21.23 ms
Size 22.30 KiB 730.72 KiB 708.41 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
879fb28 1243.18 ms 1255.98 ms 12.80 ms
98752f3 1226.18 ms 1251.38 ms 25.20 ms
e0904ef 1231.85 ms 1252.38 ms 20.53 ms
3f6c30b 1252.98 ms 1266.22 ms 13.24 ms
26530fe 1233.98 ms 1250.06 ms 16.08 ms
742d4b6 1196.56 ms 1216.54 ms 19.98 ms
8b1c6a9 1216.92 ms 1230.90 ms 13.98 ms
643853e 1225.75 ms 1247.00 ms 21.25 ms
8e76be4 1272.67 ms 1286.38 ms 13.71 ms
dacf894 1223.96 ms 1250.41 ms 26.45 ms

App size

Revision Plain With Sentry Diff
879fb28 22.84 KiB 402.88 KiB 380.03 KiB
98752f3 20.76 KiB 435.09 KiB 414.33 KiB
e0904ef 21.58 KiB 614.64 KiB 593.06 KiB
3f6c30b 22.85 KiB 408.88 KiB 386.03 KiB
26530fe 21.58 KiB 714.93 KiB 693.35 KiB
742d4b6 21.58 KiB 546.20 KiB 524.62 KiB
8b1c6a9 21.58 KiB 706.97 KiB 685.38 KiB
643853e 21.58 KiB 655.73 KiB 634.15 KiB
8e76be4 20.76 KiB 427.66 KiB 406.89 KiB
dacf894 20.76 KiB 426.34 KiB 405.58 KiB

Previous results on branch: fix/finish-ttid-ttfd-spans-on-new-ui-view-controller

Startup times

Revision Plain With Sentry Diff
c3b3b44 1230.88 ms 1256.09 ms 25.21 ms
976696d 1234.52 ms 1251.48 ms 16.96 ms
5ad561b 1225.22 ms 1245.67 ms 20.44 ms
c0dc9e4 1236.85 ms 1258.12 ms 21.27 ms

App size

Revision Plain With Sentry Diff
c3b3b44 22.30 KiB 730.79 KiB 708.48 KiB
976696d 22.30 KiB 730.52 KiB 708.21 KiB
5ad561b 22.30 KiB 730.71 KiB 708.41 KiB
c0dc9e4 22.30 KiB 730.71 KiB 708.40 KiB

@philipphofmann philipphofmann changed the title fix: Finish TTFD on new UIViewController fix: Finish TTFD on new UIViewControllers Nov 12, 2024
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

@brustolin
Copy link
Contributor

brustolin commented Nov 12, 2024

BTW, I just realized a problem that goes like this:

  • ViewControllerA opens, starts a HTTP request, user clicks a button and opens ViewControllerB.
  • ViewControllerB starts an HTTP request.
  • ViewControllerA’s HTTP request ends and SentrySDK.reportFullyDisplayed is called.

This will finish TTFD for ViewControllerB.

@philipphofmann
Copy link
Member Author

philipphofmann commented Nov 12, 2024

BTW, I just realized a problem that goes like this:

  • ViewControllerA opens, starts a HTTP request, user clicks a button and opens ViewControllerB.
  • ViewControllerB starts an HTTP request.
  • ViewControllerA’s HTTP request ends and SentrySDK.reportFullyDisplayed is called.

This will finish TTFD for ViewControllerB.

Do you think this PR adds this problem or that the problem still exists and needs to be addressed @brustolin?

Why do you think the ViewControllerA’s HTTP request ending does call SentrySDK.reportFullyDisplayed? Do you mean the user calls it? Before this PR calling SentrySDK.reportFullyDisplayed when being on ViewControllerB ends finishes TTFD of ViewControllerB. The same applies to this PR, but the difference now is that the TTFD of UIViewControllerA doesn't timeout any more.

@philipphofmann philipphofmann changed the title fix: Finish TTFD on new UIViewControllers fix: Finish TTFD when binding transaction to scope Nov 12, 2024
@philipphofmann
Copy link
Member Author

@brustolin, I now fixed GH-4513 in this PR, as I discovered that this PR on its own doesn't really make sense without fixing GH-4513. The changes required to do so weren't that big.

@brustolin
Copy link
Contributor

Do you think this PR adds this problem or that the problem still exists and needs to be addressed @brustolin?

I think this PR adds this problem. Of course, by fixing another problem, we need to choose.

Why do you think the ViewControllerA’s HTTP request ending does call SentrySDK.reportFullyDisplayed? Do you mean the user calls it?

The user would call it in the code.

Before this PR calling SentrySDK.reportFullyDisplayed when being on ViewControllerB ends finishes TTFD of ViewControllerB. The same applies to this PR, but the difference now is that the TTFD of UIViewControllerA doesn't timeout any more.

🤔 I need to check If I can reproduce what Im claiming.

@philipphofmann
Copy link
Member Author

I'm not sure if this PR adds the problem. Yes, please try if you can reproduce it, ideally with a test.

@brustolin
Copy link
Contributor

@philipphofmann I wrote the test to reproduce what Im talking about in here

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

@philipphofmann philipphofmann merged commit e680b99 into main Nov 13, 2024
62 of 65 checks passed
@philipphofmann philipphofmann deleted the fix/finish-ttid-ttfd-spans-on-new-ui-view-controller branch November 13, 2024 15:30
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.

Finish TTFD span when new transaction bound to scope
2 participants