From 59503b6b7f937433c60d8e7ecb722de43f72b0cf Mon Sep 17 00:00:00 2001 From: Yusuke Tsutsumi Date: Fri, 3 Apr 2020 09:59:08 -0700 Subject: [PATCH 1/5] sdk: span parents are now always spancontext 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. --- .../tests/pymongo/test_pymongo_functional.py | 2 +- .../src/opentelemetry/ext/jaeger/__init__.py | 4 +--- .../tests/test_shim.py | 11 ++++++++--- .../ext/otcollector/trace_exporter/__init__.py | 17 ++--------------- .../tests/test_otcollector_trace_exporter.py | 5 ++++- .../src/opentelemetry/trace/__init__.py | 10 +++++----- .../src/opentelemetry/sdk/trace/__init__.py | 12 +++++++----- opentelemetry-sdk/tests/context/test_asyncio.py | 2 +- opentelemetry-sdk/tests/trace/test_trace.py | 4 ++-- 9 files changed, 31 insertions(+), 36 deletions(-) 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 7b018dab25..e1bfa51296 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 @@ -63,7 +63,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.assertEqual(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-jaeger/src/opentelemetry/ext/jaeger/__init__.py b/ext/opentelemetry-ext-jaeger/src/opentelemetry/ext/jaeger/__init__.py index 8a8d40b260..cffaa8cc0f 100644 --- a/ext/opentelemetry-ext-jaeger/src/opentelemetry/ext/jaeger/__init__.py +++ b/ext/opentelemetry-ext-jaeger/src/opentelemetry/ext/jaeger/__init__.py @@ -195,9 +195,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): + if span.parent is not None: parent_id = span.parent.span_id 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 9f62b90ddf..afadaafb99 100644 --- a/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py +++ b/ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py @@ -292,7 +292,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. @@ -320,7 +321,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) @@ -329,7 +332,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 495579574c..926014093d 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 74c1d08872..4a0a556137 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-api/src/opentelemetry/trace/__init__.py b/opentelemetry-api/src/opentelemetry/trace/__init__.py index 856745e077..f83c073e42 100644 --- a/opentelemetry-api/src/opentelemetry/trace/__init__.py +++ b/opentelemetry-api/src/opentelemetry/trace/__init__.py @@ -84,7 +84,7 @@ logger = getLogger(__name__) # TODO: quarantine -ParentSpan = typing.Optional[typing.Union["Span", "SpanContext"]] +Parent = typing.Optional[typing.Union["SpanContext", "Span"]] class LinkBase(abc.ABC): @@ -512,7 +512,7 @@ def get_current_span(self) -> "Span": def start_span( self, name: str, - parent: ParentSpan = CURRENT_SPAN, + parent: Parent = CURRENT_SPAN, kind: SpanKind = SpanKind.INTERNAL, attributes: typing.Optional[types.Attributes] = None, links: typing.Sequence[Link] = (), @@ -564,7 +564,7 @@ def start_span( def start_as_current_span( self, name: str, - parent: ParentSpan = CURRENT_SPAN, + parent: Parent = CURRENT_SPAN, kind: SpanKind = SpanKind.INTERNAL, attributes: typing.Optional[types.Attributes] = None, links: typing.Sequence[Link] = (), @@ -644,7 +644,7 @@ def get_current_span(self) -> "Span": def start_span( self, name: str, - parent: ParentSpan = Tracer.CURRENT_SPAN, + parent: Parent = Tracer.CURRENT_SPAN, kind: SpanKind = SpanKind.INTERNAL, attributes: typing.Optional[types.Attributes] = None, links: typing.Sequence[Link] = (), @@ -658,7 +658,7 @@ def start_span( def start_as_current_span( self, name: str, - parent: ParentSpan = Tracer.CURRENT_SPAN, + parent: Parent = Tracer.CURRENT_SPAN, kind: SpanKind = SpanKind.INTERNAL, attributes: typing.Optional[types.Attributes] = None, links: typing.Sequence[Link] = (), diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index c162ea5c1c..9f6c6f222f 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -216,7 +216,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, @@ -503,7 +503,7 @@ def get_current_span(self): def start_as_current_span( self, name: str, - parent: trace_api.ParentSpan = trace_api.Tracer.CURRENT_SPAN, + parent: trace_api.Parent = trace_api.Tracer.CURRENT_SPAN, kind: trace_api.SpanKind = trace_api.SpanKind.INTERNAL, attributes: Optional[types.Attributes] = None, links: Sequence[trace_api.Link] = (), @@ -514,7 +514,7 @@ def start_as_current_span( def start_span( # pylint: disable=too-many-locals self, name: str, - parent: trace_api.ParentSpan = trace_api.Tracer.CURRENT_SPAN, + parent: trace_api.Parent = trace_api.Tracer.CURRENT_SPAN, kind: trace_api.SpanKind = trace_api.SpanKind.INTERNAL, attributes: Optional[types.Attributes] = None, links: Sequence[trace_api.Link] = (), @@ -531,7 +531,9 @@ 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( + "expected Span, SpanContext or None object for trace parent." + ) if parent_context is None or not parent_context.is_valid(): parent = parent_context = None @@ -577,7 +579,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 f213753c59..4fa653205b 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 ccb1ccd1e4..0628d460cc 100644 --- a/opentelemetry-sdk/tests/trace/test_trace.py +++ b/opentelemetry-sdk/tests/trace/test_trace.py @@ -233,7 +233,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) @@ -322,7 +322,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. From 71684e87e0043090c19bb30d3dfdcd73e46f001e Mon Sep 17 00:00:00 2001 From: Yusuke Tsutsumi Date: Fri, 3 Apr 2020 21:59:17 -0700 Subject: [PATCH 2/5] addressing feedback (clarifying docstring) --- opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index 9f6c6f222f..2c21ec0921 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -195,8 +195,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 From 6ddd9b01974012e7b96b531b866e2f03110168f3 Mon Sep 17 00:00:00 2001 From: Yusuke Tsutsumi Date: Thu, 16 Apr 2020 21:26:41 -0700 Subject: [PATCH 3/5] Addressing feedback --- .../tests/pymongo/test_pymongo_functional.py | 2 +- .../src/opentelemetry/ext/jaeger/__init__.py | 4 +--- opentelemetry-api/src/opentelemetry/trace/__init__.py | 10 +++++----- .../src/opentelemetry/sdk/trace/__init__.py | 8 +++----- 4 files changed, 10 insertions(+), 14 deletions(-) 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 e1bfa51296..d4200b14c2 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 @@ -63,7 +63,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, root_span.get_context()) + 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-jaeger/src/opentelemetry/ext/jaeger/__init__.py b/ext/opentelemetry-ext-jaeger/src/opentelemetry/ext/jaeger/__init__.py index cffaa8cc0f..23ed6e725d 100644 --- a/ext/opentelemetry-ext-jaeger/src/opentelemetry/ext/jaeger/__init__.py +++ b/ext/opentelemetry-ext-jaeger/src/opentelemetry/ext/jaeger/__init__.py @@ -194,9 +194,7 @@ def _translate_to_jaeger(spans: Span): status = span.status - parent_id = 0 - if span.parent is not None: - 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/opentelemetry-api/src/opentelemetry/trace/__init__.py b/opentelemetry-api/src/opentelemetry/trace/__init__.py index f83c073e42..856745e077 100644 --- a/opentelemetry-api/src/opentelemetry/trace/__init__.py +++ b/opentelemetry-api/src/opentelemetry/trace/__init__.py @@ -84,7 +84,7 @@ logger = getLogger(__name__) # TODO: quarantine -Parent = typing.Optional[typing.Union["SpanContext", "Span"]] +ParentSpan = typing.Optional[typing.Union["Span", "SpanContext"]] class LinkBase(abc.ABC): @@ -512,7 +512,7 @@ def get_current_span(self) -> "Span": def start_span( self, name: str, - parent: Parent = CURRENT_SPAN, + parent: ParentSpan = CURRENT_SPAN, kind: SpanKind = SpanKind.INTERNAL, attributes: typing.Optional[types.Attributes] = None, links: typing.Sequence[Link] = (), @@ -564,7 +564,7 @@ def start_span( def start_as_current_span( self, name: str, - parent: Parent = CURRENT_SPAN, + parent: ParentSpan = CURRENT_SPAN, kind: SpanKind = SpanKind.INTERNAL, attributes: typing.Optional[types.Attributes] = None, links: typing.Sequence[Link] = (), @@ -644,7 +644,7 @@ def get_current_span(self) -> "Span": def start_span( self, name: str, - parent: Parent = Tracer.CURRENT_SPAN, + parent: ParentSpan = Tracer.CURRENT_SPAN, kind: SpanKind = SpanKind.INTERNAL, attributes: typing.Optional[types.Attributes] = None, links: typing.Sequence[Link] = (), @@ -658,7 +658,7 @@ def start_span( def start_as_current_span( self, name: str, - parent: Parent = Tracer.CURRENT_SPAN, + parent: ParentSpan = Tracer.CURRENT_SPAN, kind: SpanKind = SpanKind.INTERNAL, attributes: typing.Optional[types.Attributes] = None, links: typing.Sequence[Link] = (), diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index 2c21ec0921..d48826f423 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -503,7 +503,7 @@ def get_current_span(self): def start_as_current_span( self, name: str, - parent: trace_api.Parent = trace_api.Tracer.CURRENT_SPAN, + parent: trace_api.ParentSpan = trace_api.Tracer.CURRENT_SPAN, kind: trace_api.SpanKind = trace_api.SpanKind.INTERNAL, attributes: Optional[types.Attributes] = None, links: Sequence[trace_api.Link] = (), @@ -514,7 +514,7 @@ def start_as_current_span( def start_span( # pylint: disable=too-many-locals self, name: str, - parent: trace_api.Parent = trace_api.Tracer.CURRENT_SPAN, + parent: trace_api.ParentSpan = trace_api.Tracer.CURRENT_SPAN, kind: trace_api.SpanKind = trace_api.SpanKind.INTERNAL, attributes: Optional[types.Attributes] = None, links: Sequence[trace_api.Link] = (), @@ -531,9 +531,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( - "expected Span, SpanContext or None object for trace parent." - ) + 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 From 07406967d7715ac14add181123befcf3ea716813 Mon Sep 17 00:00:00 2001 From: Alex Boten Date: Fri, 24 Apr 2020 08:40:36 -0700 Subject: [PATCH 4/5] fix tests --- .../tests/mysql/test_mysql_functional.py | 2 +- .../tests/postgres/test_psycopg_functional.py | 2 +- 2 files changed, 2 insertions(+), 2 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 36fd7bf3d3..d0261d2f63 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 c53baf81a6..0a43926145 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 From 959730d07953def62329735027c604095030029c Mon Sep 17 00:00:00 2001 From: Alex Boten Date: Fri, 24 Apr 2020 16:34:27 -0700 Subject: [PATCH 5/5] fix pymysql tests --- .../tests/pymysql/test_pymysql_functional.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 e4ee65af30..a7dc213655 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)