Merge 95eab2193a04b99a7f3212400b1dee28f5f037f7 into 24bcdb3f3c5aa2d8d2000dc34d54a8002a914616

Conflicts:
	poetry.lock

changelog.d/18584.removal

@@ -0,0 +1 @@
+Remove `metrics` listener (serves metrics at `/metrics`) type in favor of `http` listener with `metrics` resource (serves metrics at `/_synapse/metrics`).

docs/metrics-howto.md

@@ -60,8 +60,11 @@
 
       # beginning of the new metrics listener
       - port: 9000
-        type: metrics
+        type: http
         bind_addresses: ['::1', '127.0.0.1']
+        resources:
+          - names: [metrics]
+            compress: false
     ```
 
 1.  Restart Synapse.

docs/usage/configuration/config_documentation.md

@@ -494,7 +494,7 @@ Options for each entry include:
 
 * `bind_addresses` (array|null): A list of local addresses to listen on. The default is "all local interfaces".
 
-* `type` (string): The type of listener. Normally `http`, but other valid options are [`manhole`](../../manhole.md) and [`metrics`](../../metrics-howto.md).
+* `type` (string): The type of listener. Normally `http`, but other valid options are [`manhole`](../../manhole.md).
 
 * `tls` (boolean): Set to true to enable TLS for this listener. Will use the TLS key/cert specified in tls_private_key_path/tls_certificate_path.
 

poetry.lock

@@ -1,4 +1,4 @@
-# This file is automatically @generated by Poetry 2.1.3 and should not be changed by hand.
+# This file is automatically @generated by Poetry 2.1.2 and should not be changed by hand.
 
 [[package]]
 name = "annotated-types"
@@ -1721,6 +1721,7 @@ files = [
     {file = "psycopg2-2.9.10-cp311-cp311-win_amd64.whl", hash = "sha256:0435034157049f6846e95103bd8f5a668788dd913a7c30162ca9503fdf542cb4"},
     {file = "psycopg2-2.9.10-cp312-cp312-win32.whl", hash = "sha256:65a63d7ab0e067e2cdb3cf266de39663203d38d6a8ed97f5ca0cb315c73fe067"},
     {file = "psycopg2-2.9.10-cp312-cp312-win_amd64.whl", hash = "sha256:4a579d6243da40a7b3182e0430493dbd55950c493d8c68f4eec0b302f6bbf20e"},
+    {file = "psycopg2-2.9.10-cp313-cp313-win_amd64.whl", hash = "sha256:91fd603a2155da8d0cfcdbf8ab24a2d54bca72795b90d2a3ed2b6da8d979dee2"},
     {file = "psycopg2-2.9.10-cp39-cp39-win32.whl", hash = "sha256:9d5b3b94b79a844a986d029eee38998232451119ad653aea42bb9220a8c5066b"},
     {file = "psycopg2-2.9.10-cp39-cp39-win_amd64.whl", hash = "sha256:88138c8dedcbfa96408023ea2b0c369eda40fe5d75002c0964c78f46f11fa442"},
     {file = "psycopg2-2.9.10.tar.gz", hash = "sha256:12ec0b40b0273f95296233e8750441339298e6a572f7039da5b260e3c8b60e11"},
@@ -3388,4 +3389,4 @@ user-search = ["pyicu"]
 [metadata]
 lock-version = "2.1"
 python-versions = "^3.9.0"
-content-hash = "9824e42dfc0e128129ee0c8641f7fe639bf47574cdd3f052dd995941abc6e44b"
+content-hash = "ad95ef9da5a5f71fd0da41719e94b2920c8ba119f8bf942b25c2b50661ffa42b"

pyproject.toml

@@ -1,38 +1,38 @@
 [tool.towncrier]
-    package = "synapse"
+package = "synapse"
-    filename = "CHANGES.md"
+filename = "CHANGES.md"
-    directory = "changelog.d"
+directory = "changelog.d"
-    issue_format = "[\\#{issue}](https://github.com/element-hq/synapse/issues/{issue})"
+issue_format = "[\\#{issue}](https://github.com/element-hq/synapse/issues/{issue})"
 
-    [[tool.towncrier.type]]
+[[tool.towncrier.type]]
-        directory = "feature"
+directory = "feature"
-        name = "Features"
+name = "Features"
-        showcontent = true
+showcontent = true
 
-    [[tool.towncrier.type]]
+[[tool.towncrier.type]]
-        directory = "bugfix"
+directory = "bugfix"
-        name = "Bugfixes"
+name = "Bugfixes"
-        showcontent = true
+showcontent = true
 
-    [[tool.towncrier.type]]
+[[tool.towncrier.type]]
-        directory = "docker"
+directory = "docker"
-        name = "Updates to the Docker image"
+name = "Updates to the Docker image"
-        showcontent = true
+showcontent = true
 
-    [[tool.towncrier.type]]
+[[tool.towncrier.type]]
-        directory = "doc"
+directory = "doc"
-        name = "Improved Documentation"
+name = "Improved Documentation"
-        showcontent = true
+showcontent = true
 
-    [[tool.towncrier.type]]
+[[tool.towncrier.type]]
-        directory = "removal"
+directory = "removal"
-        name = "Deprecations and Removals"
+name = "Deprecations and Removals"
-        showcontent = true
+showcontent = true
 
-    [[tool.towncrier.type]]
+[[tool.towncrier.type]]
-        directory = "misc"
+directory = "misc"
-        name = "Internal Changes"
+name = "Internal Changes"
-        showcontent = true
+showcontent = true
 
 [tool.ruff]
 line-length = 88
@@ -47,11 +47,7 @@ target-version = "py39"
 # flake8-bugbear compatible checks. Its error codes are described at
 # https://beta.ruff.rs/docs/rules/#flake8-bugbear-b
 #  B023: Functions defined inside a loop must not use variables redefined in the loop
-ignore = [
+ignore = ["B023", "E501", "E731"]
-    "B023",
-    "E501",
-    "E731",
-]
 select = [
     # pycodestyle
     "E",
@@ -82,7 +78,15 @@ select = [
 
 [tool.ruff.lint.isort]
 combine-as-imports = true
-section-order = ["future", "standard-library", "third-party", "twisted", "first-party", "testing", "local-folder"]
+section-order = [
+    "future",
+    "standard-library",
+    "third-party",
+    "twisted",
+    "first-party",
+    "testing",
+    "local-folder",
+]
 known-first-party = ["synapse"]
 
 [tool.ruff.lint.isort.sections]
@@ -107,9 +111,7 @@ authors = ["Matrix.org Team and Contributors <packages@matrix.org>"]
 license = "AGPL-3.0-or-later"
 readme = "README.rst"
 repository = "https://github.com/element-hq/synapse"
-packages = [
+packages = [{ include = "synapse" }]
-    { include = "synapse" },
-]
 classifiers = [
     "Development Status :: 5 - Production/Stable",
     "Topic :: Communications :: Chat",
@@ -125,7 +127,7 @@ include = [
     { path = "INSTALL.md", format = "sdist" },
     { path = "mypy.ini", format = "sdist" },
     { path = "scripts-dev", format = "sdist" },
-    { path = "synmark", format="sdist" },
+    { path = "synmark", format = "sdist" },
     { path = "sytest-blacklist", format = "sdist" },
     { path = "tests", format = "sdist" },
     { path = "UPGRADE.rst", format = "sdist" },
@@ -135,9 +137,7 @@ include = [
     { path = "rust/build.rs", format = "sdist" },
     { path = "rust/src/**", format = "sdist" },
 ]
-exclude = [
+exclude = [{ path = "synapse/*.so", format = "sdist" }]
-    { path = "synapse/*.so", format = "sdist"}
-]
 
 [tool.poetry.build]
 script = "build_rust.py"
@@ -178,7 +178,7 @@ signedjson = "^1.1.0"
 service-identity = ">=18.1.0"
 # Twisted 18.9 introduces some logger improvements that the structured
 # logger utilises
-Twisted = {extras = ["tls"], version = ">=18.9.0"}
+Twisted = { extras = ["tls"], version = ">=18.9.0" }
 treq = ">=15.1"
 # Twisted has required pyopenssl 16.0 since about Twisted 16.6.
 pyOpenSSL = ">=16.0.0"
@@ -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"
@@ -297,7 +299,9 @@ all = [
     # matrix-synapse-ldap3
     "matrix-synapse-ldap3",
     # postgres
-    "psycopg2", "psycopg2cffi", "psycopg2cffi-compat",
+    "psycopg2",
+    "psycopg2cffi",
+    "psycopg2cffi-compat",
     # saml2
     "pysaml2",
     # oidc and jwt
@@ -307,9 +311,11 @@ all = [
     # sentry
     "sentry-sdk",
     # opentracing
-    "jaeger-client", "opentracing",
+    "jaeger-client",
+    "opentracing",
     # redis
-    "txredisapi", "hiredis",
+    "txredisapi",
+    "hiredis",
     # cache-memory
     "pympler",
     # improved user search
@@ -398,7 +404,7 @@ enable = "pypy"
 # We temporarily pin Rust to 1.82.0 to work around
 # https://github.com/element-hq/synapse/issues/17988
 before-all = "sh .ci/before_build_wheel.sh"
-environment= { PATH = "$PATH:$HOME/.cargo/bin" }
+environment = { PATH = "$PATH:$HOME/.cargo/bin" }
 
 # For some reason if we don't manually clean the build directory we
 # can end up polluting the next build with a .so that is for the wrong

schema/synapse-config.schema.yaml

@@ -451,8 +451,7 @@ properties:
           type: string
           description: >-
             The type of listener. Normally `http`, but other valid options are
-            [`manhole`](../../manhole.md) and
+            [`manhole`](../../manhole.md).
-            [`metrics`](../../metrics-howto.md).
           enum:
             - http
             - manhole

synapse/app/_base.py

@@ -283,42 +283,6 @@ def register_start(
     reactor.callWhenRunning(lambda: defer.ensureDeferred(wrapper()))
 
 
-def listen_metrics(bind_addresses: StrCollection, port: int) -> None:
-    """
-    Start Prometheus metrics server.
-    """
-    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/generic_worker.py

@@ -283,23 +283,6 @@ class GenericWorkerServer(HomeServer):
                     raise ConfigError(
                         "Can not using a unix socket for manhole at this time."
                     )
-
-            elif listener.type == "metrics":
-                if not self.config.metrics.enable_metrics:
-                    logger.warning(
-                        "Metrics listener configured, but enable_metrics is not True!"
-                    )
-                else:
-                    if isinstance(listener, TCPListenerConfig):
-                        _base.listen_metrics(
-                            listener.bind_addresses,
-                            listener.port,
-                        )
-                    else:
-                        raise ConfigError(
-                            "Can not use a unix socket for metrics at this time."
-                        )
-
             else:
                 logger.warning("Unsupported listener type: %s", listener.type)
 

synapse/app/homeserver.py

@@ -286,22 +286,6 @@ class SynapseHomeServer(HomeServer):
                     raise ConfigError(
                         "Can not use a unix socket for manhole at this time."
                     )
-            elif listener.type == "metrics":
-                if not self.config.metrics.enable_metrics:
-                    logger.warning(
-                        "Metrics listener configured, but enable_metrics is not True!"
-                    )
-                else:
-                    if isinstance(listener, TCPListenerConfig):
-                        _base.listen_metrics(
-                            listener.bind_addresses,
-                            listener.port,
-                        )
-                    else:
-                        raise ConfigError(
-                            "Can not use a unix socket for metrics at this time."
-                        )
-
             else:
                 # this shouldn't happen, as the listener type should have been checked
                 # during parsing

synapse/metrics/__init__.py

@@ -25,6 +25,7 @@ import logging
 import os
 import platform
 import threading
+from importlib import metadata
 from typing import (
     Callable,
     Dict,
@@ -41,19 +42,28 @@ from typing import (
 )
 
 import attr
-from prometheus_client import CollectorRegistry, Counter, Gauge, Histogram, Metric
+from pkg_resources import parse_version
-from prometheus_client.core import (
+from prometheus_client import (
     REGISTRY,
+    CollectorRegistry,
+    Counter,
+    Gauge,
+    Histogram,
+    Metric,
+    generate_latest,
+)
+from prometheus_client.core import (
     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
@@ -66,6 +76,47 @@ all_gauges: Dict[str, Collector] = {}
 
 HAVE_PROC_SELF_STAT = os.path.exists("/proc/self/stat")
 
+CONTENT_TYPE_LATEST = "text/plain; version=0.0.4; charset=utf-8"
+
+
+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_prometheus_client_use_created_metrics(False)
+
 
 class _RegistryProxy:
     @staticmethod
@@ -474,6 +525,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",

synapse/metrics/_twisted_exposition.py

@@ -1,45 +0,0 @@
-#
-# This file is licensed under the Affero General Public License (AGPL) version 3.
-#
-# Copyright 2019 Matrix.org Foundation C.I.C.
-# Copyright 2015-2019 Prometheus Python Client Developers
-# Copyright (C) 2023 New Vector, Ltd
-#
-# This program is free software: you can redistribute it and/or modify
-# it under the terms of the GNU Affero General Public License as
-# published by the Free Software Foundation, either version 3 of the
-# License, or (at your option) any later version.
-#
-# See the GNU Affero General Public License for more details:
-# <https://www.gnu.org/licenses/agpl-3.0.html>.
-#
-# Originally licensed under the Apache License, Version 2.0:
-# <http://www.apache.org/licenses/LICENSE-2.0>.
-#
-# [This file includes modifications made by New Vector Limited]
-#
-#
-
-from prometheus_client import REGISTRY, CollectorRegistry, generate_latest
-
-from twisted.web.resource import Resource
-from twisted.web.server import Request
-
-CONTENT_TYPE_LATEST = "text/plain; version=0.0.4; charset=utf-8"
-
-
-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

tests/metrics/test_metrics.py

@@ -18,14 +18,10 @@
 # [This file includes modifications made by New Vector Limited]
 #
 #
-from importlib import metadata
 from typing import Dict, Protocol, Tuple
-from unittest.mock import patch
 
-from pkg_resources import parse_version
 from prometheus_client.core import Sample
 
-from synapse.app._base import _set_prometheus_client_use_created_metrics
 from synapse.metrics import REGISTRY, InFlightGauge, generate_latest
 from synapse.util.caches.deferred_cache import DeferredCache
 
@@ -184,30 +180,3 @@ class CacheMetricsTests(unittest.HomeserverTestCase):
 
         self.assertEqual(items["synapse_util_caches_cache_size"], "1.0")
         self.assertEqual(items["synapse_util_caches_cache_max_size"], "777.0")
-
-
-class PrometheusMetricsHackTestCase(unittest.HomeserverTestCase):
-    if parse_version(metadata.version("prometheus_client")) < parse_version("0.14.0"):
-        skip = "prometheus-client too old"
-
-    def test_created_metrics_disabled(self) -> None:
-        """
-        Tests that a brittle hack, to disable `_created` metrics, works.
-        This involves poking at the internals of prometheus-client.
-        It's not the end of the world if this doesn't work.
-
-        This test gives us a way to notice if prometheus-client changes
-        their internals.
-        """
-        import prometheus_client.metrics
-
-        PRIVATE_FLAG_NAME = "_use_created"
-
-        # By default, the pesky `_created` metrics are enabled.
-        # Check this assumption is still valid.
-        self.assertTrue(getattr(prometheus_client.metrics, PRIVATE_FLAG_NAME))
-
-        with patch("prometheus_client.metrics") as mock:
-            setattr(mock, PRIVATE_FLAG_NAME, True)
-            _set_prometheus_client_use_created_metrics(False)
-            self.assertFalse(getattr(mock, PRIVATE_FLAG_NAME, False))

tests/storage/test_event_metrics.py

@@ -18,9 +18,8 @@
 # [This file includes modifications made by New Vector Limited]
 #
 #
-from prometheus_client import generate_latest
 
-from synapse.metrics import REGISTRY
+from synapse.metrics import REGISTRY, generate_latest
 from synapse.types import UserID, create_requester
 
 from tests.unittest import HomeserverTestCase