Propagate trace_flags from parent span to child span #603
Conversation
Codecov Report
@@ Coverage Diff @@
## main #603 +/- ##
==========================================
+ Coverage 94.40% 94.42% +0.01%
==========================================
Files 191 191
Lines 9023 9053 +30
==========================================
+ Hits 8518 8548 +30
Misses 505 505
|
sdk/src/trace/span.cc
Outdated
| trace_api::SpanId span_id = GenerateRandomSpanId(); | ||
| bool is_parent_span_valid = false; | ||
| trace_api::SpanId span_id = GenerateRandomSpanId(); | ||
| trace_api::TraceFlags trace_flags = parent_span_context.trace_flags(); |
There was a problem hiding this comment.
I was thinking about propagating trace_flags only for valid SpanContext, but since we could create a invalid SpanContextwith non-zerotrace_flags`, so I choose to do propagation even for invalid span. Let me know if you prefer the other case.
Thanks @stevchin for trying opentelemetry-cpp, reporting this issue and helping on troubleshooting! |
sdk/src/trace/span.cc
Outdated
| @@ -94,7 +95,7 @@ Span::Span(std::shared_ptr<Tracer> &&tracer, | |||
| } | |||
|
|
|||
| span_context_ = std::unique_ptr<trace_api::SpanContext>( | |||
| new trace_api::SpanContext(trace_id, span_id, trace_api::TraceFlags(), false, | |||
| new trace_api::SpanContext(trace_id, span_id, trace_flags, false, | |||
There was a problem hiding this comment.
Not sure if I am missing something here - Why do we need to copy the parent's trace_flags to child span. For sampling-bit in child's span trace_flag it should be determined as below -
- Run sampling algo to decide whether child span should be sampled - the sampling algo may use parent's span trace_flags for decision making.
- In case the child span should be sampled ( as decided by above sampling algo ) - we should set the sampling-bit in child span trace_flag.
Or is there more information in parent's trace_flag ( other than sampling-bit) which we want to copy from parent ? If that is the case, we should copy that first and then overwrite sampling bit based on decision from above logic.
Though our current implementation is also wrong - as it's assigning 0x00 to trace_flags even if it's is supposed to be sampled. This should be fixed.
There was a problem hiding this comment.
Do you mean we set kIsSampled directly in the above code line because the location implies sampling should be enabled, instead of copying trace_flags from parent SpanContext? If so, I think we could keep the same sampled value as parent to be consistent and leave the decision to sampler, like even the IsSampled is off, some sampler may still sample it anyway. But I am fine to set the sampled bit here too.
There was a problem hiding this comment.
If it helps, how it is done in JS:
https://github.com/open-telemetry/opentelemetry-js/blob/71ba83a0dc51118e08e3148c788b81fe711003e7/packages/opentelemetry-tracing/src/Tracer.ts#L96
and python:
There was a problem hiding this comment.
Do you mean we set kIsSampled directly in the above code line because the location implies sampling should be enabled, instead of copying trace_flags from parent SpanContext?
Yes, as we are creating the span (and sampling decision happens before that in tracer), that means decision has already been made by sampler to sample it, and we should just be setting sampling-bit in the span's trace_flag irrespective of what is there in parent's trace_flag.
There was a problem hiding this comment.
@lalitb thanks for the links. Set sampled based sampler decision makes sense. Updated the PR accordingly.
Fixes #602
Changes
trace_flagsis propagated from parent span to child span at span creation time.For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes