Add B3 single propagation format support #281

indrekj · 2021-09-30T13:41:41Z

B3 single format looks like this: b3={TraceId}-{SpanId}-{SamplingState}-{ParentSpanId},
where the last two fields are optional.

OpenTelemetry B3 requirements:

B3 has both single and multi-header encodings. It also has semantics
that do not map directly to OpenTelemetry such as a debug trace flag,
and allowing spans from both sides of request to share the same id. To
maximize compatibility between OpenTelemetry and Zipkin implementations,
the following guidelines have been established for B3 context
propagation.

When extracting B3, propagators:

MUST attempt to extract B3 encoded using single and multi-header formats. The single-header variant takes precedence over the multi-header version. 🟢
MUST preserve a debug trace flag, if received, and propagate it with subsequent requests. Additionally, an OpenTelemetry implementation MUST set the sampled trace flag when the debug flag is set. 🟡 We do not preserve the debug flag at the moment. We didn't do this before and not doing this in this PR either. We however do set sampling to 1 if that flag is present.
MUST NOT reuse X-B3-SpanId as the id for the server-side span. 🟢

When injecting B3, propagators:

MUST default to injecting B3 using the single-header format 🟢
MUST provide configuration to change the default injection format to B3 multi-header 🟢
MUST NOT propagate X-B3-ParentSpanId as OpenTelemetry does not support reusing the same id for both sides of a request. 🟢

codecov · 2021-09-30T13:42:56Z

Codecov Report

Merging #281 (13b7dba) into main (29af143) will increase coverage by 0.92%.
The diff coverage is 84.28%.

@@            Coverage Diff             @@
##             main     #281      +/-   ##
==========================================
+ Coverage   36.59%   37.51%   +0.92%     
==========================================
  Files          45       47       +2     
  Lines        3178     3228      +50     
==========================================
+ Hits         1163     1211      +48     
- Misses       2015     2017       +2

Flag	Coverage Δ
api	`66.03% <84.05%> (+2.83%)`	⬆️
elixir	`14.90% <0.00%> (-1.52%)`	⬇️
erlang	`37.50% <84.28%> (+0.93%)`	⬆️
exporter	`19.75% <ø> (ø)`
sdk	`79.16% <100.00%> (+0.21%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
apps/opentelemetry_api/src/otel_propagator_b3.erl	`63.63% <63.63%> (ø)`
...opentelemetry_api/src/otel_propagator_b3single.erl	`85.71% <85.71%> (ø)`
.../opentelemetry_api/src/otel_propagator_b3multi.erl	`86.66% <90.47%> (+18.80%)`	⬆️
apps/opentelemetry/src/otel_configuration.erl	`79.16% <100.00%> (+0.29%)`	⬆️
apps/opentelemetry_api/src/otel_propagator.erl	`87.50% <100.00%> (+16.07%)`	⬆️
apps/opentelemetry/src/otel_resource_detector.erl	`92.85% <0.00%> (+1.42%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29af143...13b7dba. Read the comment docs.

tsloughter · 2021-10-01T17:33:37Z

apps/opentelemetry_api/src/otel_propagator_b3multi.erl

@@ -91,58 +90,132 @@ inject(Ctx, Carrier, CarrierSet, _Options) ->
            Carrier
    end.

+% Extract trace context from the supplied carrier. The b3 single header takes
+% precedence over the multi-header format.


Hm, this is interesting. Does the otel spec also say this or only the b3 spec? I'm confused about how in otel a user could specify b3 or b3multi but its supposed to use both either way?

That's in the otel spec: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/context/api-propagators.md#b3-requirements . First point in the Extract section. That's also how it's done in the ruby implementation: https://github.com/open-telemetry/opentelemetry-ruby/blob/main/propagator/b3/lib/opentelemetry/propagator/b3/text_map_extractor.rb#L48

Ah, the use of being able to specify one or the other is for injection, makes sense.

ferd · 2021-10-01T15:02:06Z

apps/opentelemetry_api/src/otel_propagator_b3.erl

+%% Since `trace_context' and `baggage' are the two default propagators the
+%% global TextMap Propagators must be configured if B3 is to be used for
+%% propagation:
+%%
+%% ```
+%% {text_map_propagators, [b3, baggage]},
+%% '''
+%%
+%% ```
+%% CompositePropagator = otel_propagator_text_map_composite:create([b3, baggage]),
+%% opentelemetry:set_text_map_propagator(CompositePropagator).
+%% '''
+%%
+%% It is also possible to set a separate list of injectors or extractors.
+%% For example, if the service should extract B3 encoded context but you
+%% only want to inject context encoded with the W3C TraceContext format
+%% (maybe you have some services only supporting B3 that are making requests
+%% to your server but you have no reason to continue propagating in both
+%% formats when communicating to other services further down the stack).
+%% In that case you would instead set configuration like:
+%%
+%%
+%% ```
+%% {text_map_extractors, [b3, trace_context, baggage]},
+%% {text_map_injectors, [trace_context, baggage]},
+%% '''
+%%
+%% Or using calls to {@link opentelemetry} at runtime:
+%%
+%% ```
+%% B3CompositePropagator = otel_propagator_text_map_composite:create([b3, trace_context, baggage]),
+%% CompositePropagator = otel_propagator_text_map_composite:create([trace_context, baggage]),
+%% opentelemetry:set_text_map_extractor(B3CompositePropagator),
+%% opentelemetry:set_text_map_injector(CompositePropagator).
+%% '''
+%% @end


This is super helpful doc, A+

99% copied from the existing b3multi module though

apps/opentelemetry_api/src/otel_propagator_b3.erl

ferd · 2021-10-01T15:06:19Z

apps/opentelemetry_api/src/otel_propagator_b3.erl

+        #span_ctx{trace_id=TraceId,
+                  span_id=SpanId,
+                  trace_flags=TraceOptions} when TraceId =/= 0 andalso SpanId =/= 0 ->
+            Options = case TraceOptions band 1 of 1 -> <<"1">>; _ -> <<"0">> end,


Suggested change

Options = case TraceOptions band 1 of 1 -> <<"1">>; _ -> <<"0">> end,

Options = <<(TraceOptions band 1 + $0)>>,

Is this too much code golf? Should technically be faster since band 1 is always gonna be 0 or 1 anyway.
I see lower that there's a <<"d">> debug option that should come in, which I guess makes this suggestion irrelevant?

Debug flag currently isn't propagated so here we deal only with 1 and 0. Tbh I like the explicit version a bit more (I'm quite new to erlang so the code golf currently is a bit hard to grasp though I think I understand what is happening). I would keep it as is unless you feel strongly about changing it.

Yea, the case version is good.

apps/opentelemetry_api/src/otel_propagator_b3multi.erl

tsloughter · 2021-10-03T14:01:49Z

A bit of a nitpick but I think this will be clearer if the module otel_propagator_b3 is used to call extract of both modules otel_propagator_b3multi and a new module otel_propagator_b3single.

Injection can be based on a option to the propagator. So the configuration will then turn b3 into {otel_propagator_b3, b3single} and b3multi into {otel_propagator_b3, b3multi}.

Does that make sense?

indrekj · 2021-10-04T08:35:08Z

@tsloughter Yes, updated.

apps/opentelemetry_api/src/otel_propagator_b3.erl

[B3 single format][1] looks like this: `b3={TraceId}-{SpanId}-{SamplingState}-{ParentSpanId}`, where the last two fields are optional. [OpenTelemetry B3 requirements][2]: B3 has both single and multi-header encodings. It also has semantics that do not map directly to OpenTelemetry such as a debug trace flag, and allowing spans from both sides of request to share the same id. To maximize compatibility between OpenTelemetry and Zipkin implementations, the following guidelines have been established for B3 context propagation. When extracting B3, propagators: * MUST attempt to extract B3 encoded using single and multi-header formats. The single-header variant takes precedence over the multi-header version. * MUST preserve a debug trace flag, if received, and propagate it with subsequent requests. Additionally, an OpenTelemetry implementation MUST set the sampled trace flag when the debug flag is set. * MUST NOT reuse X-B3-SpanId as the id for the server-side span. When injecting B3, propagators: * MUST default to injecting B3 using the single-header format * MUST provide configuration to change the default injection format to B3 multi-header * MUST NOT propagate X-B3-ParentSpanId as OpenTelemetry does not support reusing the same id for both sides of a request. [1]: https://github.com/openzipkin/b3-propagation#single-header [2]: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/context/api-propagators.md#b3-requirements

github-actions bot added language-erlang scope-api scope-sdk labels Sep 30, 2021

indrekj force-pushed the b3-single branch 2 times, most recently from a9de390 to a143b39 Compare October 1, 2021 09:26

indrekj changed the title ~~[WIP] Add B3 single propagation format support~~ Add B3 single propagation format support Oct 1, 2021

indrekj force-pushed the b3-single branch from a143b39 to af27b2f Compare October 1, 2021 09:51

indrekj marked this pull request as ready for review October 1, 2021 09:54

indrekj requested a review from a team October 1, 2021 09:54

tsloughter reviewed Oct 1, 2021

View reviewed changes

ferd reviewed Oct 1, 2021

View reviewed changes

indrekj force-pushed the b3-single branch 5 times, most recently from a391e12 to c6bd101 Compare October 4, 2021 08:17

tsloughter reviewed Oct 4, 2021

View reviewed changes

apps/opentelemetry_api/src/otel_propagator_b3.erl Outdated Show resolved Hide resolved

tsloughter reviewed Oct 4, 2021

View reviewed changes

apps/opentelemetry_api/src/otel_propagator_b3.erl Outdated Show resolved Hide resolved

indrekj force-pushed the b3-single branch from c6bd101 to 6d96371 Compare October 4, 2021 12:58

indrekj force-pushed the b3-single branch from 6d96371 to 13b7dba Compare October 4, 2021 12:59

tsloughter approved these changes Oct 4, 2021

View reviewed changes

tsloughter merged commit b70723b into open-telemetry:main Oct 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add B3 single propagation format support #281

Add B3 single propagation format support #281

Uh oh!

indrekj commented Sep 30, 2021 •

edited

Loading

Uh oh!

codecov bot commented Sep 30, 2021 •

edited

Loading

Uh oh!

tsloughter Oct 1, 2021

Uh oh!

indrekj Oct 1, 2021

Uh oh!

tsloughter Oct 3, 2021

Uh oh!

ferd Oct 1, 2021

Uh oh!

indrekj Oct 1, 2021

Uh oh!

Uh oh!

ferd Oct 1, 2021

Uh oh!

indrekj Oct 4, 2021 •

edited

Loading

Uh oh!

tsloughter Oct 4, 2021

Uh oh!

Uh oh!

tsloughter commented Oct 3, 2021

Uh oh!

indrekj commented Oct 4, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

	Options = case TraceOptions band 1 of 1 -> <<"1">>; _ -> <<"0">> end,
	Options = <<(TraceOptions band 1 + $0)>>,

Add B3 single propagation format support #281

Add B3 single propagation format support #281

Uh oh!

Conversation

indrekj commented Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tsloughter Oct 1, 2021

Choose a reason for hiding this comment

Uh oh!

indrekj Oct 1, 2021

Choose a reason for hiding this comment

Uh oh!

tsloughter Oct 3, 2021

Choose a reason for hiding this comment

Uh oh!

ferd Oct 1, 2021

Choose a reason for hiding this comment

Uh oh!

indrekj Oct 1, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ferd Oct 1, 2021

Choose a reason for hiding this comment

Uh oh!

indrekj Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsloughter Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tsloughter commented Oct 3, 2021

Uh oh!

indrekj commented Oct 4, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

indrekj commented Sep 30, 2021 •

edited

Loading

codecov bot commented Sep 30, 2021 •

edited

Loading

indrekj Oct 4, 2021 •

edited

Loading