Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Fix overcounting of pushers when they are replaced #13296

Merged
merged 5 commits into from
Jul 18, 2022
Merged

Fix overcounting of pushers when they are replaced #13296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c028e8e
Unabbreviate "pusher" variable
Jul 15, 2022
ee4b002
Fix overcounting of pushers when they are replaced
Jul 15, 2022
8449e79
Add newsfile
Jul 15, 2022
8e9ad3d
Update changelog.d/13296.bugfix
squahtx Jul 15, 2022
fa3fe78
Use values from Pusher instead of PusherConfig
Jul 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/13296.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a bug introduced in v1.18.0 where the `synapse_pushers` metric would overcount pushers when they are replaced.
27 changes: 16 additions & 11 deletions synapse/push/pusherpool.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -346,23 +346,28 @@ 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)
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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be clearer to use pusher.app_id or appid_pushkey consistently? Are those the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out that pusher_config.app_id and pusher.app_id are the same. I'll update things to use pusher instead of pusher_config once the pusher has been created.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, I wonder why pushers isn't just a Dict[str, Dict[Tuple[str, str], Pusher], but whatever that's getting into too much refactoring.

byuser[appid_pushkey].on_stop()
byuser[appid_pushkey] = p
previous_pusher = byuser[appid_pushkey]
previous_pusher.on_stop()

synapse_pushers.labels(type(p).__name__, p.app_id).inc()
synapse_pushers.labels(
type(previous_pusher).__name__, previous_pusher.app_id
).dec()
Comment on lines +359 to +361
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do the decrementing than incrementing cancel or can the app_ids be different or...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They ought to cancel most of the time - the app_ids must be the same, otherwise appid_pushkey would be different.

However, I can't convince myself that they always cancel. I'm not sure that type(previous_pusher).__name__ will always be the same as type(pusher).__name__.

byuser[appid_pushkey] = pusher

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
# 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
Expand All @@ -372,9 +377,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)
Expand Down