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.

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):

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)