From 676ea6dfc6d2bf451e51b78670b6e2db1849886b Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Wed, 26 Jan 2022 10:41:42 +0000 Subject: [PATCH 1/5] Convert labels to str ourselves This is done internally (if you read the code) but the type annotations now require str. --- synapse/handlers/register.py | 12 ++++++------ synapse/metrics/_gc.py | 4 ++-- synapse/replication/http/_base.py | 4 ++-- synapse/replication/tcp/external_cache.py | 2 +- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index f08a516a7588..b05c5d1bb15c 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -68,11 +68,11 @@ def init_counters_for_auth_provider(auth_provider_id: str) -> None: until the user first logs in/registers. """ for is_guest in (True, False): - login_counter.labels(guest=is_guest, auth_provider=auth_provider_id) + login_counter.labels(guest=str(is_guest), auth_provider=auth_provider_id) for shadow_banned in (True, False): registration_counter.labels( - guest=is_guest, - shadow_banned=shadow_banned, + guest=str(is_guest), + shadow_banned=str(shadow_banned), auth_provider=auth_provider_id, ) @@ -341,8 +341,8 @@ async def register_user( fail_count += 1 registration_counter.labels( - guest=make_guest, - shadow_banned=shadow_banned, + guest=str(make_guest), + shadow_banned=str(shadow_banned), auth_provider=(auth_provider_id or ""), ).inc() @@ -775,7 +775,7 @@ async def register_device( ) login_counter.labels( - guest=is_guest, + guest=str(is_guest), auth_provider=(auth_provider_id or ""), ).inc() diff --git a/synapse/metrics/_gc.py b/synapse/metrics/_gc.py index 2bc909efa0d3..1090d84579bb 100644 --- a/synapse/metrics/_gc.py +++ b/synapse/metrics/_gc.py @@ -123,8 +123,8 @@ def _maybe_gc() -> None: _last_gc[i] = end - gc_time.labels(i).observe(end - start) - gc_unreachable.labels(i).set(unreachable) + gc_time.labels(str(i)).observe(end - start) + gc_unreachable.labels(str(i)).set(unreachable) gc_task = task.LoopingCall(_maybe_gc) gc_task.start(0.1) diff --git a/synapse/replication/http/_base.py b/synapse/replication/http/_base.py index 585332b244a4..5d8b14c44214 100644 --- a/synapse/replication/http/_base.py +++ b/synapse/replication/http/_base.py @@ -257,13 +257,13 @@ async def send_request(*, instance_name="master", **kwargs): # We convert to SynapseError as we know that it was a SynapseError # on the main process that we should send to the client. (And # importantly, not stack traces everywhere) - _outgoing_request_counter.labels(cls.NAME, e.code).inc() + _outgoing_request_counter.labels(cls.NAME, str(e.code)).inc() raise e.to_synapse_error() except Exception as e: _outgoing_request_counter.labels(cls.NAME, "ERR").inc() raise SynapseError(502, "Failed to talk to main process") from e - _outgoing_request_counter.labels(cls.NAME, 200).inc() + _outgoing_request_counter.labels(cls.NAME, "200").inc() return result return send_request diff --git a/synapse/replication/tcp/external_cache.py b/synapse/replication/tcp/external_cache.py index aaf91e5e0253..5fcf44ff78aa 100644 --- a/synapse/replication/tcp/external_cache.py +++ b/synapse/replication/tcp/external_cache.py @@ -115,7 +115,7 @@ async def get(self, cache_name: str, key: str) -> Optional[Any]: logger.debug("Got cache result %s %s: %r", cache_name, key, result) - get_counter.labels(cache_name, result is not None).inc() + get_counter.labels(cache_name, str(result is not None)).inc() if not result: return None From 03fbcb42f8f6fa55bd34a0a7863cbdff31b2942d Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Wed, 26 Jan 2022 11:10:20 +0000 Subject: [PATCH 2/5] Add cast for RegistryProxy so that it can be a CollectorRegistry --- synapse/metrics/__init__.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/synapse/metrics/__init__.py b/synapse/metrics/__init__.py index 9e6c1b2f3b54..cca084c18c21 100644 --- a/synapse/metrics/__init__.py +++ b/synapse/metrics/__init__.py @@ -30,6 +30,7 @@ Type, TypeVar, Union, + cast, ) import attr @@ -60,7 +61,7 @@ HAVE_PROC_SELF_STAT = os.path.exists("/proc/self/stat") -class RegistryProxy: +class _RegistryProxy: @staticmethod def collect() -> Iterable[Metric]: for metric in REGISTRY.collect(): @@ -68,6 +69,13 @@ def collect() -> Iterable[Metric]: yield metric +# A little bit nasty, but collect() above is static so a Protocol doesn't work. +# _RegistryProxy matches the signature of a CollectorRegistry instance enough +# for it to be usable in the contexts in which we use it. +# TODO Do something nicer about this. +RegistryProxy = cast(CollectorRegistry, _RegistryProxy) + + @attr.s(slots=True, hash=True, auto_attribs=True) class LaterGauge: From 61491e5a69d386458623d520f850e612af7db25b Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Wed, 26 Jan 2022 11:12:19 +0000 Subject: [PATCH 3/5] Newsfile Signed-off-by: Olivier Wilkinson (reivilibre) --- changelog.d/11832.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/11832.misc diff --git a/changelog.d/11832.misc b/changelog.d/11832.misc new file mode 100644 index 000000000000..5ff117d93326 --- /dev/null +++ b/changelog.d/11832.misc @@ -0,0 +1 @@ +Fix type errors introduced by new annotations in the Prometheus Client library. \ No newline at end of file From cfacbc41090dc1611196f0e6037fbdbfa81537bd Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Wed, 2 Feb 2022 10:29:30 +0000 Subject: [PATCH 4/5] Revert "Avoid type annotation problems in prom-client (#11834)" This reverts commit 4cc1b3a2cb229cb877e77335a26eb8bde9382a56. --- synapse/python_dependencies.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/synapse/python_dependencies.py b/synapse/python_dependencies.py index 80786464c2fb..22b4606ae0ea 100644 --- a/synapse/python_dependencies.py +++ b/synapse/python_dependencies.py @@ -76,8 +76,7 @@ "msgpack>=0.5.2", "phonenumbers>=8.2.0", # we use GaugeHistogramMetric, which was added in prom-client 0.4.0. - # 0.13.0 has an incorrect type annotation, see #11832. - "prometheus_client>=0.4.0,<0.13.0", + "prometheus_client>=0.4.0", # we use `order`, which arrived in attrs 19.2.0. # Note: 21.1.0 broke `/sync`, see #9936 "attrs>=19.2.0,!=21.1.0", From da7abe01d45c89f01c55bbadf7ddf8175668e013 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Wed, 2 Feb 2022 10:30:55 +0000 Subject: [PATCH 5/5] Revert "Convert labels to str ourselves" This reverts commit 676ea6dfc6d2bf451e51b78670b6e2db1849886b. --- synapse/handlers/register.py | 12 ++++++------ synapse/metrics/_gc.py | 4 ++-- synapse/replication/http/_base.py | 4 ++-- synapse/replication/tcp/external_cache.py | 2 +- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 8cdca2aa44a6..a719d5eef351 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -68,11 +68,11 @@ def init_counters_for_auth_provider(auth_provider_id: str) -> None: until the user first logs in/registers. """ for is_guest in (True, False): - login_counter.labels(guest=str(is_guest), auth_provider=auth_provider_id) + login_counter.labels(guest=is_guest, auth_provider=auth_provider_id) for shadow_banned in (True, False): registration_counter.labels( - guest=str(is_guest), - shadow_banned=str(shadow_banned), + guest=is_guest, + shadow_banned=shadow_banned, auth_provider=auth_provider_id, ) @@ -343,8 +343,8 @@ async def register_user( fail_count += 1 registration_counter.labels( - guest=str(make_guest), - shadow_banned=str(shadow_banned), + guest=make_guest, + shadow_banned=shadow_banned, auth_provider=(auth_provider_id or ""), ).inc() @@ -777,7 +777,7 @@ async def register_device( ) login_counter.labels( - guest=str(is_guest), + guest=is_guest, auth_provider=(auth_provider_id or ""), ).inc() diff --git a/synapse/metrics/_gc.py b/synapse/metrics/_gc.py index 1090d84579bb..2bc909efa0d3 100644 --- a/synapse/metrics/_gc.py +++ b/synapse/metrics/_gc.py @@ -123,8 +123,8 @@ def _maybe_gc() -> None: _last_gc[i] = end - gc_time.labels(str(i)).observe(end - start) - gc_unreachable.labels(str(i)).set(unreachable) + gc_time.labels(i).observe(end - start) + gc_unreachable.labels(i).set(unreachable) gc_task = task.LoopingCall(_maybe_gc) gc_task.start(0.1) diff --git a/synapse/replication/http/_base.py b/synapse/replication/http/_base.py index 5d8b14c44214..585332b244a4 100644 --- a/synapse/replication/http/_base.py +++ b/synapse/replication/http/_base.py @@ -257,13 +257,13 @@ async def send_request(*, instance_name="master", **kwargs): # We convert to SynapseError as we know that it was a SynapseError # on the main process that we should send to the client. (And # importantly, not stack traces everywhere) - _outgoing_request_counter.labels(cls.NAME, str(e.code)).inc() + _outgoing_request_counter.labels(cls.NAME, e.code).inc() raise e.to_synapse_error() except Exception as e: _outgoing_request_counter.labels(cls.NAME, "ERR").inc() raise SynapseError(502, "Failed to talk to main process") from e - _outgoing_request_counter.labels(cls.NAME, "200").inc() + _outgoing_request_counter.labels(cls.NAME, 200).inc() return result return send_request diff --git a/synapse/replication/tcp/external_cache.py b/synapse/replication/tcp/external_cache.py index 5fcf44ff78aa..aaf91e5e0253 100644 --- a/synapse/replication/tcp/external_cache.py +++ b/synapse/replication/tcp/external_cache.py @@ -115,7 +115,7 @@ async def get(self, cache_name: str, key: str) -> Optional[Any]: logger.debug("Got cache result %s %s: %r", cache_name, key, result) - get_counter.labels(cache_name, str(result is not None)).inc() + get_counter.labels(cache_name, result is not None).inc() if not result: return None