Passing tests and context manager doesn't seem to be needed

synapse/logging/tracing.py

@@ -173,7 +173,6 @@ def set_fates(clotho, lachesis, atropos, father="Zues", mother="Themis"):
     TYPE_CHECKING,
     Any,
     Callable,
-    Collection,
     Dict,
     Generator,
     Iterable,
@@ -182,31 +181,30 @@ def set_fates(clotho, lachesis, atropos, father="Zues", mother="Themis"):
     Optional,
     Pattern,
     Sequence,
-    Type,
     TypeVar,
     Union,
     cast,
     overload,
 )
 
-import attr
 from typing_extensions import ParamSpec
 
 from twisted.internet import defer
 from twisted.web.http import Request
 from twisted.web.http_headers import Headers
 
 from synapse.config import ConfigError
-from synapse.util import json_decoder, json_encoder
 
 if TYPE_CHECKING:
     from synapse.http.site import SynapseRequest
     from synapse.server import HomeServer
 
 # Helper class
 
-# This will always returns the fixed value given for any accessed property
+
 class _DummyLookup(object):
+    """This will always returns the fixed value given for any accessed property"""
+
     def __init__(self, value):
         self.value = value
 
@@ -215,6 +213,8 @@ def __getattribute__(self, name):
 
 
 class DummyLink(ABC):
+    """Dummy placeholder for `opentelemetry.trace.Link`"""
+
     def __init__(self):
         self.not_implemented_message = (
             "opentelemetry wasn't imported so this is just a dummy link placeholder"
@@ -225,7 +225,7 @@ def context(self):
         raise NotImplementedError(self.not_implemented_message)
 
     @property
-    def context(self):
+    def attributes(self):
         raise NotImplementedError(self.not_implemented_message)
 
 
@@ -256,8 +256,10 @@ def context(self):
 
 logger = logging.getLogger(__name__)
 
-# FIXME: Rename to `SynapseAttributes` so it matches OpenTelemetry `SpanAttributes`
+
 class SynapseTags:
+    """FIXME: Rename to `SynapseAttributes` so it matches OpenTelemetry `SpanAttributes`"""
+
     # The message ID of any to_device message processed
     TO_DEVICE_MESSAGE_ID = "to_device.message_id"
 

tests/logging/test_tracing.py

@@ -12,21 +12,14 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-import logging
-from typing import cast
-
 from twisted.internet import defer
 from twisted.test.proto_helpers import MemoryReactorClock
 
-from synapse.logging.context import (
-    LoggingContext,
-    make_deferred_yieldable,
-    run_in_background,
-)
+from synapse.logging.context import make_deferred_yieldable, run_in_background
 from synapse.logging.tracing import start_active_span
 from synapse.util import Clock
 
-logger = logging.getLogger(__name__)
+from tests.unittest import TestCase
 
 try:
     import opentelemetry
@@ -38,8 +31,6 @@
 except ImportError:
     opentelemetry = None  # type: ignore[assignment]
 
-from tests.unittest import TestCase
-
 
 class LogContextScopeManagerTestCase(TestCase):
     """
@@ -120,7 +111,7 @@ def test_nested_spans(self) -> None:
             self.assertIsNotNone(child_span1.end_time)
             self.assertIsNotNone(child_span2.end_time)
 
-        # Active span is unset outside of the with scopes
+        # Active span is unset now that we're outside of the `with` scopes
         self.assertEqual(
             opentelemetry.trace.get_current_span(), opentelemetry.trace.INVALID_SPAN
         )
@@ -131,57 +122,53 @@ def test_nested_spans(self) -> None:
             ["child_span2", "child_span1", "root_span"],
         )
 
-    # def test_overlapping_spans(self) -> None:
-    #     """Overlapping spans which are not neatly nested should work"""
-    #     reactor = MemoryReactorClock()
-    #     clock = Clock(reactor)
+    def test_overlapping_spans(self) -> None:
+        """Overlapping spans which are not neatly nested should work"""
+        reactor = MemoryReactorClock()
+        clock = Clock(reactor)
 
-    #     scopes = []
-
-    #     async def task(i: int):
-    #         scope = start_active_span(
-    #             f"task{i}",
-    #             tracer=self._tracer,
-    #         )
-    #         scopes.append(scope)
-
-    #         self.assertEqual(self._tracer.active_span, scope.span)
-    #         await clock.sleep(4)
-    #         self.assertEqual(self._tracer.active_span, scope.span)
-    #         scope.close()
+        async def task(i: int):
+            with start_active_span(
+                f"task{i}",
+                tracer=self._tracer,
+            ) as span1:
+                self.assertEqual(opentelemetry.trace.get_current_span(), span1)
+                await clock.sleep(4)
+                self.assertEqual(opentelemetry.trace.get_current_span(), span1)
 
-    #     async def root():
-    #         with start_active_span("root span", tracer=self._tracer) as root_scope:
-    #             self.assertEqual(self._tracer.active_span, root_scope.span)
-    #             scopes.append(root_scope)
+        async def root():
+            with start_active_span("root_span", tracer=self._tracer) as root_span:
+                self.assertEqual(opentelemetry.trace.get_current_span(), root_span)
 
-    #             d1 = run_in_background(task, 1)
-    #             await clock.sleep(2)
-    #             d2 = run_in_background(task, 2)
+                d1 = run_in_background(task, 1)
+                await clock.sleep(2)
+                d2 = run_in_background(task, 2)
 
-    #             # because we did run_in_background, the active span should still be the
-    #             # root.
-    #             self.assertEqual(self._tracer.active_span, root_scope.span)
+                # because we did run_in_background, the active span should still be the
+                # root.
+                self.assertEqual(opentelemetry.trace.get_current_span(), root_span)
 
-    #             await make_deferred_yieldable(
-    #                 defer.gatherResults([d1, d2], consumeErrors=True)
-    #             )
+                await make_deferred_yieldable(
+                    defer.gatherResults([d1, d2], consumeErrors=True)
+                )
 
-    #             self.assertEqual(self._tracer.active_span, root_scope.span)
+                self.assertEqual(opentelemetry.trace.get_current_span(), root_span)
 
-    #     with LoggingContext("root context"):
-    #         # start the test off
-    #         d1 = defer.ensureDeferred(root())
+        # start the test off
+        d1 = defer.ensureDeferred(root())
 
-    #         # let the tasks complete
-    #         reactor.pump((2,) * 8)
+        # let the tasks complete
+        reactor.pump((2,) * 8)
 
-    #         self.successResultOf(d1)
-    #         self.assertIsNone(self._tracer.active_span)
+        self.successResultOf(d1)
+        # Active span is unset now that we're outside of the `with` scopes
+        self.assertEqual(
+            opentelemetry.trace.get_current_span(), opentelemetry.trace.INVALID_SPAN
+        )
 
-    #     # the spans should be reported in order of their finishing: task 1, task 2,
-    #     # root.
-    #     self.assertEqual(
-    #         self._reporter.get_spans(),
-    #         [scopes[1].span, scopes[2].span, scopes[0].span],
-    #     )
+        # the spans should be reported in order of their finishing: task 1, task 2,
+        # root.
+        self.assertListEqual(
+            [span.name for span in self._exporter.get_finished_spans()],
+            ["task1", "task2", "root_span"],
+        )