Make B3 propagator spec compliant

[marcinzaremba]

Description

Adjust B3 propagator to not modify context in case of:

• missing headers
• invalid headers
• empty/no headers

Contributes to #1727

Type of change

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Unit tests extended and modified accordingly to reflect the new behavior.

Does This PR Require a Contrib Repo Change?

• No.

Checklist:

• Followed the style guidelines of this project
• Changelogs have been updated
• Unit tests have been added
• Documentation has been updated

[toumorokoshi]

LGTM, thanks!

[srikanthccv]

extract MUST NOT store a new value in the Context, in order to preserve any previously existing valid value.

If prev key doesn't exist in context, then is it allowed to set the default span context? From what I understand the returned context should have the cross cutting concern that could be SpanContext,Baggage or may be something else. It would be invalid span context if nothing already exists or can't be obtained from carrier.

[lzchen]

Since we are removing trace.set_span_in_context, will this make it so that, if in the missing/empty/no headers, the span ISN'T set in the current context

[marcinzaremba]

I do not know the exact details of the code base yet, need to dig deeper. Speaking about design: shouldn't it be a responsibility of upper layers, not propagators themselves? Single propagator is not aware of work of others propagators and potentially they can work together (through a CompositePropagator) towards the common result. Current approach effectively renders such use case really difficult if multiple propagators taking care of the same cross cutting concern can possibly override their results with default values even if previous ones successfully extracted information. The mentioned fragment from the spec seems to address that usage clearly by disallowing any modification of the context when information is not successfully extracted.
I also can't help to ask: is it currently necessary to set at least one propagator globally in order for tracing to work properly even within the realm of single service? (if the propagators themselves are responsible for setting default values there)

[marcinzaremba]

Took a look. In the codebase the problem seems to be solved on fetching/obtaining the current span: https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-api/src/opentelemetry/trace/propagation/__init__.py#L37. If no current span is present in the context, an invalid span is returned. So propagators do not need to be concerned with it.
Regarding Baggage: as far as I understand baggage is an optional, arbitrary key-value storage so there are no expectations towards it having any content by default.

[marcinzaremba]

So to fully answer the question: such an approach will left context with a current span unset, however that does not have any negative impact, because by using the official, mentioned API one always obtains any span (invalid in case it is not set) if requested.

[lzchen]

Isn't there the case of, the previous current span is a valid one, and we call extract using the propagator with empty headers. Without this change, someone calling "get_current_span()` would result in an invalid span, but after your change, since the context is not set, the current span is the previous valid span? Isn't that unwarranted change in behaviour?

[srikanthccv]

is it currently necessary to set at least one propagator globally in order for tracing to work properly even within the realm of single service?

Yes, but there is a default propagator set which is tracecontext,baggage.

[marcinzaremba]

Isn't there the case of, the previous current span is a valid one, and we call extract using the propagator with empty headers. Without this change, someone calling "get_current_span()` would result in an invalid span, but after your change, since the context is not set, the current span is the previous valid span? Isn't that unwarranted change in behaviour?

This all boils down to the understanding of the spec. If one sees the interpretation of the spec as posted in the attached issue, that is just adjusting the current, incorrect behavior (which has presented impact elsewhere - CompositePropagator usage) towards being compliant with the spec. If one sees the interpretation differently that it is indeed an unwarranted change in behavior.
As there seems to be a lack of clarity how to interpret the spec, who can help to give an authoritative answer on the spec?
Looking more at practical aspect and weighing impact:
when one is not concerned by the issues that the chosen approach introduces (possible conflicts with CompositePropagator), probably does not have more than one global propagator currently touching the same (meaning two propagators concerned with span context; excluding cases when there are two separate ones for baggage and span context) cross cutting concern so the case you're describing is very unlikely to impact the users that might be impacted. Because how come they already have a valid current span, before the single extraction that they have currently set?

[srikanthccv]

I think there is a little confusion here. The call to extract doesn't itself change the current context and one has to use attach to achieve that.

>>> from opentelemetry.trace.propagation.textmap import DictGetter
>>> from opentelemetry.trace.propagation.tracecontext import TraceContextTextMapPropagator
>>> from opentelemetry import context
>>> current_ctx = context.get_current()
>>> current_ctx
{}
>>> from opentelemetry.trace.propagation import get_current_span
>>> get_current_span()
DefaultSpan(SpanContext(trace_id=0x00000000000000000000000000000000, span_id=0x0000000000000000, trace_flags=0x00, trace_state=[], is_remote=False))
>>> carrier = {'traceparent': '00-a9c3b99a95cc045e573e163c3ac80a77-d99d251a8caecd06-01'}
>>> valid_ctx = TraceContextTextMapPropagator().extract(carrier=carrier, getter=DictGetter())
>>> context.get_current()
{}
>>> get_current_span()
DefaultSpan(SpanContext(trace_id=0x00000000000000000000000000000000, span_id=0x0000000000000000, trace_flags=0x00, trace_state=[], is_remote=False))
>>> valid_ctx
{'current-span': DefaultSpan(SpanContext(trace_id=0xa9c3b99a95cc045e573e163c3ac80a77, span_id=0xd99d251a8caecd06, trace_flags=0x01, trace_state=[], is_remote=True))}
>>> from opentelemetry.context import attach
>>> token = attach(valid_ctx)
>>> context.get_current()
{'current-span': DefaultSpan(SpanContext(trace_id=0xa9c3b99a95cc045e573e163c3ac80a77, span_id=0xd99d251a8caecd06, trace_flags=0x01, trace_state=[], is_remote=True))}
>>> get_current_span()
DefaultSpan(SpanContext(trace_id=0xa9c3b99a95cc045e573e163c3ac80a77, span_id=0xd99d251a8caecd06, trace_flags=0x01, trace_state=[], is_remote=True))
>>> invalid_carrier = {}
>>> invalid_ctx = TraceContextTextMapPropagator().extract(carrier=invalid_carrier, getter=DictGetter())
>>> context.get_current()
{'current-span': DefaultSpan(SpanContext(trace_id=0xa9c3b99a95cc045e573e163c3ac80a77, span_id=0xd99d251a8caecd06, trace_flags=0x01, trace_state=[], is_remote=True))}
>>> get_current_span()
DefaultSpan(SpanContext(trace_id=0xa9c3b99a95cc045e573e163c3ac80a77, span_id=0xd99d251a8caecd06, trace_flags=0x01, trace_state=[], is_remote=True))

