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: App start ends when first frame is drawn #3530

Merged
merged 3 commits into from Jan 3, 2024

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Jan 3, 2024

When performanceV2 is enabled, the app start ends when the app draws the first frame instead of when it receives the
UIWindowDidBecomeVisibleNotification.

This PR is based on #3525.

📜 Description

💡 Motivation and Context

This the change requested in #3461, but in a non-breaking way behind the feature flag performanceV2.

💚 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.
Copy link

codecov bot commented Jan 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1a4da1b) 89.159% compared to head (794fe78) 89.258%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3530       +/-   ##
=============================================
+ Coverage   89.159%   89.258%   +0.098%     
=============================================
  Files          528       528               
  Lines        57527     57600       +73     
  Branches     20609     20630       +21     
=============================================
+ Hits         51291     51413      +122     
- Misses        5200      5276       +76     
+ Partials      1036       911      -125     
Files Coverage Δ
Sources/Sentry/SentryAppStartTracker.m 97.590% <100.000%> (+1.461%) ⬆️
Sources/Sentry/SentryAppStartTrackingIntegration.m 100.000% <100.000%> (ø)
Sources/Sentry/SentryOptions.m 97.321% <100.000%> (+0.018%) ⬆️
Sources/Sentry/SentryTracer.m 97.159% <100.000%> (ø)
.../AppStartTracking/SentryAppStartTrackerTests.swift 98.481% <100.000%> (+1.205%) ⬆️
...cking/SentryAppStartTrackingIntegrationTests.swift 92.523% <100.000%> (+0.856%) ⬆️
Tests/SentryTests/SentryOptionsTest.m 97.811% <100.000%> (+0.007%) ⬆️
...ts/SentryTests/Transaction/SentryTracerTests.swift 98.874% <100.000%> (+0.015%) ⬆️

... and 39 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 1a4da1b...794fe78. Read the comment docs.

Copy link

github-actions bot commented Jan 3, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1225.35 ms 1241.34 ms 15.99 ms
Size 21.58 KiB 418.60 KiB 397.02 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
ab0012c 1209.06 ms 1228.78 ms 19.72 ms
51307b7 1223.08 ms 1240.76 ms 17.68 ms
15cf6bf 1241.06 ms 1248.18 ms 7.12 ms
3437454 1254.04 ms 1259.50 ms 5.46 ms
25bcc50 1209.53 ms 1224.86 ms 15.33 ms
984eb2d 1220.62 ms 1235.24 ms 14.62 ms
d80d410 1231.50 ms 1240.14 ms 8.64 ms
7192d9e 1221.86 ms 1248.28 ms 26.42 ms
7bb0873 1215.65 ms 1235.00 ms 19.35 ms
e7b566f 1199.57 ms 1218.76 ms 19.19 ms

App size

Revision Plain With Sentry Diff
ab0012c 22.85 KiB 415.09 KiB 392.24 KiB
51307b7 22.85 KiB 407.63 KiB 384.78 KiB
15cf6bf 22.85 KiB 405.38 KiB 382.53 KiB
3437454 22.85 KiB 408.87 KiB 386.02 KiB
25bcc50 20.76 KiB 427.22 KiB 406.46 KiB
984eb2d 20.76 KiB 425.77 KiB 405.01 KiB
d80d410 20.76 KiB 425.71 KiB 404.95 KiB
7192d9e 20.76 KiB 431.71 KiB 410.95 KiB
7bb0873 22.85 KiB 407.09 KiB 384.24 KiB
e7b566f 22.85 KiB 414.80 KiB 391.95 KiB

Previous results on branch: fix/app-start-duration

Startup times

Revision Plain With Sentry Diff
6b310a3 1197.22 ms 1211.82 ms 14.60 ms
2cfa65e 1198.80 ms 1213.63 ms 14.84 ms

App size

Revision Plain With Sentry Diff
6b310a3 21.58 KiB 418.67 KiB 397.08 KiB
2cfa65e 21.58 KiB 418.67 KiB 397.08 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

Just wondering what's the relevance of this now that it aligns with TTID, both info will be always the same.

Base automatically changed from ref/app-start-measurement-remove-deprecated-init to main January 3, 2024 13:19
@philipphofmann
Copy link
Member Author

Just wondering what's the relevance of this now that it aligns with TTID, both info will be always the same.

Yeah, I had the same thought. I don't think this is necessarily bad, but it's redundant. Maybe we need a different concept for app starts. I will keep this in the back of my head and bring it up in our Mobile Starfish discussions.

@philipphofmann philipphofmann merged commit ce54c6a into main Jan 3, 2024
69 of 70 checks passed
@philipphofmann philipphofmann deleted the fix/app-start-duration branch January 3, 2024 15:37
Copy link

github-actions bot commented Jan 3, 2024

Fails
🚫 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- App start ends when first frame is drawn ([#3530](https://github.com/getsentry/sentry-cocoa/pull/3530))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 794fe78

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants