From 4df927bd48548db66ab160497669533bf19ad037 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 23 Jul 2025 14:49:17 -0500 Subject: [PATCH 01/27] Add in base linting for metrics From https://github.com/element-hq/synapse/pull/18656 --- scripts-dev/mypy_synapse_plugin.py | 107 ++++++++++++++++++++++++++++- 1 file changed, 105 insertions(+), 2 deletions(-) diff --git a/scripts-dev/mypy_synapse_plugin.py b/scripts-dev/mypy_synapse_plugin.py index a15c3c005cf..3657ccadbf4 100644 --- a/scripts-dev/mypy_synapse_plugin.py +++ b/scripts-dev/mypy_synapse_plugin.py @@ -28,8 +28,13 @@ import mypy.types from mypy.erasetype import remove_instance_last_known_values from mypy.errorcodes import ErrorCode -from mypy.nodes import ARG_NAMED_OPT, TempNode, Var -from mypy.plugin import FunctionSigContext, MethodSigContext, Plugin +from mypy.nodes import ARG_NAMED_OPT, ListExpr, NameExpr, TempNode, Var +from mypy.plugin import ( + FunctionLike, + FunctionSigContext, + MethodSigContext, + Plugin, +) from mypy.typeops import bind_self from mypy.types import ( AnyType, @@ -43,11 +48,30 @@ UnionType, ) +PROMETHEUS_METRIC_MISSING_SERVER_NAME_LABEL = ErrorCode( + "missing-server-name-label", + "`SERVER_NAME_LABEL` required in metric", + category="per-homeserver-tenant-metrics", +) + class SynapsePlugin(Plugin): + def get_function_signature_hook( + self, fullname: str + ) -> Optional[Callable[[FunctionSigContext], FunctionLike]]: + if fullname in ( + "prometheus_client.metrics.Gauge", + # TODO: Add other prometheus_client metrics that need checking as we + # refactor, see https://github.com/element-hq/synapse/issues/18592 + ): + return check_prometheus_metric_instantiation + + return None + def get_method_signature_hook( self, fullname: str ) -> Optional[Callable[[MethodSigContext], CallableType]]: + # print(f"m fullname={fullname}") if fullname.startswith( ( "synapse.util.caches.descriptors.CachedFunction.__call__", @@ -65,6 +89,85 @@ def get_method_signature_hook( return None +def check_prometheus_metric_instantiation(ctx: FunctionSigContext) -> CallableType: + """ + Ensure that the `prometheus_client` metrics include the `SERVER_NAME_LABEL` label + when instantiated. + + This is important because we support multiple Synapse instances running in the same + process, where all metrics share a single global `REGISTRY`. The `server_name` label + ensures metrics are correctly separated by homeserver. + + There are also some metrics that apply at the process level, such as CPU usage, + Python garbage collection, Twisted reactor tick time which shouldn't have the + `SERVER_NAME_LABEL`. In those cases, use use a type ignore comment to disable the + check, e.g. `# type: ignore[missing-server-name-label]`. + """ + # The true signature, this isn't being modified so this is what will be returned. + signature: CallableType = ctx.default_signature + + # Sanity check the arguments are still as expected in this version of + # `prometheus_client`. ex. `Counter(name, documentation, labelnames, ...)` + # + # `signature.arg_names` should be: ["name", "documentation", "labelnames", ...] + if len(signature.arg_names) < 3 or signature.arg_names[2] != "labelnames": + ctx.api.fail( + f"Expected the 3rd argument of {signature.name} to be 'labelnames', but got " + f"{signature.arg_names[2]}", + ctx.context, + ) + return signature + + # Ensure mypy is passing the correct number of arguments because we are doing some + # dirty indexing into `ctx.args` later on. + assert len(ctx.args) == len(signature.arg_names), ( + f"Expected the list of arguments in the {signature.name} signature ({len(signature.arg_names)})" + f"to match the number of arguments from the function signature context ({len(ctx.args)})" + ) + + # Check if the `labelnames` argument includes `SERVER_NAME_LABEL` + # + # `ctx.args` should look like this: + # ``` + # [ + # [StrExpr("name")], + # [StrExpr("documentation")], + # [ListExpr([StrExpr("label1"), StrExpr("label2")])] + # ... + # ] + # ``` + labelnames_arg_expression = ctx.args[2][0] if len(ctx.args[2]) > 0 else None + if isinstance(labelnames_arg_expression, ListExpr): + # Check if the `labelnames` argument includes the `server_name` label (`SERVER_NAME_LABEL`). + for labelname_expression in labelnames_arg_expression.items: + if ( + isinstance(labelname_expression, NameExpr) + and labelname_expression.fullname == "synapse.metrics.SERVER_NAME_LABEL" + ): + # Found the `SERVER_NAME_LABEL`, all good! + break + else: + ctx.api.fail( + f"Expected {signature.name} to include `SERVER_NAME_LABEL` in the list of labels. " + "If this is a process-level metric (vs homeserver-level), use a type ignore comment " + "to disable this check.", + ctx.context, + code=PROMETHEUS_METRIC_MISSING_SERVER_NAME_LABEL, + ) + else: + ctx.api.fail( + f"Expected the `labelnames` argument of {signature.name} to be a list of label names " + f"(including `SERVER_NAME_LABEL`), but got {labelnames_arg_expression}. " + "If this is a process-level metric (vs homeserver-level), use a type ignore comment " + "to disable this check.", + ctx.context, + code=PROMETHEUS_METRIC_MISSING_SERVER_NAME_LABEL, + ) + return signature + + return signature + + def _get_true_return_type(signature: CallableType) -> mypy.types.Type: """ Get the "final" return type of a callable which might return an Awaitable/Deferred. From cec25ddc3478c2e25ed59fc9f8db4a60535fbea1 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 23 Jul 2025 15:24:25 -0500 Subject: [PATCH 02/27] Support `labelnames` argument being a Tuple expression ``` synapse/metrics/__init__.py:522: error: Expected the `labelnames` argument of Histogram to be a list of label names (including `SERVER_NAME_LABEL`), but got TupleExpr:528( StrExpr(type) StrExpr(reason) NameExpr(SERVER_NAME_LABEL [synapse.metrics.SERVER_NAME_LABEL])). If this is a process-level metric (vs homeserver-level), use a type ignore comment to disable this check. [missing-server-name-label] ``` --- scripts-dev/mypy_synapse_plugin.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts-dev/mypy_synapse_plugin.py b/scripts-dev/mypy_synapse_plugin.py index 3657ccadbf4..fbb46c9f87e 100644 --- a/scripts-dev/mypy_synapse_plugin.py +++ b/scripts-dev/mypy_synapse_plugin.py @@ -28,7 +28,7 @@ import mypy.types from mypy.erasetype import remove_instance_last_known_values from mypy.errorcodes import ErrorCode -from mypy.nodes import ARG_NAMED_OPT, ListExpr, NameExpr, TempNode, Var +from mypy.nodes import ARG_NAMED_OPT, ListExpr, NameExpr, TempNode, TupleExpr, Var from mypy.plugin import ( FunctionLike, FunctionSigContext, @@ -137,7 +137,7 @@ def check_prometheus_metric_instantiation(ctx: FunctionSigContext) -> CallableTy # ] # ``` labelnames_arg_expression = ctx.args[2][0] if len(ctx.args[2]) > 0 else None - if isinstance(labelnames_arg_expression, ListExpr): + if isinstance(labelnames_arg_expression, (ListExpr, TupleExpr)): # Check if the `labelnames` argument includes the `server_name` label (`SERVER_NAME_LABEL`). for labelname_expression in labelnames_arg_expression.items: if ( From d62d564fec3e6753e3d3e21e64074bdb35c9952a Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 24 Jul 2025 15:07:21 -0500 Subject: [PATCH 03/27] Make label name arg position/name generic --- scripts-dev/mypy_synapse_plugin.py | 64 ++++++++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 4 deletions(-) diff --git a/scripts-dev/mypy_synapse_plugin.py b/scripts-dev/mypy_synapse_plugin.py index fbb46c9f87e..d4775867102 100644 --- a/scripts-dev/mypy_synapse_plugin.py +++ b/scripts-dev/mypy_synapse_plugin.py @@ -25,11 +25,13 @@ from typing import Callable, Optional, Tuple, Type, Union +import attr import mypy.types from mypy.erasetype import remove_instance_last_known_values from mypy.errorcodes import ErrorCode from mypy.nodes import ARG_NAMED_OPT, ListExpr, NameExpr, TempNode, TupleExpr, Var from mypy.plugin import ( + ClassDefContext, FunctionLike, FunctionSigContext, MethodSigContext, @@ -55,10 +57,48 @@ ) +@attr.s(auto_attribs=True) +class ArgLocation: + arg_name: str + arg_position: int + + +# Ideally, we'd be able to cross-check this list in the lint itself by checking for +# anything that inherits from `MetricWrapperBase` or `Metric` but I don't know of a good +# mypy hook to inspect this. +prometheus_metric_fullname_to_label_arg_map = { + "prometheus_client.metrics.MetricWrapperBase": ArgLocation("labelnames", 2), + "prometheus_client.metrics.Counter": ArgLocation("labelnames", 2), + "prometheus_client.metrics.Histogram": ArgLocation("labelnames", 2), + "prometheus_client.metrics.Gauge": ArgLocation("labelnames", 2), + "prometheus_client.metrics.Summary": ArgLocation("labelnames", 2), + "prometheus_client.metrics.Info": ArgLocation("labelnames", 2), + "prometheus_client.metrics.Enum": ArgLocation("labelnames", 2), + "prometheus_client.metrics_core.UnknownMetricFamily": ArgLocation("labels", 3), + "prometheus_client.metrics_core.CounterMetricFamily": ArgLocation("labels", 3), + "prometheus_client.metrics_core.GaugeMetricFamily": ArgLocation("labels", 3), + "prometheus_client.metrics_core.SummaryMetricFamily": ArgLocation("labels", 3), + "prometheus_client.metrics_core.InfoMetricFamily": ArgLocation("labels", 3), + "prometheus_client.metrics_core.HistogramMetricFamily": ArgLocation("labels", 3), + "prometheus_client.metrics_core.GaugeHistogramMetricFamily": ArgLocation( + "labels", 3 + ), + "prometheus_client.metrics_core.StateSetMetricFamily": ArgLocation("labels", 3), + # TODO: "synapse.metrics.GaugeHistogramMetricFamilyWithLabels": ArgLocation("labelnames", 4), +} +""" +Map from the fullname of the Prometheus `Metric`/`Collector` classes to the keyword +argument name and positional index of the label names. +""" + + class SynapsePlugin(Plugin): def get_function_signature_hook( self, fullname: str ) -> Optional[Callable[[FunctionSigContext], FunctionLike]]: + # TODO: Use `prometheus_metric_fullname_to_label_arg_map.keys()` once we've + # updated all of the metrics, see + # https://github.com/element-hq/synapse/issues/18592 if fullname in ( "prometheus_client.metrics.Gauge", # TODO: Add other prometheus_client metrics that need checking as we @@ -104,16 +144,28 @@ def check_prometheus_metric_instantiation(ctx: FunctionSigContext) -> CallableTy check, e.g. `# type: ignore[missing-server-name-label]`. """ # The true signature, this isn't being modified so this is what will be returned. - signature: CallableType = ctx.default_signature + signature = ctx.default_signature + + # For whatever reason, the `fullname` here is different from the `fullname` we see + # in `get_function_signature_hook(...)` so let's get our familiar version. + fullname = signature.definition.fullname.removesuffix(".__init__") + arg_location = prometheus_metric_fullname_to_label_arg_map.get(fullname) + assert arg_location is not None, ( + f"Expected to find {fullname} in `prometheus_metric_fullname_to_label_arg_map`, " + "but it was not found. This is a problem with our custom mypy plugin. Please add it to the map." + ) # Sanity check the arguments are still as expected in this version of # `prometheus_client`. ex. `Counter(name, documentation, labelnames, ...)` # # `signature.arg_names` should be: ["name", "documentation", "labelnames", ...] - if len(signature.arg_names) < 3 or signature.arg_names[2] != "labelnames": + if ( + len(signature.arg_names) < (arg_location.arg_position + 1) + or signature.arg_names[arg_location.arg_position] != arg_location.arg_name + ): ctx.api.fail( f"Expected the 3rd argument of {signature.name} to be 'labelnames', but got " - f"{signature.arg_names[2]}", + f"{signature.arg_names[arg_location.arg_position]}", ctx.context, ) return signature @@ -136,7 +188,11 @@ def check_prometheus_metric_instantiation(ctx: FunctionSigContext) -> CallableTy # ... # ] # ``` - labelnames_arg_expression = ctx.args[2][0] if len(ctx.args[2]) > 0 else None + labelnames_arg_expression = ( + ctx.args[arg_location.arg_position][0] + if len(ctx.args[arg_location.arg_position]) > 0 + else None + ) if isinstance(labelnames_arg_expression, (ListExpr, TupleExpr)): # Check if the `labelnames` argument includes the `server_name` label (`SERVER_NAME_LABEL`). for labelname_expression in labelnames_arg_expression.items: From 325caec886574ea66d977b18e946ea4d8914c3eb Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 24 Jul 2025 15:24:26 -0500 Subject: [PATCH 04/27] Update wording --- scripts-dev/mypy_synapse_plugin.py | 35 ++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/scripts-dev/mypy_synapse_plugin.py b/scripts-dev/mypy_synapse_plugin.py index d4775867102..8ece2827d91 100644 --- a/scripts-dev/mypy_synapse_plugin.py +++ b/scripts-dev/mypy_synapse_plugin.py @@ -59,13 +59,19 @@ @attr.s(auto_attribs=True) class ArgLocation: - arg_name: str - arg_position: int + keyword_name: str + """ + The keyword argument name for this argument + """ + position: int + """ + The positional index of this argument + """ -# Ideally, we'd be able to cross-check this list in the lint itself by checking for -# anything that inherits from `MetricWrapperBase` or `Metric` but I don't know of a good -# mypy hook to inspect this. +# FIXME: Ideally, we'd be able to cross-check this list to make sure it includes +# everything as part of the lints by checking for anything that inherits from +# `MetricWrapperBase` or `Metric` but I don't know of a good mypy hook to inspect this. prometheus_metric_fullname_to_label_arg_map = { "prometheus_client.metrics.MetricWrapperBase": ArgLocation("labelnames", 2), "prometheus_client.metrics.Counter": ArgLocation("labelnames", 2), @@ -89,6 +95,9 @@ class ArgLocation: """ Map from the fullname of the Prometheus `Metric`/`Collector` classes to the keyword argument name and positional index of the label names. + +This is useful because different metrics have different signatures for passing in label +names and we just need to know where to look. """ @@ -100,7 +109,8 @@ def get_function_signature_hook( # updated all of the metrics, see # https://github.com/element-hq/synapse/issues/18592 if fullname in ( - "prometheus_client.metrics.Gauge", + # "prometheus_client.metrics.Gauge", + "prometheus_client.metrics_core.GaugeMetricFamily", # TODO: Add other prometheus_client metrics that need checking as we # refactor, see https://github.com/element-hq/synapse/issues/18592 ): @@ -152,7 +162,8 @@ def check_prometheus_metric_instantiation(ctx: FunctionSigContext) -> CallableTy arg_location = prometheus_metric_fullname_to_label_arg_map.get(fullname) assert arg_location is not None, ( f"Expected to find {fullname} in `prometheus_metric_fullname_to_label_arg_map`, " - "but it was not found. This is a problem with our custom mypy plugin. Please add it to the map." + f"but it was not found. This is a problem with our custom mypy plugin. " + f"Please add it to the map. Context: {ctx.context}" ) # Sanity check the arguments are still as expected in this version of @@ -160,12 +171,12 @@ def check_prometheus_metric_instantiation(ctx: FunctionSigContext) -> CallableTy # # `signature.arg_names` should be: ["name", "documentation", "labelnames", ...] if ( - len(signature.arg_names) < (arg_location.arg_position + 1) - or signature.arg_names[arg_location.arg_position] != arg_location.arg_name + len(signature.arg_names) < (arg_location.position + 1) + or signature.arg_names[arg_location.position] != arg_location.keyword_name ): ctx.api.fail( f"Expected the 3rd argument of {signature.name} to be 'labelnames', but got " - f"{signature.arg_names[arg_location.arg_position]}", + f"{signature.arg_names[arg_location.position]}", ctx.context, ) return signature @@ -189,8 +200,8 @@ def check_prometheus_metric_instantiation(ctx: FunctionSigContext) -> CallableTy # ] # ``` labelnames_arg_expression = ( - ctx.args[arg_location.arg_position][0] - if len(ctx.args[arg_location.arg_position]) > 0 + ctx.args[arg_location.position][0] + if len(ctx.args[arg_location.position]) > 0 else None ) if isinstance(labelnames_arg_expression, (ListExpr, TupleExpr)): From 53bfbc69cb4e410041f18a649c9ea88401310d0e Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 24 Jul 2025 15:34:22 -0500 Subject: [PATCH 05/27] Explain why `prometheus_client.metrics_core.Metric` isn't included here --- scripts-dev/mypy_synapse_plugin.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/scripts-dev/mypy_synapse_plugin.py b/scripts-dev/mypy_synapse_plugin.py index 8ece2827d91..9ee7e8a2430 100644 --- a/scripts-dev/mypy_synapse_plugin.py +++ b/scripts-dev/mypy_synapse_plugin.py @@ -80,6 +80,11 @@ class ArgLocation: "prometheus_client.metrics.Summary": ArgLocation("labelnames", 2), "prometheus_client.metrics.Info": ArgLocation("labelnames", 2), "prometheus_client.metrics.Enum": ArgLocation("labelnames", 2), + # We don't include `prometheus_client.metrics_core.Metric` here because it "is + # intended only for internal use by the [Prometheus] instrumentation client" and + # custom collectors should use the metric families listed below instead. + # Additionally, it doesn't have a `labelnames` argument. + # "prometheus_client.metrics_core.Metric" "prometheus_client.metrics_core.UnknownMetricFamily": ArgLocation("labels", 3), "prometheus_client.metrics_core.CounterMetricFamily": ArgLocation("labels", 3), "prometheus_client.metrics_core.GaugeMetricFamily": ArgLocation("labels", 3), @@ -90,6 +95,7 @@ class ArgLocation: "labels", 3 ), "prometheus_client.metrics_core.StateSetMetricFamily": ArgLocation("labels", 3), + # Our custom metrics: # TODO: "synapse.metrics.GaugeHistogramMetricFamilyWithLabels": ArgLocation("labelnames", 4), } """ From 395b33b1fd203315badbe0913d1844002b082084 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 24 Jul 2025 15:34:54 -0500 Subject: [PATCH 06/27] Disambiguate --- scripts-dev/mypy_synapse_plugin.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/scripts-dev/mypy_synapse_plugin.py b/scripts-dev/mypy_synapse_plugin.py index 9ee7e8a2430..4e7ef32c9f1 100644 --- a/scripts-dev/mypy_synapse_plugin.py +++ b/scripts-dev/mypy_synapse_plugin.py @@ -71,7 +71,9 @@ class ArgLocation: # FIXME: Ideally, we'd be able to cross-check this list to make sure it includes # everything as part of the lints by checking for anything that inherits from -# `MetricWrapperBase` or `Metric` but I don't know of a good mypy hook to inspect this. +# `prometheus_client.metrics.MetricWrapperBase` or +# `prometheus_client.metrics_core.Metric` but I don't know of a good mypy hook to +# inspect this. prometheus_metric_fullname_to_label_arg_map = { "prometheus_client.metrics.MetricWrapperBase": ArgLocation("labelnames", 2), "prometheus_client.metrics.Counter": ArgLocation("labelnames", 2), From 7818657378719287f6c0006d569615dcc96b40db Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 24 Jul 2025 15:35:09 -0500 Subject: [PATCH 07/27] Remove debug log --- scripts-dev/mypy_synapse_plugin.py | 1 - 1 file changed, 1 deletion(-) diff --git a/scripts-dev/mypy_synapse_plugin.py b/scripts-dev/mypy_synapse_plugin.py index 4e7ef32c9f1..2886d52109e 100644 --- a/scripts-dev/mypy_synapse_plugin.py +++ b/scripts-dev/mypy_synapse_plugin.py @@ -129,7 +129,6 @@ def get_function_signature_hook( def get_method_signature_hook( self, fullname: str ) -> Optional[Callable[[MethodSigContext], CallableType]]: - # print(f"m fullname={fullname}") if fullname.startswith( ( "synapse.util.caches.descriptors.CachedFunction.__call__", From 6bb5fefc0f98b25f60cba249f6354fc95286e487 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 24 Jul 2025 17:11:26 -0500 Subject: [PATCH 08/27] Fix lints in in lint --- scripts-dev/mypy_synapse_plugin.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/scripts-dev/mypy_synapse_plugin.py b/scripts-dev/mypy_synapse_plugin.py index 2886d52109e..75b386c849f 100644 --- a/scripts-dev/mypy_synapse_plugin.py +++ b/scripts-dev/mypy_synapse_plugin.py @@ -162,10 +162,15 @@ def check_prometheus_metric_instantiation(ctx: FunctionSigContext) -> CallableTy """ # The true signature, this isn't being modified so this is what will be returned. signature = ctx.default_signature + definition = signature.definition + assert definition is not None, ( + f"Expected the signature definition to be set for anything passed to " + f"`check_prometheus_metric_instantiation`, but it was None. Context: {ctx.context}" + ) # For whatever reason, the `fullname` here is different from the `fullname` we see # in `get_function_signature_hook(...)` so let's get our familiar version. - fullname = signature.definition.fullname.removesuffix(".__init__") + fullname = definition.fullname.removesuffix(".__init__") arg_location = prometheus_metric_fullname_to_label_arg_map.get(fullname) assert arg_location is not None, ( f"Expected to find {fullname} in `prometheus_metric_fullname_to_label_arg_map`, " From 67587427cfa4379b27d9af214d2ce0f69ad319cb Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 28 Jul 2025 17:07:57 -0500 Subject: [PATCH 09/27] Add cross-check for `prometheus_metric_fullname_to_label_arg_map` --- scripts-dev/mypy_synapse_plugin.py | 102 +++++++++++++++++++++++------ 1 file changed, 81 insertions(+), 21 deletions(-) diff --git a/scripts-dev/mypy_synapse_plugin.py b/scripts-dev/mypy_synapse_plugin.py index be03210d241..d1654b67b99 100644 --- a/scripts-dev/mypy_synapse_plugin.py +++ b/scripts-dev/mypy_synapse_plugin.py @@ -23,7 +23,8 @@ can crop up, e.g the cache descriptors. """ -from typing import Callable, Optional, Tuple, Type, Union +import enum +from typing import Callable, Optional, Tuple, Type, Union, Mapping import attr import mypy.types @@ -31,6 +32,7 @@ from mypy.errorcodes import ErrorCode from mypy.nodes import ARG_NAMED_OPT, ListExpr, NameExpr, TempNode, TupleExpr, Var from mypy.plugin import ( + ClassDefContext, FunctionLike, FunctionSigContext, MethodSigContext, @@ -56,6 +58,12 @@ ) +class Sentinel(enum.Enum): + # defining a sentinel in this way allows mypy to correctly handle the + # type of a dictionary lookup and subsequent type narrowing. + UNSET_SENTINEL = object() + + @attr.s(auto_attribs=True) class ArgLocation: keyword_name: str @@ -68,12 +76,11 @@ class ArgLocation: """ -# FIXME: Ideally, we'd be able to cross-check this list to make sure it includes -# everything as part of the lints by checking for anything that inherits from -# `prometheus_client.metrics.MetricWrapperBase` or -# `prometheus_client.metrics_core.Metric` but I don't know of a good mypy hook to -# inspect this. -prometheus_metric_fullname_to_label_arg_map = { +# This should include anything that inherits from `prometheus_client.registry.Collector` +# (`synapse.metrics._types.Collector`) or `prometheus_client.metrics_core.Metric`. This +# is enforced by `analyze_prometheus_metric_classes`. +prometheus_metric_fullname_to_label_arg_map: Mapping[str, Optional[ArgLocation]] = { + # `Collector` subclasses: "prometheus_client.metrics.MetricWrapperBase": ArgLocation("labelnames", 2), "prometheus_client.metrics.Counter": ArgLocation("labelnames", 2), "prometheus_client.metrics.Histogram": ArgLocation("labelnames", 2), @@ -81,21 +88,39 @@ class ArgLocation: "prometheus_client.metrics.Summary": ArgLocation("labelnames", 2), "prometheus_client.metrics.Info": ArgLocation("labelnames", 2), "prometheus_client.metrics.Enum": ArgLocation("labelnames", 2), + "synapse.metrics.LaterGauge": ArgLocation("labelnames", 2), + "synapse.metrics.InFlightGauge": ArgLocation("labels", 2), + "synapse.metrics.GaugeBucketCollector": ArgLocation("labelnames", 2), + # `Collector` subclasses (that we ignore): These should always fail because they + # don't have a `labelnames` argument, but we include them here so that they fail the + # lint and people need to manually allow via a type ignore comment. + "prometheus_client.registry.Collector": None, + "prometheus_client.registry._EmptyCollector": None, + "prometheus_client.registry.CollectorRegistry": None, + "prometheus_client.process_collector.ProcessCollector": None, + "prometheus_client.platform_collector.PlatformCollector": None, + "prometheus_client.gc_collector.GCCollector": None, + "synapse.metrics._gc.GCCounts": None, + "synapse.metrics._gc.PyPyGCStats": None, + "synapse.metrics._reactor_metrics.ReactorLastSeenMetric": None, + "synapse.metrics.CPUMetrics": None, + "synapse.util.metrics.DynamicCollectorRegistry": None, + "synapse.metrics.background_process_metrics._Collector": None, # We don't include `prometheus_client.metrics_core.Metric` here because it "is # intended only for internal use by the [Prometheus] instrumentation client" and # custom collectors should use the metric families listed below instead. # Additionally, it doesn't have a `labelnames` argument. # "prometheus_client.metrics_core.Metric" - "prometheus_client.metrics_core.UnknownMetricFamily": ArgLocation("labels", 3), - "prometheus_client.metrics_core.CounterMetricFamily": ArgLocation("labels", 3), - "prometheus_client.metrics_core.GaugeMetricFamily": ArgLocation("labels", 3), - "prometheus_client.metrics_core.SummaryMetricFamily": ArgLocation("labels", 3), - "prometheus_client.metrics_core.InfoMetricFamily": ArgLocation("labels", 3), - "prometheus_client.metrics_core.HistogramMetricFamily": ArgLocation("labels", 3), - "prometheus_client.metrics_core.GaugeHistogramMetricFamily": ArgLocation( - "labels", 3 - ), - "prometheus_client.metrics_core.StateSetMetricFamily": ArgLocation("labels", 3), + # "prometheus_client.metrics_core.UnknownMetricFamily": ArgLocation("labels", 3), + # "prometheus_client.metrics_core.CounterMetricFamily": ArgLocation("labels", 3), + # "prometheus_client.metrics_core.GaugeMetricFamily": ArgLocation("labels", 3), + # "prometheus_client.metrics_core.SummaryMetricFamily": ArgLocation("labels", 3), + # "prometheus_client.metrics_core.InfoMetricFamily": ArgLocation("labels", 3), + # "prometheus_client.metrics_core.HistogramMetricFamily": ArgLocation("labels", 3), + # "prometheus_client.metrics_core.GaugeHistogramMetricFamily": ArgLocation( + # "labels", 3 + # ), + # "prometheus_client.metrics_core.StateSetMetricFamily": ArgLocation("labels", 3), # Our custom metrics: # TODO: "synapse.metrics.GaugeHistogramMetricFamilyWithLabels": ArgLocation("labelnames", 4), } @@ -109,6 +134,11 @@ class ArgLocation: class SynapsePlugin(Plugin): + def get_base_class_hook( + self, fullname: str + ) -> Optional[Callable[[ClassDefContext], None]]: + return analyze_prometheus_metric_classes + def get_function_signature_hook( self, fullname: str ) -> Optional[Callable[[FunctionSigContext], FunctionLike]]: @@ -117,7 +147,7 @@ def get_function_signature_hook( # https://github.com/element-hq/synapse/issues/18592 if fullname in ( "prometheus_client.metrics.Counter", - "prometheus_client.metrics_core.GaugeMetricFamily", + # "prometheus_client.metrics_core.GaugeMetricFamily", # TODO: Add other prometheus_client metrics that need checking as we # refactor, see https://github.com/element-hq/synapse/issues/18592 ): @@ -145,6 +175,31 @@ def get_method_signature_hook( return None +def analyze_prometheus_metric_classes(ctx: ClassDefContext) -> None: + """ + Cross-check the list of Prometheus metric classes against the + `prometheus_metric_fullname_to_label_arg_map` to ensure the list is exhaustive and + up-to-date. + """ + + # TODO: Consider `prometheus_client.metrics_core.Metric` + + if any( + ancestor_type.fullname + in ( + # All of the Prometheus metric classes inherit from the `Collector`. + "prometheus_client.registry.Collector", + "synapse.metrics._types.Collector", + ) + for ancestor_type in ctx.cls.info.mro + ): + assert ctx.cls.fullname in prometheus_metric_fullname_to_label_arg_map, ( + f"Expected {ctx.cls.fullname} to be in `prometheus_metric_fullname_to_label_arg_map`, " + f"but it was not found. This is a problem with our custom mypy plugin. " + f"Please add it to the map." + ) + + def check_prometheus_metric_instantiation(ctx: FunctionSigContext) -> CallableType: """ Ensure that the `prometheus_client` metrics include the `SERVER_NAME_LABEL` label @@ -170,12 +225,17 @@ def check_prometheus_metric_instantiation(ctx: FunctionSigContext) -> CallableTy # For whatever reason, the `fullname` here is different from the `fullname` we see # in `get_function_signature_hook(...)` so let's get our familiar version. fullname = definition.fullname.removesuffix(".__init__") - arg_location = prometheus_metric_fullname_to_label_arg_map.get(fullname) - assert arg_location is not None, ( + arg_location = prometheus_metric_fullname_to_label_arg_map.get( + fullname, Sentinel.UNSET_SENTINEL + ) + assert arg_location is not Sentinel.UNSET_SENTINEL, ( f"Expected to find {fullname} in `prometheus_metric_fullname_to_label_arg_map`, " f"but it was not found. This is a problem with our custom mypy plugin. " f"Please add it to the map. Context: {ctx.context}" ) + if arg_location is None: + # Ignore this metric as it doesn't have a `labelnames` argument. + return signature # Sanity check the arguments are still as expected in this version of # `prometheus_client`. ex. `Counter(name, documentation, labelnames, ...)` @@ -186,7 +246,7 @@ def check_prometheus_metric_instantiation(ctx: FunctionSigContext) -> CallableTy or signature.arg_names[arg_location.position] != arg_location.keyword_name ): ctx.api.fail( - f"Expected the 3rd argument of {signature.name} to be 'labelnames', but got " + f"Expected the 3rd argument of {signature.name} to be `labelnames`/`labels`, but got " f"{signature.arg_names[arg_location.position]}", ctx.context, ) From c53d34da5da1f9b9be43b9367a974c67c0ba8e51 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 28 Jul 2025 17:51:03 -0500 Subject: [PATCH 10/27] Ignore some process-level metrics --- scripts-dev/mypy_synapse_plugin.py | 62 ++++++++++++++++++++---------- synapse/metrics/_gc.py | 3 +- 2 files changed, 44 insertions(+), 21 deletions(-) diff --git a/scripts-dev/mypy_synapse_plugin.py b/scripts-dev/mypy_synapse_plugin.py index d1654b67b99..c204282037c 100644 --- a/scripts-dev/mypy_synapse_plugin.py +++ b/scripts-dev/mypy_synapse_plugin.py @@ -24,7 +24,7 @@ """ import enum -from typing import Callable, Optional, Tuple, Type, Union, Mapping +from typing import Callable, Mapping, Optional, Tuple, Type, Union import attr import mypy.types @@ -72,7 +72,7 @@ class ArgLocation: """ position: int """ - The positional index of this argument + The 0-based positional index of this argument """ @@ -91,9 +91,9 @@ class ArgLocation: "synapse.metrics.LaterGauge": ArgLocation("labelnames", 2), "synapse.metrics.InFlightGauge": ArgLocation("labels", 2), "synapse.metrics.GaugeBucketCollector": ArgLocation("labelnames", 2), - # `Collector` subclasses (that we ignore): These should always fail because they - # don't have a `labelnames` argument, but we include them here so that they fail the - # lint and people need to manually allow via a type ignore comment. + # These should always fail because they don't have a `labelnames` argument, but we + # include them here so that they fail the lint and people need to manually allow via + # a type ignore comment as the source of truth should be in the source code. "prometheus_client.registry.Collector": None, "prometheus_client.registry._EmptyCollector": None, "prometheus_client.registry.CollectorRegistry": None, @@ -104,6 +104,7 @@ class ArgLocation: "synapse.metrics._gc.PyPyGCStats": None, "synapse.metrics._reactor_metrics.ReactorLastSeenMetric": None, "synapse.metrics.CPUMetrics": None, + "synapse.metrics.jemalloc.JemallocCollector": None, "synapse.util.metrics.DynamicCollectorRegistry": None, "synapse.metrics.background_process_metrics._Collector": None, # We don't include `prometheus_client.metrics_core.Metric` here because it "is @@ -134,6 +135,9 @@ class ArgLocation: class SynapsePlugin(Plugin): + # Unfortunately, because mypy only chooses the first plugin that returns a non-None + # value, this check doesn't get run during our normal mypy lint process because + # `mypy_zope` also uses the `get_base_class_hook` method. def get_base_class_hook( self, fullname: str ) -> Optional[Callable[[ClassDefContext], None]]: @@ -147,11 +151,12 @@ def get_function_signature_hook( # https://github.com/element-hq/synapse/issues/18592 if fullname in ( "prometheus_client.metrics.Counter", + "synapse.metrics._gc.PyPyGCStats", # "prometheus_client.metrics_core.GaugeMetricFamily", # TODO: Add other prometheus_client metrics that need checking as we # refactor, see https://github.com/element-hq/synapse/issues/18592 ): - return check_prometheus_metric_instantiation + return lambda ctx: check_prometheus_metric_instantiation(ctx, fullname) return None @@ -182,6 +187,13 @@ def analyze_prometheus_metric_classes(ctx: ClassDefContext) -> None: up-to-date. """ + fullname = ctx.cls.fullname + # Strip off the unique identifier for classes that are dynamically created inside + # functions. ex. `synapse.metrics.jemalloc.JemallocCollector@185` (this is the line + # number) + if "@" in fullname: + fullname = fullname.split("@", 1)[0] + # TODO: Consider `prometheus_client.metrics_core.Metric` if any( @@ -193,14 +205,16 @@ def analyze_prometheus_metric_classes(ctx: ClassDefContext) -> None: ) for ancestor_type in ctx.cls.info.mro ): - assert ctx.cls.fullname in prometheus_metric_fullname_to_label_arg_map, ( - f"Expected {ctx.cls.fullname} to be in `prometheus_metric_fullname_to_label_arg_map`, " + assert fullname in prometheus_metric_fullname_to_label_arg_map, ( + f"Expected {fullname} to be in `prometheus_metric_fullname_to_label_arg_map`, " f"but it was not found. This is a problem with our custom mypy plugin. " f"Please add it to the map." ) -def check_prometheus_metric_instantiation(ctx: FunctionSigContext) -> CallableType: +def check_prometheus_metric_instantiation( + ctx: FunctionSigContext, fullname: str +) -> CallableType: """ Ensure that the `prometheus_client` metrics include the `SERVER_NAME_LABEL` label when instantiated. @@ -213,18 +227,16 @@ def check_prometheus_metric_instantiation(ctx: FunctionSigContext) -> CallableTy Python garbage collection, Twisted reactor tick time which shouldn't have the `SERVER_NAME_LABEL`. In those cases, use use a type ignore comment to disable the check, e.g. `# type: ignore[missing-server-name-label]`. + + Args: + ctx: The `FunctionSigContext` from mypy. + fullname: The fully qualified name of the function being called, + e.g. `"prometheus_client.metrics.Counter"` """ # The true signature, this isn't being modified so this is what will be returned. signature = ctx.default_signature - definition = signature.definition - assert definition is not None, ( - f"Expected the signature definition to be set for anything passed to " - f"`check_prometheus_metric_instantiation`, but it was None. Context: {ctx.context}" - ) - # For whatever reason, the `fullname` here is different from the `fullname` we see - # in `get_function_signature_hook(...)` so let's get our familiar version. - fullname = definition.fullname.removesuffix(".__init__") + # Find where the label names argument is in the function signature. arg_location = prometheus_metric_fullname_to_label_arg_map.get( fullname, Sentinel.UNSET_SENTINEL ) @@ -233,8 +245,18 @@ def check_prometheus_metric_instantiation(ctx: FunctionSigContext) -> CallableTy f"but it was not found. This is a problem with our custom mypy plugin. " f"Please add it to the map. Context: {ctx.context}" ) + # People should be using `# type: ignore[missing-server-name-label]` for + # process-level metrics that should not have the `SERVER_NAME_LABEL`. if arg_location is None: - # Ignore this metric as it doesn't have a `labelnames` argument. + ctx.api.fail( + f"{signature.name} does not have a `labelnames`/`labels` argument " + "(if this is untrue, update `prometheus_metric_fullname_to_label_arg_map` " + "in our custom mypy plugin) and should probably have a type ignore comment, " + "e.g. `# type: ignore[missing-server-name-label]`. The reason we don't " + "automatically ignore this is the source of truth should be in the source code.", + ctx.context, + code=PROMETHEUS_METRIC_MISSING_SERVER_NAME_LABEL, + ) return signature # Sanity check the arguments are still as expected in this version of @@ -246,8 +268,8 @@ def check_prometheus_metric_instantiation(ctx: FunctionSigContext) -> CallableTy or signature.arg_names[arg_location.position] != arg_location.keyword_name ): ctx.api.fail( - f"Expected the 3rd argument of {signature.name} to be `labelnames`/`labels`, but got " - f"{signature.arg_names[arg_location.position]}", + f"Expected argument number {arg_location.position + 1} of {signature.name} to be `labelnames`/`labels`, " + f"but got {signature.arg_names[arg_location.position]}", ctx.context, ) return signature diff --git a/synapse/metrics/_gc.py b/synapse/metrics/_gc.py index d16481a0f66..dd8a541648c 100644 --- a/synapse/metrics/_gc.py +++ b/synapse/metrics/_gc.py @@ -208,4 +208,5 @@ def collect(self) -> Iterable[Metric]: if running_on_pypy: - REGISTRY.register(PyPyGCStats()) + # This is a process-level metric, so it does not have the `SERVER_NAME_LABEL`. + REGISTRY.register(PyPyGCStats()) # type: ignore[missing-server-name-label] From 07e79fef637492641aa748b2ecc28ccddd42dc2c Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 28 Jul 2025 17:59:02 -0500 Subject: [PATCH 11/27] Ignore process-level metrics --- scripts-dev/mypy_synapse_plugin.py | 19 ++++++++++++++++--- synapse/metrics/__init__.py | 3 ++- synapse/metrics/_gc.py | 3 ++- synapse/metrics/_reactor_metrics.py | 3 ++- synapse/metrics/background_process_metrics.py | 4 +++- synapse/util/caches/__init__.py | 5 ++++- 6 files changed, 29 insertions(+), 8 deletions(-) diff --git a/scripts-dev/mypy_synapse_plugin.py b/scripts-dev/mypy_synapse_plugin.py index c204282037c..f9a59e47126 100644 --- a/scripts-dev/mypy_synapse_plugin.py +++ b/scripts-dev/mypy_synapse_plugin.py @@ -76,9 +76,6 @@ class ArgLocation: """ -# This should include anything that inherits from `prometheus_client.registry.Collector` -# (`synapse.metrics._types.Collector`) or `prometheus_client.metrics_core.Metric`. This -# is enforced by `analyze_prometheus_metric_classes`. prometheus_metric_fullname_to_label_arg_map: Mapping[str, Optional[ArgLocation]] = { # `Collector` subclasses: "prometheus_client.metrics.MetricWrapperBase": ArgLocation("labelnames", 2), @@ -129,6 +126,10 @@ class ArgLocation: Map from the fullname of the Prometheus `Metric`/`Collector` classes to the keyword argument name and positional index of the label names. +This should include anything that inherits from `prometheus_client.registry.Collector` +(`synapse.metrics._types.Collector`) or `prometheus_client.metrics_core.Metric`. This is +enforced by `analyze_prometheus_metric_classes`. + This is useful because different metrics have different signatures for passing in label names and we just need to know where to look. """ @@ -151,7 +152,19 @@ def get_function_signature_hook( # https://github.com/element-hq/synapse/issues/18592 if fullname in ( "prometheus_client.metrics.Counter", + "prometheus_client.registry.Collector", + "prometheus_client.registry._EmptyCollector", + "prometheus_client.registry.CollectorRegistry", + "prometheus_client.process_collector.ProcessCollector", + "prometheus_client.platform_collector.PlatformCollector", + "prometheus_client.gc_collector.GCCollector", + "synapse.metrics._gc.GCCounts", "synapse.metrics._gc.PyPyGCStats", + "synapse.metrics._reactor_metrics.ReactorLastSeenMetric", + "synapse.metrics.CPUMetrics", + "synapse.metrics.jemalloc.JemallocCollector", + "synapse.util.metrics.DynamicCollectorRegistry", + "synapse.metrics.background_process_metrics._Collector", # "prometheus_client.metrics_core.GaugeMetricFamily", # TODO: Add other prometheus_client metrics that need checking as we # refactor, see https://github.com/element-hq/synapse/issues/18592 diff --git a/synapse/metrics/__init__.py b/synapse/metrics/__init__.py index bb4213a0603..cbb2fa01be0 100644 --- a/synapse/metrics/__init__.py +++ b/synapse/metrics/__init__.py @@ -465,7 +465,8 @@ def collect(self) -> Iterable[Metric]: yield sys -REGISTRY.register(CPUMetrics()) +# This is a process-level metric, so it does not have the `SERVER_NAME_LABEL`. +REGISTRY.register(CPUMetrics()) # type: ignore[missing-server-name-label] # diff --git a/synapse/metrics/_gc.py b/synapse/metrics/_gc.py index dd8a541648c..a469cf5e878 100644 --- a/synapse/metrics/_gc.py +++ b/synapse/metrics/_gc.py @@ -101,7 +101,8 @@ def install_gc_manager() -> None: if running_on_pypy: return - REGISTRY.register(GCCounts()) + # This is a process-level metric, so it does not have the `SERVER_NAME_LABEL`. + REGISTRY.register(GCCounts()) # type: ignore[missing-server-name-label] gc.disable() diff --git a/synapse/metrics/_reactor_metrics.py b/synapse/metrics/_reactor_metrics.py index fda0cd018be..edc804664e4 100644 --- a/synapse/metrics/_reactor_metrics.py +++ b/synapse/metrics/_reactor_metrics.py @@ -165,4 +165,5 @@ def collect(self) -> Iterable[Metric]: if wrapper: - REGISTRY.register(ReactorLastSeenMetric(wrapper)) + # This is a process-level metric, so it does not have the `SERVER_NAME_LABEL`. + REGISTRY.register(ReactorLastSeenMetric(wrapper)) # type: ignore[missing-server-name-label] diff --git a/synapse/metrics/background_process_metrics.py b/synapse/metrics/background_process_metrics.py index 9671621f8cd..f7f2d88885e 100644 --- a/synapse/metrics/background_process_metrics.py +++ b/synapse/metrics/background_process_metrics.py @@ -167,7 +167,9 @@ def collect(self) -> Iterable[Metric]: yield from m.collect() -REGISTRY.register(_Collector()) +# The `SERVER_NAME_LABEL` is included in the individual metrics added to this registry, +# so we don't need to worry about it on the collector itself. +REGISTRY.register(_Collector()) # type: ignore[missing-server-name-label] class _BackgroundProcess: diff --git a/synapse/util/caches/__init__.py b/synapse/util/caches/__init__.py index 3087ad6adc2..607815741f8 100644 --- a/synapse/util/caches/__init__.py +++ b/synapse/util/caches/__init__.py @@ -42,7 +42,10 @@ # We track cache metrics in a special registry that lets us update the metrics # just before they are returned from the scrape endpoint. -CACHE_METRIC_REGISTRY = DynamicCollectorRegistry() +# +# The `SERVER_NAME_LABEL` is included in the individual metrics added to this registry, +# so we don't need to worry about it on the collector itself. +CACHE_METRIC_REGISTRY = DynamicCollectorRegistry() # type: ignore[missing-server-name-label] caches_by_name: Dict[str, Sized] = {} From 49020469b3d5aa6590df435955077761cf4b200a Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 28 Jul 2025 18:02:39 -0500 Subject: [PATCH 12/27] Fix `JemallocCollector` edge case --- scripts-dev/mypy_synapse_plugin.py | 6 ++++++ synapse/metrics/jemalloc.py | 3 ++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/scripts-dev/mypy_synapse_plugin.py b/scripts-dev/mypy_synapse_plugin.py index f9a59e47126..7644e2fe8ae 100644 --- a/scripts-dev/mypy_synapse_plugin.py +++ b/scripts-dev/mypy_synapse_plugin.py @@ -147,6 +147,12 @@ def get_base_class_hook( def get_function_signature_hook( self, fullname: str ) -> Optional[Callable[[FunctionSigContext], FunctionLike]]: + # Strip off the unique identifier for classes that are dynamically created inside + # functions. ex. `synapse.metrics.jemalloc.JemallocCollector@185` (this is the line + # number) + if "@" in fullname: + fullname = fullname.split("@", 1)[0] + # TODO: Use `prometheus_metric_fullname_to_label_arg_map.keys()` once we've # updated all of the metrics, see # https://github.com/element-hq/synapse/issues/18592 diff --git a/synapse/metrics/jemalloc.py b/synapse/metrics/jemalloc.py index 321ff580833..af1353863a8 100644 --- a/synapse/metrics/jemalloc.py +++ b/synapse/metrics/jemalloc.py @@ -230,7 +230,8 @@ def collect(self) -> Iterable[Metric]: yield g - REGISTRY.register(JemallocCollector()) + # This is a process-level metric, so it does not have the `SERVER_NAME_LABEL`. + REGISTRY.register(JemallocCollector()) # type: ignore[missing-server-name-label] logger.debug("Added jemalloc stats") From da386312942718044eae541d6d312e40cc037bef Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 28 Jul 2025 18:07:25 -0500 Subject: [PATCH 13/27] Support almost all metric types --- scripts-dev/mypy_synapse_plugin.py | 9 +++++++++ tests/metrics/test_metrics.py | 3 ++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/scripts-dev/mypy_synapse_plugin.py b/scripts-dev/mypy_synapse_plugin.py index 7644e2fe8ae..62d8920e3e5 100644 --- a/scripts-dev/mypy_synapse_plugin.py +++ b/scripts-dev/mypy_synapse_plugin.py @@ -157,7 +157,16 @@ def get_function_signature_hook( # updated all of the metrics, see # https://github.com/element-hq/synapse/issues/18592 if fullname in ( + "prometheus_client.metrics.MetricWrapperBase", "prometheus_client.metrics.Counter", + # "prometheus_client.metrics.Histogram", + # "prometheus_client.metrics.Gauge", + "prometheus_client.metrics.Summary", + "prometheus_client.metrics.Info", + "prometheus_client.metrics.Enum", + # "synapse.metrics.LaterGauge", + "synapse.metrics.InFlightGauge", + # "synapse.metrics.GaugeBucketCollector", "prometheus_client.registry.Collector", "prometheus_client.registry._EmptyCollector", "prometheus_client.registry.CollectorRegistry", diff --git a/tests/metrics/test_metrics.py b/tests/metrics/test_metrics.py index e92d5f6dfa1..23dc2c0cad1 100644 --- a/tests/metrics/test_metrics.py +++ b/tests/metrics/test_metrics.py @@ -59,7 +59,8 @@ class MetricEntry(Protocol): foo: int bar: int - gauge: InFlightGauge[MetricEntry] = InFlightGauge( + # This is a test and does not matter if it uses `SERVER_NAME_LABEL`. + gauge: InFlightGauge[MetricEntry] = InFlightGauge( # type: ignore[missing-server-name-label] "test1", "", labels=["test_label"], sub_metrics=["foo", "bar"] ) From b46040a95557e02715065f2ccd28df817ff96674 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 28 Jul 2025 18:08:34 -0500 Subject: [PATCH 14/27] Add some cross-links --- scripts-dev/mypy_synapse_plugin.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/scripts-dev/mypy_synapse_plugin.py b/scripts-dev/mypy_synapse_plugin.py index 62d8920e3e5..57144dc695c 100644 --- a/scripts-dev/mypy_synapse_plugin.py +++ b/scripts-dev/mypy_synapse_plugin.py @@ -159,13 +159,17 @@ def get_function_signature_hook( if fullname in ( "prometheus_client.metrics.MetricWrapperBase", "prometheus_client.metrics.Counter", + # Tracked by https://github.com/element-hq/synapse/pull/18724 # "prometheus_client.metrics.Histogram", + # Tracked by https://github.com/element-hq/synapse/pull/18725 # "prometheus_client.metrics.Gauge", "prometheus_client.metrics.Summary", "prometheus_client.metrics.Info", "prometheus_client.metrics.Enum", + # Tracked by https://github.com/element-hq/synapse/pull/18714 # "synapse.metrics.LaterGauge", "synapse.metrics.InFlightGauge", + # Tracked by https://github.com/element-hq/synapse/pull/18715 # "synapse.metrics.GaugeBucketCollector", "prometheus_client.registry.Collector", "prometheus_client.registry._EmptyCollector", From 8d706acc71852030a734f2217c28c03782943af4 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 28 Jul 2025 18:25:06 -0500 Subject: [PATCH 15/27] Lint `Metric` types --- scripts-dev/mypy_synapse_plugin.py | 73 ++++++++++++++++++------------ 1 file changed, 44 insertions(+), 29 deletions(-) diff --git a/scripts-dev/mypy_synapse_plugin.py b/scripts-dev/mypy_synapse_plugin.py index 57144dc695c..2faf359f7e3 100644 --- a/scripts-dev/mypy_synapse_plugin.py +++ b/scripts-dev/mypy_synapse_plugin.py @@ -88,9 +88,6 @@ class ArgLocation: "synapse.metrics.LaterGauge": ArgLocation("labelnames", 2), "synapse.metrics.InFlightGauge": ArgLocation("labels", 2), "synapse.metrics.GaugeBucketCollector": ArgLocation("labelnames", 2), - # These should always fail because they don't have a `labelnames` argument, but we - # include them here so that they fail the lint and people need to manually allow via - # a type ignore comment as the source of truth should be in the source code. "prometheus_client.registry.Collector": None, "prometheus_client.registry._EmptyCollector": None, "prometheus_client.registry.CollectorRegistry": None, @@ -104,34 +101,43 @@ class ArgLocation: "synapse.metrics.jemalloc.JemallocCollector": None, "synapse.util.metrics.DynamicCollectorRegistry": None, "synapse.metrics.background_process_metrics._Collector": None, - # We don't include `prometheus_client.metrics_core.Metric` here because it "is - # intended only for internal use by the [Prometheus] instrumentation client" and + # + # `Metric` subclasses: + # + # Note: `prometheus_client.metrics_core.Metric` itself should not be used because it + # "is intended only for internal use by the [Prometheus] instrumentation client" and # custom collectors should use the metric families listed below instead. # Additionally, it doesn't have a `labelnames` argument. - # "prometheus_client.metrics_core.Metric" - # "prometheus_client.metrics_core.UnknownMetricFamily": ArgLocation("labels", 3), - # "prometheus_client.metrics_core.CounterMetricFamily": ArgLocation("labels", 3), - # "prometheus_client.metrics_core.GaugeMetricFamily": ArgLocation("labels", 3), - # "prometheus_client.metrics_core.SummaryMetricFamily": ArgLocation("labels", 3), - # "prometheus_client.metrics_core.InfoMetricFamily": ArgLocation("labels", 3), - # "prometheus_client.metrics_core.HistogramMetricFamily": ArgLocation("labels", 3), - # "prometheus_client.metrics_core.GaugeHistogramMetricFamily": ArgLocation( - # "labels", 3 - # ), - # "prometheus_client.metrics_core.StateSetMetricFamily": ArgLocation("labels", 3), - # Our custom metrics: - # TODO: "synapse.metrics.GaugeHistogramMetricFamilyWithLabels": ArgLocation("labelnames", 4), + "prometheus_client.metrics_core.Metric": None, + "prometheus_client.metrics_core.UnknownMetricFamily": ArgLocation("labels", 3), + "prometheus_client.metrics_core.CounterMetricFamily": ArgLocation("labels", 3), + "prometheus_client.metrics_core.GaugeMetricFamily": ArgLocation("labels", 3), + "prometheus_client.metrics_core.SummaryMetricFamily": ArgLocation("labels", 3), + "prometheus_client.metrics_core.InfoMetricFamily": ArgLocation("labels", 3), + "prometheus_client.metrics_core.HistogramMetricFamily": ArgLocation("labels", 3), + "prometheus_client.metrics_core.GaugeHistogramMetricFamily": ArgLocation( + "labels", 3 + ), + "prometheus_client.metrics_core.StateSetMetricFamily": ArgLocation("labels", 3), + "synapse.metrics.GaugeHistogramMetricFamilyWithLabels": ArgLocation( + "labelnames", 4 + ), } """ Map from the fullname of the Prometheus `Metric`/`Collector` classes to the keyword -argument name and positional index of the label names. - -This should include anything that inherits from `prometheus_client.registry.Collector` -(`synapse.metrics._types.Collector`) or `prometheus_client.metrics_core.Metric`. This is -enforced by `analyze_prometheus_metric_classes`. - -This is useful because different metrics have different signatures for passing in label -names and we just need to know where to look. +argument name and positional index of the label names. This map is useful because +different metrics have different signatures for passing in label names and we just need +to know where to look. + +This map should include any metrics that we collect with Prometheus. Which corresponds +to anything that inherits from `prometheus_client.registry.Collector` +(`synapse.metrics._types.Collector`) or `prometheus_client.metrics_core.Metric`. The +exhaustiveness of this list is enforced by `analyze_prometheus_metric_classes`. + +The entries with `None` always fail the lint because they don't have a `labelnames` +argument (therefore, no `SERVER_NAME_LABEL`), but we include them here so that people +can notice and manually allow via a type ignore comment as the source of truth +should be in the source code. """ @@ -184,9 +190,16 @@ def get_function_signature_hook( "synapse.metrics.jemalloc.JemallocCollector", "synapse.util.metrics.DynamicCollectorRegistry", "synapse.metrics.background_process_metrics._Collector", - # "prometheus_client.metrics_core.GaugeMetricFamily", - # TODO: Add other prometheus_client metrics that need checking as we - # refactor, see https://github.com/element-hq/synapse/issues/18592 + "prometheus_client.metrics_core.Metric", + "prometheus_client.metrics_core.UnknownMetricFamily", + "prometheus_client.metrics_core.CounterMetricFamily", + "prometheus_client.metrics_core.GaugeMetricFamily", + "prometheus_client.metrics_core.SummaryMetricFamily", + "prometheus_client.metrics_core.InfoMetricFamily", + "prometheus_client.metrics_core.HistogramMetricFamily", + "prometheus_client.metrics_core.GaugeHistogramMetricFamily", + "prometheus_client.metrics_core.StateSetMetricFamily", + "synapse.metrics.GaugeHistogramMetricFamilyWithLabels", ): return lambda ctx: check_prometheus_metric_instantiation(ctx, fullname) @@ -234,6 +247,8 @@ def analyze_prometheus_metric_classes(ctx: ClassDefContext) -> None: # All of the Prometheus metric classes inherit from the `Collector`. "prometheus_client.registry.Collector", "synapse.metrics._types.Collector", + # TODO + "prometheus_client.metrics_core.Metric", ) for ancestor_type in ctx.cls.info.mro ): From 478edc24124ab81898a6138857b37f38e352e10e Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 28 Jul 2025 18:35:25 -0500 Subject: [PATCH 16/27] Ignore `Metric` usage where necessary --- scripts-dev/mypy_synapse_plugin.py | 4 +++- synapse/metrics/__init__.py | 22 ++++++++++++++++------ synapse/metrics/_gc.py | 9 ++++++--- synapse/metrics/_reactor_metrics.py | 3 ++- synapse/metrics/jemalloc.py | 3 ++- 5 files changed, 29 insertions(+), 12 deletions(-) diff --git a/scripts-dev/mypy_synapse_plugin.py b/scripts-dev/mypy_synapse_plugin.py index 2faf359f7e3..61aa9651f38 100644 --- a/scripts-dev/mypy_synapse_plugin.py +++ b/scripts-dev/mypy_synapse_plugin.py @@ -116,7 +116,7 @@ class ArgLocation: "prometheus_client.metrics_core.InfoMetricFamily": ArgLocation("labels", 3), "prometheus_client.metrics_core.HistogramMetricFamily": ArgLocation("labels", 3), "prometheus_client.metrics_core.GaugeHistogramMetricFamily": ArgLocation( - "labels", 3 + "labels", 4 ), "prometheus_client.metrics_core.StateSetMetricFamily": ArgLocation("labels", 3), "synapse.metrics.GaugeHistogramMetricFamilyWithLabels": ArgLocation( @@ -201,6 +201,8 @@ def get_function_signature_hook( "prometheus_client.metrics_core.StateSetMetricFamily", "synapse.metrics.GaugeHistogramMetricFamilyWithLabels", ): + # Because it's difficult to determine the `fullname` of the function in the + # callback, let's just pass it in while we have it. return lambda ctx: check_prometheus_metric_instantiation(ctx, fullname) return None diff --git a/synapse/metrics/__init__.py b/synapse/metrics/__init__.py index cbb2fa01be0..f8d2c59841f 100644 --- a/synapse/metrics/__init__.py +++ b/synapse/metrics/__init__.py @@ -169,7 +169,9 @@ class LaterGauge(Collector): ] def collect(self) -> Iterable[Metric]: - g = GaugeMetricFamily(self.name, self.desc, labels=self.labels) + # The decision to add `SERVER_NAME_LABEL` is from the `LaterGauge` usage itself + # (we don't enforce it here, one level up). + g = GaugeMetricFamily(self.name, self.desc, labels=self.labels) # type: ignore[missing-server-name-label] try: calls = self.caller() @@ -303,7 +305,9 @@ def collect(self) -> Iterable[Metric]: Note: may be called by a separate thread. """ - in_flight = GaugeMetricFamily( + # The decision to add `SERVER_NAME_LABEL` is from the `GaugeBucketCollector` + # usage itself (we don't enforce it here, one level up). + in_flight = GaugeMetricFamily( # type: ignore[missing-server-name-label] self.name + "_total", self.desc, labels=self.labels ) @@ -327,7 +331,9 @@ def collect(self) -> Iterable[Metric]: yield in_flight for name in self.sub_metrics: - gauge = GaugeMetricFamily( + # The decision to add `SERVER_NAME_LABEL` is from the `InFlightGauge` usage + # itself (we don't enforce it here, one level up). + gauge = GaugeMetricFamily( # type: ignore[missing-server-name-label] "_".join([self.name, name]), "", labels=self.labels ) for key, metrics in metrics_by_key.items(): @@ -422,7 +428,9 @@ def _values_to_metric(self, values: Iterable[float]) -> GaugeHistogramMetricFami # that bucket or below. accumulated_values = itertools.accumulate(bucket_values) - return GaugeHistogramMetricFamily( + # The decision to add `SERVER_NAME_LABEL` is from the `GaugeBucketCollector` + # usage itself (we don't enforce it here, one level up). + return GaugeHistogramMetricFamily( # type: ignore[missing-server-name-label] self._name, self._documentation, buckets=list( @@ -456,11 +464,13 @@ def collect(self) -> Iterable[Metric]: line = s.read() raw_stats = line.split(") ", 1)[1].split(" ") - user = GaugeMetricFamily("process_cpu_user_seconds_total", "") + # This is a process-level metric, so it does not have the `SERVER_NAME_LABEL`. + user = GaugeMetricFamily("process_cpu_user_seconds_total", "") # type: ignore[missing-server-name-label] user.add_metric([], float(raw_stats[11]) / self.ticks_per_sec) yield user - sys = GaugeMetricFamily("process_cpu_system_seconds_total", "") + # This is a process-level metric, so it does not have the `SERVER_NAME_LABEL`. + sys = GaugeMetricFamily("process_cpu_system_seconds_total", "") # type: ignore[missing-server-name-label] sys.add_metric([], float(raw_stats[12]) / self.ticks_per_sec) yield sys diff --git a/synapse/metrics/_gc.py b/synapse/metrics/_gc.py index a469cf5e878..72b03cf7041 100644 --- a/synapse/metrics/_gc.py +++ b/synapse/metrics/_gc.py @@ -82,7 +82,8 @@ class GCCounts(Collector): def collect(self) -> Iterable[Metric]: - cm = GaugeMetricFamily("python_gc_counts", "GC object counts", labels=["gen"]) + # This is a process-level metric, so it does not have the `SERVER_NAME_LABEL`. + cm = GaugeMetricFamily("python_gc_counts", "GC object counts", labels=["gen"]) # type: ignore[missing-server-name-label] for n, m in enumerate(gc.get_count()): cm.add_metric([str(n)], m) @@ -177,7 +178,8 @@ def collect(self) -> Iterable[Metric]: # # Total time spent in GC: 0.073 # s.total_gc_time - pypy_gc_time = CounterMetricFamily( + # This is a process-level metric, so it does not have the `SERVER_NAME_LABEL`. + pypy_gc_time = CounterMetricFamily( # type: ignore[missing-server-name-label] "pypy_gc_time_seconds_total", "Total time spent in PyPy GC", labels=[], @@ -185,7 +187,8 @@ def collect(self) -> Iterable[Metric]: pypy_gc_time.add_metric([], s.total_gc_time / 1000) yield pypy_gc_time - pypy_mem = GaugeMetricFamily( + # This is a process-level metric, so it does not have the `SERVER_NAME_LABEL`. + pypy_mem = GaugeMetricFamily( # type: ignore[missing-server-name-label] "pypy_memory_bytes", "Memory tracked by PyPy allocator", labels=["state", "class", "kind"], diff --git a/synapse/metrics/_reactor_metrics.py b/synapse/metrics/_reactor_metrics.py index edc804664e4..446e38c75eb 100644 --- a/synapse/metrics/_reactor_metrics.py +++ b/synapse/metrics/_reactor_metrics.py @@ -114,7 +114,8 @@ def __init__(self, call_wrapper: CallWrapper): self._call_wrapper = call_wrapper def collect(self) -> Iterable[Metric]: - cm = GaugeMetricFamily( + # This is a process-level metric, so it does not have the `SERVER_NAME_LABEL`. + cm = GaugeMetricFamily( # type: ignore[missing-server-name-label] "python_twisted_reactor_last_seen", "Seconds since the Twisted reactor was last seen", ) diff --git a/synapse/metrics/jemalloc.py b/synapse/metrics/jemalloc.py index af1353863a8..fb8adbe060b 100644 --- a/synapse/metrics/jemalloc.py +++ b/synapse/metrics/jemalloc.py @@ -188,7 +188,8 @@ class JemallocCollector(Collector): def collect(self) -> Iterable[Metric]: stats.refresh_stats() - g = GaugeMetricFamily( + # This is a process-level metric, so it does not have the `SERVER_NAME_LABEL`. + g = GaugeMetricFamily( # type: ignore[missing-server-name-label] "jemalloc_stats_app_memory_bytes", "The stats reported by jemalloc", labels=["type"], From 62ce1e05b32240580fc047737ea71f3daa6dcde0 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 28 Jul 2025 18:36:32 -0500 Subject: [PATCH 17/27] Add changelog --- changelog.d/18733.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/18733.misc diff --git a/changelog.d/18733.misc b/changelog.d/18733.misc new file mode 100644 index 00000000000..f38647d4c65 --- /dev/null +++ b/changelog.d/18733.misc @@ -0,0 +1 @@ +Update metrics linting to be able to handle custom metrics. From 9166d55c46421ed7acb89fc58aa8333764623e7b Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 28 Jul 2025 19:17:42 -0500 Subject: [PATCH 18/27] Remove irrelevant comment --- scripts-dev/mypy_synapse_plugin.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/scripts-dev/mypy_synapse_plugin.py b/scripts-dev/mypy_synapse_plugin.py index 61aa9651f38..16ed6bf7f28 100644 --- a/scripts-dev/mypy_synapse_plugin.py +++ b/scripts-dev/mypy_synapse_plugin.py @@ -103,11 +103,6 @@ class ArgLocation: "synapse.metrics.background_process_metrics._Collector": None, # # `Metric` subclasses: - # - # Note: `prometheus_client.metrics_core.Metric` itself should not be used because it - # "is intended only for internal use by the [Prometheus] instrumentation client" and - # custom collectors should use the metric families listed below instead. - # Additionally, it doesn't have a `labelnames` argument. "prometheus_client.metrics_core.Metric": None, "prometheus_client.metrics_core.UnknownMetricFamily": ArgLocation("labels", 3), "prometheus_client.metrics_core.CounterMetricFamily": ArgLocation("labels", 3), From 2d947a4a3343a604ab49b1e87fc8bb37fc0bec49 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 28 Jul 2025 19:19:56 -0500 Subject: [PATCH 19/27] Beef up comment --- scripts-dev/mypy_synapse_plugin.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/scripts-dev/mypy_synapse_plugin.py b/scripts-dev/mypy_synapse_plugin.py index 16ed6bf7f28..a1ae3bbc5b1 100644 --- a/scripts-dev/mypy_synapse_plugin.py +++ b/scripts-dev/mypy_synapse_plugin.py @@ -137,9 +137,12 @@ class ArgLocation: class SynapsePlugin(Plugin): - # Unfortunately, because mypy only chooses the first plugin that returns a non-None - # value, this check doesn't get run during our normal mypy lint process because - # `mypy_zope` also uses the `get_base_class_hook` method. + # FIXME: Unfortunately, because mypy only chooses the first plugin that returns a + # non-None value, this check doesn't get run during our normal mypy lint process + # because `mypy_zope` also uses the `get_base_class_hook` method. Currently, this + # only works if you comment out the `mypy_zope` plugin and squint through the output + # caused by those failures. Unaware of a suitable workaround to actually enforce + # this on a regulard basis and in CI. Related issue: https://github.com/python/mypy/issues/19524 def get_base_class_hook( self, fullname: str ) -> Optional[Callable[[ClassDefContext], None]]: From 89076a430e8e95f2055ebbd4ed64a8f31e1abb0a Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 28 Jul 2025 19:21:25 -0500 Subject: [PATCH 20/27] Mark off TODO about `Metric` --- scripts-dev/mypy_synapse_plugin.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/scripts-dev/mypy_synapse_plugin.py b/scripts-dev/mypy_synapse_plugin.py index a1ae3bbc5b1..64bdffb903e 100644 --- a/scripts-dev/mypy_synapse_plugin.py +++ b/scripts-dev/mypy_synapse_plugin.py @@ -239,15 +239,13 @@ def analyze_prometheus_metric_classes(ctx: ClassDefContext) -> None: if "@" in fullname: fullname = fullname.split("@", 1)[0] - # TODO: Consider `prometheus_client.metrics_core.Metric` - if any( ancestor_type.fullname in ( # All of the Prometheus metric classes inherit from the `Collector`. "prometheus_client.registry.Collector", "synapse.metrics._types.Collector", - # TODO + # And custom metrics that inherit from `Metric`. "prometheus_client.metrics_core.Metric", ) for ancestor_type in ctx.cls.info.mro From ab5a756770c685aa5b5e00c209f84a47b6cd0cd3 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 29 Jul 2025 13:51:54 -0500 Subject: [PATCH 21/27] Uncomment merged lints --- scripts-dev/mypy_synapse_plugin.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/scripts-dev/mypy_synapse_plugin.py b/scripts-dev/mypy_synapse_plugin.py index 599da8ec17b..49e3b8479a6 100644 --- a/scripts-dev/mypy_synapse_plugin.py +++ b/scripts-dev/mypy_synapse_plugin.py @@ -169,11 +169,9 @@ def get_function_signature_hook( "prometheus_client.metrics.Summary", "prometheus_client.metrics.Info", "prometheus_client.metrics.Enum", - # Tracked by https://github.com/element-hq/synapse/pull/18714 - # "synapse.metrics.LaterGauge", + "synapse.metrics.LaterGauge", "synapse.metrics.InFlightGauge", - # Tracked by https://github.com/element-hq/synapse/pull/18715 - # "synapse.metrics.GaugeBucketCollector", + "synapse.metrics.GaugeBucketCollector", "prometheus_client.registry.Collector", "prometheus_client.registry._EmptyCollector", "prometheus_client.registry.CollectorRegistry", From bec7bfbea46bee355ae4cdbc762f45f633210c43 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 29 Jul 2025 15:38:18 -0500 Subject: [PATCH 22/27] We can now use the full list --- scripts-dev/mypy_synapse_plugin.py | 41 +++--------------------------- 1 file changed, 3 insertions(+), 38 deletions(-) diff --git a/scripts-dev/mypy_synapse_plugin.py b/scripts-dev/mypy_synapse_plugin.py index 40e2238fb24..b6ed65ee4be 100644 --- a/scripts-dev/mypy_synapse_plugin.py +++ b/scripts-dev/mypy_synapse_plugin.py @@ -157,44 +157,9 @@ def get_function_signature_hook( if "@" in fullname: fullname = fullname.split("@", 1)[0] - # TODO: Use `prometheus_metric_fullname_to_label_arg_map.keys()` once we've - # updated all of the metrics, see - # https://github.com/element-hq/synapse/issues/18592 - if fullname in ( - "prometheus_client.metrics.MetricWrapperBase", - "prometheus_client.metrics.Counter", - "prometheus_client.metrics.Histogram", - "prometheus_client.metrics.Gauge", - "prometheus_client.metrics.Summary", - "prometheus_client.metrics.Info", - "prometheus_client.metrics.Enum", - "synapse.metrics.LaterGauge", - "synapse.metrics.InFlightGauge", - "synapse.metrics.GaugeBucketCollector", - "prometheus_client.registry.Collector", - "prometheus_client.registry._EmptyCollector", - "prometheus_client.registry.CollectorRegistry", - "prometheus_client.process_collector.ProcessCollector", - "prometheus_client.platform_collector.PlatformCollector", - "prometheus_client.gc_collector.GCCollector", - "synapse.metrics._gc.GCCounts", - "synapse.metrics._gc.PyPyGCStats", - "synapse.metrics._reactor_metrics.ReactorLastSeenMetric", - "synapse.metrics.CPUMetrics", - "synapse.metrics.jemalloc.JemallocCollector", - "synapse.util.metrics.DynamicCollectorRegistry", - "synapse.metrics.background_process_metrics._Collector", - "prometheus_client.metrics_core.Metric", - "prometheus_client.metrics_core.UnknownMetricFamily", - "prometheus_client.metrics_core.CounterMetricFamily", - "prometheus_client.metrics_core.GaugeMetricFamily", - "prometheus_client.metrics_core.SummaryMetricFamily", - "prometheus_client.metrics_core.InfoMetricFamily", - "prometheus_client.metrics_core.HistogramMetricFamily", - "prometheus_client.metrics_core.GaugeHistogramMetricFamily", - "prometheus_client.metrics_core.StateSetMetricFamily", - "synapse.metrics.GaugeHistogramMetricFamilyWithLabels", - ): + # Look for any Prometheus metrics to make sure they have the `SERVER_NAME_LABEL` + # label. + if fullname in prometheus_metric_fullname_to_label_arg_map.keys(): # Because it's difficult to determine the `fullname` of the function in the # callback, let's just pass it in while we have it. return lambda ctx: check_prometheus_metric_instantiation(ctx, fullname) From 21ef4b4c8a52696c4ddf046109d9f36389de69b0 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 31 Jul 2025 17:19:10 -0500 Subject: [PATCH 23/27] Workaround mypy limitation only calling `get_base_class_hook` for one plugin See https://github.com/element-hq/synapse/pull/18733#discussion_r2237747149 Discussed at https://matrix.to/#/!vcyiEtMVHIhWXcJAfl:sw1v.org/$yA64EMlsudEUdu2tRxQDcsF1HLqblXCOLCX9282asFw?via=matrix.org&via=element.io&via=node.marinchik.ink --- mypy.ini | 5 ++++- scripts-dev/mypy_synapse_plugin.py | 27 ++++++++++++++++++++------- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/mypy.ini b/mypy.ini index cf64248cc52..5ef3a378495 100644 --- a/mypy.ini +++ b/mypy.ini @@ -1,6 +1,6 @@ [mypy] namespace_packages = True -plugins = pydantic.mypy, mypy_zope:plugin, scripts-dev/mypy_synapse_plugin.py +plugins = scripts-dev/mypy_synapse_plugin.py, pydantic.mypy, mypy_zope:plugin follow_imports = normal show_error_codes = True show_traceback = True @@ -99,3 +99,6 @@ ignore_missing_imports = True [mypy-multipart.*] ignore_missing_imports = True + +[mypy-mypy_zope.*] +ignore_missing_imports = True diff --git a/scripts-dev/mypy_synapse_plugin.py b/scripts-dev/mypy_synapse_plugin.py index b6ed65ee4be..9f5174b67f5 100644 --- a/scripts-dev/mypy_synapse_plugin.py +++ b/scripts-dev/mypy_synapse_plugin.py @@ -44,12 +44,14 @@ CallableType, Instance, NoneType, + Options, TupleType, TypeAliasType, TypeVarType, UninhabitedType, UnionType, ) +from mypy_zope import plugin as mypy_zope_plugin PROMETHEUS_METRIC_MISSING_SERVER_NAME_LABEL = ErrorCode( "missing-server-name-label", @@ -137,16 +139,27 @@ class ArgLocation: class SynapsePlugin(Plugin): - # FIXME: Unfortunately, because mypy only chooses the first plugin that returns a - # non-None value, this check doesn't get run during our normal mypy lint process - # because `mypy_zope` also uses the `get_base_class_hook` method. Currently, this - # only works if you comment out the `mypy_zope` plugin and squint through the output - # caused by those failures. Unaware of a suitable workaround to actually enforce - # this on a regulard basis and in CI. Related issue: https://github.com/python/mypy/issues/19524 + def __init__(self, options: Options): + super().__init__(options) + self.mypy_zope_plugin = mypy_zope_plugin("TODO")(options) + def get_base_class_hook( self, fullname: str ) -> Optional[Callable[[ClassDefContext], None]]: - return analyze_prometheus_metric_classes + def _get_base_class_hook(ctx: ClassDefContext) -> None: + # Run any `get_base_class_hook` checks from other plugins first. + # + # Unfortunately, because mypy only chooses the first plugin that returns a + # non-None value (known-limitation, c.f. + # https://github.com/python/mypy/issues/19524), we have to workaround this + # by putting our custom plugin first and then calling the other plugin's + # hook manually. + self.mypy_zope_plugin.get_base_class_hook(fullname)(ctx) + + # Now run our own checks + analyze_prometheus_metric_classes(ctx) + + return _get_base_class_hook def get_function_signature_hook( self, fullname: str From 61a6f3922c8525596037b3220f55c0b478db2113 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 31 Jul 2025 17:31:55 -0500 Subject: [PATCH 24/27] More sound logic --- scripts-dev/mypy_synapse_plugin.py | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/scripts-dev/mypy_synapse_plugin.py b/scripts-dev/mypy_synapse_plugin.py index 9f5174b67f5..8b12977af2f 100644 --- a/scripts-dev/mypy_synapse_plugin.py +++ b/scripts-dev/mypy_synapse_plugin.py @@ -137,11 +137,15 @@ class ArgLocation: should be in the source code. """ +# Unbound at this point because we don't know the mypy version yet. +# This is set in the `plugin(...)` function below. +MypyZopePluginClass: Type[Plugin] + class SynapsePlugin(Plugin): def __init__(self, options: Options): super().__init__(options) - self.mypy_zope_plugin = mypy_zope_plugin("TODO")(options) + self.mypy_zope_plugin = MypyZopePluginClass(options) def get_base_class_hook( self, fullname: str @@ -151,10 +155,11 @@ def _get_base_class_hook(ctx: ClassDefContext) -> None: # # Unfortunately, because mypy only chooses the first plugin that returns a # non-None value (known-limitation, c.f. - # https://github.com/python/mypy/issues/19524), we have to workaround this - # by putting our custom plugin first and then calling the other plugin's - # hook manually. - self.mypy_zope_plugin.get_base_class_hook(fullname)(ctx) + # https://github.com/python/mypy/issues/19524), we workaround this by + # putting our custom plugin first in the plugin order and then calling the + # other plugin's hook manually followed by our own checks. + if callback := self.mypy_zope_plugin.get_base_class_hook(fullname): + callback(ctx) # Now run our own checks analyze_prometheus_metric_classes(ctx) @@ -654,10 +659,12 @@ def is_cacheable( def plugin(version: str) -> Type[SynapsePlugin]: + global MypyZopePluginClass # This is the entry point of the plugin, and lets us deal with the fact # that the mypy plugin interface is *not* stable by looking at the version # string. # # However, since we pin the version of mypy Synapse uses in CI, we don't # really care. + MypyZopePluginClass = mypy_zope_plugin(version) return SynapsePlugin From 4965340dd5e9793587cabf6fd98db24d6a37ffa0 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 31 Jul 2025 17:45:15 -0500 Subject: [PATCH 25/27] Use lint instead of assert --- scripts-dev/mypy_synapse_plugin.py | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/scripts-dev/mypy_synapse_plugin.py b/scripts-dev/mypy_synapse_plugin.py index 8b12977af2f..9a604ab606b 100644 --- a/scripts-dev/mypy_synapse_plugin.py +++ b/scripts-dev/mypy_synapse_plugin.py @@ -33,6 +33,7 @@ from mypy.nodes import ARG_NAMED_OPT, ListExpr, NameExpr, TempNode, TupleExpr, Var from mypy.plugin import ( ClassDefContext, + Context, FunctionLike, FunctionSigContext, MethodSigContext, @@ -59,6 +60,12 @@ category="per-homeserver-tenant-metrics", ) +PROMETHEUS_METRIC_MISSING_FROM_LIST_TO_CHECK = ErrorCode( + "metric-type-missing-from-list", + "Every Prometheus metric type must be included in the `prometheus_metric_fullname_to_label_arg_map`.", + category="per-homeserver-tenant-metrics", +) + class Sentinel(enum.Enum): # defining a sentinel in this way allows mypy to correctly handle the @@ -229,11 +236,14 @@ def analyze_prometheus_metric_classes(ctx: ClassDefContext) -> None: ) for ancestor_type in ctx.cls.info.mro ): - assert fullname in prometheus_metric_fullname_to_label_arg_map, ( - f"Expected {fullname} to be in `prometheus_metric_fullname_to_label_arg_map`, " - f"but it was not found. This is a problem with our custom mypy plugin. " - f"Please add it to the map." - ) + if fullname not in prometheus_metric_fullname_to_label_arg_map: + ctx.api.fail( + f"Expected {fullname} to be in `prometheus_metric_fullname_to_label_arg_map`, " + f"but it was not found. This is a problem with our custom mypy plugin. " + f"Please add it to the map.", + Context(), + code=PROMETHEUS_METRIC_MISSING_FROM_LIST_TO_CHECK, + ) def check_prometheus_metric_instantiation( From a7573a734e3e783902ff747c35f7f6813dae3817 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 1 Aug 2025 12:13:53 -0500 Subject: [PATCH 26/27] Make sure pydantic mypy hooks are still being called See https://github.com/element-hq/synapse/pull/18733#discussion_r2248409418 --- mypy.ini | 7 +++++++ scripts-dev/mypy_synapse_plugin.py | 24 +++++++++++++++++++++++- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/mypy.ini b/mypy.ini index 5ef3a378495..97bc9b70148 100644 --- a/mypy.ini +++ b/mypy.ini @@ -1,5 +1,12 @@ [mypy] namespace_packages = True +# Our custom mypy plugin should remain first in this list. +# +# mypy has a limitation where it only chooses the first plugin that returns a non-None +# value for each hook (known-limitation, c.f. +# https://github.com/python/mypy/issues/19524). We workaround this by putting our custom +# plugin first in the plugin order and then manually calling any other conflicting +# plugin hooks in our own plugin followed by our own checks. plugins = scripts-dev/mypy_synapse_plugin.py, pydantic.mypy, mypy_zope:plugin follow_imports = normal show_error_codes = True diff --git a/scripts-dev/mypy_synapse_plugin.py b/scripts-dev/mypy_synapse_plugin.py index 9a604ab606b..610dec415af 100644 --- a/scripts-dev/mypy_synapse_plugin.py +++ b/scripts-dev/mypy_synapse_plugin.py @@ -37,6 +37,7 @@ FunctionLike, FunctionSigContext, MethodSigContext, + MypyFile, Plugin, ) from mypy.typeops import bind_self @@ -53,6 +54,7 @@ UnionType, ) from mypy_zope import plugin as mypy_zope_plugin +from pydantic.mypy import plugin as mypy_pydantic_plugin PROMETHEUS_METRIC_MISSING_SERVER_NAME_LABEL = ErrorCode( "missing-server-name-label", @@ -146,14 +148,31 @@ class ArgLocation: # Unbound at this point because we don't know the mypy version yet. # This is set in the `plugin(...)` function below. +MypyPydanticPluginClass: Type[Plugin] MypyZopePluginClass: Type[Plugin] class SynapsePlugin(Plugin): def __init__(self, options: Options): super().__init__(options) + self.mypy_pydantic_plugin = MypyPydanticPluginClass(options) self.mypy_zope_plugin = MypyZopePluginClass(options) + def set_modules(self, modules: dict[str, MypyFile]) -> None: + """ + This is called by mypy internals. We have to override this to ensure it's also + called for any other plugins that we're manually handling. + + Here is how mypy describes it: + + > [`self._modules`] can't be set in `__init__` because it is executed too soon + > in `build.py`. Therefore, `build.py` *must* set it later before graph processing + > starts by calling `set_modules()`. + """ + super().set_modules(modules) + self.mypy_pydantic_plugin.set_modules(modules) + self.mypy_zope_plugin.set_modules(modules) + def get_base_class_hook( self, fullname: str ) -> Optional[Callable[[ClassDefContext], None]]: @@ -165,6 +184,8 @@ def _get_base_class_hook(ctx: ClassDefContext) -> None: # https://github.com/python/mypy/issues/19524), we workaround this by # putting our custom plugin first in the plugin order and then calling the # other plugin's hook manually followed by our own checks. + if callback := self.mypy_pydantic_plugin.get_base_class_hook(fullname): + callback(ctx) if callback := self.mypy_zope_plugin.get_base_class_hook(fullname): callback(ctx) @@ -669,12 +690,13 @@ def is_cacheable( def plugin(version: str) -> Type[SynapsePlugin]: - global MypyZopePluginClass + global MypyPydanticPluginClass, MypyZopePluginClass # This is the entry point of the plugin, and lets us deal with the fact # that the mypy plugin interface is *not* stable by looking at the version # string. # # However, since we pin the version of mypy Synapse uses in CI, we don't # really care. + MypyPydanticPluginClass = mypy_pydantic_plugin(version) MypyZopePluginClass = mypy_zope_plugin(version) return SynapsePlugin From d2efe1d35d8b5a65d36c5f3f29479107e6855ae4 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 1 Aug 2025 12:23:45 -0500 Subject: [PATCH 27/27] Add more explicit instructions --- mypy.ini | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/mypy.ini b/mypy.ini index 97bc9b70148..ae903f858af 100644 --- a/mypy.ini +++ b/mypy.ini @@ -7,6 +7,10 @@ namespace_packages = True # https://github.com/python/mypy/issues/19524). We workaround this by putting our custom # plugin first in the plugin order and then manually calling any other conflicting # plugin hooks in our own plugin followed by our own checks. +# +# If you add a new plugin, make sure to check whether the hooks being used conflict with +# our custom plugin hooks and if so, manually call the other plugin's hooks in our +# custom plugin. (also applies to if the plugin is updated in the future) plugins = scripts-dev/mypy_synapse_plugin.py, pydantic.mypy, mypy_zope:plugin follow_imports = normal show_error_codes = True