From c028e8e81ceff0971fd7e35f9938088bd78bae14 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Fri, 15 Jul 2022 17:33:44 +0100 Subject: [PATCH 1/5] Unabbreviate "pusher" variable --- synapse/push/pusherpool.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/synapse/push/pusherpool.py b/synapse/push/pusherpool.py index d0cc657b4464..cf1815ec6a22 100644 --- a/synapse/push/pusherpool.py +++ b/synapse/push/pusherpool.py @@ -328,7 +328,7 @@ async def _start_pusher(self, pusher_config: PusherConfig) -> Optional[Pusher]: return None try: - p = self.pusher_factory.create_pusher(pusher_config) + pusher = self.pusher_factory.create_pusher(pusher_config) except PusherConfigException as e: logger.warning( "Pusher incorrectly configured id=%i, user=%s, appid=%s, pushkey=%s: %s", @@ -346,7 +346,7 @@ async def _start_pusher(self, pusher_config: PusherConfig) -> Optional[Pusher]: ) return None - if not p: + if not pusher: return None appid_pushkey = "%s:%s" % (pusher_config.app_id, pusher_config.pushkey) @@ -354,9 +354,9 @@ async def _start_pusher(self, pusher_config: PusherConfig) -> Optional[Pusher]: byuser = self.pushers.setdefault(pusher_config.user_name, {}) if appid_pushkey in byuser: byuser[appid_pushkey].on_stop() - byuser[appid_pushkey] = p + byuser[appid_pushkey] = pusher - synapse_pushers.labels(type(p).__name__, p.app_id).inc() + synapse_pushers.labels(type(pusher).__name__, pusher.app_id).inc() # Check if there *may* be push to process. We do this as this check is a # lot cheaper to do than actually fetching the exact rows we need to @@ -372,9 +372,9 @@ async def _start_pusher(self, pusher_config: PusherConfig) -> Optional[Pusher]: # risk missing push. have_notifs = True - p.on_started(have_notifs) + pusher.on_started(have_notifs) - return p + return pusher async def remove_pusher(self, app_id: str, pushkey: str, user_id: str) -> None: appid_pushkey = "%s:%s" % (app_id, pushkey) From ee4b002870a7e76a5675b67f48e2a87440fb3f15 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Fri, 15 Jul 2022 17:35:53 +0100 Subject: [PATCH 2/5] Fix overcounting of pushers when they are replaced Signed-off-by: Sean Quah --- synapse/push/pusherpool.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/synapse/push/pusherpool.py b/synapse/push/pusherpool.py index cf1815ec6a22..6bceb832c567 100644 --- a/synapse/push/pusherpool.py +++ b/synapse/push/pusherpool.py @@ -353,7 +353,12 @@ async def _start_pusher(self, pusher_config: PusherConfig) -> Optional[Pusher]: byuser = self.pushers.setdefault(pusher_config.user_name, {}) if appid_pushkey in byuser: - byuser[appid_pushkey].on_stop() + previous_pusher = byuser[appid_pushkey] + previous_pusher.on_stop() + + synapse_pushers.labels( + type(previous_pusher).__name__, previous_pusher.app_id + ).dec() byuser[appid_pushkey] = pusher synapse_pushers.labels(type(pusher).__name__, pusher.app_id).inc() From 8449e791fe69b9e0917f258aefe588f39357a4c9 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Fri, 15 Jul 2022 17:38:58 +0100 Subject: [PATCH 3/5] Add newsfile --- changelog.d/13296.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/13296.bugfix diff --git a/changelog.d/13296.bugfix b/changelog.d/13296.bugfix new file mode 100644 index 000000000000..048b774916e1 --- /dev/null +++ b/changelog.d/13296.bugfix @@ -0,0 +1 @@ +Fix a bug introduced in 1.18 where the `synapse_pushers` metric would overcount pushers when they are replaced. From 8e9ad3ddfbc4e21a923b1d067dbb19a5c66d7857 Mon Sep 17 00:00:00 2001 From: Sean Quah <8349537+squahtx@users.noreply.github.com> Date: Fri, 15 Jul 2022 18:04:13 +0100 Subject: [PATCH 4/5] Update changelog.d/13296.bugfix Co-authored-by: Patrick Cloke --- changelog.d/13296.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/13296.bugfix b/changelog.d/13296.bugfix index 048b774916e1..ff0eb2b4a1b3 100644 --- a/changelog.d/13296.bugfix +++ b/changelog.d/13296.bugfix @@ -1 +1 @@ -Fix a bug introduced in 1.18 where the `synapse_pushers` metric would overcount pushers when they are replaced. +Fix a bug introduced in v1.18.0 where the `synapse_pushers` metric would overcount pushers when they are replaced. From fa3fe78b5eb8ad779c7254866007d5d0e61b87b9 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Mon, 18 Jul 2022 16:41:19 +0100 Subject: [PATCH 5/5] Use values from Pusher instead of PusherConfig --- synapse/push/pusherpool.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/synapse/push/pusherpool.py b/synapse/push/pusherpool.py index 6bceb832c567..1e0ef44fc786 100644 --- a/synapse/push/pusherpool.py +++ b/synapse/push/pusherpool.py @@ -349,9 +349,9 @@ async def _start_pusher(self, pusher_config: PusherConfig) -> Optional[Pusher]: if not pusher: return None - appid_pushkey = "%s:%s" % (pusher_config.app_id, pusher_config.pushkey) + appid_pushkey = "%s:%s" % (pusher.app_id, pusher.pushkey) - byuser = self.pushers.setdefault(pusher_config.user_name, {}) + byuser = self.pushers.setdefault(pusher.user_id, {}) if appid_pushkey in byuser: previous_pusher = byuser[appid_pushkey] previous_pusher.on_stop() @@ -366,8 +366,8 @@ async def _start_pusher(self, pusher_config: PusherConfig) -> Optional[Pusher]: # Check if there *may* be push to process. We do this as this check is a # lot cheaper to do than actually fetching the exact rows we need to # push. - user_id = pusher_config.user_name - last_stream_ordering = pusher_config.last_stream_ordering + user_id = pusher.user_id + last_stream_ordering = pusher.last_stream_ordering if last_stream_ordering: have_notifs = await self.store.get_if_maybe_push_in_range_for_user( user_id, last_stream_ordering