From 232aaa48225b45cf6a5993dae4469b554c2e0c84 Mon Sep 17 00:00:00 2001 From: Bastian Krol Date: Wed, 10 May 2023 09:43:20 +0200 Subject: [PATCH] fix(w3c trace context): do not pass down unknown flags. The W3C trace context specification mandates that a participant must only send known flags downstream. See https://www.w3.org/TR/trace-context/#other-flags: "The behavior of other flags, such as (00000100) is not defined and is reserved for future use. Vendors MUST set those to zero." In particular, this commit changes the way traceparent.py handles the flags. Instead of persisting the complete flags field from traceparent, we specifically parse the flag(s) that we understand and keep them as individual boolean attributes. This will also make it easier to handle the random trace ID flag correctly when updating support to W3C trace context level 2. In addition we now correctly pass down the version field as 00, since that is the traceparent version we support (instead of passing down the incoming version value). Signed-off-by: Bastian Krol --- instana/w3c_trace_context/traceparent.py | 30 ++++++++++++--------- tests/frameworks/test_django.py | 4 ++- tests/propagators/test_http_propagator.py | 8 +++--- tests/w3c_trace_context/test_traceparent.py | 16 ++++++++--- 4 files changed, 38 insertions(+), 20 deletions(-) diff --git a/instana/w3c_trace_context/traceparent.py b/instana/w3c_trace_context/traceparent.py index a3876a9ce..6c2605ff5 100644 --- a/instana/w3c_trace_context/traceparent.py +++ b/instana/w3c_trace_context/traceparent.py @@ -4,6 +4,8 @@ from ..log import logger import re +# See https://www.w3.org/TR/trace-context-2/#trace-flags for details on the bitmasks. +SAMPLED_BITMASK = 0b1; class Traceparent: SPECIFICATION_VERSION = "00" @@ -27,15 +29,16 @@ def get_traceparent_fields(traceparent): """ Parses the validated traceparent header into its fields and returns the fields :param traceparent: the original validated traceparent header - :return: version, trace_id, parent_id, trace_flags + :return: version, trace_id, parent_id, sampled_flag """ try: traceparent_properties = traceparent.split("-") version = traceparent_properties[0] trace_id = traceparent_properties[1] parent_id = traceparent_properties[2] - trace_flags = traceparent_properties[3] - return version, trace_id, parent_id, trace_flags + flags = int(traceparent_properties[3]) + sampled_flag = (flags & SAMPLED_BITMASK) == SAMPLED_BITMASK + return version, trace_id, parent_id, sampled_flag except Exception: # This method is intended to be called with a version 00 validated traceparent # This exception handling is added just for making sure we do not throw any unhandled exception # if somebody calls the method in the future without a validated traceparent @@ -51,21 +54,24 @@ def update_traceparent(self, traceparent, in_trace_id, in_span_id, level): :param level: instana level, used to determine the value of sampled flag of the traceparent header :return: the updated traceparent header """ - mask = 1 << 0 - trace_flags = 0 if traceparent is None: # modify the trace_id part only when it was not present at all trace_id = in_trace_id.zfill(32) - version = self.SPECIFICATION_VERSION else: - version, trace_id, _, trace_flags = self.get_traceparent_fields(traceparent) - trace_flags = int(trace_flags, 16) + # - We do not need the incoming upstream parent span ID for the header we sent downstream. + # - We also do not care about the incoming version: The version field we sent downstream needs to match the + # format of the traceparent header we produce here, so we always send the version _we_ support downstream, + # even if the header coming from upstream supported a different version. + # - Finally, we also do not care about the incoming sampled flag , we only need to communicate our own + # sampling decision downstream. The sampling decisions from our upstream is irrelevant for what we send + # downstream. + _, trace_id, _, _ = self.get_traceparent_fields(traceparent) parent_id = in_span_id.zfill(16) - trace_flags = (trace_flags & ~mask) | ((level << 0) & mask) - trace_flags = format(trace_flags, '0>2x') + flags = level & SAMPLED_BITMASK + flags = format(flags, '0>2x') - traceparent = "{version}-{traceid}-{parentid}-{trace_flags}".format(version=version, + traceparent = "{version}-{traceid}-{parentid}-{flags}".format(version=self.SPECIFICATION_VERSION, traceid=trace_id, parentid=parent_id, - trace_flags=trace_flags) + flags=flags) return traceparent diff --git a/tests/frameworks/test_django.py b/tests/frameworks/test_django.py index 00a4a7960..5be8c4b87 100644 --- a/tests/frameworks/test_django.py +++ b/tests/frameworks/test_django.py @@ -338,7 +338,9 @@ def test_with_incoming_context(self): self.assertEqual('1', response.headers['X-INSTANA-L']) assert ('traceparent' in response.headers) - self.assertEqual('01-4bf92f3577b34da6a3ce929d0e0e4736-{}-01'.format(django_span.s), + # The incoming traceparent header had version 01 (which does not exist at the time of writing), but since we + # support version 00, we also need to pass down 00 for the version field. + self.assertEqual('00-4bf92f3577b34da6a3ce929d0e0e4736-{}-01'.format(django_span.s), response.headers['traceparent']) assert ('tracestate' in response.headers) diff --git a/tests/propagators/test_http_propagator.py b/tests/propagators/test_http_propagator.py index e93041ae0..42284d442 100644 --- a/tests/propagators/test_http_propagator.py +++ b/tests/propagators/test_http_propagator.py @@ -28,7 +28,7 @@ def test_extract_carrier_dict(self, mock_validate, mock_get_traceparent_fields): 'X-INSTANA-L': '1, correlationType=web; correlationId=1234567890abcdef' } mock_validate.return_value = '00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01' - mock_get_traceparent_fields.return_value = ["00", "4bf92f3577b34da6a3ce929d0e0e4736", "00f067aa0ba902b7", "01"] + mock_get_traceparent_fields.return_value = ["00", "4bf92f3577b34da6a3ce929d0e0e4736", "00f067aa0ba902b7", True] ctx = self.hptc.extract(carrier) self.assertEqual(ctx.correlation_id, '1234567890abcdef') self.assertEqual(ctx.correlation_type, "web") @@ -54,7 +54,7 @@ def test_extract_carrier_list(self, mock_validate, mock_get_traceparent_fields): ('X-INSTANA-L', '1')] mock_validate.return_value = '00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01' - mock_get_traceparent_fields.return_value = ["00", "4bf92f3577b34da6a3ce929d0e0e4736", "00f067aa0ba902b7", "01"] + mock_get_traceparent_fields.return_value = ["00", "4bf92f3577b34da6a3ce929d0e0e4736", "00f067aa0ba902b7", True] ctx = self.hptc.extract(carrier) self.assertIsNone(ctx.correlation_id) self.assertIsNone(ctx.correlation_type) @@ -124,7 +124,7 @@ def test_extract_carrier_dict_corrupted_level_header(self, mock_validate, mock_g 'X-INSTANA-L': '1, correlationTypeweb; correlationId1234567890abcdef' } mock_validate.return_value = '00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01' - mock_get_traceparent_fields.return_value = ["00", "4bf92f3577b34da6a3ce929d0e0e4736", "00f067aa0ba902b7", "01"] + mock_get_traceparent_fields.return_value = ["00", "4bf92f3577b34da6a3ce929d0e0e4736", "00f067aa0ba902b7", True] ctx = self.hptc.extract(carrier) self.assertIsNone(ctx.correlation_id) self.assertIsNone(ctx.correlation_type) @@ -156,7 +156,7 @@ def test_extract_carrier_dict_level_header_not_splitable(self, mock_validate, mo 'X-INSTANA-L': ['1'] } mock_validate.return_value = '00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01' - mock_get_traceparent_fields.return_value = ["00", "4bf92f3577b34da6a3ce929d0e0e4736", "00f067aa0ba902b7", "01"] + mock_get_traceparent_fields.return_value = ["00", "4bf92f3577b34da6a3ce929d0e0e4736", "00f067aa0ba902b7", True] ctx = self.hptc.extract(carrier) self.assertIsNone(ctx.correlation_id) self.assertIsNone(ctx.correlation_type) diff --git a/tests/w3c_trace_context/test_traceparent.py b/tests/w3c_trace_context/test_traceparent.py index 11c293810..98d4b6f7d 100644 --- a/tests/w3c_trace_context/test_traceparent.py +++ b/tests/w3c_trace_context/test_traceparent.py @@ -23,21 +23,31 @@ def test_validate_traceparent_None(self): def test_get_traceparent_fields(self): traceparent = "00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01" - version, trace_id, parent_id, trace_flags = self.tp.get_traceparent_fields(traceparent) + version, trace_id, parent_id, sampled_flag = self.tp.get_traceparent_fields(traceparent) self.assertEqual(trace_id, "4bf92f3577b34da6a3ce929d0e0e4736") self.assertEqual(parent_id, "00f067aa0ba902b7") + self.assertTrue(sampled_flag) + + def test_get_traceparent_fields_unsampled(self): + traceparent = "00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-00" + version, trace_id, parent_id, sampled_flag = self.tp.get_traceparent_fields(traceparent) + self.assertEqual(trace_id, "4bf92f3577b34da6a3ce929d0e0e4736") + self.assertEqual(parent_id, "00f067aa0ba902b7") + self.assertFalse(sampled_flag) def test_get_traceparent_fields_None_input(self): traceparent = None - version, trace_id, parent_id, trace_flags = self.tp.get_traceparent_fields(traceparent) + version, trace_id, parent_id, sampled_flag = self.tp.get_traceparent_fields(traceparent) self.assertIsNone(trace_id) self.assertIsNone(parent_id) + self.assertFalse(sampled_flag) def test_get_traceparent_fields_string_input_no_dash(self): traceparent = "invalid" - version, trace_id, parent_id, trace_flags = self.tp.get_traceparent_fields(traceparent) + version, trace_id, parent_id, sampled_flag = self.tp.get_traceparent_fields(traceparent) self.assertIsNone(trace_id) self.assertIsNone(parent_id) + self.assertFalse(sampled_flag) def test_update_traceparent(self): traceparent = "00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01"