Clean up MetricsResource and Prometheus hacks (#18687)

Clean up `MetricsResource`, Prometheus hacks
(`_set_prometheus_client_use_created_metrics`), and better document why
we care about having a separate `metrics` listener type.

These clean-up changes have been split out from
#18584 since that PR was
closed.

Author: MadLittleMods

changelog.d/18687.misc

@@ -0,0 +1 @@
+Clean up `MetricsResource` and Prometheus hacks.

poetry.lock

@@ -195,7 +195,9 @@ pymacaroons = ">=0.13.0"
 msgpack = ">=0.5.2"
 phonenumbers = ">=8.2.0"
 # we use GaugeHistogramMetric, which was added in prom-client 0.4.0.
-prometheus-client = ">=0.4.0"
+# `prometheus_client.metrics` was added in 0.5.0, so we require that too.
+# We chose 0.6.0 as that is the current version in Debian Buster (oldstable).
+prometheus-client = ">=0.6.0"
 # we use `order`, which arrived in attrs 19.2.0.
 # Note: 21.1.0 broke `/sync`, see https://github.com/matrix-org/synapse/issues/9936
 attrs = ">=19.2.0,!=21.1.0"

pyproject.toml

@@ -286,39 +286,26 @@ async def wrapper() -> None:
 def listen_metrics(bind_addresses: StrCollection, port: int) -> None:
     """
     Start Prometheus metrics server.
+
+    This method runs the metrics server on a different port, in a different thread to
+    Synapse. This can make it more resilient to heavy load in Synapse causing metric
+    requests to be slow or timeout.
+
+    Even though `start_http_server_prometheus(...)` uses `threading.Thread` behind the
+    scenes (where all threads share the GIL and only one thread can execute Python
+    bytecode at a time), this still works because the metrics thread can preempt the
+    Twisted reactor thread between bytecode boundaries and the metrics thread gets
+    scheduled with roughly equal priority to the Twisted reactor thread.
     """
     from prometheus_client import start_http_server as start_http_server_prometheus
 
     from synapse.metrics import RegistryProxy
 
     for host in bind_addresses:
         logger.info("Starting metrics listener on %s:%d", host, port)
-        _set_prometheus_client_use_created_metrics(False)
         start_http_server_prometheus(port, addr=host, registry=RegistryProxy)
 
 
-def _set_prometheus_client_use_created_metrics(new_value: bool) -> None:
-    """
-    Sets whether prometheus_client should expose `_created`-suffixed metrics for
-    all gauges, histograms and summaries.
-    There is no programmatic way to disable this without poking at internals;
-    the proper way is to use an environment variable which prometheus_client
-    loads at import time.
-
-    The motivation for disabling these `_created` metrics is that they're
-    a waste of space as they're not useful but they take up space in Prometheus.
-    """
-
-    import prometheus_client.metrics
-
-    if hasattr(prometheus_client.metrics, "_use_created"):
-        prometheus_client.metrics._use_created = new_value
-    else:
-        logger.error(
-            "Can't disable `_created` metrics in prometheus_client (brittle hack broken?)"
-        )
-
-
 def listen_manhole(
     bind_addresses: StrCollection,
     port: int,

synapse/app/_base.py

@@ -25,6 +25,7 @@
 import os
 import platform
 import threading
+from importlib import metadata
 from typing import (
     Callable,
     Dict,
@@ -41,19 +42,28 @@
 )
 
 import attr
-from prometheus_client import CollectorRegistry, Counter, Gauge, Histogram, Metric
+from pkg_resources import parse_version
+from prometheus_client import (
+    CollectorRegistry,
+    Counter,
+    Gauge,
+    Histogram,
+    Metric,
+    generate_latest,
+)
 from prometheus_client.core import (
     REGISTRY,
     GaugeHistogramMetricFamily,
     GaugeMetricFamily,
 )
 
 from twisted.python.threadpool import ThreadPool
+from twisted.web.resource import Resource
+from twisted.web.server import Request
 
 # This module is imported for its side effects; flake8 needn't warn that it's unused.
 import synapse.metrics._reactor_metrics  # noqa: F401
 from synapse.metrics._gc import MIN_TIME_BETWEEN_GCS, install_gc_manager
-from synapse.metrics._twisted_exposition import MetricsResource, generate_latest
 from synapse.metrics._types import Collector
 from synapse.types import StrSequence
 from synapse.util import SYNAPSE_VERSION
@@ -81,6 +91,53 @@
 single process." (source: https://prometheus.io/docs/concepts/jobs_instances/)
 """
 
+CONTENT_TYPE_LATEST = "text/plain; version=0.0.4; charset=utf-8"
+"""
+Content type of the latest text format for Prometheus metrics.
+
+Pulled directly from the prometheus_client library.
+"""
+
+
+def _set_prometheus_client_use_created_metrics(new_value: bool) -> None:
+    """
+    Sets whether prometheus_client should expose `_created`-suffixed metrics for
+    all gauges, histograms and summaries.
+
+    There is no programmatic way in the old versions of `prometheus_client` to disable
+    this without poking at internals; the proper way in the old `prometheus_client`
+    versions (> `0.14.0` < `0.18.0`) is to use an environment variable which
+    prometheus_client loads at import time. For versions > `0.18.0`, we can use the
+    dedicated `disable_created_metrics()`/`enable_created_metrics()`.
+
+    The motivation for disabling these `_created` metrics is that they're a waste of
+    space as they're not useful but they take up space in Prometheus. It's not the end
+    of the world if this doesn't work.
+    """
+    import prometheus_client.metrics
+
+    if hasattr(prometheus_client.metrics, "_use_created"):
+        prometheus_client.metrics._use_created = new_value
+    # Just log an error for old versions that don't support disabling the unecessary
+    # metrics. It's not the end of the world if this doesn't work as it just means extra
+    # wasted space taken up in Prometheus but things keep working.
+    elif parse_version(metadata.version("prometheus_client")) < parse_version("0.14.0"):
+        logger.error(
+            "Can't disable `_created` metrics in prometheus_client (unsupported `prometheus_client` version, too old)"
+        )
+    # If the attribute doesn't exist on a newer version, this is a sign that the brittle
+    # hack is broken. We should consider updating the minimum version of
+    # `prometheus_client` to a version (> `0.18.0`) where we can use dedicated
+    # `disable_created_metrics()`/`enable_created_metrics()` functions.
+    else:
+        raise Exception(
+            "Can't disable `_created` metrics in prometheus_client (brittle hack broken?)"
+        )
+
+
+# Set this globally so it applies wherever we generate/collect metrics
+_set_prometheus_client_use_created_metrics(False)
+
 
 class _RegistryProxy:
     @staticmethod
@@ -508,6 +565,23 @@ def register_threadpool(name: str, threadpool: ThreadPool) -> None:
     )
 
 
+class MetricsResource(Resource):
+    """
+    Twisted ``Resource`` that serves prometheus metrics.
+    """
+
+    isLeaf = True
+
+    def __init__(self, registry: CollectorRegistry = REGISTRY):
+        self.registry = registry
+
+    def render_GET(self, request: Request) -> bytes:
+        request.setHeader(b"Content-Type", CONTENT_TYPE_LATEST.encode("ascii"))
+        response = generate_latest(self.registry)
+        request.setHeader(b"Content-Length", str(len(response)))
+        return response
+
+
 __all__ = [
     "Collector",
     "MetricsResource",