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
Add ttid span to ActivityLifecycleIntegration #2369
Conversation
|
…tyStarted(), not onActivityCreated() anymore
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
81a1a6c | 328.73 ms | 421.28 ms | 92.55 ms |
ecf9680 | 326.55 ms | 360.26 ms | 33.71 ms |
ecf9680 | 303.12 ms | 346.35 ms | 43.23 ms |
38e4f11 | 358.20 ms | 433.73 ms | 75.53 ms |
703d523 | 330.27 ms | 377.66 ms | 47.39 ms |
81a1a6c | 294.04 ms | 341.19 ms | 47.15 ms |
ecf9680 | 321.55 ms | 385.52 ms | 63.97 ms |
4a9c176 | 320.62 ms | 334.68 ms | 14.06 ms |
3695453 | 301.78 ms | 371.14 ms | 69.36 ms |
b85d8aa | 289.35 ms | 335.92 ms | 46.56 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
81a1a6c | 1.73 MiB | 2.32 MiB | 612.47 KiB |
ecf9680 | 1.73 MiB | 2.32 MiB | 612.39 KiB |
ecf9680 | 1.73 MiB | 2.32 MiB | 612.39 KiB |
38e4f11 | 1.73 MiB | 2.32 MiB | 609.82 KiB |
703d523 | 1.73 MiB | 2.33 MiB | 613.23 KiB |
81a1a6c | 1.73 MiB | 2.32 MiB | 612.47 KiB |
ecf9680 | 1.73 MiB | 2.32 MiB | 612.39 KiB |
4a9c176 | 1.73 MiB | 2.33 MiB | 612.69 KiB |
3695453 | 1.73 MiB | 2.32 MiB | 611.62 KiB |
b85d8aa | 1.73 MiB | 2.32 MiB | 611.62 KiB |
Previous results on branch: feat/android-ttid
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
8d63d7f | 327.04 ms | 366.00 ms | 38.96 ms |
8aeeb7f | 348.82 ms | 398.40 ms | 49.58 ms |
d125af1 | 325.12 ms | 361.04 ms | 35.92 ms |
2c9607d | 328.42 ms | 398.77 ms | 70.35 ms |
89f6386 | 279.98 ms | 356.45 ms | 76.47 ms |
15034e6 | 346.20 ms | 376.02 ms | 29.82 ms |
abb2f2e | 346.82 ms | 395.04 ms | 48.22 ms |
0e7deaa | 353.50 ms | 386.40 ms | 32.90 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
8d63d7f | 1.73 MiB | 2.33 MiB | 613.28 KiB |
8aeeb7f | 1.73 MiB | 2.33 MiB | 614.04 KiB |
d125af1 | 1.73 MiB | 2.33 MiB | 614.08 KiB |
2c9607d | 1.73 MiB | 2.33 MiB | 612.93 KiB |
89f6386 | 1.73 MiB | 2.33 MiB | 614.05 KiB |
15034e6 | 1.73 MiB | 2.32 MiB | 609.87 KiB |
abb2f2e | 1.73 MiB | 2.32 MiB | 609.87 KiB |
0e7deaa | 1.73 MiB | 2.32 MiB | 609.87 KiB |
Codecov ReportBase: 80.21% // Head: 80.21% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## main #2369 +/- ##
=========================================
Coverage 80.21% 80.21%
Complexity 3772 3772
=========================================
Files 302 302
Lines 14221 14221
Branches 1885 1885
=========================================
Hits 11408 11408
Misses 2072 2072
Partials 741 741 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java
Outdated
Show resolved
Hide resolved
I've seen Firebase is taking a slightly different approach using |
I'll try to implement it in the same way just to compare the values and see, thanks for posting it |
Firebase implementation logs a lot of things, but the approach on using |
…pan per activity updated ttidSpan finishing with Firebase's method: span is finished after the very first frame has been drawn using view.getViewTreeObserver().addOnDrawListener added FirstDrawDoneListener Sentry.getSpan() could cause issues
...y-android-core/src/main/java/io/sentry/android/core/internal/util/FirstDrawDoneListener.java
Show resolved
Hide resolved
@@ -206,7 +205,7 @@ protected void onResume() { | |||
final ISpan span = Sentry.getSpan(); | |||
if (span != null) { | |||
span.setMeasurement("screen_load_count", screenLoadCount, new MeasurementUnit.Custom("test")); | |||
span.finish(SpanStatus.OK); | |||
// span.finish(SpanStatus.OK); |
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.
@romtsn I found that using Sentry.getSpan()
and then span.finish()
is finishing the ttid span
Is it something to be worried about? Would a user be already using something like this?
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.
yes, it's possible that the users already do this to finish the activity transaction whenever they think it's suitable for them.
Once we implement #1828, this should not matter anymore, but until then, we should probably mention this in the docs + migration docs, that calling span.finish in onResume will change how ttid span is measured.
sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java
Outdated
Show resolved
Hide resolved
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.
Looks good to me overall, great job 👍 One more thing I'd like to ask you - could you enable auto-finish for activity transactions and test if it works fine with the ttid spans? I think it shouldn't change anything but just for the sake of correctness.
Actually after looking at this one more time, I think it will affect the ttid span, because currently we're finishing it as cancelled inside the Perhaps we should only finish the ttid span as cancelled in |
Yep, auto-finish stops the ttid. Let's finish the span only in What about the |
yeah I think it makes sense, otherwise it will run indefinitely right? if we switched the screens in the meantime |
It would end when the previous activity is destroyed, but if an activity is started without destroying the previous one, then it would run indefinitely, yeah |
…tarts, not anymore if auto-finish option is enabled
📜 Description
Added ttid (time to initial display) span to ActivityLifecycleIntegration
💡 Motivation and Context
We want to add the ttid info to the transactions, so we are adding a span to the automatic activity tracking transactions
💚 How did you test it?
Added unit test to check ttid span is created and cancelled in case activity finishes early
📝 Checklist
🔮 Next steps