-
Notifications
You must be signed in to change notification settings - Fork 404
Refactor Measure block metrics to be homeserver-scoped
#18591
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Fix `HomeserverMetricsManager` lints Fix `metrics_manager` missing in `Measure` `__slots__`
Not finished: - `synapse/http/federation/well_known_resolver.py` - `measure_func`
1962fc8 to
5c5090f
Compare
…round Fix mypy complaints ``` synapse/handlers/delayed_events.py:266: error: Cannot determine type of "validator" [has-type] synapse/handlers/delayed_events.py:267: error: Cannot determine type of "event_builder_factory" [has-type] ```
5c5090f to
e46690c
Compare
This matches the pattern of other homeserver attributes like `hs.get_clock()`
Measure block metrics to be homeserver-scopedMeasure block metrics to be be ready to be homeserver-scoped
| def listen_metrics( | ||
| bind_addresses: StrCollection, port: int, metrics_manager: HomeserverMetricsManager | ||
| ) -> None: | ||
| """ | ||
| Start Prometheus metrics server. | ||
| """ | ||
| from prometheus_client import start_http_server as start_http_server_prometheus | ||
| from prometheus_client import ( | ||
| REGISTRY, | ||
| CollectorRegistry, | ||
| start_http_server as start_http_server_prometheus, | ||
| ) | ||
|
|
||
| from synapse.metrics import RegistryProxy | ||
| from synapse.metrics import CombinedRegistryProxy | ||
|
|
||
| combined_registry_proxy = CombinedRegistryProxy( | ||
| [ | ||
| # TODO: Remove `REGISTRY` once all metrics have been migrated to the | ||
| # homeserver specific metrics collector registry, see | ||
| # https://github.com/element-hq/synapse/issues/18592 | ||
| REGISTRY, | ||
| metrics_manager.metrics_collector_registry, | ||
| ] | ||
| ) | ||
| # Cheeky cast but matches the signature of a `CollectorRegistry` instance enough | ||
| # for it to be usable in the contexts in which we use it. | ||
| # TODO Do something nicer about this. | ||
| registry = cast(CollectorRegistry, combined_registry_proxy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this work can be removed if we decide that it's okay to drop the type: metrics listener (part of #18584)
Measure block metrics to be be ready to be homeserver-scopedMeasure block metrics to be homeserver-scoped
| @staticmethod | ||
| def collect() -> Iterable[Metric]: | ||
| for metric in REGISTRY.collect(): | ||
| if not metric.name.startswith("__"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any metrics that start with __ so I've removed this logic
| for metric in REGISTRY.collect(): | ||
| if not metric.name.startswith("__"): | ||
| yield metric | ||
| class CombinedRegistryProxy: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CombinedRegistryProxy can be removed if we decide that it's okay to drop the type: metrics listener (part of #18584) since it's the only usage.
| # While we're in the middle of the refactor of metrics in Synapse, we need to | ||
| # merge the metrics from the global registry and the homeserver specific metrics | ||
| # collector registry. | ||
| # | ||
| # TODO: Remove `generate_latest(REGISTRY)` once all homeserver metrics have been | ||
| # migrated to the homeserver specific metrics collector registry, see | ||
| # https://github.com/element-hq/synapse/issues/18592 | ||
| response = generate_latest(REGISTRY) + generate_latest( | ||
| self.metrics_manager.metrics_collector_registry | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using CombinedRegistryProxy here, I've decided to do something way more straight-forward and avoid the cheeky casting.
We have to use CombinedRegistryProxy in the other spot because we don't control how it collects metrics.
| self.in_flight: InFlightGauge[BlockInFlightMetric] = InFlightGauge( | ||
| "synapse_util_metrics_block_in_flight", | ||
| "", | ||
| labels=["block_name"], | ||
| # Matches the fields in the `BlockInFlightMetric` | ||
| sub_metrics=["real_time_max", "real_time_sum"], | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, this specific metric doesn't use the homeserver metrics_collector_registry. This is planned for a future PR where we update InFlightGauge to support a custom registry option. Tracked in #18592
| # Cheeky cast but matches the signature of a `CollectorRegistry` instance enough | ||
| # for it to be usable in the contexts in which we use it. | ||
| # TODO Do something nicer about this. | ||
| registry = cast(CollectorRegistry, combined_registry_proxy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cast is the same thing we were doing before with the RegistryProxy, nothing new. I'm not very keen on thinking of something better here.
|
Per our decision in #18592 (comment) to add |
Refactor
Measureblock metrics to be homeserver-scoped.Spawning from #18443
Part of #18592
Testing strategy
See behavior of previous
metricslistenermetricslistener in yourhomeserver.yamlpoetry run synapse_homeserver --config-path homeserver.yamlhttp://localhost:9323/metricssynapse_util_metrics_block_count,synapse_util_metrics_block_in_flight, etc)See behavior of the
httpmetricsresourcemetricsresource to a new or existinghttplisteners in yourhomeserver.yamlpoetry run synapse_homeserver --config-path homeserver.yamlhttp://localhost:9322/_synapse/metrics(it's just aGETrequest so you can even do in the browser)synapse_util_metrics_block_count,synapse_util_metrics_block_in_flight, etc)Dev notes
HomeserverMetricsManagerTodo
MeasurePull Request Checklist
EventStoretoEventWorkerStore.".code blocks.