-
Notifications
You must be signed in to change notification settings - Fork 312
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
Update Otel Invalid Telemetry Calculation #4369
Update Otel Invalid Telemetry Calculation #4369
Conversation
Overall package sizeSelf size: 6.72 MB Dependency sizes
🤖 This report was automatically generated by heaviest-objects-in-the-universe |
BenchmarksBenchmark execution time: 2024-06-24 20:11:16 Comparing candidate commit 502b03c in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 260 metrics, 6 unstable metrics. |
OTEL_RESOURCE_ATTRIBUTES: 'foo=bar1,baz=qux1', | ||
DD_TRACE_PROPAGATION_STYLE: 'datadog', | ||
OTEL_PROPAGATORS: 'datadog,tracecontext', | ||
OTEL_RESOURCE_ATTRIBUTES: 'foo+bar13baz+qux1', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm understanding this correctly, we're no longer considering this as "invalid" because we don't have a set of recognized values, so that's why the otelInvalid.length
went down?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes precisely, I think that should also be the expected behavior cross tracers.
@@ -121,15 +121,15 @@ describe('opentelemetry', () => { | |||
OTEL_LOG_LEVEL: 'debug', | |||
DD_TRACE_SAMPLE_RATE: '0.5', | |||
OTEL_TRACES_SAMPLER: 'traceidratio', | |||
OTEL_TRACES_SAMPLER_ARG: '0.1', | |||
OTEL_TRACES_SAMPLER_ARG: '1.0', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm understanding this correctly, we're incrementing the otelHiding.length
in the test because now we consider the OTEL_TRACES_SAMPLER_ARG
config as another config that's being overlooked by its corresponding Datadog config, DD_TRACE_SAMPLE_RATE
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, since they both map to DD_TRACE_SAMPLE_RATE it makes more sense to increment them as seperate pairs of tags. since a user could have inputted both an invalid OTEL_TRACES_SAMPLER value & and an invalid OTEL_TRACES_SAMPLER_ARG value or either or. This makes it more granular.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4369 +/- ##
==========================================
- Coverage 92.64% 88.37% -4.28%
==========================================
Files 116 249 +133
Lines 4173 10740 +6567
Branches 33 33
==========================================
+ Hits 3866 9491 +5625
- Misses 307 1249 +942 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* update otel invalid telemetry calculation
* update otel invalid telemetry calculation
* update otel invalid telemetry calculation
* update otel invalid telemetry calculation
What does this PR do?
Updates the criteria on which the otel invalid telemetry metric is incremented
Motivation
Update how the invalid telemetry metric is incremented based on Stuart's clarification on the Otel ENV Var RFC
Notes
Tested locally with systems tests under this PR: DataDog/system-tests#2597