Repository navigation
Clean up MetricsResource and Prometheus hacks - #18687
Conversation
| 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. | ||
| """ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Deleted this file in favor of re-organizing the remaining pieces in more relevant file names.
| # `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" |
There was a problem hiding this comment.
Updated this dependency as we were running into the following error at run-time without it.
ModuleNotFoundError: No module named 'prometheus_client.metrics'
It seems like the only reason we were getting away with this before was we only called _set_prometheus_client_use_created_metrics() within a run-time method instead of globally.
prometheus_client.metrics was added in prometheus_client@0.5.0 as far as I can tell.
Since we can update the minimum version to 0.6.0-1, this should work sufficiently to get around this.
Our deprecation policy states that we support whatever is provided by Debian oldstable. Which for
python-prometheus-clientis currently0.6.0-1(too old).
| # Set this globally so it applies wherever we generate/collect metrics | ||
| _set_prometheus_client_use_created_metrics(False) |
There was a problem hiding this comment.
Previously, we only set this when using metrics listener type.
Now we call it globally so it applies wherever we generate_latest()
Also way better organized in this file instead of it's old spot in synapse/app/_base.py
| """ | ||
|
|
||
|
|
||
| def _set_prometheus_client_use_created_metrics(new_value: bool) -> None: |
There was a problem hiding this comment.
The logic itself in _set_prometheus_client_use_created_metrics has been updated to be more robust
… code As explained by @reivilibre, #18584 (comment)
| 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. |
There was a problem hiding this comment.
As explained by @reivilibre, #18584 (comment)
| 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. |
There was a problem hiding this comment.
We already have this in our docs but it's easy to miss when you're just staring at the code,
Lines 41 to 45 in fc10a5e
Conflicts: poetry.lock
|
Thanks for the review and taking the time to look into and explain how |
Collection runs on the prometheus_client HTTP server's own thread, concurrent with the reactor, so a gauge callback that reads reactor-owned state can raise. Today one raising collector fails the whole `/metrics` scrape with a 500, and `socketserver`'s default `handle_error` prints the traceback to `sys.stderr` with one `write()` per line, which Synapse's stdio-to-logs redirection turns into one ERROR log record per traceback line. `_RegistryProxy.collect` now gives each collector in the underlying `REGISTRY` its own error boundary: a collector that raises is logged once, named and with its traceback attached, and the rest of the scrape still returns its metrics with a 200. The ERROR rate per scrape is unchanged at one record per failing collector per scrape, which is what `LaterGauge.collect` already does; what changes is that those records group into one issue instead of one per traceback line, and that the scrape keeps being served. `RegistryProxy` also grows `restricted_registry`, which `prometheus_client.exposition._bake_output` calls for `?name[]=` scrapes and which the proxy lacked, so those scrapes raised `AttributeError`. It returns a proxy of its own rather than delegating to `CollectorRegistry.restricted_registry`, whose `RestrictedRegistry` collects from the real `REGISTRY` and so would bypass both the error boundary and the `__`-prefix filter. The error boundary lives in the registry rather than the WSGI layer because that is the behaviour Prometheus scrapers want: the remaining series keep being served regardless of how the `/metrics` endpoint happens to be wired up. `_handle_metrics_request_error` (installed as `server.handle_error`) stays, since a scraper disconnecting while its request is being read is the only error path left that reaches socketserver. The `prometheus-client` floor moves to 0.20.0, the first version whose `start_http_server` returns the `(server, thread)` pair that `listen_metrics` has unpacked since v1.135.0 (#18687). Below that floor the metrics listener could not start at all. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Clean up
MetricsResource, Prometheus hacks (_set_prometheus_client_use_created_metrics), and better document why we care about having a separatemetricslistener type.These clean-up changes have been split out from #18584 since that PR was closed.
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.