refactor: simplify b3 options #2054
Conversation
mwear
requested review from
dyladan,
Flarna,
johnbley,
legendecas,
markwolff,
mayurkale22,
naseemkullah,
obecny,
OlivierAlbertini and
vmarchaud
as code owners
Codecov Report
@@ Coverage Diff @@
## main #2054 +/- ##
=======================================
Coverage 93.03% 93.03%
=======================================
Files 153 154 +1
Lines 5972 5975 +3
Branches 1246 1246
=======================================
+ Hits 5556 5559 +3
Misses 416 416
|
| api.propagation.setGlobalPropagator(new B3MultiPropagator()); | ||
| ``` | ||
|
|
||
| ### CompositePropagator |
There was a problem hiding this comment.
is CompositePropagator not allowed anymore, as with regards to possibility I think it should still be possible ?
const api = require('@opentelemetry/api');
const { B3Propagator } = require('@opentelemetry/propagator-b3');
api.propagation.setGlobalPropagator(
new CompositePropagator({
propagators: [
new B3Propagator(),
new B3Propagator({ injectEncoding: B3InjectEncoding.MULTI_HEADER })
],
})
);
?
There was a problem hiding this comment.
By default it already extracts both. Is there a usecase where we need to also inject both? Of course you can always use a composite just like you can with any propagator, but keeping it documented feels unnecessary for such a niche usecase.
There was a problem hiding this comment.
not sure, but we had this before so if anyone is using such case it might be hard to understand how it should be done now after these changes
There was a problem hiding this comment.
It's still possible. I don't think it's a common use case, but we can document it.
|
@obecny looks like you added breaking label but there is no functionality actually changed here. |
we have now |
There is no new class. We already have |
Am I the only one that see that both classes |
|
Oh I see. @mwear was that an intended change? |
I think it is exactly what changed, end users should always use multi b3 propagator and configure it to use only single or only multi headers if they dont want both |
Yeah, that was the intention of this PR and a result of a discussion from the spec repo, namely this comment: open-telemetry/opentelemetry-specification#1562 (comment). We had a number of ways to configure b3, but we probably want to steer users towards using the I'll address upgrade notes and other docs concerns. |
Which problem is this PR solving?
Short description of the changes
Based on the discussion on open-telemetry/opentelemetry-specification#1562 we always want to extract both b3 single and multi header formats. There really isn't a use case for extracting the formats individually, so this PR reduces the b3 surface area by not exporting the
B3SinglePropagatororB3MultiPropagator.