-
Notifications
You must be signed in to change notification settings - Fork 347
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
Measure
block metrics to be homeserver-scoped.Spawning from #18443
Part of #18592
Testing strategy
See behavior of previous
metrics
listenermetrics
listener in yourhomeserver.yaml
poetry run synapse_homeserver --config-path homeserver.yaml
http://localhost:9323/metrics
synapse_util_metrics_block_count
,synapse_util_metrics_block_in_flight
, etc)See behavior of the
http
metrics
resourcemetrics
resource to a new or existinghttp
listeners in yourhomeserver.yaml
poetry run synapse_homeserver --config-path homeserver.yaml
http://localhost:9322/_synapse/metrics
(it's just aGET
request so you can even do in the browser)synapse_util_metrics_block_count
,synapse_util_metrics_block_in_flight
, etc)Dev notes
HomeserverMetricsManager
Todo
Measure
Pull Request Checklist
EventStore
toEventWorkerStore
.".code blocks
.