-
-
Notifications
You must be signed in to change notification settings - Fork 435
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
[Starfish] Attach app-start spans #3057
Conversation
|
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
d6d2b2e | 413.20 ms | 486.76 ms | 73.56 ms |
0bd723b | 412.52 ms | 496.65 ms | 84.13 ms |
c7e2fbc | 393.98 ms | 478.24 ms | 84.27 ms |
b172d4e | 352.92 ms | 446.50 ms | 93.58 ms |
4bf95dd | 345.96 ms | 414.24 ms | 68.28 ms |
4e260b3 | 388.40 ms | 468.98 ms | 80.58 ms |
93a76ca | 377.96 ms | 447.52 ms | 69.56 ms |
bc4be3b | 360.40 ms | 435.04 ms | 74.64 ms |
283d83e | 348.44 ms | 392.06 ms | 43.62 ms |
93a76ca | 397.30 ms | 455.16 ms | 57.87 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
d6d2b2e | 1.72 MiB | 2.27 MiB | 555.05 KiB |
0bd723b | 1.72 MiB | 2.29 MiB | 578.09 KiB |
c7e2fbc | 1.72 MiB | 2.29 MiB | 576.40 KiB |
b172d4e | 1.72 MiB | 2.29 MiB | 578.09 KiB |
4bf95dd | 1.72 MiB | 2.29 MiB | 576.40 KiB |
4e260b3 | 1.72 MiB | 2.27 MiB | 554.95 KiB |
93a76ca | 1.72 MiB | 2.29 MiB | 576.75 KiB |
bc4be3b | 1.72 MiB | 2.29 MiB | 576.53 KiB |
283d83e | 1.72 MiB | 2.29 MiB | 577.69 KiB |
93a76ca | 1.72 MiB | 2.29 MiB | 576.75 KiB |
Previous results on branch: feat/app-start-spans
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
6cd4f31 | 389.94 ms | 445.81 ms | 55.87 ms |
54db3ef | 384.88 ms | 452.18 ms | 67.30 ms |
0957329 | 332.91 ms | 369.15 ms | 36.23 ms |
5f1218e | 408.20 ms | 466.26 ms | 58.06 ms |
3751e60 | 414.69 ms | 454.45 ms | 39.76 ms |
3fd1e60 | 390.54 ms | 446.90 ms | 56.36 ms |
ba88e07 | 393.18 ms | 455.88 ms | 62.69 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
6cd4f31 | 1.72 MiB | 2.27 MiB | 557.15 KiB |
54db3ef | 1.72 MiB | 2.27 MiB | 556.94 KiB |
0957329 | 1.72 MiB | 2.27 MiB | 556.93 KiB |
5f1218e | 1.72 MiB | 2.29 MiB | 580.44 KiB |
3751e60 | 1.72 MiB | 2.27 MiB | 557.10 KiB |
3fd1e60 | 1.72 MiB | 2.27 MiB | 557.21 KiB |
ba88e07 | 1.72 MiB | 2.27 MiB | 556.84 KiB |
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 I got it right, you use bytecode manipulation to hook into ContentProvider.onCreate()
and Application.onCreate()
via our Gradle plugin?
I'm considering doing something similar for iOS. We could hook into similar hooks by swizzling before the SDK init, only if you choose so on a previous app launch. Andrew is doing something like that to make profiling work for app starts.
I added a few comments, but I only looked at the high level to understand what you are trying to achieve.
sentry-android-core/src/main/java/io/sentry/android/core/performance/AppStartMetrics.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroidOptions.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroid.java
Outdated
Show resolved
Hide resolved
if (appStartTimeSpan.hasNotStarted() | ||
&& android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.N) { |
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.
h
: What about using the legacy as a fallback for android.os.Build.VERSION.SDK_INT < android.os.Build.VERSION_CODES.N
Android API (24+) seems a bit high to me.
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.
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.
Not supporting 4% is still a significant number, in my opinion. Considering Compatibility is king, I think we should support older versions with something useful if it's not too much effort.
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 at our own data, API levels <24 make up <1% of events coming in, which IMO lowers the threshold for reasonable effort more. But if we can keep using existing code for a while and remove it sometime later, that could make sense.
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'd love to support more API levels, the problem I see is when we start aggregating the differently measured metrics on the backend. I'll think about this and do a quick measurement how different the old / new way is.
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 at our own data, API levels <24 make up <1% of events coming in, which IMO lowers the threshold for reasonable effort more.
I agree. If it's easily possible, let's do it. If not, please add a comment somewhere explaining why decided against it. On Cocoa, we have a decision log in the repo to document such decisions, for example.
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.
you can even put the check in the isEnableStarfish
, and if it's API <24 return false
Yes, exactly! This should already give some really good insights into the app code itself as well as any third party libs performing work on app start. I tried to keep the bytecode manipulation part as simple as possible, it just performs 2 static calls on method enter / exit. |
sentry-android-core/src/main/java/io/sentry/android/core/PerformanceAndroidEventProcessor.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/PerformanceAndroidEventProcessor.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/PerformanceAndroidEventProcessor.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroid.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/SentryPerformanceProvider.java
Outdated
Show resolved
Hide resolved
...droid-core/src/main/java/io/sentry/android/core/internal/gestures/WindowCallbackAdapter.java
Show resolved
Hide resolved
...android-core/src/main/java/io/sentry/android/core/performance/ActivityLifecycleTimeSpan.java
Outdated
Show resolved
Hide resolved
...android-core/src/main/java/io/sentry/android/core/performance/ActivityLifecycleTimeSpan.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/performance/AppStartMetrics.java
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/performance/TimeSpan.java
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/performance/TimeSpan.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/performance/TimeSpan.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/performance/TimeSpan.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/performance/TimeSpan.java
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroidOptions.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/SentryPerformanceProvider.java
Outdated
Show resolved
Hide resolved
...android-core/src/main/java/io/sentry/android/core/performance/ActivityLifecycleTimeSpan.java
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/performance/AppStartMetrics.java
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/performance/TimeSpan.java
Outdated
Show resolved
Hide resolved
public class ActivityLifecycleTimeSpan implements Comparable<ActivityLifecycleTimeSpan> { | ||
private final @NotNull TimeSpan onCreate = new TimeSpan(); | ||
private final @NotNull TimeSpan onStart = new TimeSpan(); | ||
|
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'd be curious why don't we also have onResume
span here? I think it's also happening before the first draw is done, or not always?
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, we could add this one as well 👍, let me follow up with an improvement PR here
sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/DefaultAndroidEventProcessor.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/PerformanceAndroidEventProcessor.java
Outdated
Show resolved
Hide resolved
span.getDescription(), | ||
SpanStatus.OK, | ||
APP_METRICS_ORIGN, | ||
new HashMap<>(), |
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 guess it'd be cool to provide our users a way to attach custom tags or data for these spans, especially if they will be searchable in the future. But probably food for thought for the future (maybe could bring it up to the next starfish sync). Just thinking of a case where people would like to e.g. add custom tags to onCreate/onStart spans and then aggregate them by these tags (like a feature flag value).
They could do it in beforeSend of course, but maybe not always, e.g. some data is only available at time of execution, and if they could access current TimeSpan to attach that data to, that'd be cool I guess.
parentSpanId, | ||
ActivityLifecycleIntegration.UI_LOAD_OP, | ||
span.getDescription(), | ||
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.
Also, as discussed privately, we could change our auto-instr in a way that it wraps the instrumented methods into try-catch and connects throwables to spans, then we could set the SpanStatus accordingly. And also, if we plan to surface something in the UI, based on the span <-> error connection
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! most things are just food for thoughts or future feature requests, only a couple that need to be addressed rather sooner I guess. massive work 🚀
...ry-android-core/src/test/java/io/sentry/android/core/PerformanceAndroidEventProcessorTest.kt
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/InternalSentrySdk.java
Show resolved
Hide resolved
Co-authored-by: Sentry Github Bot <[email protected]>
📜 Description
This PR attaches extra spans collected during early app init to the app start txn.
As these spans are collected when the SDK isn't even initialized, we can't use SentryTracer.
Instead a simplified
TimeSpan
object is used and the data is collected using theAppStartMetrics
singleton (formerly AppStartSate).The following spans are collected
ContentProvider.onCreate()
via Instrument ContentProvider/Application onCreate calls to measure app-start performance sentry-android-gradle-plugin#565Application.onCreate()
via Instrument ContentProvider/Application onCreate calls to measure app-start performance sentry-android-gradle-plugin#565Activity.onCreate() .onStart()
via theSentryPerformanceProvider
Pre-starfish the app start timestamp was using a static init field in
SentryPerformanceProvider
or fallback to the timeSentry.init()
was called. The end timestamp was either.onResume()
or first frame drawn.With Starfish we're only utilizing the new
Process.getStartUptimeMillis()
API (24+), end-timestamp is determined by the first frame drawn (note: there could be multiple "trampoline" activities in-between). This will very likely yield less data, but ultimately be more accurate than what we have right now.💡 Motivation and Context
Give more insights to app start and make it more actionable.
Closes #2177
💚 How did you test it?
Added unit tests.
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps