From 5f188f777dccd82af3fa9b13a1142762efcfa708 Mon Sep 17 00:00:00 2001 From: Yusuke Tsutsumi Date: Fri, 24 Apr 2020 21:26:52 -0700 Subject: [PATCH] sdk: span parents are now always spancontext (#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 Co-authored-by: Alex Boten --- .../tests/mysql/test_mysql_functional.py | 2 +- .../tests/postgres/test_psycopg_functional.py | 2 +- .../tests/pymongo/test_pymongo_functional.py | 2 +- .../tests/pymysql/test_pymysql_functional.py | 2 +- .../src/opentelemetry/ext/jaeger/__init__.py | 6 +----- .../tests/test_shim.py | 11 ++++++++--- .../ext/otcollector/trace_exporter/__init__.py | 17 ++--------------- .../tests/test_otcollector_trace_exporter.py | 5 ++++- .../src/opentelemetry/sdk/trace/__init__.py | 10 +++++----- opentelemetry-sdk/tests/context/test_asyncio.py | 2 +- opentelemetry-sdk/tests/trace/test_trace.py | 4 ++-- 11 files changed, 27 insertions(+), 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 index 36fd7bf3d3c..d0261d2f63f 100644 --- 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 index c53baf81a62..0a43926145d 100644 --- 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 index 567e0d86a0e..1b2e2694829 100644 --- 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 index e4ee65af30f..a7dc2136558 100644 --- 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 index 8a8d40b260c..23ed6e725d8 100644 --- 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 index 75e98860798..7a0913f973b 100644 --- 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/ext/opentelemetry-ext-otcollector/src/opentelemetry/ext/otcollector/trace_exporter/__init__.py b/ext/opentelemetry-ext-otcollector/src/opentelemetry/ext/otcollector/trace_exporter/__init__.py index d287bcfb4a8..fb6237e86d8 100644 --- a/ext/opentelemetry-ext-otcollector/src/opentelemetry/ext/otcollector/trace_exporter/__init__.py +++ b/ext/opentelemetry-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 index 74c1d088720..4a0a556137d 100644 --- 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 index 3d2fc96c1b8..5eff5e61307 100644 --- 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 index f213753c59f..4fa653205b0 100644 --- 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 index f9db925f11c..1094f1afb98 100644 --- 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.