fix: propagate the b3 parentspanid. fixes regression of (#1338) #1608
Conversation
Signed-off-by: dblackhall-tyro <39076155+dblackhall-tyro@users.noreply.github.com>
dblackhall-tyro
requested review from
dyladan,
legendecas,
markwolff,
mayurkale22,
mwear,
naseemkullah,
obecny,
OlivierAlbertini and
vmarchaud
as code owners
|
|
Codecov Report
@@ Coverage Diff @@
## master #1608 +/- ##
==========================================
+ Coverage 91.18% 91.21% +0.02%
==========================================
Files 164 164
Lines 5024 5040 +16
Branches 1026 1033 +7
==========================================
+ Hits 4581 4597 +16
Misses 443 443
|
There was a problem hiding this comment.
See the comments below. I don't think the parentSpanId being propagated in this PR is correct, and I don't think this can be fixed without a specification change because it is not possible to read a parent span id given the current span API. The code that previously propagated parent span id was intentionally removed as a result.
| @@ -95,11 +107,24 @@ function getTraceFlags( | |||
| export class B3MultiPropagator implements TextMapPropagator { | |||
| inject(context: Context, carrier: unknown, setter: TextMapSetter) { | |||
| const spanContext = getParentSpanContext(context); | |||
| if (!spanContext || !isSpanContextValid(spanContext)) return; | |||
| const parentSpanId = context.getValue(B3_PARENT_SPAN_ID_KEY) as | |||
There was a problem hiding this comment.
This is the parent span id of the extracted request, which is incorrect unless no other in-process spans have been created. Unfortunately, I don't see a way to propagate the parent span id for in-process spans using OpenTelemetry. While parentSpanId is available on ReadableSpan in the export pipeline, it's not part of the Span API.
For B3, the parentSpanId is propagated so that both sides of a request can share the same span id. OpenTelemetry does not support span id sharing. By omitting parentSpanId, a downstream service should generate a new span id for the inbound request.
See the b3 spec (https://github.com/openzipkin/b3-propagation):
Some propagation formats look similar to B3, but don't propagate a field named parent. Instead, they propagate a span ID field which serves the same purpose as ParentSpanId. Unlike B3, these systems use a different span ID for the client and server side of an RPC. When a server reads headers like this, it is expected to provision a new span ID for itself, and use the one it extracted as its parent.
...
In both B3 and the above example, incoming headers contain the parent's span ID, and three IDs (trace, parent, span) end up in the trace context. The difference is that B3 uses the same span ID for the client and server side of an RPC, where the latter does not.
See also this spec PR: open-telemetry/opentelemetry-specification#1045
There was a problem hiding this comment.
I just saw your comment here: open-telemetry/opentelemetry-specification#1045 (comment)
Thanks for taking this into consideration :) I will close this PR, and keep following the progress with B3.
And thanks for taking the time to explain why parentSpanId propagation was intentionally removed.
Co-authored-by: Amir Blum <amirgiraffe@gmail.com>
Signed-off-by: dblackhall-tyro 39076155+dblackhall-tyro@users.noreply.github.com
Which problem is this PR solving?
There is a regression in 0.12 with the B3Propagator not propagating the parent span id.
Short description of the changes
Created a B3_PARENT_SPAN_ID_KEY which is used to inject and extract the parent span id.