From 84c8cf54c873afe3740b7a181fb710b951905f38 Mon Sep 17 00:00:00 2001 From: Yusuke Tsutsumi Date: Wed, 23 Oct 2019 21:31:16 -0700 Subject: [PATCH 1/2] SDK Tracer treats invalid span parent like null Fixes #233. The SDK tracer will now create spans with invalid parents as brand new spans, similar to not having a parent at all. Adding this behavior to the Tracer ensures that integrations do not have to handle invalid span contexts in their own code, and ensures that behavior is consistent with w3c tracecontext (which specifies invalid results should be handled by creating new spans). --- opentelemetry-sdk/tests/trace/test_trace.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/opentelemetry-sdk/tests/trace/test_trace.py b/opentelemetry-sdk/tests/trace/test_trace.py index b7bfa1a9158..545fe7be95b 100644 --- a/opentelemetry-sdk/tests/trace/test_trace.py +++ b/opentelemetry-sdk/tests/trace/test_trace.py @@ -51,6 +51,18 @@ def test_sampler_no_sampling(self): class TestSpanCreation(unittest.TestCase): + def test_create_span_invalid_spancontext(self): + """If an invalid span context is passed as the parent, the created + span should use a new span id. + """ + tracer = trace.Tracer("test_create_span_invalid_spancontext") + new_span = tracer.create_span( + "root", parent=trace_api.INVALID_SPAN_CONTEXT + ) + self.assertNotEqual( + new_span.context.span_id, trace_api.INVALID_SPAN_ID + ) + def test_start_span_implicit(self): tracer = trace.Tracer("test_start_span_implicit") From 4ee81ab4f96941eee2c349df1fb3ab854c462cb4 Mon Sep 17 00:00:00 2001 From: Yusuke Tsutsumi Date: Thu, 24 Oct 2019 09:01:40 -0700 Subject: [PATCH 2/2] Addressing review feedback Tracer.create_span now checks is_valid for validity of the SpanContext. This is more comprehensive than a simple identity check for INVALID_SPAN_CONTEXT. Setting the parent to none if the parent context is invalid, reducing logic to handle that situation in downstream processing like exporters. --- .../src/opentelemetry/sdk/trace/__init__.py | 50 ++++++++++--------- opentelemetry-sdk/tests/trace/test_trace.py | 8 +-- 2 files changed, 32 insertions(+), 26 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index 46e9b83e35c..879d4e63858 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -376,30 +376,33 @@ def create_span( as a child of the current span in this tracer's context, or as a root span if no current span exists. """ + span_id = generate_span_id() if parent is Tracer.CURRENT_SPAN: parent = self.get_current_span() - if parent is None: - parent_context = None - new_span_context = trace_api.SpanContext( - generate_trace_id(), generate_span_id() - ) + parent_context = parent + if isinstance(parent_context, trace_api.Span): + parent_context = parent.get_context() + + if parent_context is not None and not isinstance( + parent_context, trace_api.SpanContext + ): + raise TypeError + + if parent_context is None or not parent_context.is_valid(): + parent = parent_context = None + trace_id = generate_trace_id() + trace_options = None + trace_state = None else: - if isinstance(parent, trace_api.Span): - parent_context = parent.get_context() - elif isinstance(parent, trace_api.SpanContext): - parent_context = parent - else: - # TODO: error handling - raise TypeError - - new_span_context = trace_api.SpanContext( - parent_context.trace_id, - generate_span_id(), - parent_context.trace_options, - parent_context.trace_state, - ) + trace_id = parent_context.trace_id + trace_options = parent_context.trace_options + trace_state = parent_context.trace_state + + context = trace_api.SpanContext( + trace_id, span_id, trace_options, trace_state + ) # The sampler decides whether to create a real or no-op span at the # time of span creation. No-op spans do not record events, and are not @@ -408,8 +411,8 @@ def create_span( # to include information about the sampling decision. sampling_decision = self.sampler.should_sample( parent_context, - new_span_context.trace_id, - new_span_context.span_id, + context.trace_id, + context.span_id, name, {}, # TODO: links ) @@ -417,14 +420,15 @@ def create_span( if sampling_decision.sampled: return Span( name=name, - context=new_span_context, + context=context, parent=parent, sampler=self.sampler, attributes=sampling_decision.attributes, span_processor=self._active_span_processor, kind=kind, ) - return trace_api.DefaultSpan(context=new_span_context) + + return trace_api.DefaultSpan(context=context) @contextmanager def use_span( diff --git a/opentelemetry-sdk/tests/trace/test_trace.py b/opentelemetry-sdk/tests/trace/test_trace.py index 545fe7be95b..626a5499ecf 100644 --- a/opentelemetry-sdk/tests/trace/test_trace.py +++ b/opentelemetry-sdk/tests/trace/test_trace.py @@ -54,14 +54,16 @@ class TestSpanCreation(unittest.TestCase): def test_create_span_invalid_spancontext(self): """If an invalid span context is passed as the parent, the created span should use a new span id. + + Invalid span contexts should also not be added as a parent. This + eliminates redundant error handling logic in exporters. """ tracer = trace.Tracer("test_create_span_invalid_spancontext") new_span = tracer.create_span( "root", parent=trace_api.INVALID_SPAN_CONTEXT ) - self.assertNotEqual( - new_span.context.span_id, trace_api.INVALID_SPAN_ID - ) + self.assertTrue(new_span.context.is_valid()) + self.assertIsNone(new_span.parent) def test_start_span_implicit(self): tracer = trace.Tracer("test_start_span_implicit")