search

getsentry / sentry-cocoa

The official Sentry SDK for iOS, tvOS, macOS, watchOS.
https://sentry.io/for/cocoa/
MIT License
816 stars 330 forks source link

fix: Finish TTFD when binding transaction to scope #4526

Closed philipphofmann closed 2 weeks ago

philipphofmann commented 2 weeks ago

:scroll: 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.

:bulb: Motivation and Context

Fixes GH-4513

:green_heart: How did you test it?

Unit tests and simulator.

:pencil: Checklist

You have to check all boxes before merging:

:crystal_ball: Next steps

codecov[bot] commented 2 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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](https://app.codecov.io/gh/getsentry/sentry-cocoa/pull/4526/graphs/tree.svg?width=650&height=150&src=pr&token=PTZKtOJlrs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry)](https://app.codecov.io/gh/getsentry/sentry-cocoa/pull/4526?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) ```diff @@ 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](https://app.codecov.io/gh/getsentry/sentry-cocoa/pull/4526?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) | Coverage Δ | | |---|---|---| | [Sources/Sentry/SentryPerformanceTracker.m](https://app.codecov.io/gh/getsentry/sentry-cocoa/pull/4526?src=pr&el=tree&filepath=Sources%2FSentry%2FSentryPerformanceTracker.m&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry#diff-U291cmNlcy9TZW50cnkvU2VudHJ5UGVyZm9ybWFuY2VUcmFja2VyLm0=) | `100.000% <100.000%> (ø)` | | | [Sources/Sentry/SentryTimeToDisplayTracker.m](https://app.codecov.io/gh/getsentry/sentry-cocoa/pull/4526?src=pr&el=tree&filepath=Sources%2FSentry%2FSentryTimeToDisplayTracker.m&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry#diff-U291cmNlcy9TZW50cnkvU2VudHJ5VGltZVRvRGlzcGxheVRyYWNrZXIubQ==) | `100.000% <100.000%> (ø)` | | | [.../Sentry/SentryUIViewControllerPerformanceTracker.m](https://app.codecov.io/gh/getsentry/sentry-cocoa/pull/4526?src=pr&el=tree&filepath=Sources%2FSentry%2FSentryUIViewControllerPerformanceTracker.m&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry#diff-U291cmNlcy9TZW50cnkvU2VudHJ5VUlWaWV3Q29udHJvbGxlclBlcmZvcm1hbmNlVHJhY2tlci5t) | `99.267% <100.000%> (+0.008%)` | :arrow_up: | | [...iewController/SentryTimeToDisplayTrackerTest.swift](https://app.codecov.io/gh/getsentry/sentry-cocoa/pull/4526?src=pr&el=tree&filepath=Tests%2FSentryTests%2FIntegrations%2FPerformance%2FUIViewController%2FSentryTimeToDisplayTrackerTest.swift&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry#diff-VGVzdHMvU2VudHJ5VGVzdHMvSW50ZWdyYXRpb25zL1BlcmZvcm1hbmNlL1VJVmlld0NvbnRyb2xsZXIvU2VudHJ5VGltZVRvRGlzcGxheVRyYWNrZXJUZXN0LnN3aWZ0) | `100.000% <100.000%> (ø)` | | | [...entryUIViewControllerPerformanceTrackerTests.swift](https://app.codecov.io/gh/getsentry/sentry-cocoa/pull/4526?src=pr&el=tree&filepath=Tests%2FSentryTests%2FIntegrations%2FPerformance%2FUIViewController%2FSentryUIViewControllerPerformanceTrackerTests.swift&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry#diff-VGVzdHMvU2VudHJ5VGVzdHMvSW50ZWdyYXRpb25zL1BlcmZvcm1hbmNlL1VJVmlld0NvbnRyb2xsZXIvU2VudHJ5VUlWaWV3Q29udHJvbGxlclBlcmZvcm1hbmNlVHJhY2tlclRlc3RzLnN3aWZ0) | `99.012% <100.000%> (+0.149%)` | :arrow_up: | ... and [37 files with indirect coverage changes](https://app.codecov.io/gh/getsentry/sentry-cocoa/pull/4526/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/getsentry/sentry-cocoa/pull/4526?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/getsentry/sentry-cocoa/pull/4526?dropdown=coverage&src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry). Last update [d9f518a...402b971](https://app.codecov.io/gh/getsentry/sentry-cocoa/pull/4526?dropdown=coverage&src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=getsentry).
github-actions[bot] commented 2 weeks ago

Performance metrics :rocket:

  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
879fb28ba31f00157b8728013941870f4f0f4f6c 1243.18 ms 1255.98 ms 12.80 ms
98752f38221b7244ce958ab08fc00908ba3de694 1226.18 ms 1251.38 ms 25.20 ms
e0904ef6b8b510e9739820aa29379b074b26e237 1231.85 ms 1252.38 ms 20.53 ms
3f6c30b04dc2377110490170c382b92ae85ee9a6 1252.98 ms 1266.22 ms 13.24 ms
26530fec80489397091f307088e6a03644cf5efd 1233.98 ms 1250.06 ms 16.08 ms
742d4b693cfd3cf2b7b1d62930d16daaf505d367 1196.56 ms 1216.54 ms 19.98 ms
8b1c6a90b399ed69ca52a3b97802c5667ee8153c 1216.92 ms 1230.90 ms 13.98 ms
643853e695fb398478c6af8c0c060b8fde9b3744 1225.75 ms 1247.00 ms 21.25 ms
8e76be4558a9e15fe63841bbb98faec7d921f24e 1272.67 ms 1286.38 ms 13.71 ms
dacf894bd3ce125f76ea565dbc543db3da8bf94d 1223.96 ms 1250.41 ms 26.45 ms

App size

Revision Plain With Sentry Diff
879fb28ba31f00157b8728013941870f4f0f4f6c 22.84 KiB 402.88 KiB 380.03 KiB
98752f38221b7244ce958ab08fc00908ba3de694 20.76 KiB 435.09 KiB 414.33 KiB
e0904ef6b8b510e9739820aa29379b074b26e237 21.58 KiB 614.64 KiB 593.06 KiB
3f6c30b04dc2377110490170c382b92ae85ee9a6 22.85 KiB 408.88 KiB 386.03 KiB
26530fec80489397091f307088e6a03644cf5efd 21.58 KiB 714.93 KiB 693.35 KiB
742d4b693cfd3cf2b7b1d62930d16daaf505d367 21.58 KiB 546.20 KiB 524.62 KiB
8b1c6a90b399ed69ca52a3b97802c5667ee8153c 21.58 KiB 706.97 KiB 685.38 KiB
643853e695fb398478c6af8c0c060b8fde9b3744 21.58 KiB 655.73 KiB 634.15 KiB
8e76be4558a9e15fe63841bbb98faec7d921f24e 20.76 KiB 427.66 KiB 406.89 KiB
dacf894bd3ce125f76ea565dbc543db3da8bf94d 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
c3b3b4489b4b340bc4549deea7dd624abeaf8d54 1230.88 ms 1256.09 ms 25.21 ms
976696df0c013c6a4ccbf733d6a0adcae48f24ed 1234.52 ms 1251.48 ms 16.96 ms
5ad561bed3294a1498e1c5e136921e1b0484f874 1225.22 ms 1245.67 ms 20.44 ms
c0dc9e438a9c457addd85ce03aa1bf00c4d5c72f 1236.85 ms 1258.12 ms 21.27 ms

App size

Revision Plain With Sentry Diff
c3b3b4489b4b340bc4549deea7dd624abeaf8d54 22.30 KiB 730.79 KiB 708.48 KiB
976696df0c013c6a4ccbf733d6a0adcae48f24ed 22.30 KiB 730.52 KiB 708.21 KiB
5ad561bed3294a1498e1c5e136921e1b0484f874 22.30 KiB 730.71 KiB 708.41 KiB
c0dc9e438a9c457addd85ce03aa1bf00c4d5c72f 22.30 KiB 730.71 KiB 708.40 KiB
brustolin commented 2 weeks ago

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

This will finish TTFD for ViewControllerB.

philipphofmann commented 2 weeks ago

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 commented 2 weeks ago

@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 commented 2 weeks ago

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 commented 2 weeks ago

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

brustolin commented 2 weeks ago

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