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 TTID when viewWillAppear is skipped #4417

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Oct 9, 2024

📜 Description

According to the Apple docs, viewWillAppear should be called before the UIViewController is added to the view hierarchy. There are some edge cases, though, when this doesn't happen, and we saw customers' transactions also proofing this. Therefore, we must also report the initial display here, as the customers' transactions had spans for viewWillLayoutSubViews.

💡 Motivation and Context

Fixes GH-4383

💚 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

According to the Apple docs, viewWillAppear should be called before the
UIViewController is added to the view hierarchy. There are some edge
cases, though, when this doesn't happen, and we saw customers'
transactions also proofing this. Therefore, we must also report the
initial display here, as the customers' transactions had spans for
viewWillLayoutSubViews.

Fixes GH-4383
Copy link

codecov bot commented Oct 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.321%. Comparing base (887502e) to head (518a25b).
Report is 6 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4417       +/-   ##
=============================================
+ Coverage   91.303%   91.321%   +0.018%     
=============================================
  Files          609       609               
  Lines        49570     49630       +60     
  Branches     17857     17894       +37     
=============================================
+ Hits         45259     45323       +64     
+ Misses        4218      4216        -2     
+ Partials        93        91        -2     
Files with missing lines Coverage Δ
.../Sentry/SentryUIViewControllerPerformanceTracker.m 99.086% <100.000%> (+0.008%) ⬆️
...entryUIViewControllerPerformanceTrackerTests.swift 98.404% <100.000%> (+0.179%) ⬆️

... and 6 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 887502e...518a25b. Read the comment docs.

Copy link

github-actions bot commented Oct 9, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1219.51 ms 1226.81 ms 7.30 ms
Size 21.58 KiB 704.38 KiB 682.80 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
7cd187e 1239.39 ms 1258.02 ms 18.63 ms
5e66a38 1209.10 ms 1233.90 ms 24.79 ms
d760c3f 1200.95 ms 1233.96 ms 33.00 ms
324dc7b 1210.33 ms 1244.92 ms 34.59 ms
62c15d4 1231.80 ms 1248.86 ms 17.06 ms
9faf217 1268.86 ms 1274.82 ms 5.96 ms
39b1c35 1236.35 ms 1239.90 ms 3.55 ms
5d6ce0e 1237.10 ms 1257.46 ms 20.36 ms
5c115c5 1249.04 ms 1260.36 ms 11.32 ms
50bb751 1230.94 ms 1238.69 ms 7.75 ms

App size

Revision Plain With Sentry Diff
7cd187e 20.76 KiB 401.65 KiB 380.89 KiB
5e66a38 22.85 KiB 408.88 KiB 386.03 KiB
d760c3f 22.84 KiB 403.17 KiB 380.33 KiB
324dc7b 22.85 KiB 408.84 KiB 385.99 KiB
62c15d4 22.85 KiB 411.14 KiB 388.29 KiB
9faf217 20.76 KiB 419.70 KiB 398.94 KiB
39b1c35 22.85 KiB 408.88 KiB 386.03 KiB
5d6ce0e 22.85 KiB 405.38 KiB 382.53 KiB
5c115c5 22.85 KiB 411.72 KiB 388.87 KiB
50bb751 21.58 KiB 417.86 KiB 396.27 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 have one suggestion, but it looks good to me

@philipphofmann philipphofmann merged commit 3bf3c92 into main Oct 11, 2024
66 checks passed
@philipphofmann philipphofmann deleted the fix/ttid-when-view-will-appear-skipped branch October 11, 2024 08:13
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.

TTID is wrong when viewWillAppear isn't called
2 participants