Skip to content
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

Merged
merged 15 commits into from
Oct 9, 2020
Merged

Conversation

mwear
Copy link
Member

@mwear mwear commented Sep 30, 2020

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).

As a followup I intend to move the B3 propagator to the contrib repo per the spec requirement

@codecov
Copy link

codecov bot commented Sep 30, 2020

Codecov Report

Merging #1560 into master will increase coverage by 0.79%.
The diff coverage is 100.00%.

@@            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     
Impacted Files Coverage Δ
...-core/src/context/propagation/B3MultiPropagator.ts 100.00% <100.00%> (ø)
...metry-core/src/context/propagation/B3Propagator.ts 100.00% <100.00%> (ø)
...core/src/context/propagation/B3SinglePropagator.ts 100.00% <100.00%> (ø)
...elemetry-core/src/context/propagation/b3-common.ts 100.00% <100.00%> (ø)
...lemetry-exporter-collector/src/transformMetrics.ts 96.77% <0.00%> (ø)
...ry-exporter-collector/src/CollectorExporterBase.ts 91.83% <0.00%> (ø)
...plugin-grpc-js/src/client/loadPackageDefinition.ts 100.00% <0.00%> (ø)
...ntelemetry-web/src/enums/PerformanceTimingNames.ts 100.00% <0.00%> (ø)
... and 31 more

@mwear mwear force-pushed the b3-updates branch 2 times, most recently from ef95e28 to 91d1a38 Compare September 30, 2020 04:14
@mwear mwear added this to In progress PRs in GA Burndown via automation Sep 30, 2020
GA Burndown automation moved this from In progress PRs to Approved Oct 1, 2020
Copy link
Member

@obecny obecny left a 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 ?

@dyladan
Copy link
Member

dyladan commented Oct 6, 2020

What will happen if the user chooses Multi Header Propagator but the Single Header variant is present only, and also the opposite situation ?.

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).

What will happen if booth of the headers are available

The single takes precedence.

@obecny
Copy link
Member

obecny commented Oct 6, 2020

What will happen if the user chooses Multi Header Propagator but the Single Header variant is present only, and also the opposite situation ?.

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).

What will happen if booth of the headers are available

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 ?

@mwear
Copy link
Member Author

mwear commented Oct 6, 2020

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.

@obecny
Copy link
Member

obecny commented Oct 9, 2020

@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

single -> multi -> single -> multi -> ..... etc.

  1. Can any information be lost during such flow ?
  2. Can this be easily covered with tests to verify all works fine ?

@mwear
Copy link
Member Author

mwear commented Oct 9, 2020

@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

single -> multi -> single -> multi -> ..... etc.

1. Can any information be lost during such flow ?

2. Can this be easily covered with tests to verify all works fine ?

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.
@dyladan dyladan merged commit e62c481 into open-telemetry:master Oct 9, 2020
GA Burndown automation moved this from Approved to Done Oct 9, 2020
dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
GA Burndown
  
Done
Development

Successfully merging this pull request may close these issues.

Single header B3 support
4 participants