sdk: span parents are now always spancontext (open-telemetry#548)

Exporter and span handling complexity was increasing due to the Span.parent attribute being either a Span or a SpanContext. As there is no requirement in the specification to have the parent attribute be a Span, removing this complexity and handling. Co-authored-by: Chris Kleinknecht <[email protected]> Co-authored-by: Alex Boten <[email protected]>
ocelotl · Apr 25, 2020 · 5f188f7 · 5f188f7
1 parent 216bd5a
commit 5f188f7
Show file tree

Hide file tree

Showing 11 changed files with 27 additions and 36 deletions.
diff --git a/ext/opentelemetry-ext-docker-tests/tests/mysql/test_mysql_functional.py b/ext/opentelemetry-ext-docker-tests/tests/mysql/test_mysql_functional.py
@@ -65,7 +65,7 @@ def validate_spans(self):
         self.assertEqual(root_span.name, "rootSpan")
         self.assertEqual(db_span.name, "mysql.opentelemetry-tests")
         self.assertIsNotNone(db_span.parent)
-        self.assertEqual(db_span.parent.name, root_span.name)
+        self.assertIs(db_span.parent, root_span.get_context())
         self.assertIs(db_span.kind, trace_api.SpanKind.CLIENT)
         self.assertEqual(db_span.attributes["db.instance"], MYSQL_DB_NAME)
         self.assertEqual(db_span.attributes["net.peer.name"], MYSQL_HOST)

diff --git a/ext/opentelemetry-ext-docker-tests/tests/postgres/test_psycopg_functional.py b/ext/opentelemetry-ext-docker-tests/tests/postgres/test_psycopg_functional.py
@@ -68,7 +68,7 @@ def validate_spans(self):
         self.assertEqual(root_span.name, "rootSpan")
         self.assertEqual(child_span.name, "postgresql.opentelemetry-tests")
         self.assertIsNotNone(child_span.parent)
-        self.assertEqual(child_span.parent.name, root_span.name)
+        self.assertIs(child_span.parent, root_span.get_context())
         self.assertIs(child_span.kind, trace_api.SpanKind.CLIENT)
         self.assertEqual(
             child_span.attributes["db.instance"], POSTGRES_DB_NAME

diff --git a/ext/opentelemetry-ext-docker-tests/tests/pymongo/test_pymongo_functional.py b/ext/opentelemetry-ext-docker-tests/tests/pymongo/test_pymongo_functional.py
@@ -51,7 +51,7 @@ def validate_spans(self):
         self.assertIsNot(root_span, None)
         self.assertIsNot(pymongo_span, None)
         self.assertIsNotNone(pymongo_span.parent)
-        self.assertEqual(pymongo_span.parent.name, root_span.name)
+        self.assertIs(pymongo_span.parent, root_span.get_context())
         self.assertIs(pymongo_span.kind, trace_api.SpanKind.CLIENT)
         self.assertEqual(
             pymongo_span.attributes["db.instance"], MONGODB_DB_NAME

diff --git a/ext/opentelemetry-ext-docker-tests/tests/pymysql/test_pymysql_functional.py b/ext/opentelemetry-ext-docker-tests/tests/pymysql/test_pymysql_functional.py
@@ -76,7 +76,7 @@ def validate_spans(self):
         self.assertEqual(root_span.name, "rootSpan")
         self.assertEqual(db_span.name, "mysql.opentelemetry-tests")
         self.assertIsNotNone(db_span.parent)
-        self.assertEqual(db_span.parent.name, root_span.name)
+        self.assertIs(db_span.parent, root_span.get_context())
         self.assertIs(db_span.kind, trace_api.SpanKind.CLIENT)
         self.assertEqual(db_span.attributes["db.instance"], MYSQL_DB_NAME)
         self.assertEqual(db_span.attributes["net.peer.name"], MYSQL_HOST)

diff --git a/ext/opentelemetry-ext-jaeger/src/opentelemetry/ext/jaeger/__init__.py b/ext/opentelemetry-ext-jaeger/src/opentelemetry/ext/jaeger/__init__.py
@@ -194,11 +194,7 @@ def _translate_to_jaeger(spans: Span):
 
         status = span.status
 
-        parent_id = 0
-        if isinstance(span.parent, trace_api.Span):
-            parent_id = span.parent.get_context().span_id
-        elif isinstance(span.parent, trace_api.SpanContext):
-            parent_id = span.parent.span_id
+        parent_id = span.parent.span_id if span.parent else 0
 
         tags = _extract_tags(span.attributes)
 

diff --git a/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py b/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py
@@ -281,7 +281,8 @@ def test_parent_child_implicit(self):
 
                 self.assertEqual(parent_trace_id, child_trace_id)
                 self.assertEqual(
-                    child.span.unwrap().parent, parent.span.unwrap()
+                    child.span.unwrap().parent,
+                    parent.span.unwrap().get_context(),
                 )
 
             # Verify parent span becomes the active span again.
@@ -309,7 +310,9 @@ def test_parent_child_explicit_span(self):
                 child_trace_id = child.span.unwrap().get_context().trace_id
 
                 self.assertEqual(child_trace_id, parent_trace_id)
-                self.assertEqual(child.span.unwrap().parent, parent.unwrap())
+                self.assertEqual(
+                    child.span.unwrap().parent, parent.unwrap().get_context()
+                )
 
         with self.shim.start_span("ParentSpan") as parent:
             child = self.shim.start_span("ChildSpan", child_of=parent)
@@ -318,7 +321,9 @@ def test_parent_child_explicit_span(self):
             child_trace_id = child.unwrap().get_context().trace_id
 
             self.assertEqual(child_trace_id, parent_trace_id)
-            self.assertEqual(child.unwrap().parent, parent.unwrap())
+            self.assertEqual(
+                child.unwrap().parent, parent.unwrap().get_context()
+            )
 
             child.finish()
 

diff --git a/...pentelemetry-ext-otcollector/src/opentelemetry/ext/otcollector/trace_exporter/__init__.py b/...pentelemetry-ext-otcollector/src/opentelemetry/ext/otcollector/trace_exporter/__init__.py
@@ -109,9 +109,7 @@ def translate_to_collector(spans: Sequence[Span]):
         )
 
         parent_id = 0
-        if isinstance(span.parent, trace_api.Span):
-            parent_id = span.parent.get_context().span_id
-        elif isinstance(span.parent, trace_api.SpanContext):
+        if span.parent is not None:
             parent_id = span.parent.span_id
 
         collector_span.parent_span_id = parent_id.to_bytes(8, "big")
@@ -157,18 +155,7 @@ def translate_to_collector(spans: Sequence[Span]):
                 collector_span_link.type = (
                     trace_pb2.Span.Link.Type.TYPE_UNSPECIFIED
                 )
-
-                if isinstance(span.parent, trace_api.Span):
-                    if (
-                        link.context.span_id
-                        == span.parent.get_context().span_id
-                        and link.context.trace_id
-                        == span.parent.get_context().trace_id
-                    ):
-                        collector_span_link.type = (
-                            trace_pb2.Span.Link.Type.PARENT_LINKED_SPAN
-                        )
-                elif isinstance(span.parent, trace_api.SpanContext):
+                if span.parent is not None:
                     if (
                         link.context.span_id == span.parent.span_id
                         and link.context.trace_id == span.parent.trace_id

diff --git a/ext/opentelemetry-ext-otcollector/tests/test_otcollector_trace_exporter.py b/ext/opentelemetry-ext-otcollector/tests/test_otcollector_trace_exporter.py
@@ -135,7 +135,10 @@ def test_translate_to_collector(self):
             kind=trace_api.SpanKind.SERVER,
         )
         span_3 = trace.Span(
-            name="test3", context=other_context, links=(link_2,), parent=span_2
+            name="test3",
+            context=other_context,
+            links=(link_2,),
+            parent=span_2.get_context(),
         )
         otel_spans = [span_1, span_2, span_3]
         otel_spans[0].start(start_time=start_times[0])

diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
@@ -198,8 +198,8 @@ class Span(trace_api.Span):
     Args:
         name: The name of the operation this span represents
         context: The immutable span context
-        parent: This span's parent, may be a `SpanContext` if the parent is
-            remote, null if this is a root span
+        parent: This span's parent's `SpanContext`, or
+            null if this is a root span
         sampler: The sampler used to create this span
         trace_config: TODO
         resource: Entity producing telemetry
@@ -219,7 +219,7 @@ def __init__(
         self,
         name: str,
         context: trace_api.SpanContext,
-        parent: trace_api.ParentSpan = None,
+        parent: Optional[trace_api.SpanContext] = None,
         sampler: Optional[sampling.Sampler] = None,
         trace_config: None = None,  # TODO
         resource: None = None,
@@ -594,7 +594,7 @@ def start_span(  # pylint: disable=too-many-locals
         if parent_context is not None and not isinstance(
             parent_context, trace_api.SpanContext
         ):
-            raise TypeError
+            raise TypeError("parent must be a Span, SpanContext or None.")
 
         if parent_context is None or not parent_context.is_valid():
             parent = parent_context = None
@@ -640,7 +640,7 @@ def start_span(  # pylint: disable=too-many-locals
             span = Span(
                 name=name,
                 context=context,
-                parent=parent,
+                parent=parent_context,
                 sampler=self.source.sampler,
                 resource=self.source.resource,
                 attributes=span_attributes,

diff --git a/opentelemetry-sdk/tests/context/test_asyncio.py b/opentelemetry-sdk/tests/context/test_asyncio.py
@@ -108,4 +108,4 @@ def test_with_asyncio(self):
         for span in span_list:
             if span is expected_parent:
                 continue
-            self.assertEqual(span.parent, expected_parent)
+            self.assertEqual(span.parent, expected_parent.get_context())
diff --git a/opentelemetry-sdk/tests/trace/test_trace.py b/opentelemetry-sdk/tests/trace/test_trace.py
@@ -243,7 +243,7 @@ def test_start_span_implicit(self):
             with tracer.start_span(
                 "child", kind=trace_api.SpanKind.CLIENT
             ) as child:
-                self.assertIs(child.parent, root)
+                self.assertIs(child.parent, root.get_context())
                 self.assertEqual(child.kind, trace_api.SpanKind.CLIENT)
 
                 self.assertIsNotNone(child.start_time)
@@ -332,7 +332,7 @@ def test_start_as_current_span_implicit(self):
 
             with tracer.start_as_current_span("child") as child:
                 self.assertIs(tracer.get_current_span(), child)
-                self.assertIs(child.parent, root)
+                self.assertIs(child.parent, root.get_context())
 
             # After exiting the child's scope the parent should become the
             # current span again.