-
-
Notifications
You must be signed in to change notification settings - Fork 465
fix(android): Improve app start type detection with main thread timing #4999
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
base: main
Are you sure you want to change the base?
Conversation
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| f634d01 | 359.58 ms | 433.88 ms | 74.30 ms |
| 7314dbe | 437.83 ms | 505.64 ms | 67.81 ms |
| fcec2f2 | 328.91 ms | 387.75 ms | 58.84 ms |
| ee747ae | 382.73 ms | 435.41 ms | 52.68 ms |
| d15471f | 343.13 ms | 361.47 ms | 18.34 ms |
| ee747ae | 358.21 ms | 389.41 ms | 31.20 ms |
| bbc35bb | 298.53 ms | 372.17 ms | 73.64 ms |
| fc5ccaf | 322.49 ms | 405.25 ms | 82.76 ms |
| bbc35bb | 324.88 ms | 425.73 ms | 100.85 ms |
| d15471f | 379.40 ms | 470.76 ms | 91.36 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| f634d01 | 1.58 MiB | 2.10 MiB | 533.40 KiB |
| 7314dbe | 1.58 MiB | 2.10 MiB | 533.45 KiB |
| fcec2f2 | 1.58 MiB | 2.12 MiB | 551.50 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| d15471f | 1.58 MiB | 2.13 MiB | 559.54 KiB |
| ee747ae | 1.58 MiB | 2.10 MiB | 530.95 KiB |
| bbc35bb | 1.58 MiB | 2.12 MiB | 553.01 KiB |
| fc5ccaf | 1.58 MiB | 2.13 MiB | 557.54 KiB |
| bbc35bb | 1.58 MiB | 2.12 MiB | 553.01 KiB |
| d15471f | 1.58 MiB | 2.13 MiB | 559.54 KiB |
|
@sentry review |
sentry-android-core/src/main/java/io/sentry/android/core/performance/AppStartMetrics.java
Show resolved
Hide resolved
As firstPost doesn't work on Android 34+ devices
|
@sentry review |
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| // as the next onActivityCreated will treat it as a new warm app start | ||
| if (remainingActivities == 0 && !activity.isChangingConfigurations()) { | ||
| appLaunchedInForeground = false; | ||
| appLaunchedInForeground.setValue(true); |
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.
Incorrect boolean value in onActivityDestroyed sets wrong foreground state
Medium Severity
In onActivityDestroyed(), appLaunchedInForeground.setValue(true) is set when the app moves to background, but the original code set it to false. The comment explicitly states "if the app is moving into background", yet the value is inverted. This causes warm starts to potentially be misclassified as cold starts when a user returns from background very quickly (within the same millisecond as firstIdle), because the !appLaunchedInForeground.getValue() check that previously caught this case now fails, and the firstIdle timing check uses strict > comparison which can miss edge cases.
📜 Description
Improves app start type detection by utilizing the main thread idle callback.
Fixes #4920
The
firstPostapproach as utilized in the papa library does not seem to work on API level 35+ anymore (See square/papa#63)The changes add:
firstIdlefield to track when the main thread becomes readyonActivityCreated()to detect warm starts when activity creation happens after the first main thread idle📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps