From 8b1da35bc58fa64b92ab042b0e95aac6d03e4b72 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Wed, 5 Aug 2020 13:18:33 -0600 Subject: [PATCH] opentracing-shim: Return consistent ScopeShim objects (#922) This uses the OpenTelemetry context management mechanism to store a ScopeShim object in order to make active return the same object as the one returned by start_active_span. --- .../opentracing_shim/__init__.py | 19 +++---- .../tests/test_shim.py | 50 +++++++++++++------ 2 files changed, 45 insertions(+), 24 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-opentracing-shim/src/opentelemetry/instrumentation/opentracing_shim/__init__.py b/instrumentation/opentelemetry-instrumentation-opentracing-shim/src/opentelemetry/instrumentation/opentracing_shim/__init__.py index 22c67c53ea2..6c76444e6ee 100644 --- a/instrumentation/opentelemetry-instrumentation-opentracing-shim/src/opentelemetry/instrumentation/opentracing_shim/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-opentracing-shim/src/opentelemetry/instrumentation/opentracing_shim/__init__.py @@ -91,7 +91,7 @@ from deprecated import deprecated from opentelemetry import propagators -from opentelemetry.context import Context +from opentelemetry.context import Context, attach, detach, get_value, set_value from opentelemetry.correlationcontext import get_correlation, set_correlation from opentelemetry.instrumentation.opentracing_shim import util from opentelemetry.instrumentation.opentracing_shim.version import __version__ @@ -327,6 +327,7 @@ class ScopeShim(opentracing.Scope): def __init__(self, manager, span, span_cm=None): super().__init__(manager, span) self._span_cm = span_cm + self._token = attach(set_value("scope_shim", self)) # TODO: Change type of `manager` argument to `opentracing.ScopeManager`? We # need to get rid of `manager.tracer` for this. @@ -382,6 +383,8 @@ def close(self): *finish_on_close* when activating the span. """ + detach(self._token) + if self._span_cm is not None: # We don't have error information to pass to `__exit__()` so we # pass `None` in all arguments. If the OpenTelemetry tracer @@ -460,14 +463,12 @@ def active(self): if span.get_context() == INVALID_SPAN_CONTEXT: return None - span_context = SpanContextShim(span.get_context()) - wrapped_span = SpanShim(self._tracer, span_context, span) - return ScopeShim(self, span=wrapped_span) - # TODO: The returned `ScopeShim` instance here always ends the - # corresponding span, regardless of the `finish_on_close` value used - # when activating the span. This is because here we return a *new* - # `ScopeShim` rather than returning a saved instance of `ScopeShim`. - # https://github.com/open-telemetry/opentelemetry-python/pull/211/files#r335398792 + try: + return get_value("scope_shim") + except KeyError: + span_context = SpanContextShim(span.get_context()) + wrapped_span = SpanShim(self._tracer, span_context, span) + return ScopeShim(self, span=wrapped_span) @property def tracer(self): diff --git a/instrumentation/opentelemetry-instrumentation-opentracing-shim/tests/test_shim.py b/instrumentation/opentelemetry-instrumentation-opentracing-shim/tests/test_shim.py index 8b46a4bcb3b..445ce6ce161 100644 --- a/instrumentation/opentelemetry-instrumentation-opentracing-shim/tests/test_shim.py +++ b/instrumentation/opentelemetry-instrumentation-opentracing-shim/tests/test_shim.py @@ -63,7 +63,7 @@ def test_shim_type(self): def test_start_active_span(self): """Test span creation and activation using `start_active_span()`.""" - with self.shim.start_active_span("TestSpan") as scope: + with self.shim.start_active_span("TestSpan0") as scope: # Verify correct type of Scope and Span objects. self.assertIsInstance(scope, opentracing.Scope) self.assertIsInstance(scope.span, opentracing.Span) @@ -91,7 +91,7 @@ def test_start_active_span(self): def test_start_span(self): """Test span creation using `start_span()`.""" - with self.shim.start_span("TestSpan") as span: + with self.shim.start_span("TestSpan1") as span: # Verify correct type of Span object. self.assertIsInstance(span, opentracing.Span) @@ -107,7 +107,7 @@ def test_start_span(self): def test_start_span_no_contextmanager(self): """Test `start_span()` without a `with` statement.""" - span = self.shim.start_span("TestSpan") + span = self.shim.start_span("TestSpan2") # Verify span is started. self.assertIsNotNone(span.unwrap().start_time) @@ -120,7 +120,7 @@ def test_start_span_no_contextmanager(self): def test_explicit_span_finish(self): """Test `finish()` method on `Span` objects.""" - span = self.shim.start_span("TestSpan") + span = self.shim.start_span("TestSpan3") # Verify span hasn't ended. self.assertIsNone(span.unwrap().end_time) @@ -134,7 +134,7 @@ def test_explicit_start_time(self): """Test `start_time` argument.""" now = time.time() - with self.shim.start_active_span("TestSpan", start_time=now) as scope: + with self.shim.start_active_span("TestSpan4", start_time=now) as scope: result = util.time_seconds_from_ns(scope.span.unwrap().start_time) # Tolerate inaccuracies of less than a microsecond. See Note: # https://open-telemetry.github.io/opentelemetry-python/opentelemetry.instrumentation.opentracing_shim.html @@ -145,7 +145,7 @@ def test_explicit_start_time(self): def test_explicit_end_time(self): """Test `end_time` argument of `finish()` method.""" - span = self.shim.start_span("TestSpan") + span = self.shim.start_span("TestSpan5") now = time.time() span.finish(now) @@ -159,7 +159,7 @@ def test_explicit_end_time(self): def test_explicit_span_activation(self): """Test manual activation and deactivation of a span.""" - span = self.shim.start_span("TestSpan") + span = self.shim.start_span("TestSpan6") # Verify no span is currently active. self.assertIsNone(self.shim.active_span) @@ -180,7 +180,7 @@ def test_start_active_span_finish_on_close(self): """Test `finish_on_close` argument of `start_active_span()`.""" with self.shim.start_active_span( - "TestSpan", finish_on_close=True + "TestSpan7", finish_on_close=True ) as scope: # Verify span hasn't ended. self.assertIsNone(scope.span.unwrap().end_time) @@ -189,7 +189,7 @@ def test_start_active_span_finish_on_close(self): self.assertIsNotNone(scope.span.unwrap().end_time) with self.shim.start_active_span( - "TestSpan", finish_on_close=False + "TestSpan8", finish_on_close=False ) as scope: # Verify span hasn't ended. self.assertIsNone(scope.span.unwrap().end_time) @@ -202,7 +202,7 @@ def test_start_active_span_finish_on_close(self): def test_activate_finish_on_close(self): """Test `finish_on_close` argument of `activate()`.""" - span = self.shim.start_span("TestSpan") + span = self.shim.start_span("TestSpan9") with self.shim.scope_manager.activate( span, finish_on_close=True @@ -216,7 +216,7 @@ def test_activate_finish_on_close(self): # Verify span has ended. self.assertIsNotNone(span.unwrap().end_time) - span = self.shim.start_span("TestSpan") + span = self.shim.start_span("TestSpan10") with self.shim.scope_manager.activate( span, finish_on_close=False @@ -402,13 +402,13 @@ def test_tags(self): def test_span_tracer(self): """Test the `tracer` property on `Span` objects.""" - with self.shim.start_active_span("TestSpan") as scope: + with self.shim.start_active_span("TestSpan11") as scope: self.assertEqual(scope.span.tracer, self.shim) def test_log_kv(self): """Test the `log_kv()` method on `Span` objects.""" - with self.shim.start_span("TestSpan") as span: + with self.shim.start_span("TestSpan12") as span: span.log_kv({"foo": "bar"}) self.assertEqual(span.unwrap().events[0].attributes["foo"], "bar") # Verify timestamp was generated automatically. @@ -430,7 +430,7 @@ def test_log_kv(self): def test_log(self): """Test the deprecated `log` method on `Span` objects.""" - with self.shim.start_span("TestSpan") as span: + with self.shim.start_span("TestSpan13") as span: with self.assertWarns(DeprecationWarning): span.log(event="foo", payload="bar") @@ -441,7 +441,7 @@ def test_log(self): def test_log_event(self): """Test the deprecated `log_event` method on `Span` objects.""" - with self.shim.start_span("TestSpan") as span: + with self.shim.start_span("TestSpan14") as span: with self.assertWarns(DeprecationWarning): span.log_event("foo", "bar") @@ -557,6 +557,7 @@ def test_extract_binary(self): self.shim.extract(opentracing.Format.BINARY, bytearray()) def test_baggage(self): + """Test SpanShim baggage being set and being immutable""" span_context_shim = SpanContextShim( trace.SpanContext(1234, 5678, is_remote=False) @@ -572,3 +573,22 @@ def test_baggage(self): span_shim.set_baggage_item(1, 2) self.assertTrue(span_shim.get_baggage_item(1), 2) + + def test_active(self): + """Test that the active property and start_active_span return the same + object""" + + # Verify no span is currently active. + self.assertIsNone(self.shim.active_span) + + with self.shim.start_active_span("TestSpan15") as scope: + # Verify span is active. + self.assertEqual( + self.shim.active_span.context.unwrap(), + scope.span.context.unwrap(), + ) + + self.assertIs(self.shim.scope_manager.active, scope) + + # Verify no span is active. + self.assertIsNone(self.shim.active_span)