From 8b2375c0f2f25c18b3dae9396da4ea4b3e9d7685 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Sat, 8 Sep 2018 19:36:53 -0600 Subject: [PATCH 1/7] Add option to track MAU stats (but not limit people) --- synapse/app/homeserver.py | 4 ++-- synapse/config/server.py | 6 ++++++ synapse/storage/monthly_active_users.py | 11 +++++++---- 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py index 3eb5b663de5b..7f724bc11488 100755 --- a/synapse/app/homeserver.py +++ b/synapse/app/homeserver.py @@ -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)) @@ -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 diff --git a/synapse/config/server.py b/synapse/config/server.py index c1c7c0105e41..5ff9ac288daa 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -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", [] @@ -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. # diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py index b890c152dbec..e1aa5e3e3814 100644 --- a/synapse/storage/monthly_active_users.py +++ b/synapse/storage/monthly_active_users.py @@ -199,8 +199,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: @@ -218,8 +217,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 + 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) From 45d35b29bc1f6c329f7d8898e02c526f1cd74fcc Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Sat, 8 Sep 2018 20:21:11 -0600 Subject: [PATCH 2/7] changelog --- changelog.d/3830.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/3830.feature diff --git a/changelog.d/3830.feature b/changelog.d/3830.feature new file mode 100644 index 000000000000..af472cf7635c --- /dev/null +++ b/changelog.d/3830.feature @@ -0,0 +1 @@ +Add option to track MAU stats (but not limit people) From 4b44fa0031d4784695d569078ecb29b14bd8b56e Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Sat, 8 Sep 2018 20:26:28 -0600 Subject: [PATCH 3/7] Only reap to the MAU limit if there's a limit to use --- synapse/storage/monthly_active_users.py | 61 +++++++++++++------------ 1 file changed, 31 insertions(+), 30 deletions(-) diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py index e1aa5e3e3814..ba0274658db4 100644 --- a/synapse/storage/monthly_active_users.py +++ b/synapse/storage/monthly_active_users.py @@ -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 From 33e78aec3ce89a4feaee19884c1e3d029e45e990 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 18 Sep 2018 15:11:44 -0600 Subject: [PATCH 4/7] Hopefully fix MAU unit tests --- tests/utils.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/utils.py b/tests/utils.py index 215226debfec..fb5c58caa7aa 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -153,6 +153,7 @@ def setup_test_homeserver( config.hs_disabled_limit_type = "" config.max_mau_value = 50 config.mau_trial_days = 0 + config.mau_stats_only = False config.mau_limits_reserved_threepids = [] config.admin_contact = None config.rc_messages_per_second = 10000 From 4326386e15bcc8259a825d4b008e77eea1c18f19 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 15 Oct 2018 22:39:20 -0600 Subject: [PATCH 5/7] MAU tracking tests --- synapse/storage/monthly_active_users.py | 4 +++- tests/storage/test_monthly_active_users.py | 25 ++++++++++++++++++++++ tests/test_mau.py | 17 +++++++++++++++ 3 files changed, 45 insertions(+), 1 deletion(-) diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py index 594a2fe6e72e..375d31937d30 100644 --- a/synapse/storage/monthly_active_users.py +++ b/synapse/storage/monthly_active_users.py @@ -238,7 +238,9 @@ 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: - # Optimize the db usage when not limiting usage + # 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 valid and will break the logic if max_mau_value is exceeded. if not self.hs.config.limit_usage_by_mau: yield self.upsert_monthly_active_user(user_id) else: diff --git a/tests/storage/test_monthly_active_users.py b/tests/storage/test_monthly_active_users.py index 686f12a0dcdc..329a752f69a5 100644 --- a/tests/storage/test_monthly_active_users.py +++ b/tests/storage/test_monthly_active_users.py @@ -214,3 +214,28 @@ def test_get_reserved_real_user_account(self): self.store.user_add_threepid(user2, "email", user2_email, now, now) count = self.store.get_registered_reserved_users_count() self.assertEquals(self.get_success(count), len(threepids)) + + def test_track_monthly_users_without_cap(self): + self.hs.config.limit_usage_by_mau = False + self.hs.config.mau_stats_only = True + self.hs.config.max_mau_value = 1 # should not matter + + count = self.store.get_monthly_active_count() + self.assertEqual(0, self.get_success(count)) + + self.store.upsert_monthly_active_user("@user1:server") + self.store.upsert_monthly_active_user("@user2:server") + self.pump() + + count = self.store.get_monthly_active_count() + self.assertEqual(2, self.get_success(count)) + + def test_no_users_when_not_tracking(self): + self.hs.config.limit_usage_by_mau = False + self.hs.config.mau_stats_only = False + self.store.upsert_monthly_active_user = Mock() + + self.store.populate_monthly_active_users("@user:sever") + self.pump() + + self.store.upsert_monthly_active_user.assert_not_called() diff --git a/tests/test_mau.py b/tests/test_mau.py index bdbacb844848..4d493582b1e2 100644 --- a/tests/test_mau.py +++ b/tests/test_mau.py @@ -184,6 +184,23 @@ def test_trial_users_cant_come_back(self): self.assertEqual(e.code, 403) self.assertEqual(e.errcode, Codes.RESOURCE_LIMIT_EXCEEDED) + def test_tracked_but_not_limited(self): + self.hs.config.max_mau_value = 1 # should not matter + self.hs.config.limit_usage_by_mau = False + self.hs.config.mau_stats_only = True + + # Simply being able to create 2 users indicates that the + # limit was not reached. + token1 = self.create_user("kermit1") + self.do_sync_for_user(token1) + token2 = self.create_user("kermit2") + self.do_sync_for_user(token2) + + # We do want to verify that the number of tracked users + # matches what we want though + count = self.store.get_monthly_active_count() + self.assertEqual(2, self.get_success(count)) + def create_user(self, localpart): request_data = json.dumps( { From ab4616c7875682da48ae07613f321fc78ef2eef4 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Mon, 15 Oct 2018 22:50:31 -0600 Subject: [PATCH 6/7] Fix tests --- tests/test_mau.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_mau.py b/tests/test_mau.py index 4d493582b1e2..3466d1796ca4 100644 --- a/tests/test_mau.py +++ b/tests/test_mau.py @@ -199,7 +199,8 @@ def test_tracked_but_not_limited(self): # We do want to verify that the number of tracked users # matches what we want though count = self.store.get_monthly_active_count() - self.assertEqual(2, self.get_success(count)) + self.reactor.advance(100) + self.assertEqual(2, self.successResultOf(count)) def create_user(self, localpart): request_data = json.dumps( From 7f29eb25bc030d0611bbc50c15308c7b496bfbcc Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 16 Oct 2018 09:33:04 -0600 Subject: [PATCH 7/7] Fix typo --- synapse/storage/monthly_active_users.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py index 375d31937d30..894339f231eb 100644 --- a/synapse/storage/monthly_active_users.py +++ b/synapse/storage/monthly_active_users.py @@ -240,7 +240,7 @@ def populate_monthly_active_users(self, user_id): if last_seen_timestamp is None: # 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 valid and will break the logic if max_mau_value is exceeded. + # adds no value and will break the logic if max_mau_value is exceeded. if not self.hs.config.limit_usage_by_mau: yield self.upsert_monthly_active_user(user_id) else: