-
Notifications
You must be signed in to change notification settings - Fork 804
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
fix: use invalid parent for sampler when options.root #2185
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2185 +/- ##
==========================================
+ Coverage 92.72% 93.79% +1.07%
==========================================
Files 141 53 -88
Lines 5087 1709 -3378
Branches 1045 356 -689
==========================================
- Hits 4717 1603 -3114
+ Misses 370 106 -264
|
@@ -87,7 +87,9 @@ export class Tracer implements api.Tracer { | |||
const attributes = sanitizeAttributes(options.attributes); | |||
// make sampling decision | |||
const samplingResult = this._sampler.shouldSample( | |||
context, | |||
options.root | |||
? api.setSpanContext(context, api.INVALID_SPAN_CONTEXT) |
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.
I think context.deleteValue(SPAN_KEY)
would be the correct approach here. I have seen several places around where checks like if (getSpan(context) != null)
are done to detect presence of a parent.
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.
Spec states that "The Span's SpanContext may be invalid to indicate a root span." and there is no function to remove span from context currently. We don't have the SPAN_KEY outside of the API
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.
ok.
Maybe we should add deleteSpan(context)
(and deleteBaggage(context)
) at some time?
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.
Maybe we should add deleteSpan(context) (and deleteBaggage(context)) at some time?
totally make sense
Fixes #2166 by providing a parent span with an invalid span context to the sampler
From the spec: