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.

Author: toumorokoshi

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,23 +411,24 @@ 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
         )
 
         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(

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