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

Option to suppress resource exceeded alerting #6173

Merged
merged 19 commits into from Oct 24, 2019

Conversation

@neilisfragile
Copy link
Contributor

neilisfragile commented Oct 6, 2019

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

…sable-mau-alerting-for-small-instances
@neilisfragile neilisfragile marked this pull request as ready for review Oct 11, 2019
@neilisfragile neilisfragile requested a review from matrix-org/synapse-core Oct 11, 2019
@neilisfragile 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 neilisfragile added this to In progress in Homeserver Task Board via automation Oct 11, 2019
@neilisfragile neilisfragile self-assigned this Oct 11, 2019
@neilisfragile neilisfragile changed the title Config option to suppress server resource limited exceeded. Option to suppress resource exceeded alerting Oct 11, 2019
synapse/config/server.py Outdated Show resolved Hide resolved
@neilisfragile neilisfragile requested a review from anoadragon453 Oct 11, 2019
Copy link
Member

richvdh left a comment

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

@@ -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:

This comment has been minimized.

Copy link
@richvdh

richvdh Oct 17, 2019

Member

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).

This comment has been minimized.

Copy link
@neilisfragile

neilisfragile Oct 18, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@richvdh

richvdh Oct 22, 2019

Member

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

This comment has been minimized.

Copy link
@neilisfragile

neilisfragile Oct 22, 2019

Author Contributor

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

@neilisfragile neilisfragile requested a review from richvdh Oct 18, 2019
…sable-mau-alerting-for-small-instances
@neilisfragile neilisfragile dismissed richvdh’s stale review Oct 22, 2019

points addressed

Copy link
Member

richvdh left a comment

couple more tweaks

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

This comment has been minimized.

Copy link
@richvdh

richvdh Oct 22, 2019

Member

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?

This comment has been minimized.

Copy link
@neilisfragile

neilisfragile Oct 22, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@neilisfragile

neilisfragile Oct 22, 2019

Author Contributor

fixed

This comment has been minimized.

Copy link
@neilisfragile

neilisfragile Oct 23, 2019

Author Contributor

unfixed, will address in a separate PR.

return

is_auth_blocking = False
if self._config.mau_limit_alerting:

This comment has been minimized.

Copy link
@richvdh

richvdh Oct 22, 2019

Member

this condition is redundant, it will always be True

@@ -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:

This comment has been minimized.

Copy link
@richvdh

richvdh Oct 22, 2019

Member

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

@richvdh

This comment has been minimized.

Copy link
Member

richvdh commented Oct 22, 2019

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?

neilisfragile and others added 5 commits Oct 22, 2019
Co-Authored-By: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
…ub.com:matrix-org/synapse into neilj/disable-mau-alerting-for-small-instances
@neilisfragile

This comment has been minimized.

Copy link
Contributor Author

neilisfragile commented Oct 22, 2019

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
@neilisfragile neilisfragile requested a review from richvdh Oct 23, 2019
@richvdh

This comment has been minimized.

Copy link
Member

richvdh commented Oct 23, 2019

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?


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

This comment has been minimized.

Copy link
@richvdh

richvdh Oct 23, 2019

Member

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
if not self._config.mau_limit_alerting:
# Alerting disabled, reset room if necessary and return
is_auth_blocking = False
event_limit_type = ""

This comment has been minimized.

Copy link
@richvdh

richvdh Oct 23, 2019

Member

None would be a more conventional sentinel here

Suggested change
event_limit_type = ""
event_limit_type = None

This comment has been minimized.

Copy link
@richvdh

richvdh Oct 23, 2019

Member

shouldn't it be called limit_type rather than event_limit_type?

@@ -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 comment has been minimized.

Copy link
@richvdh

richvdh Oct 23, 2019

Member

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.

richvdh added 2 commits Oct 24, 2019
@richvdh richvdh merged commit 2794b79 into develop Oct 24, 2019
18 checks passed
18 checks passed
buildkite/synapse Build #5092 passed (19 minutes, 51 seconds)
Details
buildkite/synapse/check-sample-config Passed (1 minute, 25 seconds)
Details
buildkite/synapse/check-style Passed (1 minute, 38 seconds)
Details
buildkite/synapse/isort Passed (18 seconds)
Details
buildkite/synapse/mypy Passed (22 seconds)
Details
buildkite/synapse/newspaper-newsfile Passed (14 seconds)
Details
buildkite/synapse/packaging Passed (18 seconds)
Details
buildkite/synapse/pipeline Passed (4 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-postgres-9-dot-5 Passed (17 minutes, 52 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-sqlite Passed (6 minutes, 42 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-sqlite-slash-old-deps Passed (8 minutes, 42 seconds)
Details
buildkite/synapse/python-3-dot-6-slash-sqlite Passed (6 minutes, 43 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-11 Passed (17 minutes, 5 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-9-dot-5 Passed (17 minutes, 31 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-sqlite Passed (6 minutes, 27 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-postgres-9-dot-6-slash-monolith Passed (14 minutes, 28 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-postgres-9-dot-6-slash-workers Passed (14 minutes, 54 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-sqlite-slash-monolith Passed (15 minutes, 22 seconds)
Details
Homeserver Task Board automation moved this from In progress to Done Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4 participants
You can’t perform that action at this time.