Remove metrics listener type in favor of http listener with metrics resource

[MadLittleMods]

Remove metrics listener type in favor of http listener with metrics resource (which already exists).

This is spawning from wanting to refactor which registry the metrics are pulled from and noticing two sources of truth. The refactor itself is spawning from running multiple Synapse in the same Python process and wanting separate metrics (see #18592)

This will require homeserver admins to update their homeserver configs in some cases. I've drafted upgrade notes for the release below.

Split out from #18443

Part of #18592

Why did we have a separate metrics listener type?

See discussion below: #18584 (comment)

Draft upgrade notes

Drafted to make it easy to add to docs/upgrade.md for whoever is making the release.

Upgrade notes

Upgrading to vUPCOMING

Dedicated metrics listener removed in favor of metrics resource in the http listener

The dedicated type: metrics listener has been removed. Use the metrics resource in an type: http listener instead.

Example before:

homeserver.yaml

listeners:
  - port: 9000
    type: metrics
    bind_addresses: ['::1', '127.0.0.1']

Example after:

homeserver.yaml

listeners:
  - port: 9000
    type: http
    bind_addresses: ['::1', '127.0.0.1']
    resources:
      - names: [metrics]
        compress: false

Note: The endpoint path changes from /metrics to /_synapse/metrics - update your metrics scraper (e.g., Prometheus) accordingly.

See the Metrics How-to page for more information on how to setup and configure metrics.

Testing strategy

See behavior of previous metrics listener

1. Add the metrics listener in your homeserver.yaml

   listeners:
     - port: 9323
       type: metrics
       bind_addresses: ['127.0.0.1']

2. Start the homeserver: poetry run synapse_homeserver --config-path homeserver.yaml
3. Fetch http://localhost:9323/metrics
4. Observe response: example from this PR

See behavior of the http metrics resource

1. Add the metrics resource to a new or existing http listeners in your homeserver.yaml

   listeners:
     - port: 9322
       type: http
       bind_addresses: ['127.0.0.1']
       resources:
         - names: [metrics]
           compress: false

2. Start the homeserver: poetry run synapse_homeserver --config-path homeserver.yaml
3. Fetch http://localhost:9322/_synapse/metrics (it's just a GET request so you can even do in the browser)
4. Observe response: example, example from develop

There is no difference in the response from this PR to develop or from this resource to the metrics listener being removed.

Dev notes

• metrics listener uses listen_metrics
• http listener with metrics resource uses MetricsResource

Related PRs:

• matrix-org/synapse@b98b4c1
• Support Prometheus_client 0.4.0+ matrix-org/synapse#5636
• Remove legacy Prometheus metrics names. They were deprecated in Synapse v1.69.0 and disabled by default in Synapse v1.71.0. matrix-org/synapse#14538

_set_prometheus_client_use_created_metrics

_set_prometheus_client_use_created_metrics() was added in matrix-org/synapse#13540 (2022-08-16)

But now this functionality has dedicated functions inprometheus_client via disable_created_metrics()/enable_created_metrics() which was added in prometheus/client_python#973 (2023-10-26). Part of [email protected] (2023-10-30).

Our deprecation policy states that we support whatever is provided by Debian oldstable. Which for python-prometheus-client is currently 0.6.0-1 (too old).

Todo

• Figure out _set_prometheus_client_use_created_metrics

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• Code style is correct (run the linters)

[MadLittleMods]

Opinions on calling out the upgrade notes from the changelog entry itself? It looks like usually we prefer to call it out at the top of the changelog section for the version. Doesn't seem like it would hurt to have it in both places. The only problem being the chicken and egg problem of the upgrade notes being written later in order to have the link filled in.

Suggested change

	Remove `metrics` listener (serves metrics at `/metrics`) type in favor of `http` listener with `metrics` resource (serves metrics at `/_synapse/metrics`).
	Remove `metrics` listener (serves metrics at `/metrics`) type in favor of `http` listener with `metrics` resource (serves metrics at `/_synapse/metrics`). Please check the [relevant section in the upgrade notes](TODO).

[MadLittleMods]

The only change is here.

The rest is auto-formatting which I can remove from the diff if prompted.

[MadLittleMods]

Deleted this file in favor of re-organizing the remaining pieces in more relevant file names.

[MadLittleMods]

Removed this test as it claims to "gives us a way to notice if prometheus-client changes their internals." but we already have that benefit from the hack function _set_prometheus_client_use_created_metrics itself (it will yell if the _use_created attribute is no longer used).

The test provides no further benefit beside asserting that the attribute was set which feels pretty useless. A proper test would ensure that before running the utility, we see the legacy _created metrics and after we don't.

[MadLittleMods]

Stray thing that I noticed. Making it consistent that we pull from synapse.metrics for generate_latest since it's something we're explicitly exporting

synapse/synapse/metrics/__init__.py

Lines 477 to 480 in 3cabaa8

	__all__ = [
	"Collector",
	"MetricsResource",
	"generate_latest",

[MadLittleMods]

It's unclear to me why metrics was its own listener type before. The only difference I can see is that they are served on different paths: /metrics vs /_synapse/metrics. Feels like we just need to include this in the upgrade notes (drafted in the PR description).

[MadLittleMods]

One thing that @erikjohnston pointed out is that the dedicated metrics listener uses it's own thread and doesn't rely on the Twisted reactor so it still works regardless of how well Synapse is working. It's unclear if we really care about this benefit. It does have a theoretical benefit of being able to monitor a really bogged down Synapse.

Looking at the git history, the threading aspect just seems like the way it was originally implemented and not something explicitly called out for the benefit described above.

[MadLittleMods]

Even with the updated conclusion from #18592 (comment) which most likely means we will continue to use the global REGISTRY, I think it's still worth distilling down to a single way to setup metrics; for the config clarity sake and for the codebase single source of truth sake.

(assuming we can't come up with a benefit/compelling reason for why we want both)

[erikjohnston]

Having thought about it a bit: I think it is worth keeping the separate metrics listener. I can't find where (and I looked), but I believe we have ported stuff to use the separate metrics listener in the past to get metrics from the process when its under extreme load (to the extent where twisted can't process new requests).

Even if we keep the separate listener, I think we should still make them render from the same source and remove any differences. If needed its easy enough to change the metrics listener to not use the inbuilt prometheus one, but instead replace it by spawning a thread and starting a basic web server that responds to /metrics.

[MadLittleMods]

separate metrics listener in the past to get metrics from the process when its under extreme load (to the extent where twisted can't process new requests).

How sure are we that we care about this? Feels like a use case that we're just holding onto especially when we can't find any evidence.

From what I'm reading, threading.Thread (which prometheus_client.start_http_server_prometheus(...) uses), shares the same process and memory space so it doesn't even help us in the scenarios where Synapse is under extreme load.

[...] the threading module operates within a single process, meaning that all threads share the same memory space. However, the GIL limits the performance gains of threading when it comes to CPU-bound tasks, as only one thread can execute Python bytecode at a time.

-- https://docs.python.org/3/library/threading.html#gil-and-performance-considerations

---

Even if we keep the separate listener, I think we should still make them render from the same source and remove any differences.

I don't see an easy way forward as I don't know how to start another Twisted server in another thread in order to share the MetricsResource. It's a pretty simple resource so trying to abstract it any further isn't useful and we might as well keep the status-quo of disparate handling 🤷

I can pull out some of the clean-up from this PR to ship separately.

[reivilibre]

We relatively often have episodes on matrix.org where the reactor tick times are in the multiples of seconds or tens of seconds — this seems like a case where the separate metrics thread is a winner to me, so that metric exposition continues to work in this situation.

The reason why this helps, despite the GIL, is because the metrics thread can preempt the reactor thread between bytecode boundaries and the metrics thread gets scheduled with roughly equal priority to the reactor thread, whereas (afaik) there isn't an easy mechanism to do something similar between separate twisted tasks/deferreds on the same reactor (and deferreds are co-operatively scheduled so we can't actually force one to yield to another).

For illustration: if you want to disrupt metrics reporting for the reactor version, all you need to do is time.sleep(5), or open a file on a slow disk, or run a computationally-heavy loop without any yield points.

The more realistic case is likely just having lots of tasks in the reactor queue. In this case, the mental model is that the metrics request is scheduled with roughly equal priority to all the user requests (which, in some dodgy scenarios, there could be thousands of).

[MadLittleMods]

Thanks for the extra details @reivilibre 🙂

That proves well enough that this actually works how we want and we've already expressed interest in wanting the benefit ⏩

[MadLittleMods]

Here is some prior art that I missed earlier that confirms why we care about having this:

synapse/docs/metrics-howto.md

Lines 41 to 45 in fc10a5e

	The second method runs the metrics server on a different port, in a
	different thread to Synapse. This can make it more resilient to
	heavy load meaning metrics cannot be retrieved, and can be exposed
	to just internal networks easier. The served metrics are available
	over HTTP only, and will be available at `/_synapse/metrics`.

And traced it back to the PR that introduced this language, matrix-org/synapse#3274, which also states:

This should allow us to be resistant to Twisted melting down meaning we don't get metrics, in theory.

When I originally traced back to where the metrics listener type was introduced, I thought it was matrix-org/synapse#5636 (which doesn't have the context) but it actually goes further back to that PR.

[MadLittleMods]

In addition to the docs we already have, I've also added the context and @reivilibre's explanation for why it works to some comments in the code itself, see #18687