-
Notifications
You must be signed in to change notification settings - Fork 795
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
feat: b3 single header support #1560
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1560 +/- ##
==========================================
+ Coverage 92.23% 93.03% +0.79%
==========================================
Files 125 161 +36
Lines 2872 4938 +2066
Branches 543 995 +452
==========================================
+ Hits 2649 4594 +1945
- Misses 223 344 +121
|
ef95e28
to
91d1a38
Compare
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.
What will happen if the user chooses Multi Header Propagator but the Single Header variant is present only, and also the opposite situation ?.
What will happen if booth of the headers are available, according to spec
The single-header variant takes precedence over the multiple header one when extracting fields.
Do you cover such case with tests / code ?
If I understand correctly, the multi and single propagator is not intended to be used directly. They are delegated from the B3 propagator. On extract, the single is called before the multi. On inject, only one of them is called (single by default, but multi is configurable).
The single takes precedence. |
I'm interested in situation when backend sends multi client use single and sends to a different backend which is then using single or multi. Would that have any impact ? |
There is a long discussion about the various scenarios on the spec PR: open-telemetry/opentelemetry-specification#961. Ulitmately, we arrived at the recommendation to extract single and multi formats, but inject single by default. The way this is implemented users wouldn't be limited to the default and could tailor B3 to their needs. I don't see any reason why they couldn't use a Composite propagator in conjunction with the b3 single and mutli propagators to inject and extract both formats, for example. |
@mwear unless I'm not aware of something then please correct me if I'm wrong but I'm missing here the TEST case when you will be switching multiple times between 2 different formats. Imagine we have a couple services that talk to each other and you want to test scenario when they send request between each other and every time extracting and injecting headers in different format
|
There are absolutely situations where alternating these formats will break the trace. If any of the services are unable to extract b3 single header, the trace will break, for example. The only way to ensure that context is not lost in this scenario would be to inject and extract both B3 multi and B3 single for every request. This is possible by using a composite propagator configured with the B3 single and B3 multi propagators, it's just not the default. What is implemented in this PR is what is specified here: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/context/api-propagators.md#b3-requirements. We are not trying to cover all possible scenarios out of the box, but we do allow enough configurability that users would be able to tailor B3 to the needs of their system. |
B3 has an option to propagate both span id and a parent span id so that both sides of request can share the same span id. This is a usecase that we do not support in OpenTelemetry and couldn't support even if we wanted to because parent span id is not readable outside of the export pipeline. This buggy implementation stores and propagates the extracted parent span id, which will break zipkin implementations as a result.
Which problem is this PR solving?
Short description of the changes
Per the spec a B3 propagator should extract context in both single and multi-header formats, and inject context in the single header format (with the option to change the format to multi header).
B3Propagator
toB3MultiPropagator
(the diff isn't completely clear on this point, unfortunately)B3SinglePropagator
B3Propagator
that delegates injection and extraction toB3MultiPropagator
andB3SinglePropagator
instancesAs a followup I intend to move the B3 propagator to the contrib repo per the spec requirement