So the problem current propagator implementation is when there are multiple propagators set the last one in the list might end up overwriting a valid value set by earlier one.

opentelemetry-python/opentelemetry-api/src/opentelemetry/propagators/composite.py

Lines 36 to 51 in 25edfef

	def extract(
	self,
	carrier: textmap.CarrierT,
	context: typing.Optional[Context] = None,
	getter: textmap.Getter = textmap.default_getter,
	) -> Context:
	"""Run each of the configured propagators with the given context and carrier.
	Propagators are run in the order they are configured, if multiple
	propagators write the same context key, the propagator later in the list
	will override previous propagators.
	See `opentelemetry.propagators.textmap.TextMapPropagator.extract`
	"""
	for propagator in self._propagators:
	context = propagator.extract(carrier, context, getter=getter)
	return context # type: ignore

>>> from opentelemetry.trace.propagation.textmap import DictGetter
>>> from opentelemetry.trace.propagation.tracecontext import TraceContextTextMapPropagator
>>> carrier = {'traceparent': '00-a9c3b99a95cc045e573e163c3ac80a77-d99d251a8caecd06-01'}
>>> context = TraceContextTextMapPropagator().extract(carrier=valid_w3c_carrier, getter=DictGetter())
>>> context
{'current-span': DefaultSpan(SpanContext(trace_id=0xa9c3b99a95cc045e573e163c3ac80a77, span_id=0xd99d251a8caecd06, trace_flags=0x01, trace_state=[], is_remote=True))}
>>> from opentelemetry.propagators.b3 import B3Format
>>> carrier = {}
>>> returned_ctx = B3Format().extract(getter=DictGetter(), carrier=carrier, context=context)
>>> returned_ctx
{'current-span': DefaultSpan(SpanContext(trace_id=0x00000000000000000000000000000000, span_id=0x0000000000000000, trace_flags=0x00, trace_state=[], is_remote=True))}

Although the passed context has valid value set the B3 propagator replaced it with default invalid span context. I hope I am understanding the problem right?

[marcinzaremba]

@lonewolf3739 Indeed. I wouldn't describe the problem better.

[lzchen]

The call to extract doesn't itself change the current context and one has to use attach to achieve that.

This answers my question. It is fine.

[marcinzaremba]

If prev key doesn't exist in context, then is it allowed to set the default span context? From what I understand the returned context should have the cross cutting concern that could be SpanContext,Baggage or may be something else. It would be invalid span context if nothing already exists or can't be obtained from carrier.

@lonewolf3739 you might look at the answer to @lzchen. I think the question was really similar in nature.
In the meantime to build more evidence I took a peek at how both Java and Ruby approach it and it seems they do it in favor of the interpration of the spec mentioned here:

• Ruby: https://github.com/open-telemetry/opentelemetry-ruby/blob/main/propagator/b3/lib/opentelemetry/propagator/b3/text_map_extractor.rb#L36; https://github.com/open-telemetry/opentelemetry-ruby/blob/main/propagator/b3/lib/opentelemetry/propagator/b3/text_map_extractor.rb#L48
• Java: https://github.com/open-telemetry/opentelemetry-java/blob/main/context/src/main/java/io/opentelemetry/context/propagation/TextMapPropagator.java#L118; https://github.com/open-telemetry/opentelemetry-java/blob/main/extensions/trace-propagators/src/main/java/io/opentelemetry/extension/trace/propagation/B3PropagatorExtractorSingleHeader.java#L33

[srikanthccv]

Java doesn't seem to be consistent across propagators. https://github.com/open-telemetry/opentelemetry-java/blob/18df74eb197ac0e5d0ba4e4ff87a03d6afa688d1/extensions/trace-propagators/src/main/java/io/opentelemetry/extension/trace/propagation/JaegerPropagator.java#L150-L155. As the get_current_span is handling the empty context without current-span I think it is fine just returning context as is.

[lzchen]

@lonewolf3739
Are you comments addressed? Please approve if so.

[srikanthccv]

LGTM, Thanks.

[marcinzaremba]

Thank you all for the very good feedback and discussion. It was really nice experience.