Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add option to track MAU stats (but not limit people) #3830

Merged
merged 11 commits into from
Nov 15, 2018
1 change: 1 addition & 0 deletions changelog.d/3830.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add option to track MAU stats (but not limit people)
4 changes: 2 additions & 2 deletions synapse/app/homeserver.py
Original file line number Diff line number Diff line change
Expand Up @@ -532,7 +532,7 @@ def generate_user_daily_visit_stats():
@defer.inlineCallbacks
def generate_monthly_active_users():
count = 0
if hs.config.limit_usage_by_mau:
if hs.config.limit_usage_by_mau or hs.config.mau_stats_only:
count = yield hs.get_datastore().get_monthly_active_count()
current_mau_gauge.set(float(count))
max_mau_gauge.set(float(hs.config.max_mau_value))
Expand All @@ -541,7 +541,7 @@ def generate_monthly_active_users():
hs.config.mau_limits_reserved_threepids
)
generate_monthly_active_users()
if hs.config.limit_usage_by_mau:
if hs.config.limit_usage_by_mau or hs.config.mau_stats_only:
clock.looping_call(generate_monthly_active_users, 5 * 60 * 1000)
# End of monthly active user settings

Expand Down
6 changes: 6 additions & 0 deletions synapse/config/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ def read_config(self, config):
self.max_mau_value = config.get(
"max_mau_value", 0,
)
self.mau_stats_only = config.get("mau_stats_only", False)

self.mau_limits_reserved_threepids = config.get(
"mau_limit_reserved_threepids", []
Expand Down Expand Up @@ -372,6 +373,11 @@ def default_config(self, server_name, **kwargs):
# max_mau_value: 50
# mau_trial_days: 2
#
# If enabled, the metrics for the number of monthly active users will
# be populated, however no one will be limited. If limit_usage_by_mau
# is true, this is implied to be true.
# mau_stats_only: False
#
# Sometimes the server admin will want to ensure certain accounts are
# never blocked by mau checking. These accounts are specified here.
#
Expand Down
72 changes: 38 additions & 34 deletions synapse/storage/monthly_active_users.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,37 +87,38 @@ def _reap_users(txn):

txn.execute(sql, query_args)

# If MAU user count still exceeds the MAU threshold, then delete on
# a least recently active basis.
# Note it is not possible to write this query using OFFSET due to
# incompatibilities in how sqlite and postgres support the feature.
# sqlite requires 'LIMIT -1 OFFSET ?', the LIMIT must be present
# While Postgres does not require 'LIMIT', but also does not support
# negative LIMIT values. So there is no way to write it that both can
# support
safe_guard = self.hs.config.max_mau_value - len(self.reserved_users)
# Must be greater than zero for postgres
safe_guard = safe_guard if safe_guard > 0 else 0
query_args = [safe_guard]

base_sql = """
DELETE FROM monthly_active_users
WHERE user_id NOT IN (
SELECT user_id FROM monthly_active_users
ORDER BY timestamp DESC
LIMIT ?
if self.hs.config.limit_usage_by_mau:
# If MAU user count still exceeds the MAU threshold, then delete on
# a least recently active basis.
# Note it is not possible to write this query using OFFSET due to
# incompatibilities in how sqlite and postgres support the feature.
# sqlite requires 'LIMIT -1 OFFSET ?', the LIMIT must be present
# While Postgres does not require 'LIMIT', but also does not support
# negative LIMIT values. So there is no way to write it that both can
# support
safe_guard = self.hs.config.max_mau_value - len(self.reserved_users)
# Must be greater than zero for postgres
safe_guard = safe_guard if safe_guard > 0 else 0
query_args = [safe_guard]

base_sql = """
DELETE FROM monthly_active_users
WHERE user_id NOT IN (
SELECT user_id FROM monthly_active_users
ORDER BY timestamp DESC
LIMIT ?
)
"""
# Need if/else since 'AND user_id NOT IN ({})' fails on Postgres
# when len(reserved_users) == 0. Works fine on sqlite.
if len(self.reserved_users) > 0:
query_args.extend(self.reserved_users)
sql = base_sql + """ AND user_id NOT IN ({})""".format(
','.join(questionmarks)
)
"""
# Need if/else since 'AND user_id NOT IN ({})' fails on Postgres
# when len(reserved_users) == 0. Works fine on sqlite.
if len(self.reserved_users) > 0:
query_args.extend(self.reserved_users)
sql = base_sql + """ AND user_id NOT IN ({})""".format(
','.join(questionmarks)
)
else:
sql = base_sql
txn.execute(sql, query_args)
else:
sql = base_sql
txn.execute(sql, query_args)

yield self.runInteraction("reap_monthly_active_users", _reap_users)
# It seems poor to invalidate the whole cache, Postgres supports
Expand Down Expand Up @@ -199,8 +200,7 @@ def populate_monthly_active_users(self, user_id):
Args:
user_id(str): the user_id to query
"""

if self.hs.config.limit_usage_by_mau:
if self.hs.config.limit_usage_by_mau or self.hs.config.mau_stats_only:
# Trial users and guests should not be included as part of MAU group
is_guest = yield self.is_guest(user_id)
if is_guest:
Expand All @@ -218,8 +218,12 @@ def populate_monthly_active_users(self, user_id):
# but only update if we have not previously seen the user for
# LAST_SEEN_GRANULARITY ms
if last_seen_timestamp is None:
count = yield self.get_monthly_active_count()
if count < self.hs.config.max_mau_value:
# Optimize the db usage when not limiting usage
Copy link
Contributor

Choose a reason for hiding this comment

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

I had to look at it a few times to understand the intent - I'm not sure I can suggest a more expressive way to do via code, but a more descriptive comment would be helpful.

I'm not sure it will necessarily help a great deal with db load since get_monthly_active_count is cached but the logic expects max_mau_value to be set and so I agree that it needs the if check.

Perhaps something like

"In the case where mau_stats_only is True and limit_usage_by_mau is False, there is no point in checking get_monthly_active_count - it adds no value and will break the logic if max_mau_value is exceeded "

Copy link
Member

Choose a reason for hiding this comment

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

I had to look at it a few times to understand the intent

(I had the same reaction tbh)

if not self.hs.config.limit_usage_by_mau:
yield self.upsert_monthly_active_user(user_id)
else:
count = yield self.get_monthly_active_count()
if count < self.hs.config.max_mau_value:
yield self.upsert_monthly_active_user(user_id)
elif now - last_seen_timestamp > LAST_SEEN_GRANULARITY:
yield self.upsert_monthly_active_user(user_id)