Skip to content

Comments

[Starfish] Attach app-start spans#3057

Merged
markushi merged 20 commits intomainfrom
feat/app-start-spans
Dec 13, 2023
Merged

[Starfish] Attach app-start spans#3057
markushi merged 20 commits intomainfrom
feat/app-start-spans

Conversation

@markushi
Copy link
Member

@markushi markushi commented Nov 21, 2023 •
edited
Loading

📜 Description

image

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 the AppStartMetrics singleton (formerly AppStartSate).

The following spans are collected

Pre-starfish the app start timestamp was using a static init field in SentryPerformanceProvider or fallback to the time Sentry.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

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

@github-actions
Copy link
Contributor

github-actions bot commented Nov 21, 2023 •
edited
Loading

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 064cae6

@markushi
Copy link
Member Author

markushi commented Nov 22, 2023

@github-actions
Copy link
Contributor

github-actions bot commented Nov 24, 2023 •
edited
Loading

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 298.54 ms 337.42 ms 38.88 ms
Size 1.72 MiB 2.27 MiB 556.93 KiB

Baseline results on branch: main

Startup times

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

philipphofmann
philipphofmann reviewed Nov 24, 2023
View reviewed changes
Copy link
Member

@philipphofmann philipphofmann left a 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/SentryAndroid.java Outdated
Comment on lines 129 to 130
if (appStartTimeSpan.hasNotStarted()
&& android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.N) {
Copy link
Member

@philipphofmann philipphofmann Nov 24, 2023

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.

Copy link
Member Author

@markushi markushi Nov 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As of Oct 2023, API Level 24+ is available on 96% of all devices, so I think it's fine.
image

Copy link
Member

@philipphofmann philipphofmann Nov 24, 2023

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.

Copy link
Contributor

@kahest kahest Nov 24, 2023

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.

Copy link
Member Author

@markushi markushi Nov 27, 2023 •
edited
Loading

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.

Copy link
Member

@philipphofmann philipphofmann Nov 27, 2023

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.

Copy link
Contributor

@stefanosiano stefanosiano Nov 28, 2023

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

@markushi
Copy link
Member Author

markushi commented Nov 24, 2023

If I got it right, you use bytecode manipulation to hook into ContentProvider.onCreate() and Application.onCreate() via our Gradle plugin?

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.

@kahest kahest linked an issue Nov 27, 2023 that may be closed by this pull request
Track app start data #3035
Closed
romtsn
romtsn reviewed Dec 5, 2023
View reviewed changes
...android-core/src/main/java/io/sentry/android/core/performance/ActivityLifecycleTimeSpan.java
public class ActivityLifecycleTimeSpan implements Comparable<ActivityLifecycleTimeSpan> {
private final @NotNull TimeSpan onCreate = new TimeSpan();
private final @NotNull TimeSpan onStart = new TimeSpan();

Copy link
Member

@romtsn romtsn Dec 5, 2023

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?

Copy link
Member Author

@markushi markushi Dec 5, 2023

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

romtsn
romtsn reviewed Dec 5, 2023
View reviewed changes
sentry-android-core/src/main/java/io/sentry/android/core/PerformanceAndroidEventProcessor.java
span.getDescription(),
SpanStatus.OK,
APP_METRICS_ORIGN,
new HashMap<>(),
Copy link
Member

@romtsn romtsn Dec 5, 2023 •
edited
Loading

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.

romtsn
romtsn reviewed Dec 5, 2023
View reviewed changes
sentry-android-core/src/main/java/io/sentry/android/core/PerformanceAndroidEventProcessor.java
parentSpanId,
ActivityLifecycleIntegration.UI_LOAD_OP,
span.getDescription(),
SpanStatus.OK,
Copy link
Member

@romtsn romtsn Dec 5, 2023

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

romtsn
romtsn approved these changes Dec 5, 2023
View reviewed changes
Copy link
Member

@romtsn romtsn left a 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 🚀

@markushi markushi merged commit 7eccfdb into main Dec 13, 2023
@markushi markushi deleted the feat/app-start-spans branch December 13, 2023 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@philipphofmann philipphofmann philipphofmann left review comments

@romtsn romtsn romtsn approved these changes

@adinauer adinauer Awaiting requested review from adinauer adinauer is a code owner

+2 more reviewers

@kahest kahest kahest left review comments

@stefanosiano stefanosiano stefanosiano approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Track app start data Report app start more granularly

5 participants

@markushi @kahest @philipphofmann @romtsn @stefanosiano