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

feat(app-start): Add Android app start-related spans #2927

Merged
merged 8 commits into from
Jan 11, 2024

Conversation

narsaynorath
Copy link
Member

These spans are nested operations under the app.start.* spans on Android. Start collecting them for the Sentry product so we have some data before moving into the default config.

Note: This only adds the application portion of the spans. We are currently awaiting a new span that tracks the system portion of the app start and I will add that as another span op afterwards.

These spans are listed out here for further information:

These spans are nested operations under the app.start.* spans on
Android. Start collecting them for the Sentry product so we have some
data before moving into the default config.
@narsaynorath narsaynorath requested a review from jjbayer January 9, 2024 20:36
@narsaynorath narsaynorath requested a review from a team as a code owner January 9, 2024 20:36
@@ -82,6 +82,26 @@ pub(crate) fn scrub_span_description(span: &Span) -> Option<String> {
// They are low-cardinality.
Some(description.to_owned())
}
("contentprovider", "load") => {
Copy link
Member Author

@narsaynorath narsaynorath Jan 9, 2024

Choose a reason for hiding this comment

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

@jjbayer You pointed me to this file initially, is this the only change I need to make to have these new span ops ingested internally?

Copy link
Member

Choose a reason for hiding this comment

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

You'll also want to extend the MOBILE_OPS constant in defaults.rs:

/// A list of `span.op` patterns we want to enable for mobile.
const MOBILE_OPS: &[&str] = &["app.*", "ui.load*"];

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, I was thinking about doing that but I was worried adding the entry to default.rs would enable it incorrectly for all orgs. I've added them now and also regenerated some snapshots with these new ops

Copy link
Member

Choose a reason for hiding this comment

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

That is a good point! I just merged #2924, so the Feature::SpanMetricsExtractionAllModules flag and only extend the mobile ops conditionally.

@jjbayer jjbayer self-assigned this Jan 10, 2024
@narsaynorath
Copy link
Member Author

@jjbayer Just a heads up I've addressed your comment in this PR and implemented the change we discussed to update the mobile ops conditionally. Totally open to seeing alternative implementations if you have suggestions on how to do this part better, but I found a way to do it in a single block 👍

@jjbayer
Copy link
Member

jjbayer commented Jan 11, 2024

@jjbayer Just a heads up I've addressed your comment in this PR and implemented the change we discussed to update the mobile ops conditionally. Totally open to seeing alternative implementations if you have suggestions on how to do this part better, but I found a way to do it in a single block 👍

@narsaynorath looks good to me. There's definitely a way to do it with iterator chaining, but IMO not worth the effort and it would not increase readability.

@narsaynorath narsaynorath enabled auto-merge (squash) January 11, 2024 18:45
@narsaynorath narsaynorath merged commit 118cafc into master Jan 11, 2024
20 checks passed
@narsaynorath narsaynorath deleted the nar/feat/spans-collect-more-app-start-spans branch January 11, 2024 19:05
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.

2 participants