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.

Sign up for GitHub

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

Make B3 propagator spec compliant #1728

Merged
merged 6 commits into from
Mar 31, 2021
Merged

Make B3 propagator spec compliant #1728

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
711cda4
Make B3 propagator spec-compliant
Mar 30, 2021
1e68c97
Relax assertions to be accurate with spec, separate tests for better …
Mar 31, 2021
cc4f8e0
Fix lint and add a changelog entry
Mar 31, 2021
92ae1a9
Merge branch 'main' into non-compliant-propagators
Mar 31, 2021
bc227ef
Correct changelog merge
Mar 31, 2021
9a8b475
Update CHANGELOG.md
marcinzaremba Mar 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased](https://github.com/open-telemetry/opentelemetry-python/compare/v1.0.0...HEAD)

### Added
- Added `py.typed` file to every package. This should resolve a bunch of mypy
errors for users.
([#1720](https://github.com/open-telemetry/opentelemetry-python/pull/1720))

### Changed
- Adjust `B3Format` propagator to be spec compliant by not modifying context
when propagation headers are not present/invalid/empty
([#1728]https://github.com/open-telemetry/opentelemetry-python/pull/1728)
marcinzaremba marked this conversation as resolved.
Show resolved Hide resolved


## [1.0.0](https://github.com/open-telemetry/opentelemetry-python/releases/tag/v1.0.0) - 2021-03-26
### Added
Expand Down
21 changes: 8 additions & 13 deletions propagator/opentelemetry-propagator-b3/src/opentelemetry/propagators/b3/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ def extract(
context: typing.Optional[Context] = None,
getter: Getter = default_getter,
) -> Context:
trace_id = format_trace_id(trace.INVALID_TRACE_ID)
span_id = format_span_id(trace.INVALID_SPAN_ID)
trace_id = trace.INVALID_TRACE_ID
span_id = trace.INVALID_SPAN_ID
sampled = "0"
flags = None

Expand All @@ -73,8 +73,6 @@ def extract(
trace_id, span_id, sampled = fields
elif len(fields) == 4:
trace_id, span_id, sampled, _ = fields
else:
return trace.set_span_in_context(trace.INVALID_SPAN)
else:
trace_id = (
_extract_first_element(getter.get(carrier, self.TRACE_ID_KEY))
Expand All @@ -94,18 +92,15 @@ def extract(
)

if (
self._trace_id_regex.fullmatch(trace_id) is None
trace_id == trace.INVALID_TRACE_ID
or span_id == trace.INVALID_SPAN_ID
or self._trace_id_regex.fullmatch(trace_id) is None
srikanthccv marked this conversation as resolved.
Show resolved Hide resolved
or self._span_id_regex.fullmatch(span_id) is None
):
id_generator = trace.get_tracer_provider().id_generator
trace_id = id_generator.generate_trace_id()
span_id = id_generator.generate_span_id()
sampled = "0"

else:
trace_id = int(trace_id, 16)
span_id = int(span_id, 16)
return context
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@marcinzaremba marcinzaremba Mar 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@marcinzaremba marcinzaremba Mar 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@srikanthccv srikanthccv Mar 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


trace_id = int(trace_id, 16)
span_id = int(span_id, 16)
options = 0
# The b3 spec provides no defined behavior for both sample and
# flag values set. Since the setting of at least one implies
Expand Down
92 changes: 38 additions & 54 deletions propagator/opentelemetry-propagator-b3/tests/test_b3_format.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
# limitations under the License.

import unittest
from unittest.mock import Mock, patch
from unittest.mock import Mock

import opentelemetry.propagators.b3 as b3_format # pylint: disable=no-name-in-module,import-error
import opentelemetry.sdk.trace as trace
Expand Down Expand Up @@ -231,89 +231,73 @@ def test_64bit_trace_id(self):
new_carrier[FORMAT.TRACE_ID_KEY], "0" * 16 + trace_id_64_bit
)

def test_invalid_single_header(self):
"""If an invalid single header is passed, return an
invalid SpanContext.
"""
def test_extract_invalid_single_header(self):
"""Given unparsable header, do not modify context"""
old_ctx = {}

carrier = {FORMAT.SINGLE_HEADER_KEY: "0-1-2-3-4-5-6-7"}
ctx = FORMAT.extract(carrier)
span_context = trace_api.get_current_span(ctx).get_span_context()
self.assertEqual(span_context.trace_id, trace_api.INVALID_TRACE_ID)
self.assertEqual(span_context.span_id, trace_api.INVALID_SPAN_ID)
new_ctx = FORMAT.extract(carrier, old_ctx)

self.assertDictEqual(new_ctx, old_ctx)

def test_extract_missing_trace_id(self):
"""Given no trace ID, do not modify context"""
old_ctx = {}

def test_missing_trace_id(self):
"""If a trace id is missing, populate an invalid trace id."""
carrier = {
FORMAT.SPAN_ID_KEY: self.serialized_span_id,
FORMAT.FLAGS_KEY: "1",
}
new_ctx = FORMAT.extract(carrier, old_ctx)

ctx = FORMAT.extract(carrier)
span_context = trace_api.get_current_span(ctx).get_span_context()
self.assertEqual(span_context.trace_id, trace_api.INVALID_TRACE_ID)

@patch(
"opentelemetry.sdk.trace.id_generator.RandomIdGenerator.generate_trace_id"
)
@patch(
"opentelemetry.sdk.trace.id_generator.RandomIdGenerator.generate_span_id"
)
def test_invalid_trace_id(
self, mock_generate_span_id, mock_generate_trace_id
):
"""If a trace id is invalid, generate a trace id."""
self.assertDictEqual(new_ctx, old_ctx)

mock_generate_trace_id.configure_mock(return_value=1)
mock_generate_span_id.configure_mock(return_value=2)
def test_extract_invalid_trace_id(self):
"""Given invalid trace ID, do not modify context"""
old_ctx = {}

carrier = {
FORMAT.TRACE_ID_KEY: "abc123",
FORMAT.SPAN_ID_KEY: self.serialized_span_id,
FORMAT.FLAGS_KEY: "1",
}
new_ctx = FORMAT.extract(carrier, old_ctx)

ctx = FORMAT.extract(carrier)
span_context = trace_api.get_current_span(ctx).get_span_context()
self.assertDictEqual(new_ctx, old_ctx)

self.assertEqual(span_context.trace_id, 1)
self.assertEqual(span_context.span_id, 2)

@patch(
"opentelemetry.sdk.trace.id_generator.RandomIdGenerator.generate_trace_id"
)
@patch(
"opentelemetry.sdk.trace.id_generator.RandomIdGenerator.generate_span_id"
)
def test_invalid_span_id(
self, mock_generate_span_id, mock_generate_trace_id
):
"""If a span id is invalid, generate a trace id."""

mock_generate_trace_id.configure_mock(return_value=1)
mock_generate_span_id.configure_mock(return_value=2)
def test_extract_invalid_span_id(self):
"""Given invalid span ID, do not modify context"""
old_ctx = {}

carrier = {
FORMAT.TRACE_ID_KEY: self.serialized_trace_id,
FORMAT.SPAN_ID_KEY: "abc123",
FORMAT.FLAGS_KEY: "1",
}
new_ctx = FORMAT.extract(carrier, old_ctx)

ctx = FORMAT.extract(carrier)
span_context = trace_api.get_current_span(ctx).get_span_context()
self.assertDictEqual(new_ctx, old_ctx)

self.assertEqual(span_context.trace_id, 1)
self.assertEqual(span_context.span_id, 2)
def test_extract_missing_span_id(self):
"""Given no span ID, do not modify context"""
old_ctx = {}

def test_missing_span_id(self):
"""If a trace id is missing, populate an invalid trace id."""
carrier = {
FORMAT.TRACE_ID_KEY: self.serialized_trace_id,
FORMAT.FLAGS_KEY: "1",
}
new_ctx = FORMAT.extract(carrier, old_ctx)

self.assertDictEqual(new_ctx, old_ctx)

def test_extract_empty_carrier(self):
"""Given no headers at all, do not modify context"""
old_ctx = {}

carrier = {}
new_ctx = FORMAT.extract(carrier, old_ctx)

ctx = FORMAT.extract(carrier)
span_context = trace_api.get_current_span(ctx).get_span_context()
self.assertEqual(span_context.span_id, trace_api.INVALID_SPAN_ID)
self.assertDictEqual(new_ctx, old_ctx)

@staticmethod
def test_inject_empty_context():
Expand Down