diff --git a/.changelog/33.fixed b/.changelog/33.fixed new file mode 100644 index 00000000000..0f8aae6aa67 --- /dev/null +++ b/.changelog/33.fixed @@ -0,0 +1,2 @@ +Jaeger propagator no longer forces the DEBUG flag on inject; a normally-sampled span now propagates only the SAMPLED flag +`ParentBased` sampler now forwards the incoming `trace_state` to its delegate sampler, and the SDK now passes the parent's `trace_state` into `should_sample` diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index fe800c3695a..910ebb926d1 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -1221,8 +1221,19 @@ def start_span( # pylint: disable=too-many-locals # The sampler may also add attributes to the newly-created span, e.g. # to include information about the sampling result. # The sampler may also modify the parent span context's tracestate + trace_state = ( + parent_span_context.trace_state + if parent_span_context is not None + else None + ) sampling_result = self.sampler.should_sample( - context, trace_id, name, kind, attributes, links + context, + trace_id, + name, + kind, + attributes, + links, + trace_state, ) trace_flags = ( diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/sampling.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/sampling.py index 1652220dfc9..b1b52e4172a 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/sampling.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/sampling.py @@ -363,6 +363,7 @@ def should_sample( kind=kind, attributes=attributes, links=links, + trace_state=trace_state, ) def get_description(self): diff --git a/opentelemetry-sdk/tests/trace/test_sampling.py b/opentelemetry-sdk/tests/trace/test_sampling.py index 1d33a1a2c2c..c9e3151a9a9 100644 --- a/opentelemetry-sdk/tests/trace/test_sampling.py +++ b/opentelemetry-sdk/tests/trace/test_sampling.py @@ -524,3 +524,52 @@ def implicit_parent_context(span: trace.Span): context_api.detach(token) self.exec_parent_based(implicit_parent_context) + + def test_parent_based_forwards_trace_state_to_delegate(self): + # ParentBased must forward the trace_state it receives to the delegate + # sampler it selects, so that custom delegate samplers can act on it. + class _RecordingSampler(sampling.Sampler): + def __init__(self): + self.received_trace_state = "not-called" + + def should_sample( + self, + parent_context, + trace_id, + name, + kind=None, + attributes=None, + links=None, + trace_state=None, + ): + self.received_trace_state = trace_state + return sampling.ALWAYS_ON.should_sample( + parent_context, + trace_id, + name, + kind, + attributes, + links, + trace_state, + ) + + def get_description(self): + return "RecordingSampler" + + trace_state = trace.TraceState([("key", "value")]) + delegate = _RecordingSampler() + sampler = sampling.ParentBased( + root=sampling.ALWAYS_OFF, + remote_parent_sampled=delegate, + ) + context = self._create_parent( + TO_SAMPLED, is_remote=True, trace_state=trace_state + ) + sampler.should_sample( + context, + 0x8000000000000000, + "remote sampled parent", + trace.SpanKind.INTERNAL, + trace_state=trace_state, + ) + self.assertEqual(delegate.received_trace_state, trace_state) diff --git a/propagator/opentelemetry-propagator-jaeger/src/opentelemetry/propagators/jaeger/__init__.py b/propagator/opentelemetry-propagator-jaeger/src/opentelemetry/propagators/jaeger/__init__.py index 79bb1d89ea8..155c76eafa5 100644 --- a/propagator/opentelemetry-propagator-jaeger/src/opentelemetry/propagators/jaeger/__init__.py +++ b/propagator/opentelemetry-propagator-jaeger/src/opentelemetry/propagators/jaeger/__init__.py @@ -73,9 +73,12 @@ def inject( span_parent_id = ( span.parent.span_id if span.is_recording() and span.parent else 0 ) - trace_flags = span_context.trace_flags - if trace_flags.sampled: - trace_flags |= self.DEBUG_FLAG + # Only propagate the SAMPLED flag. The DEBUG flag must not be set on + # inject for a normally-sampled span, since that would force downstream + # Jaeger components into a forced-keep decision. + trace_flags = trace.TraceFlags( + span_context.trace_flags & trace.TraceFlags.SAMPLED + ) # set span identity setter.set( diff --git a/propagator/opentelemetry-propagator-jaeger/tests/test_jaeger_propagator.py b/propagator/opentelemetry-propagator-jaeger/tests/test_jaeger_propagator.py index 481b56b9196..e55303132fc 100644 --- a/propagator/opentelemetry-propagator-jaeger/tests/test_jaeger_propagator.py +++ b/propagator/opentelemetry-propagator-jaeger/tests/test_jaeger_propagator.py @@ -93,14 +93,14 @@ def test_sampled_flag_set(self): ) self.assertEqual(1, sample_flag_value) - def test_debug_flag_set(self): + def test_debug_flag_not_set_for_sampled_span(self): + # Injecting a normally-sampled span must set only the SAMPLED bit and + # must never force the Jaeger DEBUG (forced-keep) flag. old_carrier = {FORMAT.TRACE_ID_KEY: self.serialized_uber_trace_id} _, new_carrier = get_context_new_carrier(old_carrier) - debug_flag_value = ( - int(new_carrier[FORMAT.TRACE_ID_KEY].split(":")[3]) - & FORMAT.DEBUG_FLAG - ) - self.assertEqual(FORMAT.DEBUG_FLAG, debug_flag_value) + flags = int(new_carrier[FORMAT.TRACE_ID_KEY].split(":")[3]) + self.assertEqual(1, flags & 0x01) + self.assertEqual(0, flags & FORMAT.DEBUG_FLAG) def test_sample_debug_flags_unset(self): uber_trace_id = jaeger._format_uber_trace_id( # pylint: disable=protected-access