From 386285b21959c971ca9094256a594b364a7f17c2 Mon Sep 17 00:00:00 2001
From: Jorik Schellekens <joriks@matrix.org>
Date: Thu, 4 Jul 2019 10:18:53 +0100
Subject: [PATCH] Return noop context manager if not tracing

---
 synapse/util/tracerutils.py | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/synapse/util/tracerutils.py b/synapse/util/tracerutils.py
index 075e932c91e1..85d4712c4d93 100644
--- a/synapse/util/tracerutils.py
+++ b/synapse/util/tracerutils.py
@@ -94,8 +94,10 @@ def init_tracer(config):
         The config used by the homserver. Here it's used to set the service
         name to the homeserver's.
     """
+    global opentracing
     if not config.tracer_config.get("tracer_enabled", False):
         # We don't have a tracer
+        opentracing = None
         return
 
     if not opentracing:
@@ -148,7 +150,7 @@ def start_active_span(
     finish_on_close=True,
 ):
     if opentracing is None:
-        return _noop_context_manager
+        return _noop_context_manager()
     else:
         # We need to enter the scope here for the logcontext to become active
         return opentracing.tracer.start_active_span(
@@ -232,6 +234,9 @@ def start_active_span_from_context(
     # Twisted encodes the values as lists whereas opentracing doesn't.
     # So, we take the first item in the list.
     # Also, twisted uses byte arrays while opentracing expects strings.
+    if opentracing is None:
+        return _noop_context_manager()
+
     header_dict = {k.decode(): v[0].decode() for k, v in headers.getAllRawHeaders()}
     context = opentracing.tracer.extract(opentracing.Format.HTTP_HEADERS, header_dict)
 
@@ -313,7 +318,7 @@ def trace_servlet(func):
     @wraps(func)
     @defer.inlineCallbacks
     def f(request, *args, **kwargs):
-        with start_active_span_from_context(
+        scope = start_active_span_from_context(
             request.requestHeaders,
             "incoming-client-request",
             tags={
@@ -323,8 +328,16 @@ def f(request, *args, **kwargs):
                 tags.HTTP_URL: request.get_redacted_uri(),
                 tags.PEER_HOST_IPV6: request.getClientIP(),
             },
-        ):
+        )
+        # A context manager would be the most logical here but defer.returnValue
+        # raises an exception in order to provide the return value. This causes
+        # opentracing to mark each request as erroring, in order to avoid this we
+        # need to give the finally clause explicitly.
+        scope.__enter__()
+        try:
             result = yield defer.maybeDeferred(func, request, *args, **kwargs)
             defer.returnValue(result)
+        finally:
+            scope.__exit__(None, None, None)
 
     return f