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

@@ -364,21 +364,29 @@ def create_span(
         span_id = generate_span_id()
         if parent is Tracer.CURRENT_SPAN:
             parent = self.get_current_span()
-        if parent in {None, trace_api.INVALID_SPAN_CONTEXT}:
-            context = trace_api.SpanContext(generate_trace_id(), 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 = 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:
-                raise TypeError
-            context = trace_api.SpanContext(
-                parent_context.trace_id,
-                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
+        )
         return Span(
             name=name,
             context=context,

opentelemetry-sdk/tests/trace/test_trace.py

@@ -30,14 +30,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")