From c03f2e05f0c8d4230768bbe726abd835028c0e5f Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Wed, 22 Jul 2020 17:22:07 -0600 Subject: [PATCH 1/8] Handle B3 trace_id and span_id correctly Fixes #933 These fields are not being handled correctly when one or both of them receive invalid values. --- .../sdk/trace/propagation/b3_format.py | 22 ++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/b3_format.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/b3_format.py index c1f71d4d87e..540e3f3bdd4 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/b3_format.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/b3_format.py @@ -13,8 +13,10 @@ # limitations under the License. import typing +from re import fullmatch import opentelemetry.trace as trace +from opentelemetry.sdk.trace import generate_trace_id, generate_span_id from opentelemetry.context import Context from opentelemetry.trace.propagation.httptextformat import ( Getter, @@ -95,6 +97,13 @@ def extract( or flags ) + if ( + fullmatch(r"[\da-fA-F]{16}|[\da-fA-F]{32}", trace_id) is None or + fullmatch(r"[\da-fA-F]{16}", span_id) is None + ): + trace_id = generate_trace_id() + span_id = generate_span_id() + options = 0 # The b3 spec provides no defined behavior for both sample and # flag values set. Since the setting of at least one implies @@ -102,12 +111,19 @@ def extract( # header is set to allow. if sampled in self._SAMPLE_PROPAGATE_VALUES or flags == "1": options |= trace.TraceFlags.SAMPLED + + if isinstance(trace_id, str): + trace_id = int(trace_id, 16) + + if isinstance(span_id, str): + span_id = int(span_id, 16) + return trace.set_span_in_context( trace.DefaultSpan( trace.SpanContext( # trace an span ids are encoded in hex, so must be converted - trace_id=int(trace_id, 16), - span_id=int(span_id, 16), + trace_id=trace_id, + span_id=span_id, is_remote=True, trace_flags=trace.TraceFlags(options), trace_state=trace.TraceState(), @@ -123,7 +139,7 @@ def inject( ) -> None: span = trace.get_current_span(context=context) - if span.get_context() == trace.INVALID_SPAN_CONTEXT: + if span is None: return sampled = (trace.TraceFlags.SAMPLED & span.context.trace_flags) != 0 From b043b52af37f50b6bc0671d7235ba18b920b82d2 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Thu, 23 Jul 2020 17:33:53 -0600 Subject: [PATCH 2/8] More fixing WIP --- opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py | 6 +++++- .../src/opentelemetry/sdk/trace/propagation/b3_format.py | 1 + 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index b76ba49ad4a..569718e80bf 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -796,7 +796,11 @@ def start_span( # pylint: disable=too-many-locals ) if sampling_decision.sampled: - options = context.trace_flags | trace_api.TraceFlags.SAMPLED + # FIXME investigate this to make sure that this is the right + # approach. There seems to be an issue here currently because if + # context.trace_flags is 0, then options will be always set to 1. + # options = context.trace_flags | trace_api.TraceFlags.SAMPLED + options = context.trace_flags context.trace_flags = trace_api.TraceFlags(options) if attributes is None: span_attributes = sampling_decision.attributes diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/b3_format.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/b3_format.py index 540e3f3bdd4..f9054a0ff76 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/b3_format.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/b3_format.py @@ -103,6 +103,7 @@ def extract( ): trace_id = generate_trace_id() span_id = generate_span_id() + sampled = "0" options = 0 # The b3 spec provides no defined behavior for both sample and From 78497cefbf10c67e7b4d5b35d89541ae4660f75b Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Mon, 27 Jul 2020 09:54:05 -0600 Subject: [PATCH 3/8] Revert context trace flags handling --- opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index 569718e80bf..ebcbddba9d1 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -796,11 +796,7 @@ def start_span( # pylint: disable=too-many-locals ) if sampling_decision.sampled: - # FIXME investigate this to make sure that this is the right - # approach. There seems to be an issue here currently because if - # context.trace_flags is 0, then options will be always set to 1. - # options = context.trace_flags | trace_api.TraceFlags.SAMPLED - options = context.trace_flags + options = context.trace_flags | trace_api.TraceFlags.SAMPLED context.trace_flags = trace_api.TraceFlags(options) if attributes is None: span_attributes = sampling_decision.attributes @@ -868,7 +864,7 @@ def __init__( shutdown_on_exit: bool = True, active_span_processor: Union[ SynchronousMultiSpanProcessor, ConcurrentMultiSpanProcessor - ] = None, + ]=None, ): self._active_span_processor = ( active_span_processor or SynchronousMultiSpanProcessor() From d0360a1b66752169c38a0513c085b37adc2c6e03 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Mon, 27 Jul 2020 12:33:24 -0600 Subject: [PATCH 4/8] Fix lint --- opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py | 2 +- .../src/opentelemetry/sdk/trace/propagation/b3_format.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index ebcbddba9d1..b76ba49ad4a 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -864,7 +864,7 @@ def __init__( shutdown_on_exit: bool = True, active_span_processor: Union[ SynchronousMultiSpanProcessor, ConcurrentMultiSpanProcessor - ]=None, + ] = None, ): self._active_span_processor = ( active_span_processor or SynchronousMultiSpanProcessor() diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/b3_format.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/b3_format.py index f9054a0ff76..71d7eb1dee2 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/b3_format.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/b3_format.py @@ -16,8 +16,8 @@ from re import fullmatch import opentelemetry.trace as trace -from opentelemetry.sdk.trace import generate_trace_id, generate_span_id from opentelemetry.context import Context +from opentelemetry.sdk.trace import generate_span_id, generate_trace_id from opentelemetry.trace.propagation.httptextformat import ( Getter, HTTPTextFormat, @@ -98,8 +98,8 @@ def extract( ) if ( - fullmatch(r"[\da-fA-F]{16}|[\da-fA-F]{32}", trace_id) is None or - fullmatch(r"[\da-fA-F]{16}", span_id) is None + fullmatch(r"[\da-fA-F]{16}|[\da-fA-F]{32}", trace_id) is None + or fullmatch(r"[\da-fA-F]{16}", span_id) is None ): trace_id = generate_trace_id() span_id = generate_span_id() From 96d50725380e426a532e541886a8146509756c42 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Mon, 27 Jul 2020 12:39:45 -0600 Subject: [PATCH 5/8] Revert return crterium --- .../src/opentelemetry/sdk/trace/propagation/b3_format.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/b3_format.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/b3_format.py index 71d7eb1dee2..a34b8cdd53d 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/b3_format.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/b3_format.py @@ -140,7 +140,7 @@ def inject( ) -> None: span = trace.get_current_span(context=context) - if span is None: + if span.get_context() == trace.INVALID_SPAN_CONTEXT: return sampled = (trace.TraceFlags.SAMPLED & span.context.trace_flags) != 0 From d754b586bbed492923b7ef804d3ea71563da5898 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Wed, 5 Aug 2020 13:03:10 -0600 Subject: [PATCH 6/8] Use compiled regexes --- .../src/opentelemetry/sdk/trace/propagation/b3_format.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/b3_format.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/b3_format.py index a34b8cdd53d..22fc1e34945 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/b3_format.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/b3_format.py @@ -13,7 +13,7 @@ # limitations under the License. import typing -from re import fullmatch +from re import compile as re_compile import opentelemetry.trace as trace from opentelemetry.context import Context @@ -39,6 +39,8 @@ class B3Format(HTTPTextFormat): SAMPLED_KEY = "x-b3-sampled" FLAGS_KEY = "x-b3-flags" _SAMPLE_PROPAGATE_VALUES = set(["1", "True", "true", "d"]) + _trace_id_regex = re_compile(r"[\da-fA-F]{16}|[\da-fA-F]{32}") + _span_id_regex = re_compile(r"[\da-fA-F]{16}") def extract( self, @@ -98,8 +100,8 @@ def extract( ) if ( - fullmatch(r"[\da-fA-F]{16}|[\da-fA-F]{32}", trace_id) is None - or fullmatch(r"[\da-fA-F]{16}", span_id) is None + self._trace_id_regex.fullmatch(trace_id) is None + or self._span_id_regex.fullmatch(span_id) is None ): trace_id = generate_trace_id() span_id = generate_span_id() From 22eb3dc77c7b0ff0ddac8283b924536acc70edb4 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Wed, 5 Aug 2020 14:15:50 -0600 Subject: [PATCH 7/8] WIP --- .../opentelemetry/sdk/trace/propagation/b3_format.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/b3_format.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/b3_format.py index 22fc1e34945..901a5772f83 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/b3_format.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/propagation/b3_format.py @@ -107,6 +107,10 @@ def extract( span_id = generate_span_id() sampled = "0" + else: + 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 @@ -115,12 +119,6 @@ def extract( if sampled in self._SAMPLE_PROPAGATE_VALUES or flags == "1": options |= trace.TraceFlags.SAMPLED - if isinstance(trace_id, str): - trace_id = int(trace_id, 16) - - if isinstance(span_id, str): - span_id = int(span_id, 16) - return trace.set_span_in_context( trace.DefaultSpan( trace.SpanContext( From 1a0a03972fa0b867a51674f6ade4ccf675740626 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Wed, 5 Aug 2020 16:21:51 -0600 Subject: [PATCH 8/8] Add test case --- .../tests/trace/propagation/test_b3_format.py | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/opentelemetry-sdk/tests/trace/propagation/test_b3_format.py b/opentelemetry-sdk/tests/trace/propagation/test_b3_format.py index a5bd1baaa48..bc508f3fd91 100644 --- a/opentelemetry-sdk/tests/trace/propagation/test_b3_format.py +++ b/opentelemetry-sdk/tests/trace/propagation/test_b3_format.py @@ -13,6 +13,7 @@ # limitations under the License. import unittest +from unittest.mock import patch import opentelemetry.sdk.trace as trace import opentelemetry.sdk.trace.propagation.b3_format as b3_format @@ -245,6 +246,50 @@ def test_missing_trace_id(self): span_context = trace_api.get_current_span(ctx).get_context() self.assertEqual(span_context.trace_id, trace_api.INVALID_TRACE_ID) + @patch("opentelemetry.sdk.trace.propagation.b3_format.generate_trace_id") + @patch("opentelemetry.sdk.trace.propagation.b3_format.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.""" + + mock_generate_trace_id.configure_mock(return_value=1) + mock_generate_span_id.configure_mock(return_value=2) + + carrier = { + FORMAT.TRACE_ID_KEY: "abc123", + FORMAT.SPAN_ID_KEY: self.serialized_span_id, + FORMAT.FLAGS_KEY: "1", + } + + ctx = FORMAT.extract(get_as_list, carrier) + span_context = trace_api.get_current_span(ctx).get_context() + + self.assertEqual(span_context.trace_id, 1) + self.assertEqual(span_context.span_id, 2) + + @patch("opentelemetry.sdk.trace.propagation.b3_format.generate_trace_id") + @patch("opentelemetry.sdk.trace.propagation.b3_format.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) + + carrier = { + FORMAT.TRACE_ID_KEY: self.serialized_trace_id, + FORMAT.SPAN_ID_KEY: "abc123", + FORMAT.FLAGS_KEY: "1", + } + + ctx = FORMAT.extract(get_as_list, carrier) + span_context = trace_api.get_current_span(ctx).get_context() + + self.assertEqual(span_context.trace_id, 1) + self.assertEqual(span_context.span_id, 2) + def test_missing_span_id(self): """If a trace id is missing, populate an invalid trace id.""" carrier = {