Skip to content
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

start AppStart span when installing activity lifecycle instrumentation #578

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

beekhc
Copy link
Contributor

@beekhc beekhc commented Sep 5, 2024

This fixes issue #560 by calling AppStartupTimer#start in ActivityLifecycleInstrumentation#install.

Since start is idempotent, this will still work if a way to pass in a pre-started timer to the instrumentation is added later.

Added tests based on SlowRenderingInstrumentationTest. To verify the changes:

./gradlew :instrumentation:activity:testDebugUnitTest
./gradlew check

@beekhc beekhc requested a review from a team September 5, 2024 22:54
Copy link

linux-foundation-easycla bot commented Sep 5, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: beekhc / name: Bee Klimt (d7b4015)

Copy link
Contributor

@LikeTheSalad LikeTheSalad left a comment

Choose a reason for hiding this comment

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

Nice! Thank you for taking the time 🙏

@beekhc
Copy link
Contributor Author

beekhc commented Sep 9, 2024

I don't have permission to merge this myself. Did you want to wait for a second reviewer, or do you want to merge it yourself?

Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

I am still wondering if we won't be better served long-term by having the timer started even earlier in the lifecycle, like perhaps as early as the agent can....but for now, it seems like starting it at all is an improvement over not starting it. 😬

@breedx-splk breedx-splk merged commit 87be431 into open-telemetry:main Sep 9, 2024
3 checks passed
@beekhc beekhc deleted the beekhc.startup branch September 10, 2024 03:14
@LikeTheSalad
Copy link
Contributor

I am still wondering if we won't be better served long-term by having the timer started even earlier in the lifecycle, like perhaps as early as the agent can....but for now, it seems like starting it at all is an improvement over not starting it. 😬

I see what you mean. I think this use case might even require the timer to start before the agent is initialized, to be fully accurate. But yeah for now it's better to have it started rather than getting nothing.

@bidetofevil
Copy link
Contributor

If the goal is to start the span as early as possible, you can always back-date the start time to when we know the process started. In API 24, Android added Process.getStartElapsedRealtime() and on 33, they added Process.getStartRequestedElapsedRealtime(). Both will get you pretty accurate times for when the process started without having to read the proc file's timestamp like we had to do in the old days.

@beekhc
Copy link
Contributor Author

beekhc commented Sep 10, 2024

I would be happy to make a follow-up PR trying that.

One possible downside of this approach is you might get people asking "why does my app take longer to launch in API 24 and later?" when in reality, it's just measuring something different on different versions. But maybe that's an edge case not worth worrying about?

@bidetofevil
Copy link
Contributor

A lot of apps have minSdk set to 24+ now, and the ones that don't likely have very low usage on API < 24. Given this, I think it's OK if we document this appropriately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants