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: Use correct rendered frames timestamp #3531

Merged
merged 5 commits into from Jan 4, 2024

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Jan 3, 2024

📜 Description

Use the timestamp of the DisplayLink wrapper when a new frame is reported for TTID/TTFD and the app start. This PR is based on #3530.

💡 Motivation and Context

I noticed that the end timestamps of TTID and the app start initial frame render didn't match properly in #3530.

Previously

The duration of the app start and the TTID don't match.

CleanShot 2024-01-03 at 10 50 04

Fixed version

Now they do.

CleanShot 2024-01-03 at 10 51 03

💚 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

SentryAppStartMeasurement is only plublic for hybrid SDKs since
#2458. Therefore, we can
remove the deprecated init method.
When performanceV2 is enabled, the app start ends when the app draws the
first frame instead of when it receives the
UIWindowDidBecomeVisibleNotification.
Use the timestamp of the DisplayLink wrapper when a new frame is
reported for TTID/TTFD and the app start.
Copy link

codecov bot commented Jan 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ce54c6a) 89.284% compared to head (79301a8) 89.237%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3531       +/-   ##
=============================================
- Coverage   89.284%   89.237%   -0.048%     
=============================================
  Files          528       528               
  Lines        57655     57634       -21     
  Branches     20657     20649        -8     
=============================================
- Hits         51477     51431       -46     
- Misses        5155      5287      +132     
+ Partials      1023       916      -107     
Files Coverage Δ
SentryTestUtils/TestCurrentDateProvider.swift 82.500% <100.000%> (+0.448%) ⬆️
Sources/Sentry/SentryAppStartTracker.m 97.604% <100.000%> (+0.014%) ⬆️
Sources/Sentry/SentryFramesTracker.m 100.000% <100.000%> (ø)
Sources/Sentry/SentryTimeToDisplayTracker.m 100.000% <100.000%> (ø)
.../AppStartTracking/SentryAppStartTrackerTests.swift 98.492% <100.000%> (+0.011%) ⬆️
...ance/FramesTracking/SentryFramesTrackerTests.swift 99.099% <100.000%> (+0.016%) ⬆️

... and 44 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 ce54c6a...79301a8. Read the comment docs.

Copy link

github-actions bot commented Jan 3, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1199.94 ms 1217.04 ms 17.10 ms
Size 21.58 KiB 418.70 KiB 397.12 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
3437454 1225.64 ms 1234.31 ms 8.67 ms
25bcc50 1233.47 ms 1234.27 ms 0.80 ms
7bb0873 1360.94 ms 1362.24 ms 1.30 ms
e7b566f 1197.49 ms 1229.46 ms 31.97 ms
7bc3c0d 1222.86 ms 1244.90 ms 22.04 ms
326b7eb 1252.86 ms 1259.56 ms 6.70 ms
c78683b 1246.71 ms 1258.70 ms 11.99 ms
5616e0a 1224.12 ms 1249.86 ms 25.74 ms
f715499 1234.26 ms 1259.40 ms 25.14 ms
216bdf9 1193.69 ms 1217.90 ms 24.21 ms

App size

Revision Plain With Sentry Diff
3437454 22.85 KiB 408.87 KiB 386.02 KiB
25bcc50 20.76 KiB 427.22 KiB 406.46 KiB
7bb0873 22.85 KiB 407.09 KiB 384.24 KiB
e7b566f 22.85 KiB 414.80 KiB 391.95 KiB
7bc3c0d 20.76 KiB 427.36 KiB 406.59 KiB
326b7eb 20.76 KiB 432.31 KiB 411.55 KiB
c78683b 20.76 KiB 435.24 KiB 414.48 KiB
5616e0a 22.85 KiB 407.44 KiB 384.59 KiB
f715499 20.76 KiB 427.23 KiB 406.47 KiB
216bdf9 21.58 KiB 418.13 KiB 396.54 KiB

Previous results on branch: fix/rendered-frame-timestamp

Startup times

Revision Plain With Sentry Diff
e5e7fb4 1215.28 ms 1229.82 ms 14.55 ms
e669be6 1225.63 ms 1252.06 ms 26.43 ms

App size

Revision Plain With Sentry Diff
e5e7fb4 21.58 KiB 418.76 KiB 397.17 KiB
e669be6 21.58 KiB 418.70 KiB 397.12 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.

LGTM

Base automatically changed from fix/app-start-duration to main January 3, 2024 15:37
@philipphofmann philipphofmann merged commit f74904f into main Jan 4, 2024
68 of 70 checks passed
@philipphofmann philipphofmann deleted the fix/rendered-frame-timestamp branch January 4, 2024 08:29
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.

None yet

2 participants