Synchrotrons always wake up sync streams for presence updates

[erikjohnston]

When calculating presence changes we decide whether we should actively wake up sync streams, or if they should just be bundled up the next time we send a response for another reason. We do not appear to propagate whether to wake up sync streams across replication, and we appear to always notify for presence updates:

synapse/synapse/app/generic_worker.py

Lines 381 to 390 in 6d02eb2

	async def notify_from_replication(self, states, stream_id):
	parties = await get_interested_parties(self.store, states)
	room_ids_to_states, users_to_states = parties
	self.notifier.on_new_event(
	"presence_key",
	stream_id,
	rooms=room_ids_to_states.keys(),
	users=users_to_states.keys(),
	)

It's unclear how much this will help.

(Note that we batch up presence updates that we don't notify for and persist/replicate them once a minute).

[realtyem]

I think I found a great spot to poke this to model this behavior. However, I also think I need a little more information for my testing branch before I offer a fix.

(I also need to make sure my assumption that the notifier from presence is called with a on_new_event() and the sync handler picks up with wait_for_events(), if this isn't right please let me know.)

The code for should_notify()...

synapse/synapse/handlers/presence.py

Lines 1502 to 1535 in 78cfa55

	def should_notify(old_state: UserPresenceState, new_state: UserPresenceState) -> bool:
	"""Decides if a presence state change should be sent to interested parties."""
	if old_state == new_state:
	return False
	if old_state.status_msg != new_state.status_msg:
	notify_reason_counter.labels("status_msg_change").inc()
	return True
	if old_state.state != new_state.state:
	notify_reason_counter.labels("state_change").inc()
	state_transition_counter.labels(old_state.state, new_state.state).inc()
	return True
	if old_state.state == PresenceState.ONLINE:
	if new_state.currently_active != old_state.currently_active:
	notify_reason_counter.labels("current_active_change").inc()
	return True
	if (
	new_state.last_active_ts - old_state.last_active_ts
	> LAST_ACTIVE_GRANULARITY
	):
	# Only notify about last active bumps if we're not currently active
	if not new_state.currently_active:
	notify_reason_counter.labels("last_active_change_online").inc()
	return True
	elif new_state.last_active_ts - old_state.last_active_ts > LAST_ACTIVE_GRANULARITY:
	# Always notify for a transition where last active gets bumped.
	notify_reason_counter.labels("last_active_change_not_online").inc()
	return True
	return False

Which condition doesn't pass muster? Quick summary of what it looks like:
Notify for:

• if status message changed
• if the state changed for any reason
• if user came online
  • if currently active changed
  • if last active difference is less than 60 seconds AND not currently active

Do nothing for:

• if state hasn't changed
• anything else

Keeping in mind, some unit tests may have to be adjusted based on how aggressively this changes. Complement tests should not, as they should be 'correct' behavior based on spec.

For Monolith deployments:
It currently appears that the notifier is poked inside _persist_and_notify() but the decision to actually send that notification is inside should_notify(). Both of these are inside _update_states().

For Worker deployments:
It currently appears that the notifier is poked inside notify_from_replication() however before that function is called is a should_notify() inside process_replication_rows().

So, it looks like should_notify() is the best place to group these together for like decisions.

We do not appear to propagate whether to wake up sync streams across replication, and we appear to always notify for presence updates

For this part, I think this may have been done?