Emit warning when TraceIdRatioBasedSampler is used as child sampler#7937
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7937 +/- ##
=========================================
Coverage 90.09% 90.10%
- Complexity 7432 7438 +6
=========================================
Files 834 834
Lines 22537 22546 +9
Branches 2236 2236
=========================================
+ Hits 20304 20314 +10
Misses 1532 1532
+ Partials 701 700 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
6e2eef7 to
db30494
Compare
|
All required checks are passing. This implements the spec-required compatibility warning for TraceIdRatioBasedSampler when used as a child sampler. Logging is limited to ParentBasedSamplerBuilder and does not affect sampling behavior. Happy to adjust if there are any suggestions. |
| if (!warnedRemoteParentSampled && remoteParentSampled instanceof TraceIdRatioBasedSampler) { | ||
| warnedRemoteParentSampled = true; | ||
| logger.warning( | ||
| "TraceIdRatioBasedSampler is being used as a child sampler (remoteParentSampled). " | ||
| + "This configuration is discouraged per the OpenTelemetry specification " | ||
| + "and may lead to unexpected sampling behavior."); | ||
| } |
There was a problem hiding this comment.
This is generally only going to be done once for a given builder instance. I don't think we need the extra checks to make sure it only gets logged once.
There was a problem hiding this comment.
That makes sense - agreed.
I’ll remove the warned* guards and emit the warning unconditionally when a TraceIdRatioBasedSampler is set as a child sampler. That keeps the builder simpler and aligns with typical usage.
I’ll push a small follow-up update shortly.
There was a problem hiding this comment.
It looks like one Windows CI job (windows-latest, 8) failed while the rest passed. This may be transient. would you mind re-running that job when convenient?
|
Thank you for your contribution @Gosling-dude! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. |
What does this change do?
Why is this needed?
Implementation notes:
How was this tested?
Closes #7790