From 63f559ec257da7868a58699cbac598f3c57bd889 Mon Sep 17 00:00:00 2001 From: Carlos Alberto Cortez Date: Tue, 15 Oct 2019 09:49:40 +0200 Subject: [PATCH] Refactor current span handling for newly created spans. (#198) 1. Make Tracer.start_span() simply create and start the Span, without setting it as the current instance. 2. Add an extra Tracer.start_as_current_span() to create the Span and set it as the current instance automatically. Co-Authored-By: Chris Kleinknecht --- .../src/opentelemetry/trace/__init__.py | 89 ++++++++++--- .../src/opentelemetry/sdk/trace/__init__.py | 17 ++- opentelemetry-sdk/tests/trace/test_trace.py | 119 +++++++++++++----- 3 files changed, 171 insertions(+), 54 deletions(-) diff --git a/opentelemetry-api/src/opentelemetry/trace/__init__.py b/opentelemetry-api/src/opentelemetry/trace/__init__.py index 18eced4504..1b0a3e758f 100644 --- a/opentelemetry-api/src/opentelemetry/trace/__init__.py +++ b/opentelemetry-api/src/opentelemetry/trace/__init__.py @@ -26,16 +26,16 @@ to use the API package alone without a supporting implementation. The tracer supports creating spans that are "attached" or "detached" from the -context. By default, new spans are "attached" to the context in that they are +context. New spans are "attached" to the context in that they are created as children of the currently active span, and the newly-created span -becomes the new active span:: +can optionally become the new active span:: from opentelemetry.trace import tracer # Create a new root span, set it as the current span in context - with tracer.start_span("parent"): + with tracer.start_as_current_span("parent"): # Attach a new child and update the current span - with tracer.start_span("child"): + with tracer.start_as_current_span("child"): do_work(): # Close child span, set parent as current # Close parent span, set default span as current @@ -62,6 +62,7 @@ """ import enum +import types as python_types import typing from contextlib import contextmanager @@ -226,6 +227,26 @@ def is_recording_events(self) -> bool: events with the add_event operation and attributes using set_attribute. """ + def __enter__(self) -> "Span": + """Invoked when `Span` is used as a context manager. + + Returns the `Span` itself. + """ + return self + + def __exit__( + self, + exc_type: typing.Optional[typing.Type[BaseException]], + exc_val: typing.Optional[BaseException], + exc_tb: typing.Optional[python_types.TracebackType], + ) -> typing.Optional[bool]: + """Ends context manager and calls `end` on the `Span`. + + Returns False. + """ + self.end() + return False + class TraceOptions(int): """A bitmask that represents options specific to the trace. @@ -376,30 +397,64 @@ def get_current_span(self) -> "Span": # pylint: disable=no-self-use return INVALID_SPAN - @contextmanager # type: ignore def start_span( self, name: str, parent: ParentSpan = CURRENT_SPAN, kind: SpanKind = SpanKind.INTERNAL, - ) -> typing.Iterator["Span"]: - """Context manager for span creation. + ) -> "Span": + """Starts a span. - Create a new span. Start the span and set it as the current span in - this tracer's context. + Create a new span. Start the span without setting it as the current + span in this tracer's context. By default the current span will be used as parent, but an explicit parent can also be specified, either a `Span` or a `SpanContext`. If the specified value is `None`, the created span will be a root span. - On exiting the context manager stop the span and set its parent as the + The span can be used as context manager. On exiting, the span will be + ended. + + Example:: + + # tracer.get_current_span() will be used as the implicit parent. + # If none is found, the created span will be a root instance. + with tracer.start_span("one") as child: + child.add_event("child's event") + + Applications that need to set the newly created span as the current + instance should use :meth:`start_as_current_span` instead. + + Args: + name: The name of the span to be created. + parent: The span's parent. Defaults to the current span. + kind: The span's kind (relationship to parent). Note that is + meaningful even if there is no parent. + + Returns: + The newly-created span. + """ + # pylint: disable=unused-argument,no-self-use + return INVALID_SPAN + + @contextmanager # type: ignore + def start_as_current_span( + self, + name: str, + parent: ParentSpan = CURRENT_SPAN, + kind: SpanKind = SpanKind.INTERNAL, + ) -> typing.Iterator["Span"]: + """Context manager for creating a new span and set it + as the current span in this tracer's context. + + On exiting the context manager stops the span and set its parent as the current span. Example:: - with tracer.start_span("one") as parent: + with tracer.start_as_current_span("one") as parent: parent.add_event("parent's event") - with tracer.start_span("two") as child: + with tracer.start_as_current_span("two") as child: child.add_event("child's event") tracer.get_current_span() # returns child tracer.get_current_span() # returns parent @@ -407,15 +462,14 @@ def start_span( This is a convenience method for creating spans attached to the tracer's context. Applications that need more control over the span - lifetime should use :meth:`create_span` instead. For example:: + lifetime should use :meth:`start_span` instead. For example:: - with tracer.start_span(name) as span: + with tracer.start_as_current_span(name) as span: do_work() is equivalent to:: - span = tracer.create_span(name) - span.start() + span = tracer.start_span(name) with tracer.use_span(span, end_on_exit=True): do_work() @@ -428,6 +482,7 @@ def start_span( Yields: The newly-created span. """ + # pylint: disable=unused-argument,no-self-use yield INVALID_SPAN @@ -451,7 +506,7 @@ def create_span( Applications that need to create spans detached from the tracer's context should use this method. - with tracer.start_span(name) as span: + with tracer.start_as_current_span(name) as span: do_work() This is equivalent to:: diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py index eb754fadb8..c7b33a353d 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py @@ -322,19 +322,28 @@ def get_current_span(self): """See `opentelemetry.trace.Tracer.get_current_span`.""" return self._current_span_slot.get() - @contextmanager def start_span( self, name: str, parent: trace_api.ParentSpan = trace_api.Tracer.CURRENT_SPAN, kind: trace_api.SpanKind = trace_api.SpanKind.INTERNAL, - ) -> typing.Iterator[trace_api.Span]: + ) -> "Span": """See `opentelemetry.trace.Tracer.start_span`.""" span = self.create_span(name, parent, kind) span.start() - with self.use_span(span, end_on_exit=True): - yield span + return span + + def start_as_current_span( + self, + name: str, + parent: trace_api.ParentSpan = trace_api.Tracer.CURRENT_SPAN, + kind: trace_api.SpanKind = trace_api.SpanKind.INTERNAL, + ) -> typing.Iterator[trace_api.Span]: + """See `opentelemetry.trace.Tracer.start_as_current_span`.""" + + span = self.start_span(name, parent, kind) + return self.use_span(span, end_on_exit=True) def create_span( self, diff --git a/opentelemetry-sdk/tests/trace/test_trace.py b/opentelemetry-sdk/tests/trace/test_trace.py index dc593a9d62..b55ea897fa 100644 --- a/opentelemetry-sdk/tests/trace/test_trace.py +++ b/opentelemetry-sdk/tests/trace/test_trace.py @@ -32,17 +32,17 @@ def test_start_span_implicit(self): self.assertIsNone(tracer.get_current_span()) - with tracer.start_span("root") as root: - self.assertIs(tracer.get_current_span(), root) + root = tracer.start_span("root") + self.assertIsNotNone(root.start_time) + self.assertIsNone(root.end_time) + self.assertEqual(root.kind, trace_api.SpanKind.INTERNAL) - self.assertIsNotNone(root.start_time) - self.assertIsNone(root.end_time) - self.assertEqual(root.kind, trace_api.SpanKind.INTERNAL) + with tracer.use_span(root, True): + self.assertIs(tracer.get_current_span(), root) with tracer.start_span( "child", kind=trace_api.SpanKind.CLIENT ) as child: - self.assertIs(tracer.get_current_span(), child) self.assertIs(child.parent, root) self.assertEqual(child.kind, trace_api.SpanKind.CLIENT) @@ -64,9 +64,9 @@ def test_start_span_implicit(self): root_context.trace_options, child_context.trace_options ) - # After exiting the child's scope the parent should become the - # current span again. - self.assertIs(tracer.get_current_span(), root) + # Verify start_span() did not set the current span. + self.assertIs(tracer.get_current_span(), root) + self.assertIsNotNone(child.end_time) self.assertIsNone(tracer.get_current_span()) @@ -82,26 +82,25 @@ def test_start_span_explicit(self): self.assertIsNone(tracer.get_current_span()) + root = tracer.start_span("root") + self.assertIsNotNone(root.start_time) + self.assertIsNone(root.end_time) + # Test with the implicit root span - with tracer.start_span("root") as root: + with tracer.use_span(root, True): self.assertIs(tracer.get_current_span(), root) - self.assertIsNotNone(root.start_time) - self.assertIsNone(root.end_time) - with tracer.start_span("stepchild", other_parent) as child: - # The child should become the current span as usual, but its - # parent should be the one passed in, not the - # previously-current span. - self.assertIs(tracer.get_current_span(), child) + # The child's parent should be the one passed in, + # not the current span. self.assertNotEqual(child.parent, root) self.assertIs(child.parent, other_parent) self.assertIsNotNone(child.start_time) self.assertIsNone(child.end_time) - # The child should inherit its context fromr the explicit - # parent, not the previously-current span. + # The child should inherit its context from the explicit + # parent, not the current span. child_context = child.get_context() self.assertEqual(other_parent.trace_id, child_context.trace_id) self.assertNotEqual( @@ -114,6 +113,60 @@ def test_start_span_explicit(self): other_parent.trace_options, child_context.trace_options ) + # Verify start_span() did not set the current span. + self.assertIs(tracer.get_current_span(), root) + + # Verify ending the child did not set the current span. + self.assertIs(tracer.get_current_span(), root) + self.assertIsNotNone(child.end_time) + + def test_start_as_current_span_implicit(self): + tracer = trace.Tracer("test_start_as_current_span_implicit") + + self.assertIsNone(tracer.get_current_span()) + + with tracer.start_as_current_span("root") as root: + self.assertIs(tracer.get_current_span(), root) + + with tracer.start_as_current_span("child") as child: + self.assertIs(tracer.get_current_span(), child) + self.assertIs(child.parent, root) + + # After exiting the child's scope the parent should become the + # current span again. + self.assertIs(tracer.get_current_span(), root) + self.assertIsNotNone(child.end_time) + + self.assertIsNone(tracer.get_current_span()) + self.assertIsNotNone(root.end_time) + + def test_start_as_current_span_explicit(self): + tracer = trace.Tracer("test_start_as_current_span_explicit") + + other_parent = trace_api.SpanContext( + trace_id=0x000000000000000000000000DEADBEEF, + span_id=0x00000000DEADBEF0, + ) + + self.assertIsNone(tracer.get_current_span()) + + # Test with the implicit root span + with tracer.start_as_current_span("root") as root: + self.assertIs(tracer.get_current_span(), root) + + self.assertIsNotNone(root.start_time) + self.assertIsNone(root.end_time) + + with tracer.start_as_current_span( + "stepchild", other_parent + ) as child: + # The child should become the current span as usual, but its + # parent should be the one passed in, not the + # previously-current span. + self.assertIs(tracer.get_current_span(), child) + self.assertNotEqual(child.parent, root) + self.assertIs(child.parent, other_parent) + # After exiting the child's scope the last span on the stack should # become current, not the child's parent. self.assertNotEqual(tracer.get_current_span(), other_parent) @@ -144,7 +197,7 @@ def test_span_members(self): self.assertIsNone(tracer.get_current_span()) - with tracer.start_span("root") as root: + with tracer.start_as_current_span("root") as root: # attributes root.set_attribute("component", "http") root.set_attribute("http.method", "GET") @@ -254,7 +307,7 @@ def test_ended_span(self): span_id=trace.generate_span_id(), ) - with tracer.start_span("root") as root: + with tracer.start_as_current_span("root") as root: # everything should be empty at the beginning self.assertEqual(len(root.attributes), 0) self.assertEqual(len(root.events), 0) @@ -313,9 +366,9 @@ def test_span_processor(self): sp1 = MySpanProcessor("SP1", spans_calls_list) sp2 = MySpanProcessor("SP2", spans_calls_list) - with tracer.start_span("foo"): - with tracer.start_span("bar"): - with tracer.start_span("baz"): + with tracer.start_as_current_span("foo"): + with tracer.start_as_current_span("bar"): + with tracer.start_as_current_span("baz"): pass # at this point lists must be empty @@ -324,13 +377,13 @@ def test_span_processor(self): # add single span processor tracer.add_span_processor(sp1) - with tracer.start_span("foo"): + with tracer.start_as_current_span("foo"): expected_list.append(span_event_start_fmt("SP1", "foo")) - with tracer.start_span("bar"): + with tracer.start_as_current_span("bar"): expected_list.append(span_event_start_fmt("SP1", "bar")) - with tracer.start_span("baz"): + with tracer.start_as_current_span("baz"): expected_list.append(span_event_start_fmt("SP1", "baz")) expected_list.append(span_event_end_fmt("SP1", "baz")) @@ -347,15 +400,15 @@ def test_span_processor(self): # go for multiple span processors tracer.add_span_processor(sp2) - with tracer.start_span("foo"): + with tracer.start_as_current_span("foo"): expected_list.append(span_event_start_fmt("SP1", "foo")) expected_list.append(span_event_start_fmt("SP2", "foo")) - with tracer.start_span("bar"): + with tracer.start_as_current_span("bar"): expected_list.append(span_event_start_fmt("SP1", "bar")) expected_list.append(span_event_start_fmt("SP2", "bar")) - with tracer.start_span("baz"): + with tracer.start_as_current_span("baz"): expected_list.append(span_event_start_fmt("SP1", "baz")) expected_list.append(span_event_start_fmt("SP2", "baz")) @@ -380,9 +433,9 @@ def test_add_span_processor_after_span_creation(self): # Span processors are created but not added to the tracer yet sp = MySpanProcessor("SP1", spans_calls_list) - with tracer.start_span("foo"): - with tracer.start_span("bar"): - with tracer.start_span("baz"): + with tracer.start_as_current_span("foo"): + with tracer.start_as_current_span("bar"): + with tracer.start_as_current_span("baz"): # add span processor after spans have been created tracer.add_span_processor(sp)