-
-
Notifications
You must be signed in to change notification settings - Fork 461
Fix Frame measurements in app start transactions #3382
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
…ransactions get frames measurements) TimeSpan.getStartTimestamp is now a SentryNanotimeDate, to fix FrameMetricsCollector on app start transactions
sentry-android-core/src/main/java/io/sentry/android/core/SpanFrameMetricsCollector.java
Show resolved
Hide resolved
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 4e260b3 | 378.73 ms | 454.18 ms | 75.45 ms |
| 4bf95dd | 345.96 ms | 414.24 ms | 68.28 ms |
| 0bd723b | 395.44 ms | 472.66 ms | 77.22 ms |
| 7275aa8 | 422.39 ms | 456.94 ms | 34.55 ms |
| baaf637 | 375.71 ms | 441.58 ms | 65.87 ms |
| 7eccfdb | 366.98 ms | 440.27 ms | 73.29 ms |
| 937879e | 417.64 ms | 550.45 ms | 132.81 ms |
| 9119d59 | 407.12 ms | 509.64 ms | 102.52 ms |
| 93a76ca | 378.48 ms | 451.78 ms | 73.30 ms |
| 93a76ca | 377.96 ms | 447.52 ms | 69.56 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 4e260b3 | 1.72 MiB | 2.27 MiB | 554.95 KiB |
| 4bf95dd | 1.72 MiB | 2.29 MiB | 576.40 KiB |
| 0bd723b | 1.72 MiB | 2.29 MiB | 578.09 KiB |
| 7275aa8 | 1.70 MiB | 2.28 MiB | 590.71 KiB |
| baaf637 | 1.72 MiB | 2.27 MiB | 558.42 KiB |
| 7eccfdb | 1.72 MiB | 2.27 MiB | 556.93 KiB |
| 937879e | 1.72 MiB | 2.27 MiB | 558.42 KiB |
| 9119d59 | 1.70 MiB | 2.27 MiB | 583.84 KiB |
| 93a76ca | 1.72 MiB | 2.29 MiB | 576.75 KiB |
| 93a76ca | 1.72 MiB | 2.29 MiB | 576.75 KiB |
Previous results on branch: fix/frame-measurements-app-start
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 3519885 | 398.45 ms | 470.06 ms | 71.62 ms |
| dc8d117 | 426.29 ms | 490.19 ms | 63.90 ms |
| 0d9d1da | 536.21 ms | 601.06 ms | 64.85 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 3519885 | 1.70 MiB | 2.28 MiB | 592.14 KiB |
| dc8d117 | 1.70 MiB | 2.28 MiB | 592.12 KiB |
| 0d9d1da | 1.70 MiB | 2.28 MiB | 592.15 KiB |
markushi
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.
Looking good, that's some nice fixes! Left on comment which needs clarification around TimeSpan.getStartTimestamp(), once that is sorted out, we're good to go.
| ttfdSpan.updateEndDate(endDate); | ||
| // If the ttfd span was finished before the first frame we adjust the measurement, too | ||
| ttidSpan.setMeasurement( | ||
| ttfdSpan.setMeasurement( |
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.
well spotted!
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.
I had to revert it, as it's not a simple enough change (the ttfd span is already finished and we can't set measurements). Created an issue for this
| if (hasStarted()) { | ||
| return new SentryLongDate(DateUtils.millisToNanos(getStartTimestampMs())); | ||
| return new SentryNanotimeDate( | ||
| DateUtils.nanosToDate(DateUtils.millisToNanos(getStartTimestampMs())), System.nanoTime()); |
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.
m: since this is just a getter, it will return a different SentryDate every time it gets invoked. Would it make more sense to just provide 0 for the nanos?
| DateUtils.nanosToDate(DateUtils.millisToNanos(getStartTimestampMs())), System.nanoTime()); | |
| DateUtils.nanosToDate(DateUtils.millisToNanos(getStartTimestampMs())), 0); |
If we need nano precision, we could store the System.nanoTime() within the start() { } block and then re-use it here later.
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.
we need nano precision, as in the FrameMetricsCollector we call date.diff, which uses that value and compares it to the value of other dates, which are all System.nanotime().
I'm setting it during start()
📜 Description
performance collectors are called independently from profiling (now transactions get frames measurements)
TimeSpan.getStartTimestampis now aSentryNanotimeDate, to fixFrameMetricsCollectoron app start transactions💡 Motivation and Context
FrameMetricsCollectorchecks the start and end timestamp with a difference to a common date. The issue is that using aSentryLongDateand aSentryNanotimeDategives different results. We always useSentryNanotimeDateon Android, except for app start timestamps, which are then propagated to app start transaction, TTID and TTFD.Also,
SentryTracerwas not added to thePerformaceCollectorswhen profiling was disabled, due to previous constraints💚 How did you test it?
Added UI test
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps