Many prometheus metrics are duplicated

[richvdh]

A number of the reported Prometheus metrics are duplicated. This can be a problem for Prometheus instances configured to monitor a large number of Synapse instances.

Examples:

• we have two copies of each counter, one with and one without a _total suffix
• synapse_util_caches_cache:hits is duplicated as synapse_util_caches_cache_hits.

[reivilibre]

Which ones would we want to remove?

We use synapse_util_caches_cache:hits on the dashboard so I'd be in favour of removing synapse_util_caches_cache_hits (N.B. no hits in the dashboard JSON).

Equally, we don't use many _total-suffixed counters, so I'd be in favour of removing those.

Though I will bring to your attention that there are a few where we do:

rei@lithium ~/work/synapse/contrib $ rg _total grafana/synapse.json 
430:          "expr": "rate(process_cpu_seconds_total{instance=\"$instance\",job=~\"$job\",index=~\"$index\"}[$bucket_size])",
797:              "expr": "rate(process_cpu_system_seconds_total{instance=\"$instance\",job=~\"$job\",index=~\"$index\"}[$bucket_size])",
806:              "expr": "rate(process_cpu_user_seconds_total{instance=\"$instance\",job=~\"$job\",index=~\"$index\"}[$bucket_size])",
4668:              "expr": "sum(rate(synapse_federation_soft_failed_events_total{instance=\"$instance\"}[$bucket_size]))",
7436:              "expr": "rate(python_gc_unreachable_total{instance=\"$instance\",job=~\"$job\",index=~\"$index\"}[$bucket_size])/rate(python_gc_time_count{instance=\"$instance\",job=~\"$job\",index=~\"$index\"}[$bucket_size])",
8124:              "expr": "synapse_replication_tcp_resource_total_connections{job=~\"$job\",index=~\"$index\",instance=\"$instance\"}",

A good first step may be to correct our dashboard JSON in preparation for the change.

I also imagine this would want to be communicated as a breaking change.

Suggested steps as follows:

1. Fix dashboard JSON to not use _total at all
2. Remove *_total and synapse_util_caches_cache_* from the published metrics.

[richvdh]

Equally, we don't use many _total-suffixed counters, so I'd be in favour of removing those.

This is a bit complicated. Let me try and give some history:

There is a standard called OpenMetrics, which mandates that the names of counters end in _total (see https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#counter-1). Back in prometheus-client 0.4.0, the Prometheus maintainers changed prometheus-client to automatically add _total to the names of any counters that don't already have it, for compliance with OpenMetrics (see prometheus/client_python#300).

Of course, that broke everything for us (cf prometheus/client_python#317), and we worked around it by writing a custom exposition.py which basically forks a bunch of prometheus-client to expose both the _total and non-_total variants. Our intention was that we would get rid of the non-_total variants soon after, but here we are four years on.

Sooo... where does that leave us? There's no real reason we have to use the OpenMetrics-compliant names (the prometheus server seems happy either way), but it would still be quite nice to get rid of that custom exposition.py (and future versions of prometheus, or other OpenMetrics server implementations, might be less tolerant). So basically: I'd suggest switching to the _total variants if possible.

[richvdh]

We use synapse_util_caches_cache:hits on the dashboard so I'd be in favour of removing synapse_util_caches_cache_hits (N.B. no hits in the dashboard JSON).

According to https://prometheus.io/docs/concepts/data_model/#metric-names-and-labels, we're not supposed to use colons:

Note: The colons are reserved for user defined recording rules. They should not be used by exporters or direct instrumentation.

[reivilibre]

New steps:

• allow disabling legacy metrics via a temporary config option (this will also make it possible to update Grafana dashboards and test them before cutting these metrics out entirely) (Add experimental configuration option to allow disabling legacy Prometheus metric names. #13540)
  • non-_total counters and any metrics with : in the name
• fix cache metrics: they currently depend on being told to collect themselves by the legacy exposition code. This means they don't work if legacy metrics are disabled. (Fix cache metrics not being updated when not using the legacy exposition module. #13717)
• update Prometheus recording rules that are in the repo (Fix Prometheus recording rules to not use legacy metric names. #13718)
• update Grafana dashboard not to use the legacy metrics. (Update Grafana dashboard to not use legacy metric names. #13714)
• deploy on m.org
• deploy on EMS
• eventually, deprecate use of legacy metrics, issuing a warning in the upgrade notes (only after the Grafana dashboard has been updated) — Announce that legacy metric names are deprecated, will be turned off by default in Synapse v1.71.0 and removed altogether in Synapse v1.73.0. #14024
• switch off legacy metrics by default
• very eventually, remove legacy metrics altogether

Timeline

• v1.68.0 enable_legacy_metrics: false available
• v1.71.0 Legacy metrics disabled by default
• v1.73.0 Legacy metrics removed altogether

[reivilibre]

Finished up in #14358 ... as far as I'm aware!