Initial impl of capping MAU#3630
Conversation
| log in. | ||
| """ | ||
| if self.hs.config.limit_usage_by_mau is True: | ||
| current_mau = self.store.count_monthly_users() |
There was a problem hiding this comment.
This method will be hit very often. So you might want to either add some cache on store.count_monthly_users() or use a value which might be generated by generate_monthly_active_users
There was a problem hiding this comment.
agreed though deliberately omitted since count_monthly_users() is really only a stepping stone method to get the rest of the infra out. In current form only called regularly in the case where the config is enabled.
There was a problem hiding this comment.
ok. If that is on your radar I am fine with it for now
| action="join", | ||
| ) | ||
|
|
||
| def _check_mau_limits(self): |
There was a problem hiding this comment.
you might want to deduplicate this somehow
There was a problem hiding this comment.
same as above, and also the calculating the total count is duped with the daily count - but likley to change structure
erikjohnston
changed the title
| max_mau_value_gauge = Gauge("synapse_admin_max_mau_value", "MAU Limit") | ||
| limit_usage_by_mau_gauge = Gauge( | ||
| "synapse_admin_limit_usage_by_mau", "MAU Limiting enabled" | ||
| ) |
There was a problem hiding this comment.
I would probably not bother having limit_usage_by_mau_gauge, and simply set it to 0 or null if limit isn't enabled
| limit_usage_by_mau_gauge.set(float(hs.config.limit_usage_by_mau)) | ||
|
|
||
| generate_monthly_active_users() | ||
| clock.looping_call(generate_monthly_active_users, 5 * 60 * 1000) |
There was a problem hiding this comment.
Does it make sense for this to be done if hs.config.limit_usage_by_mau is false?
There was a problem hiding this comment.
right yes, need to be called once but the looping is a waste
| count, = txn.fetchone() | ||
| return count | ||
| finally: | ||
| txn.close() |
There was a problem hiding this comment.
This should either accept a txn argument or use runInteraction, otherwise its going to be too easy to call this on the main thread (which is bad as this method blocks)
There was a problem hiding this comment.
I was originally trying to keep things simple without deferring - will make the change
| def generate_monthly_active_users(): | ||
| count = 0 | ||
| if hs.config.limit_usage_by_mau: | ||
| count = hs.get_datastore().count_monthly_users() |
There was a problem hiding this comment.
count_monthly_users will need to be asynchronous and return a deferred, which we'll need to yield
There was a problem hiding this comment.
agreed as above
|
review items implemented |
…u_sign_in_log_in_limits
erikjohnston
left a comment
erikjohnston
left a comment
There was a problem hiding this comment.
Just needs to have the yield added, and then its good to go
| @@ -0,0 +1 @@ | |||
| Blocks registration and sign in if max mau value exceeded | |||
There was a problem hiding this comment.
Sorry, can we quickly change this to "Add ability to limit number of monthly active users on the server", or some such. Otherwise its going to be confusing for people to read the changelog who haven't had the full context.
| def generate_monthly_active_users(): | ||
| count = 0 | ||
| if hs.config.limit_usage_by_mau: | ||
| count = hs.get_datastore().count_monthly_users() |
| This method should be refactored with count_daily_users - the only | ||
| reason not to is waiting on definition of mau | ||
| returns: | ||
| defered: resolves to int |
There was a problem hiding this comment.
FWIW, we generally start the docstring on the sameline of the quotes, so something like:
"""Counts the number of users who used this homeserver in the last 30 days.
This method should be refactored with count_daily_users - the only
reason not to is waiting on definition of MAU.
Returns:
Deferred[int]
"""And that's also how you annotate return types (such that IDEs correctly interpret it)
3 participants
Tracks Mau naiving by count(*) on user_ips.
Blocks registration and sign in if value exceeded
Publishes metrics for consumption by Prometheus
The idea here is to cap the monthly active users (MAU) on a server. This is an initial cut that simply stops registration and login for if MAU is above configured threshold - @erikjohnston