Clean up opentracing configuration options (#5712)

Clean up config settings and dead code.

This is mostly about cleaning up the config format, to bring it into line with our conventions. In particular:
 * There should be a blank line after `## Section ##' headings
 * There should be a blank line between each config setting
 * There should be a `#`-only line between a comment and the setting it describes
 * We don't really do the `#  #` style commenting-out of whole sections if we can help it
 * rename `tracer_enabled` to `enabled`

While we're here, do more config parsing upfront, which makes it easier to use
later on.

Also removes redundant code from LogContextScopeManager.

Also changes the changelog fragment to a `feature` - it's exciting!

changelog.d/5544.feature

@@ -0,0 +1,2 @@
+Add support for opentracing.
+

changelog.d/5544.misc

@@ -1 +0,0 @@
-Added opentracing and configuration options.

changelog.d/5712.feature

@@ -0,0 +1,2 @@
+Add support for opentracing.
+

docs/sample_config.yaml

@@ -1409,17 +1409,34 @@ password_config:
 
 
 ## Opentracing ##
-# These settings enable opentracing which implements distributed tracing
-# This allows you to observe the causal chain of events across servers
-# including requests, key lookups etc. across any server running
-# synapse or any other other services which supports opentracing.
-# (specifically those implemented with jaeger)
 
-#opentracing:
-#  # Enable / disable tracer
-#  tracer_enabled: false
-#  # The list of homeservers we wish to expose our current traces to.
-#  # The list is a list of regexes which are matched against the
-#  # servername of the homeserver
-#  homeserver_whitelist:
-#    - ".*"
+# These settings enable opentracing, which implements distributed tracing.
+# This allows you to observe the causal chains of events across servers
+# including requests, key lookups etc., across any server running
+# synapse or any other other services which supports opentracing
+# (specifically those implemented with Jaeger).
+#
+opentracing:
+    # tracing is disabled by default. Uncomment the following line to enable it.
+    #
+    #enabled: true
+
+    # The list of homeservers we wish to send and receive span contexts and span baggage.
+    #
+    # Though it's mostly safe to send and receive span contexts to and from
+    # untrusted users since span contexts are usually opaque ids it can lead to
+    # two problems, namely:
+    # - If the span context is marked as sampled by the sending homeserver the receiver will
+    # sample it. Therefore two homeservers with wildly disparaging sampling policies
+    # could incur higher sampling counts than intended.
+    # - Span baggage can be arbitrary data. For safety this has been disabled in synapse
+    # but that doesn't prevent another server sending you baggage which will be logged
+    # to opentracing logs.
+    #
+    # This a list of regexes which are matched against the server_name of the
+    # homeserver.
+    #
+    # By defult, it is empty, so no servers are matched.
+    #
+    #homeserver_whitelist:
+    #  - ".*"

synapse/config/tracer.py

@@ -18,33 +18,52 @@ from ._base import Config, ConfigError
 
 class TracerConfig(Config):
     def read_config(self, config, **kwargs):
-        self.tracer_config = config.get("opentracing")
+        opentracing_config = config.get("opentracing")
+        if opentracing_config is None:
+            opentracing_config = {}
 
-        self.tracer_config = config.get("opentracing", {"tracer_enabled": False})
+        self.opentracer_enabled = opentracing_config.get("enabled", False)
+        if not self.opentracer_enabled:
+            return
 
-        if self.tracer_config.get("tracer_enabled", False):
         # The tracer is enabled so sanitize the config
-            # If no whitelists are given
-            self.tracer_config.setdefault("homeserver_whitelist", [])
 
-            if not isinstance(self.tracer_config.get("homeserver_whitelist"), list):
-                raise ConfigError("Tracer homesererver_whitelist config is malformed")
+        self.opentracer_whitelist = opentracing_config.get("homeserver_whitelist", [])
+        if not isinstance(self.opentracer_whitelist, list):
+            raise ConfigError("Tracer homeserver_whitelist config is malformed")
 
     def generate_config_section(cls, **kwargs):
         return """\
         ## Opentracing ##
-        # These settings enable opentracing which implements distributed tracing
-        # This allows you to observe the causal chain of events across servers
-        # including requests, key lookups etc. across any server running
-        # synapse or any other other services which supports opentracing.
-        # (specifically those implemented with jaeger)
 
-        #opentracing:
-        #  # Enable / disable tracer
-        #  tracer_enabled: false
-        #  # The list of homeservers we wish to expose our current traces to.
-        #  # The list is a list of regexes which are matched against the
-        #  # servername of the homeserver
-        #  homeserver_whitelist:
+        # These settings enable opentracing, which implements distributed tracing.
+        # This allows you to observe the causal chains of events across servers
+        # including requests, key lookups etc., across any server running
+        # synapse or any other other services which supports opentracing
+        # (specifically those implemented with Jaeger).
+        #
+        opentracing:
+            # tracing is disabled by default. Uncomment the following line to enable it.
+            #
+            #enabled: true
+
+            # The list of homeservers we wish to send and receive span contexts and span baggage.
+            #
+            # Though it's mostly safe to send and receive span contexts to and from
+            # untrusted users since span contexts are usually opaque ids it can lead to
+            # two problems, namely:
+            # - If the span context is marked as sampled by the sending homeserver the receiver will
+            # sample it. Therefore two homeservers with wildly disparaging sampling policies
+            # could incur higher sampling counts than intended.
+            # - Span baggage can be arbitrary data. For safety this has been disabled in synapse
+            # but that doesn't prevent another server sending you baggage which will be logged
+            # to opentracing logs.
+            #
+            # This a list of regexes which are matched against the server_name of the
+            # homeserver.
+            #
+            # By defult, it is empty, so no servers are matched.
+            #
+            #homeserver_whitelist:
             #  - ".*"
         """

synapse/logging/opentracing.py

@@ -1,5 +1,5 @@
 # -*- coding: utf-8 -*-
-# Copyright 2019 The Matrix.org Foundation C.I.C.d
+# Copyright 2019 The Matrix.org Foundation C.I.C.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -24,6 +24,15 @@
 # this move the methods have work very similarly to opentracing's and it should only
 # be a matter of few regexes to move over to opentracing's access patterns proper.
 
+import contextlib
+import logging
+import re
+from functools import wraps
+
+from twisted.internet import defer
+
+from synapse.config import ConfigError
+
 try:
     import opentracing
 except ImportError:
@@ -35,12 +44,6 @@ except ImportError:
     JaegerConfig = None
     LogContextScopeManager = None
 
-import contextlib
-import logging
-import re
-from functools import wraps
-
-from twisted.internet import defer
 
 logger = logging.getLogger(__name__)
 
@@ -91,7 +94,8 @@ def only_if_tracing(func):
     return _only_if_tracing_inner
 
 
-# Block everything by default
+# A regex which matches the server_names to expose traces for.
+# None means 'block everything'.
 _homeserver_whitelist = None
 
 tags = _DumTagNames
@@ -101,31 +105,24 @@ def init_tracer(config):
     """Set the whitelists and initialise the JaegerClient tracer
 
     Args:
-        config (Config)
-        The config used by the homeserver. Here it's used to set the service
-        name to the homeserver's.
+        config (HomeserverConfig): The config used by the homeserver
     """
     global opentracing
-    if not config.tracer_config.get("tracer_enabled", False):
+    if not config.opentracer_enabled:
         # We don't have a tracer
         opentracing = None
         return
 
-    if not opentracing:
-        logger.error(
-            "The server has been configure to use opentracing but opentracing is not installed."
-        )
-        raise ModuleNotFoundError("opentracing")
-
-    if not JaegerConfig:
-        logger.error(
-            "The server has been configure to use opentracing but opentracing is not installed."
+    if not opentracing or not JaegerConfig:
+        raise ConfigError(
+            "The server has been configured to use opentracing but opentracing is not "
+            "installed."
         )
 
     # Include the worker name
     name = config.worker_name if config.worker_name else "master"
 
-    set_homeserver_whitelist(config.tracer_config["homeserver_whitelist"])
+    set_homeserver_whitelist(config.opentracer_whitelist)
     jaeger_config = JaegerConfig(
         config={"sampler": {"type": "const", "param": 1}, "logging": True},
         service_name="{} {}".format(config.server_name, name),
@@ -232,7 +229,6 @@ def whitelisted_homeserver(destination):
     """Checks if a destination matches the whitelist
     Args:
         destination (String)"""
-    global _homeserver_whitelist
     if _homeserver_whitelist:
         return _homeserver_whitelist.match(destination)
     return False

synapse/logging/scopecontextmanager.py

@@ -34,9 +34,7 @@ class LogContextScopeManager(ScopeManager):
     """
 
     def __init__(self, config):
-        # Set the whitelists
-        logger.info(config.tracer_config)
-        self._homeserver_whitelist = config.tracer_config["homeserver_whitelist"]
+        pass
 
     @property
     def active(self):