Option to suppress resource exceeded alerting #6173

neilisfragile · 2019-10-06T13:49:16Z

The expected use case is to suppress MAU limiting on small instances

…sable-mau-alerting-for-small-instances

synapse/config/server.py

richvdh

I'm struggling to follow this, I'm afraid.

richvdh · 2019-10-17T13:15:56Z

synapse/server_notices/resource_limits_server_notices.py

@@ -91,6 +91,15 @@ def maybe_send_server_notice_to_user(self, user_id):
        currently_blocked, ref_events = yield self._is_room_currently_blocked(room_id)

        try:
+            if not self._config.mau_limit_alerting:


the big stack of if statements in this function is getting really hard to follow, and I'm worried that the duplicated code will get out of sync.

I think it would be clearer to make the check_auth_blocking call (ie, everything between lines 103 and 112 as it currently stands) conditional on mau_limit_alerting (leaving is_auth_blocking = False if mau_limit_alerting is false).

@richvdh I've had a go a refactoring the whole area, I think it is now easier to read, albeit still inherently fiddly.

I guess you didn't agree with my suggestion? Fair enough. It's certainly clearer now.

For reference https://gist.github.com/richvdh/092934e994a2443bd184a01357c98677 Agreed that proposal is neater, but less explicit about the blocking behaviour, so will not adopt.

…sable-mau-alerting-for-small-instances

points addressed

richvdh

couple more tweaks

richvdh · 2019-10-22T11:08:25Z

synapse/server_notices/resource_limits_server_notices.py

