From ddf77b58df3702984e758e8a9a479508f9e74c32 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 29 Apr 2025 20:10:52 -0500 Subject: [PATCH] Always clear the `event_stats` table when the background update starts So we always get the correct count regardless of how many times the background update is run, --- .../databases/main/events_bg_updates.py | 14 +++++++ tests/metrics/test_phone_home_stats.py | 6 +++ tests/storage/test_event_stats.py | 41 ++++++++++++++----- 3 files changed, 50 insertions(+), 11 deletions(-) diff --git a/synapse/storage/databases/main/events_bg_updates.py b/synapse/storage/databases/main/events_bg_updates.py index 72f34773e1..07b602dd57 100644 --- a/synapse/storage/databases/main/events_bg_updates.py +++ b/synapse/storage/databases/main/events_bg_updates.py @@ -2567,6 +2567,9 @@ class EventsBackgroundUpdatesStore(StreamWorkerStore, StateDeltasStore, SQLBaseS Then we will iterate through the `events` table in batches and update event counts until we reach the stopping point. + It's safe to run this background update multiple times (start with an empty + `progress_json`). + This data is intended to be used by the phone-home stats to keep track of total event and message counts. A trigger is preferred to counting rows in the `events` table, as said table can grow quite large. @@ -2603,6 +2606,17 @@ class EventsBackgroundUpdatesStore(StreamWorkerStore, StateDeltasStore, SQLBaseS The latest event `stream_ordering` in the `events` table when the triggers were added or `None` if the `events` table is empty. """ + # Clear the `event_stats` table so we can start fresh (the background update + # may have been run before) + txn.execute( + """ + UPDATE event_stats + SET + total_event_count = 0, + unencrypted_message_count = 0, + e2ee_event_count = 0 + """ + ) # Each time an event is inserted into the `events` table, update the stats. # diff --git a/tests/metrics/test_phone_home_stats.py b/tests/metrics/test_phone_home_stats.py index 1b3eafed5f..39e41850c6 100644 --- a/tests/metrics/test_phone_home_stats.py +++ b/tests/metrics/test_phone_home_stats.py @@ -100,6 +100,12 @@ class PhoneHomeStatsTestCase(unittest.HomeserverTestCase): """ Perform some actions on the homeserver that would bump the phone home stats. + + This creates a few users, a room, and sends some messages. Expected number of + events: + - 10 unencrypted messages + - 5 encrypted messages + - 24 total events (including room state, etc) """ # Create some users diff --git a/tests/storage/test_event_stats.py b/tests/storage/test_event_stats.py index 791ed27018..a9f94e0b89 100644 --- a/tests/storage/test_event_stats.py +++ b/tests/storage/test_event_stats.py @@ -49,6 +49,12 @@ class EventStatsTestCase(unittest.HomeserverTestCase): def _perform_user_actions(self) -> None: """ Perform some actions on the homeserver that would bump the event counts. + + This creates a few users, a room, and sends some messages. Expected number of + events: + - 10 unencrypted messages + - 5 encrypted messages + - 24 total events (including room state, etc) """ # Create some users user_1_mxid = self.register_user( @@ -144,12 +150,26 @@ class EventStatsTestCase(unittest.HomeserverTestCase): # Do things to bump the stats self._perform_user_actions() - # Keep in mind: These are already populated as the background update has already - # ran once when Synapse started and added the database triggers which are - # incrementing things as new events come in. - self.assertEqual(self.get_success(self.store.count_total_events()), 24) - self.assertEqual(self.get_success(self.store.count_total_messages()), 10) - self.assertEqual(self.get_success(self.store.count_total_e2ee_events()), 5) + # Since the background update has already run once when Synapse started, let's + # manually reset the database `event_stats` back to 0 to ensure this test is + # starting from a clean slate. We want to be able to detect 0 -> 24 instead of + # 24 -> 24 as it's not possible to prove that any work was actually done if the + # number doesn't change. + self.get_success( + self.store.db_pool.simple_update_one( + table="event_stats", + keyvalues={}, + updatevalues={ + "total_event_count": 0, + "unencrypted_message_count": 0, + "e2ee_event_count": 0, + }, + desc="reset event_stats in test preparation", + ) + ) + self.assertEqual(self.get_success(self.store.count_total_events()), 0) + self.assertEqual(self.get_success(self.store.count_total_messages()), 0) + self.assertEqual(self.get_success(self.store.count_total_e2ee_events()), 0) # Run the background update again self.get_success( @@ -164,11 +184,10 @@ class EventStatsTestCase(unittest.HomeserverTestCase): self.store.db_pool.updates._all_done = False self.wait_for_background_updates() - # We expect these values to double as the background update is being run *again* - # and will double-count the `events`. - self.assertEqual(self.get_success(self.store.count_total_events()), 48) - self.assertEqual(self.get_success(self.store.count_total_messages()), 20) - self.assertEqual(self.get_success(self.store.count_total_e2ee_events()), 10) + # Expect our `event_stats` table to be populated with the correct values + self.assertEqual(self.get_success(self.store.count_total_events()), 24) + self.assertEqual(self.get_success(self.store.count_total_messages()), 10) + self.assertEqual(self.get_success(self.store.count_total_e2ee_events()), 5) def test_background_update_without_events(self) -> None: """