Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MSC3926: Disable server-default notifications for bot users by default #3926

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Half-Shot
Copy link
Contributor

@Half-Shot Half-Shot commented Nov 8, 2022

@turt2live turt2live added push proposal A matrix spec change proposal client-server Client-Server API kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. application services labels Nov 8, 2022
Comment on lines +6 to +14
In the case of the latter two, it's not useful to calculate notifications because the majority of
application services and bots do not read notifications.

Notifications are "cleared" when a read receipt is sent into the room, but neither bots or application
services send read receipts in all cases. This can very easily lead to a lot of wasted disk space in some
implementations.

There is also the case that each notitification rule will take some amount of CPU time to process, which
is entirely wasted if the account doesn't check notifications.
Copy link
Contributor

@clokep clokep Nov 8, 2022

Choose a reason for hiding this comment

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

Combine the 1st & 3rd paragraph since they're both about generating notifications:

Suggested change
In the case of the latter two, it's not useful to calculate notifications because the majority of
application services and bots do not read notifications.
Notifications are "cleared" when a read receipt is sent into the room, but neither bots or application
services send read receipts in all cases. This can very easily lead to a lot of wasted disk space in some
implementations.
There is also the case that each notitification rule will take some amount of CPU time to process, which
is entirely wasted if the account doesn't check notifications.
In the case of the latter two, it's not useful to calculate notifications because the majority of
application services and bots do not read notifications. This leads to wasted processing time
as the homeserver will evaluate push rules to generate notification counts which will never
be read.
Notifications are "cleared" when a read receipt is sent into the room, but neither bots nor application
services send read receipts in all cases. This can easily lead toa wasted disk space or performance
issues if notifications are never cleared.

Comment on lines +27 to +28
When the `type` of registration is `m.login.application_service`, the default value of `enable_predefined_push_rules`
will be `false`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not backwards compatible, technically.

Comment on lines +24 to +25
When set, the homeserver MUST disable all rules defined in [predefined-rules](https://spec.matrix.org/v1.4/client-server-api/#predefined-rules),
effectively thereby disabling the calculation of notifications. The user will still be able to enable these notification rules if they so wish.
Copy link
Contributor

@clokep clokep Nov 8, 2022

Choose a reason for hiding this comment

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

Suggested change
When set, the homeserver MUST disable all rules defined in [predefined-rules](https://spec.matrix.org/v1.4/client-server-api/#predefined-rules),
effectively thereby disabling the calculation of notifications. The user will still be able to enable these notification rules if they so wish.
When set, the homeserver MUST disable all [predefined push rules](https://spec.matrix.org/v1.4/client-server-api/#predefined-rules)
for this user by enabling [the `.m.rule.master` rule](https://spec.matrix.org/v1.4/client-server-api/#default-override-rules). The user will still be able
to enable these notification rules if they so wish.

Comment on lines +43 to +46
Instead of adding a flag, the homeserver could instead only start tracking notifications when the user
/syncs. This might help with cases where users are registered but never used. It would also help with
application services where users never sync. However, this would still cause notifications to be calculated
for traditional `/sync`ing bots. Overall an explicit option at registration time seems more preferable.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Instead of adding a flag, the homeserver could instead only start tracking notifications when the user
/syncs. This might help with cases where users are registered but never used. It would also help with
application services where users never sync. However, this would still cause notifications to be calculated
for traditional `/sync`ing bots. Overall an explicit option at registration time seems more preferable.
Instead of adding a flag, the homeserver could instead only start tracking notifications when the user
makes a `/sync` request. This might help with cases where users are registered but never used. It would also help with
application services where users never sync. However, this would still cause notifications to be calculated
for traditional `/sync`ing bots. Overall an explicit option at registration time seems more preferable.


### Bots and Appservices could explicitly disable notifications

Bots and integrations could instead explicitly disable all rules on signup, rather than expecting the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Bots and integrations could instead explicitly disable all rules on signup, rather than expecting the
Bots and integrations could instead [explicitly enable the `.m.rule.master`](https://spec.matrix.org/v1.4/client-server-api/#predefined-rules)
rule on signup via a [`PUT /pushrules/global/override/.m.rule.master` request](https://spec.matrix.org/v1.4/client-server-api/#put_matrixclientv3pushrulesscopekindruleid),
rather than expecting the


## Unstable prefix

While this MSC is unstable, `enable_predefined_push_rules` should be called `org.matrix.mscXXXX.enable_predefined_push_rules`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
While this MSC is unstable, `enable_predefined_push_rules` should be called `org.matrix.mscXXXX.enable_predefined_push_rules`.
While this MSC is unstable, `enable_predefined_push_rules` should be called `org.matrix.msc3926.enable_predefined_push_rules`.

@@ -0,0 +1,74 @@
# MSC3926: Disable server-default notifications for bot users by default
Copy link
Member

Choose a reason for hiding this comment

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

fwiw, from title alone: this would be a very welcome change. t2bot.io, which offers largely bridge traffic, has disabled push processing on synapse to speed up event handling paths - it feels like a natural next step to improve (at least) CPU usage for other massive bridge deployments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understood from the Synapse team that they check users against the AS namespaces and short-circuit notification checks entirely. While that's fine, it's a bit divergent from the spec and we should seek to make it explicit/official.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
application services client-server Client-Server API kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal push
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Users registered by an appservice should not have server-default notifications enabled.
3 participants