+            ref_events ([]): The list of pinned events that are unrelated to
+            limit blocking and need to be preserved.
+        """
+        content = {"pinned": ref_events}


this seems to be an existing problem (so not necessarily for fixing as part of this PR), but why do we preserve the other events in this case and not in the _apply_limit_block_notification case?

From memory the idea was to shut everything down and so all tags should be discarded, but in retrospect this seems unhelpful.

unfixed, will address in a separate PR.

synapse/server_notices/resource_limits_server_notices.py

richvdh · 2019-10-22T11:13:54Z

synapse/server_notices/resource_limits_server_notices.py

+                return
+
+            is_auth_blocking = False
+            if self._config.mau_limit_alerting:


this condition is redundant, it will always be True

richvdh · 2019-10-22T11:14:33Z

synapse/server_notices/resource_limits_server_notices.py

@@ -91,6 +91,15 @@ def maybe_send_server_notice_to_user(self, user_id):
        currently_blocked, ref_events = yield self._is_room_currently_blocked(room_id)

        try:
+            if not self._config.mau_limit_alerting:


I guess you didn't agree with my suggestion? Fair enough. It's certainly clearer now.

richvdh · 2019-10-22T11:15:36Z

Unrelated to the change at hand, but isn't this thing massively racy? if two calls maybe_send_server_notice_to_user happen at once, we might get several copies of the server notice?

Co-Authored-By: Richard van der Hoff <[email protected]>

…ub.com:matrix-org/synapse into neilj/disable-mau-alerting-for-small-instances

neilisfragile · 2019-10-22T16:33:26Z

Unrelated to the change at hand, but isn't this thing massively racy? if two calls maybe_send_server_notice_to_user happen at once, we might get several copies of the server notice?

I can't think when multiple overlapping calls would occur, but if they did I think it would result in a more confusing dag but continue to work as expected.

…sable-mau-alerting-for-small-instances

addressed

richvdh · 2019-10-23T21:55:44Z

I can't think when multiple overlapping calls would occur

the user syncs from two devices at once?

but if they did I think it would result in a more confusing dag but continue to work as expected.

wouldn't there be two copies of the server notice?

richvdh · 2019-10-23T22:00:34Z

synapse/server_notices/resource_limits_server_notices.py

+
+            if (
+                not self._config.mau_limit_alerting
+                and event_limit_type is LimitBlockingTypes.MONTHLY_ACTIVE_USER


use == not is except when checking for None

Suggested change

and event_limit_type is LimitBlockingTypes.MONTHLY_ACTIVE_USER

and event_limit_type == LimitBlockingTypes.MONTHLY_ACTIVE_USER

richvdh · 2019-10-23T22:01:57Z

synapse/server_notices/resource_limits_server_notices.py

-            if not self._config.mau_limit_alerting:
-                # Alerting disabled, reset room if necessary and return
+            is_auth_blocking = False
+            event_limit_type = ""


None would be a more conventional sentinel here

Suggested change

event_limit_type = ""

event_limit_type = None

shouldn't it be called limit_type rather than event_limit_type?

richvdh · 2019-10-23T22:03:48Z

synapse/server_notices/resource_limits_server_notices.py

@@ -91,24 +92,28 @@ def maybe_send_server_notice_to_user(self, user_id):
        currently_blocked, ref_events = yield self._is_room_currently_blocked(room_id)

        try:
-            if not self._config.mau_limit_alerting:
-                # Alerting disabled, reset room if necessary and return
+            is_auth_blocking = False


this feels pretty redundant - indeed, you don't bother to check it at line 110. Let's just get rid of it and use event_limit_type instead.

* commit '2794b7905': Option to suppress resource exceeded alerting (#6173)

neilisfragile and others added 5 commits October 4, 2019 17:53

wip commit to suppress mau alerting for small instances

11ccbc6

switch config to a bool and write tests

e4e4364

Merge branch 'develop' of github.com:matrix-org/synapse into neilj/di…

bd8974c

…sable-mau-alerting-for-small-instances

add mau_suppress_alerting

7716de8

black and towncrier

00ae8b7

neilisfragile marked this pull request as ready for review October 11, 2019 13:06

neilisfragile requested a review from a team October 11, 2019 13:06

neilisfragile changed the title ~~wip commit to suppress mau alerting for small instances~~ Config option to suppress server resource limited exceeded. Oct 11, 2019

neilisfragile self-assigned this Oct 11, 2019

neilisfragile changed the title ~~Config option to suppress server resource limited exceeded.~~ Option to suppress resource exceeded alerting Oct 11, 2019

anoadragon453 reviewed Oct 11, 2019

View reviewed changes

synapse/config/server.py Outdated Show resolved Hide resolved

respond to review comments

dc8a2d3

neilisfragile requested a review from anoadragon453 October 11, 2019 14:57

richvdh previously requested changes Oct 17, 2019

View reviewed changes

refactor for clarity

49b4f96

neilisfragile requested a review from richvdh October 18, 2019 12:37

Merge branch 'develop' of github.com:matrix-org/synapse into neilj/di…

d186657

…sable-mau-alerting-for-small-instances

richvdh previously requested changes Oct 22, 2019

View reviewed changes

neilisfragile and others added 5 commits October 22, 2019 12:38

Update synapse/server_notices/resource_limits_server_notices.py

8adab51

Co-Authored-By: Richard van der Hoff <[email protected]>

ensure that suppresion affects only mau case

31dc1a0

Merge branch 'neilj/disable-mau-alerting-for-small-instances' of gith…

1e1afcc

…ub.com:matrix-org/synapse into neilj/disable-mau-alerting-for-small-instances

preserve other pinned events on applying block

7b83fcb

isort

9df32f3

neilisfragile added 4 commits October 23, 2019 10:15

back ref_events change

84ca716

fix broken method call

097d2d7

merge conflicts

87ff9f0

Merge branch 'develop' of github.com:matrix-org/synapse into neilj/di…

5380b08

…sable-mau-alerting-for-small-instances

neilisfragile requested a review from richvdh October 23, 2019 16:31

richvdh suggested changes Oct 23, 2019

View reviewed changes

richvdh added 2 commits October 24, 2019 10:51

Address review comments and final cleanups

1b1b2a2

fix test failures

04ec482

richvdh approved these changes Oct 24, 2019

View reviewed changes

richvdh merged commit 2794b79 into develop Oct 24, 2019

anoadragon453 mentioned this pull request Feb 1, 2021

Namespace (and possibly spec) HS_DISABLED error code #9237

Open

babolivier pushed a commit that referenced this pull request Sep 1, 2021

Option to suppress resource exceeded alerting (#6173)

759e8dd

* commit '2794b7905': Option to suppress resource exceeded alerting (#6173)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Option to suppress resource exceeded alerting #6173

Option to suppress resource exceeded alerting #6173

neilisfragile commented Oct 6, 2019 •

edited

Loading

richvdh left a comment

richvdh Oct 17, 2019

neilisfragile Oct 18, 2019

richvdh Oct 22, 2019

neilisfragile Oct 22, 2019

richvdh left a comment

richvdh Oct 22, 2019

neilisfragile Oct 22, 2019

neilisfragile Oct 22, 2019

neilisfragile Oct 23, 2019

richvdh Oct 22, 2019

richvdh Oct 22, 2019

richvdh commented Oct 22, 2019

neilisfragile commented Oct 22, 2019

richvdh commented Oct 23, 2019

richvdh Oct 23, 2019

richvdh Oct 23, 2019

richvdh Oct 23, 2019

richvdh Oct 23, 2019

	and event_limit_type is LimitBlockingTypes.MONTHLY_ACTIVE_USER
	and event_limit_type == LimitBlockingTypes.MONTHLY_ACTIVE_USER

Option to suppress resource exceeded alerting #6173

Option to suppress resource exceeded alerting #6173

Conversation

neilisfragile commented Oct 6, 2019 • edited Loading

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh commented Oct 22, 2019

neilisfragile commented Oct 22, 2019

richvdh commented Oct 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neilisfragile commented Oct 6, 2019 •

edited

Loading