[OpenTracing Shim] Support TraceFlags-only propagation without parent span #5380
Conversation
|
|
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #5380 +/- ##
============================================
- Coverage 91.38% 91.27% -0.11%
- Complexity 4959 4964 +5
============================================
Files 549 550 +1
Lines 14563 14589 +26
Branches 1356 1358 +2
============================================
+ Hits 13308 13316 +8
- Misses 868 886 +18
Partials 387 387
☔ View full report in Codecov by Sentry. |
|
@carlosalberto Can you take a look? This doesn't seem out-of-line to me, but I really don't know much about OT. |
|
@carlosalberto bump. Does this impact stabilizing the shim in #5371? |
|
I'd say NO - the reason is that is not covered in the Compatibility spec right now (which is stable already). I will prepare a PR to the Spec to update this condition. My impression is that this shouldn't be impacting the expected behavior (as Jaeger already supported this, and it's a corner case for them). cc @yurishkuro |
|
(In case I wasn't clear, while I prepare the Spec PR for this, I suggest we don't include this in the upcoming release) |
| Attributes attributes, | ||
| List<LinkData> parentLinks) { | ||
| SpanContext parentSpanContext = Span.fromContext(parentContext).getSpanContext(); | ||
| if (parentSpanContext.isSampled() |
There was a problem hiding this comment.
What's the purpose of this check? Shouldn't the presence of DEBUG_ID_TAG be enough to cause sampling?
There was a problem hiding this comment.
Because the my current implementation inPropagation.java (shim) is to check span.getSpanContext().isSampled() in order to return a non-null SpanContextShim for jaeger-debug-id case.
So I just added parentSpanContext.isSampled() for one more check here in the test sampler.
| Context context = | ||
| Context.root().with(PropagatedOpenTelemetrySpan.create(invalidSpanContextWithFlags)); | ||
| CustomTextMapPropagator textMapPropagator = new CustomTextMapPropagator(); | ||
| textMapPropagator.setExtractReturnContext(context); |
There was a problem hiding this comment.
This can never be done in a real propagator, which should be stateless. I would recommend changing CustomTextMapPropagator to look for debug header in the carrier and then constructing invalidSpanContextWithFlags in response. That would constitute a realistic test.
There was a problem hiding this comment.
Thanks for the suggestion! I will make the change.
There was a problem hiding this comment.
Is this done @ChenX1993 ?
There was a problem hiding this comment.
@carlosalberto Updated. This is done.
| @@ -197,7 +197,7 @@ public Span start() { | |||
| builder.setNoParent(); | |||
| baggage = Baggage.empty(); | |||
| } else if (mainParent != null) { | |||
| builder.setParent(Context.root().with(io.opentelemetry.api.trace.Span.wrap(mainParent))); | |||
| builder.setParent(Context.root().with(PropagatedOpenTelemetrySpan.create(mainParent))); | |||
There was a problem hiding this comment.
parents in OT are expressed via SpanContexts, not full spans, so why do we need PropagatedOpenTelemetrySpan?
There was a problem hiding this comment.
Yes, but the builder here is a OTel span builder, and the parent required here is a OTel context object with parent OTel span.
The original io.opentelemetry.api.trace.Span.wrap will return a new invalid span if the spanContext is invalid instead of wrapping the spanContext provided, so I added the PropagatedOpenTelemetrySpan to address this.
static Span wrap(SpanContext spanContext) {
if (spanContext == null) {
ApiUsageLogger.log("context is null");
return getInvalid();
}
if (!spanContext.isValid()) {
return getInvalid();
}
return PropagatedSpan.create(spanContext);
}
|
Ping @ChenX1993 |
|
Sorry for the late reply. @yurishkuro |
… span Signed-off-by: Chen Xu <chen.x@uber.com>
|
Ping @yurishkuro |
1 similar comment
|
Ping @yurishkuro |
|
Closing this PR as stale. Please re-open if interested in continuing the effort. |
Resolves #5339
Support propagating the TraceFlags without parent spans, e.g. the usecase of jaeger-debug-id.