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

Add span skip attribute for span metrics generation #1626

Merged
merged 4 commits into from
Feb 11, 2025

Conversation

grcevski
Copy link
Contributor

@grcevski grcevski commented Feb 7, 2025

Beyla can directly generate span metrics for quick access to unified metrics and service graphs without the need for trace generation. This mainly done to ensure we can generate accurate metrics even in case of trace sampling with head sampling. Tail sampling doesn't need this option, however it does usually require higher compute cost.

In case we generate span metrics directly from Beyla and we also generate traces, some data will be duplicated if the collector has also enabled the span metrics processor. To help with filtering double counted metrics, this PR introduces a new attribute which will get attached to the spans, signalling that the span metrics are already generated and they can be ignored by the span metrics processor. The span metrics processor can check for this flag and drop the trace sample from metrics.

The new attribute is span.metrics.skip = true.

@grcevski grcevski requested a review from a team as a code owner February 7, 2025 21:01
Copy link

codecov bot commented Feb 7, 2025

Codecov Report

Attention: Patch coverage is 84.21053% with 6 lines in your changes missing coverage. Please review.

Project coverage is 71.18%. Comparing base (dd004f0) to head (d40d994).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
pkg/export/alloy/traces.go 72.72% 2 Missing and 1 partial ⚠️
pkg/export/otel/traces.go 88.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1626      +/-   ##
==========================================
- Coverage   71.18%   71.18%   -0.01%     
==========================================
  Files         197      197              
  Lines       19881    19911      +30     
==========================================
+ Hits        14153    14174      +21     
- Misses       5044     5059      +15     
+ Partials      684      678       -6     
Flag Coverage Δ
integration-test 53.00% <65.78%> (+0.44%) ⬆️
k8s-integration-test 53.90% <65.78%> (-0.04%) ⬇️
oats-test 34.65% <42.10%> (+0.19%) ⬆️
unittests 47.07% <76.31%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@mariomac mariomac left a comment

Choose a reason for hiding this comment

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

LGTM! Just a small comment.

Comment on lines 63 to 66
if tr.spanMetricsEnabled {
traceAttrs[attr.SkipSpanMetrics] = struct{}{}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks duplicate (already inside getConstantAttributes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah nice catch, I moved it there so I can make the test and forgot to take out out.

@grcevski grcevski merged commit 72a37e5 into grafana:main Feb 11, 2025
13 checks passed
@grcevski grcevski deleted the add_span_skip_attribute branch February 11, 2025 01:16
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