-
-
Notifications
You must be signed in to change notification settings - Fork 372
Report pre-warmed app starts #1969
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
Conversation
Add __mod_init_func hook which gets called before main. With this info we can add one more app start span.
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you label it "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 347a8e9 | 1243.50 ms | 1265.90 ms | 22.40 ms |
| 8b040e4 | 1234.76 ms | 1244.71 ms | 9.95 ms |
| bdfccaa | 1202.83 ms | 1228.96 ms | 26.13 ms |
| 075911d | 1256.73 ms | 1271.22 ms | 14.49 ms |
| 5025d2e | 1248.52 ms | 1251.72 ms | 3.20 ms |
| 4a0c282 | 1228.60 ms | 1250.74 ms | 22.14 ms |
| d73ebd0 | 1200.83 ms | 1236.10 ms | 35.27 ms |
| c2a9b60 | 1222.10 ms | 1240.62 ms | 18.52 ms |
| 5025d2e | 1245.14 ms | 1268.58 ms | 23.44 ms |
| 6177f2d | 1206.55 ms | 1226.20 ms | 19.65 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 347a8e9 | 20.51 KiB | 333.16 KiB | 312.65 KiB |
| 8b040e4 | 20.50 KiB | 333.54 KiB | 313.04 KiB |
| bdfccaa | 20.50 KiB | 361.80 KiB | 341.29 KiB |
| 075911d | 20.51 KiB | 332.90 KiB | 312.39 KiB |
| 5025d2e | 20.51 KiB | 331.79 KiB | 311.28 KiB |
| 4a0c282 | 20.51 KiB | 333.10 KiB | 312.60 KiB |
| d73ebd0 | 20.50 KiB | 365.18 KiB | 344.68 KiB |
| c2a9b60 | 20.50 KiB | 333.54 KiB | 313.04 KiB |
| 5025d2e | 20.51 KiB | 331.79 KiB | 311.28 KiB |
| 6177f2d | 20.51 KiB | 332.90 KiB | 312.40 KiB |
Previous results on branch: fix/report-prewarmed-app-starts
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 6cf9be6 | 1216.21 ms | 1247.00 ms | 30.79 ms |
| f8f4884 | 1189.00 ms | 1233.60 ms | 44.60 ms |
| 8136d66 | 1239.44 ms | 1266.82 ms | 27.38 ms |
| 55ab513 | 1219.20 ms | 1254.55 ms | 35.35 ms |
| 58f3415 | 1235.48 ms | 1265.36 ms | 29.88 ms |
| 746f90b | 1235.90 ms | 1250.02 ms | 14.12 ms |
| 54bfce4 | 1245.84 ms | 1271.64 ms | 25.80 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 6cf9be6 | 20.75 KiB | 369.27 KiB | 348.52 KiB |
| f8f4884 | 20.75 KiB | 369.18 KiB | 348.42 KiB |
| 8136d66 | 20.75 KiB | 369.18 KiB | 348.43 KiB |
| 55ab513 | 20.75 KiB | 369.28 KiB | 348.52 KiB |
| 58f3415 | 20.75 KiB | 369.27 KiB | 348.52 KiB |
| 746f90b | 20.75 KiB | 369.18 KiB | 348.42 KiB |
| 54bfce4 | 20.75 KiB | 368.59 KiB | 347.84 KiB |
Add docs for the experimental feature enablePreWarmedAppStartTracking, which will be released with getsentry/sentry-cocoa#1969.
armcknight
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| assertAppStartMeasurementNotPutOnTransaction() | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this 500 is meant to test exceeding the maximum launch duration, we could convert that macro to a constant and expose it to this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends on fixture.appStartDuration. Good point, I changed it.
brustolin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Add docs for the experimental feature enablePreWarmedAppStartTracking, which will be released with getsentry/sentry-cocoa#1969. Co-authored-by: Liza Mock <[email protected]> Co-authored-by: Isabel <[email protected]>
📜 Description
Report pre-warmed app starts by dropping the first app start spans if pre-warming paused during these steps.
This approach will shorten the app start duration, but it represents the duration a user has to wait after clicking
the app icon until the app is responsive. We report the app start type in the appContext, so Sentry can make
changes to the UI for prewarmed app starts.
As it's really hard to test this properly, I added an experimental feature flag so we can get feedback first before making it GA. I plan to remove the feature flag in the future if we are sure this works correctly.
Docs PR getsentry/sentry-docs#5762
Normal app start with all spans
Pre-warmed with dropped spans
App Context

💡 Motivation and Context
Fixes GH-1897
💚 How did you test it?
Unit tests and simulators
📝 Checklist
🔮 Next steps