From 26d56c0e27b82b559b492f5ef011f291094f0c3c Mon Sep 17 00:00:00 2001
From: Yusuke Tsutsumi <yusuke@tsutsumi.io>
Date: Thu, 24 Oct 2019 14:06:38 -0700
Subject: [PATCH] SDK Tracer treats invalid span parent like null (#233, #235)

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).

Setting the parent to none on spans 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   | 14 ++++++
 2 files changed, 41 insertions(+), 23 deletions(-)

diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
index 46e9b83e35..879d4e6385 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 b7bfa1a915..626a5499ec 100644
--- a/opentelemetry-sdk/tests/trace/test_trace.py
+++ b/opentelemetry-sdk/tests/trace/test_trace.py
@@ -51,6 +51,20 @@ 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.
+
+        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.assertTrue(new_span.context.is_valid())
+        self.assertIsNone(new_span.parent)
+
     def test_start_span_implicit(self):
         tracer = trace.Tracer("test_start_span_implicit")