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(spans): Collect exclusive time for all spans #3268

Merged
merged 11 commits into from
Mar 18, 2024

Conversation

phacops
Copy link
Contributor

@phacops phacops commented Mar 14, 2024

We would like to start collecting the exclusive time for all spans while limiting cardinality by only tagging with low cardinality values. This is for @Zylphrex 's work with the metrics explorer.

@phacops phacops requested a review from a team as a code owner March 14, 2024 16:22
@phacops phacops requested a review from jjbayer March 14, 2024 19:08
Copy link
Contributor

@iker-barriocanal iker-barriocanal left a comment

Choose a reason for hiding this comment

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

LGTM.

To validate exclusive_time_condition, could we add a test to ensure we don't emit more than one exclusive time metric for each span?

relay-server/src/metrics_extraction/event.rs Outdated Show resolved Hide resolved
Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

What about exclusive_time_light? Do we need the transaction tag for the metrics explorer? If yes, are there queries that would benefit from using exclusive_time_light (fewer rows to scan)?

relay-dynamic-config/src/defaults.rs Outdated Show resolved Hide resolved
@Zylphrex
Copy link
Member

What about exclusive_time_light? Do we need the transaction tag for the metrics explorer? If yes, are there queries that would benefit from using exclusive_time_light (fewer rows to scan)?

@jjbayer So the metrics explorer wants to expose the ability to surface samples for metrics including this span exclusive time metric. One of the main things we want to be able to support is to query all spans hence the motivation to extract this metric for all spans instead of known module spans.

Now we want the transaction tag for the metrics explorer because it is likely one of the main ways for people to filter and narrow down the set of spans they want. To my understanding, the exclusive time light metric was specifically added as an optimization for the use cases where filtering on a transaction is not needed. There will be some queries that will benefit from using the exclusive time light metric so I will be raising this point to the team but we will still want this exclusive time metric for all spans.

@jjbayer
Copy link
Member

jjbayer commented Mar 18, 2024

There will be some queries that will benefit from using the exclusive time light metric

@Zylphrex alright, feel free to add it later in a follow-up PR!

Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

See open question about duration_condition.

(is_db.clone() | is_resource.clone() | is_mobile.clone() | is_http.clone())
& duration_condition.clone(),
),
condition: None,
Copy link
Member

Choose a reason for hiding this comment

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

Should we still apply duration_condition here? Asking because even span_per_op and spans_per_segment apply the duration condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we'd want to apply the duration condition here. We want to track absolutely all spans here and the condition was mainly created for a mobile specific use case.

@phacops phacops enabled auto-merge (squash) March 18, 2024 21:42
@phacops phacops disabled auto-merge March 18, 2024 21:47
@phacops phacops enabled auto-merge (squash) March 18, 2024 21:54
@phacops phacops merged commit 8081764 into master Mar 18, 2024
20 checks passed
@phacops phacops deleted the pierre/spans-collect-exclusive-time-for-all-spans branch March 18, 2024 22:28
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.

None yet

4 participants