Refactor metrics to be scoped to the homeserver

[MadLittleMods]

Background

As part of Element's plan to support a light form of vhosting (virtual host) (multiple homeserver tenants in a single shard), we're currently diving into the details and implications of running multiple instances of Synapse in the same Python process. Currently, metrics are combined across all Synapse instances because the Synapse codebase uses the default global REGISTRY provided by the Prometheus client.

"Per-tenant metrics" tracked internally by https://github.com/element-hq/synapse-small-hosts/issues/5

Potential solutions

(as discussed below)

Homeserver-specific CollectorRegistry (specify registry)

Prometheus has a concept of metric CollectorRegistry's and all of the metrics from the client library already support specifying the registry. We could refactor things to point to our homeserver-specific registry instead of the global REGISTRY which is the default. This is also the mechanism that naturally maps to the problem of metrics from different hosts (registries) as that's what would happen if the servers were separate.

But this still results in instance labels getting added to each metric when scraped: "When Prometheus scrapes a target, it attaches some labels automatically to the scraped time series which serve to identify the scraped target: [job, instance]" (source) which means we will get instance labels with the same amount of cardinality as if we just added them ourselves.

Add server name instance label to the metrics

Adding the instance label (server name) to the metric itself has the benefit that we can distinguish between metrics that apply on the Python process level vs the per-homeserver-tenant level. We don't need to label the the per-process metrics that can only really be measured at the Python process level (process_*, python_*) like CPU usage, Python garbage collection, Twisted reactor tick time. Which means we don't need waste space on storing duplicate metrics or have a misleading instance label for a homeserver tenant but it really applies to all tenants in the shard process.

Because instance has special meaning according to the Prometheus docs and should be "The <host>:<port> part of the target's URL that was scraped." (source) (also: "In Prometheus terms, an endpoint you can scrape is called an instance, usually corresponding to a single process.") which is different from how we're using it here; we're using our own custom server_name label.

The docs mention it's fine to provide instance label in the scraped data and we just need to have honor_labels: true configured in the Prometheus scrape config. (no longer relevant since we're using our own server_name label)

We also have a simple scrape story: The best setup seems to be to use a single scrape for the whole shard (consisting of multiple homeserver tenants) and continue to use the global REGISTRY in Synapse. All of the per-tenant data will have the appropriate instance label as necessary and per-process data won't be duplicated because it's a single scrape.

Either method will be a similar amount of bulk refactoring as we need to specify the registry or an extra label either way.

Plan forward

Add server_name labels to all homeserver-scoped metrics.

We will not be changing the the per-process metrics that can only really be measured at the Python process level (process_*, python_*) like CPU usage, Python garbage collection, Twisted reactor tick time.

This issue serves as a central place to track work remaining as we refactor the codebase.

• Refactor built-in Prometheus metrics to use homeserver-scoped registry
  • Counter -> Refactor Counter metrics to be homeserver-scoped #18656, Refactor background process metrics to be homeserver-scoped #18670
  • Histogram -> Refactor Histogram metrics to be homeserver-scoped #18724
  • Gauge -> Refactor Gauge metrics to be homeserver-scoped #18725
  • Summary -> Update metrics linting to be able to handle custom metrics #18733
  • Info -> Update metrics linting to be able to handle custom metrics #18733
  • Enum -> Update metrics linting to be able to handle custom metrics #18733
• Refactor our custom Prometheus Collector metrics to use homeserver-scoped registry
  • LaterGauge -> Refactor LaterGauge metrics to be homeserver-scoped #18714
  • InFlightGauge -> Refactor Measure block metrics to be homeserver-scoped (v2) #18601, Update metrics linting to be able to handle custom metrics #18733
  • GaugeBucketCollector -> Refactor GaugeBucketCollector metrics to be homeserver-scoped #18715
  • CPUMetrics (this is a per-process metric) -> Update metrics linting to be able to handle custom metrics #18733
  • GCCounts (this is a per-process metric) -> Update metrics linting to be able to handle custom metrics #18733
  • PyPyGCStats (this is a per-process metric) -> Update metrics linting to be able to handle custom metrics #18733
  • ReactorLastSeenMetric (this is a per-process metric) -> Update metrics linting to be able to handle custom metrics #18733
  • Background process _Collector -> Refactor background process metrics to be homeserver-scoped #18670
  • JemallocCollector (this is a per-process metric) -> Update metrics linting to be able to handle custom metrics #18733
  • DynamicCollectorRegistry -> Update metrics linting to be able to handle custom metrics #18733, Cleanly shutdown SynapseHomeServer object #18828
• Refactor wrappers around metrics:
  • Cache metrics: LruCache/@cached, CacheMetric -> Refactor cache metrics to be homeserver-scoped #18604, Cleanly shutdown SynapseHomeServer object #18828
  • Measure -> Refactor Measure block metrics to be homeserver-scoped #18591 Refactor Measure block metrics to be homeserver-scoped (v2) #18601
• Conflicting metrics that already use the server_name label:
  • synapse_federation_last_received_pdu_time -> Refactor Gauge metrics to be homeserver-scoped #18725
  • synapse_federation_last_sent_pdu_time -> Refactor Gauge metrics to be homeserver-scoped #18725
• Make sure we lint custom Metric to ensure the SERVER_NAME_LABEL is included -> Update metrics linting to be able to handle custom metrics #18733
  • UnknownMetricFamily
  • CounterMetricFamily
  • GaugeMetricFamily
  • SummaryMetricFamily
  • InfoMetricFamily
  • HistogramMetricFamily
  • GaugeHistogramMetricFamily
  • StateSetMetricFamily
  • Our own GaugeHistogramMetricFamilyWithLabels (introduced in Refactor GaugeBucketCollector metrics to be homeserver-scoped #18715)
• Add linting to prevent usage of REGISTRY (no longer relevant because we moved to adding instance labels to the metrics)
• Add linting to prevent new metrics being used that use the default REGISTRY (no longer relevant because we moved to adding instance labels to the metrics)
• Add linting to ensure metrics have a SERVER_NAME_LABEL label added. Per-process metrics should just use a lint ignore comment with the reasoning why. -> Refactor Counter metrics to be homeserver-scoped #18656
• Add linting to ensure metric reporting includes SERVER_NAME_LABEL value. Per-process metrics should just use a lint ignore comment with the reasoning why. (No need as if you pass in the wrong number of labels, then prometheus_client will raise a ValueError at runtime. Ideally, we'd be able to catch this at type-checking time but this is good enough.)

[erikjohnston]

After looking at some of the PRs, I am wondering if an easier approach would be instead to make all the relevant metrics have a server_name label instead? This is effectively what would happen on prometheus anyway, as split out metrics would still appear on prometheus as the same metrics with an extra label?

This would also make it more natural to deal with metrics that are truly per-process rather than per-tenant. I'm also thinking it may not be that much different than the implementation that we have (instead of using the right registry we make sure we use the right label), however I can well imagine there is something I'm missing here.

[MadLittleMods]

Per-homeserver-tenant metrics

After looking at some of the PRs, I am wondering if an easier approach would be instead to make all the relevant metrics have a server_name label instead? This is effectively what would happen on prometheus anyway, as split out metrics would still appear on prometheus as the same metrics with an extra label?

In terms of why we didn't go with the server_name label route initially, it was under the idea to avoid labels with high cardinality (labels with large number of unique values). I was unaware that "When Prometheus scrapes a target, it attaches some labels automatically to the scraped time series which serve to identify the scraped target: [job, instance]" (source) which means we will get labels with the same amount of cardinality no matter what.

---

In terms of evaluating this now:

My first reaction (before I looked into the automatic job/instance labeling) is that if we end up with the same amount of refactor and it's stored the same anyway, I'd prefer to use the mechanism that more naturally maps to the problem of metrics from different hosts (registries) as that's what would happen if the servers were separate. Specifying a registry is already a known pattern in the Prometheus client for all of the metrics so this isn't something exotic either.

But all of that is pretty moot (no longer relevant) given it doesn't really matter how instance label gets on the data. The docs even mention it's fine to provide it in the scraped data when honor_labels: true is configured. We just need to choose the pattern that has the most benefits for us.

Process-level metrics

This would also make it more natural to deal with metrics that are truly per-process rather than per-tenant.

To make it more clear, you're talking about the metrics that can only really be measured at the Python process level (process_*, python_*) like CPU usage, Python garbage collection, Twisted reactor tick time.

The concerns here are duplicated/wasted data and slightly misleading data because it has a instance label for a specific tenant server but it really applies to all tenants in the shard process.

The best setup seems to be to use a single scrape for the whole shard and continue to use the global REGISTRY in Synapse. All of the per-tenant data will have the appropriate instance label as necessary and per-process data won't be duplicated because it's a single scrape.

To explain the alternative if we used a homeserver-specific registry, the metrics endpoint of each homeserver tenant would duplicate the per-process data. The only way to change this is if we have an option to turn off the process-level metrics in Synapse and then have our shard process collect those process-level metrics separately. (extra complication)

Something to think about outside of Synapse: In terms of associating the per-process data with the homeserver tenant, we need metrics to track which homeserver tenants belong to which shards (process) over time. And be able to visualize that in Grafana.

Making a choice on how to add the instance label

The refactor and boilerplate bulk amount will be the same for either pattern. Given the scrape advantage and slight metric flexibility edge, I'd say adding the instance label directly to the relevant metrics is the best option 👍

@erikjohnston Really appreciate your input to make an informed evaluation of the options! 😇

[MadLittleMods]

Per the discussion with @sandhose on #18601 (comment), we're moving to the server_name label because instance has special meaning.

👍 I think I agree that instance has special meaning according to the Prometheus docs and should be "The <host>:<port> part of the target's URL that was scraped." (source) (also: "In Prometheus terms, an endpoint you can scrape is called an instance, usually corresponding to a single process.") which is different from how we're using it here. And based on following the conventions, it makes sense to use a different label.

For reference, we currently don't follow this pattern with matrix.org (see the matrix.org Prometheus scrape config using metric_relabel_configs and labels config) and hard-code the instance label to matrix.org (probably not the correct thing to do) and differentiate workers with the job label for the type of worker (correct usage) and also some index labels if we have multiple of that worker type.

Example of current metrics on matrix.org

 

Notice they all use instance="matrix.org":

(source)

synapse_util_metrics_block_count_total{block_name="_calculate_state_and_extrem", environment="live", host="grindylow.matrix.org", identifier="matrix.org", index="1", instance="matrix.org", job="synapse_event_persister", service="synapse"}
synapse_util_metrics_block_count_total{block_name="_fetch_event_list", environment="live", host="doxy.matrix.org", identifier="matrix.org", index="52", instance="matrix.org", job="synapse_synchrotron", service="synapse"}
synapse_util_metrics_block_count_total{block_name="action_for_event_by_user", environment="live", host="grindylow.matrix.org", identifier="matrix.org", index="2", instance="matrix.org", job="synapse_event_creator_users", service="synapse"}

---

I wish Prometheus had some guidance on label names to use in these situations like OpenTelemetry has with their semantic conventions for spans and attributes. The only docs I can find are https://prometheus.io/docs/concepts/jobs_instances/ and https://prometheus.io/docs/practices/naming/ which don't mention anything beyond job and instance.

server_name sounds good to me 👍

-- @MadLittleMods, #18601 (comment)