feat: support TraceState in SamplingResult #3530
Conversation
| @@ -117,6 +117,10 @@ export class Tracer implements api.Tracer { | |||
| links | |||
| ); | |||
|
|
|||
| // according to the spec an empty Tracestate means it should be cleared but | |||
There was a problem hiding this comment.
I think this depends on how you define empty tracestate.
I would interpret new TraceState() as empty and this is working as specified.
not interpreting samplingResult.traceState == null sounds correct to me and not just a backward compat solution.
There was a problem hiding this comment.
Would it make sense to handle null given the same can be done by passing the result of createTraceState ? It's shorter but I don't remember seeing null anywhere else in the API.
Either way agreed the comment isn't great, and the behaviour should probably also be described in the new field's doc comment.
There was a problem hiding this comment.
I don't think we should add special handling for null. I used == null above which includes null and undefined like the ?? operator used in your change.
If a sampler want's to clear the TraceState it should return an empty instance by calling api.createTraceState().
But I would expect that a sample is usually not clearing the whole TraceState as it may contain arbitrary keys not related to sampling. Instead I assume they only set/clear/update sampler specific keys like the ot key used by consistent probability samplers.
There was a problem hiding this comment.
Gotcha, so the behaviour should already be correct. I updated the comments.
| * the new {@link SpanContext}. Samplers SHOULD return the passed-in | ||
| * TraceState if they do not intend to change it. Leaving the value undefined |
There was a problem hiding this comment.
Question: The "passed-in TraceState", is it assumed that the sampler will read it off of the context that is passed in? Would it make sense to pass the tracestate as a separate argument to shouldSample?
There was a problem hiding this comment.
Alternatively, perhaps this comment should mention that the TraceState from the passed-in context, just to avoid confusion?
There was a problem hiding this comment.
I think it would be quite confusing to pass context and some information which is usually on context as an additional parameter. Samplers have to decide then which source they prefer and if they should use the other as fallback,
There was a problem hiding this comment.
I agree, but reading this comment made me wonder, "passed-in where?" and immediately assumed that the function signature is out of date with this comment (that's me coming from a background of having a few custom samplers, but never having worked with tracestate). I agree that having a single source of truth is preferred, so perhaps the comment should be modified to clarify what "passed-in" refers to?
There was a problem hiding this comment.
Good point, updated the doc comment to clarify
|
Any ETA on when this might land? Looking forward to this ability in the samplers! |
|
Hi @Flarna and/or @open-telemetry/javascript-approvers, any chance this PR can be fully approved and merged soon? This is a critical piece in our effort to adopt OTel and we'd appreciate any help you can give :) |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3530 +/- ##
==========================================
- Coverage 93.98% 93.24% -0.75%
==========================================
Files 260 253 -7
Lines 7802 7330 -472
Branches 1620 1534 -86
==========================================
- Hits 7333 6835 -498
- Misses 469 495 +26
|
packages/opentelemetry-core/src/trace/sampler/AlwaysOnSampler.ts
Outdated
Show resolved
Hide resolved
|
@legendecas @Flarna could you rustle up that final reviewer/approver please? |
Which problem is this PR solving?
This PR makes it possible for samplers to update the tracestate by passing it to the sampling result, as introduced in version 0.7 of the spec.
Fixes #2974
Short description of the changes
A new
traceState?: TraceStatefield has been added toSamplingResult. If present, it is used by theTracerwhen creating the new span. If not present orundefined, the existing behaviour of using the parent's applies. This maintains semver compatibility and makes it easier to forward an existing tracestate. It is still possible to clear the tracestate by explicitly passing an empty one.The builtin samplers have been updated to explicitly forward the tracestate of the passed in parent context as recommended by the spec.
Type of change
Checklist: