Add debouncing support to UserActivityTracker (#448)

* Add debouncing support to UserActivityTracker * Change UserActivitySet to be a proper Map * Fixup the tests for 141695f * Fix UserActivityState.changed population when debouncing updates * Linting * Pick 1 minute as a sensible debouncing default * Changelog Co-authored-by: Tadeusz Sośnierz <[email protected]>
matrix-org · Nov 30, 2022 · 8a782a1 · 8a782a1
1 parent 04e323a
commit 8a782a1
Show file tree

Hide file tree

Showing 4 changed files with 87 additions and 61 deletions.
diff --git a/changelog.d/448.feature b/changelog.d/448.feature
@@ -0,0 +1 @@
+Add debouncing support to UserActivityTracker (defaults to 60 seconds).
diff --git a/spec/unit/userActivity.spec.js b/spec/unit/userActivity.spec.js
@@ -9,13 +9,17 @@ const USER_THREE = "@charlie:example.com";
 const ONE_DAY = 24 * 60 * 60 * 1000;
 
 describe("userActivity", () => {
-    const emptyDataSet = () => { return { users: {} } };
+    const emptyDataSet = () => { return new Map() };
+    const instantConfig = {
+        ...UserActivityTrackerConfig.DEFAULT,
+        debounceTimeMs: 0,
+    };
     describe("updateUserActivity", () => {
         it("can update a user's activity", async () => {
             let userData;
             const trackerPromise = new Promise((resolve, _) => {
                 const tracker = new UserActivityTracker(
-                    UserActivityTrackerConfig.DEFAULT,
+                    instantConfig,
                     emptyDataSet(),
                     (data) => resolve(data.dataSet),
                 );
@@ -28,13 +32,13 @@ describe("userActivity", () => {
             });
             // This data is comitted asyncronously.
             const data = await trackerPromise;
-            expect(data).toEqual({
-                users: {[USER_ONE]: userData}
-            });
+            expect(data).toEqual(new Map([
+                [USER_ONE, userData],
+            ]));
         });
         it("can update a user's activity with metadata", () => {
             const tracker = new UserActivityTracker(
-                UserActivityTrackerConfig.DEFAULT,
+                instantConfig,
                 emptyDataSet(),
             );
             tracker.updateUserActivity(USER_ONE, { private: true }, DATE_NOW);
@@ -47,7 +51,7 @@ describe("userActivity", () => {
         });
         it("can update a user's activity twice", () => {
             const tracker = new UserActivityTracker(
-                UserActivityTrackerConfig.DEFAULT,
+                instantConfig,
                 emptyDataSet(),
             );
             tracker.updateUserActivity(USER_ONE, undefined, DATE_MINUS_ONE);
@@ -62,7 +66,7 @@ describe("userActivity", () => {
         });
         it("will not remove metadata from a user", () => {
             const tracker = new UserActivityTracker(
-                UserActivityTrackerConfig.DEFAULT,
+                instantConfig,
                 emptyDataSet(),
             );
             tracker.updateUserActivity(USER_ONE, { private: true}, DATE_MINUS_ONE);
@@ -79,7 +83,7 @@ describe("userActivity", () => {
         });
         it("will cut off a users activity after 31 days", () => {
             const tracker = new UserActivityTracker(
-                UserActivityTrackerConfig.DEFAULT,
+                instantConfig,
                 emptyDataSet(),
             );
             const LAST_EXPECTED_DATE = (DATE_NOW.getTime() - (ONE_DAY * 30)) / 1000;
@@ -95,7 +99,7 @@ describe("userActivity", () => {
     describe("countActiveUsers", () => {
         it("should have no users when the dataset is blank", () => {
             const tracker = new UserActivityTracker(
-                UserActivityTrackerConfig.DEFAULT,
+                instantConfig,
                 emptyDataSet(),
             );
             expect(tracker.countActiveUsers(DATE_NOW)).toEqual({
@@ -105,7 +109,7 @@ describe("userActivity", () => {
         });
         it("should have no users when the user hasn't been active for 3 days", () => {
             const tracker = new UserActivityTracker(
-                UserActivityTrackerConfig.DEFAULT,
+                instantConfig,
                 emptyDataSet(),
             );
             tracker.updateUserActivity(USER_ONE, undefined, DATE_MINUS_ONE);
@@ -117,7 +121,7 @@ describe("userActivity", () => {
         });
         it("should have users when the user has been active for at least 3 days", () => {
             const tracker = new UserActivityTracker(
-                UserActivityTrackerConfig.DEFAULT,
+                instantConfig,
                 emptyDataSet(),
             );
             tracker.updateUserActivity(USER_ONE, undefined, DATE_MINUS_TWO);
@@ -131,17 +135,15 @@ describe("userActivity", () => {
         it("should not include 'active' users who have not talked in 32 days", () => {
             const DATE_MINUS_THIRTY_TWO = new Date(Date.UTC(2020, 11, 30, 0));
             const tracker = new UserActivityTracker(
-                UserActivityTrackerConfig.DEFAULT,
-                {
-                    users: {
-                        [USER_ONE]: {
-                            ts: [DATE_MINUS_THIRTY_TWO.getTime() / 1000],
-                            metadata: {
-                                active: true,
-                            }
+                instantConfig,
+                new Map([
+                    [USER_ONE, {
+                        ts: [DATE_MINUS_THIRTY_TWO.getTime() / 1000],
+                        metadata: {
+                            active: true,
                         }
-                    }
-                },
+                    }]
+                ]),
             );
             expect(tracker.countActiveUsers(DATE_NOW)).toEqual({
                 allUsers: 0,
@@ -151,17 +153,15 @@ describe("userActivity", () => {
         it("should include 'active' users who have talked in 31 days", () => {
             const DATE_MINUS_THIRTY_ONE = new Date(Date.UTC(2020, 12, 1, 0));
             const tracker = new UserActivityTracker(
-                UserActivityTrackerConfig.DEFAULT,
-                {
-                    users: {
-                        [USER_ONE]: {
-                            ts: [DATE_MINUS_THIRTY_ONE.getTime() / 1000],
-                            metadata: {
-                                active: true,
-                            }
+                instantConfig,
+                new Map([
+                    [USER_ONE, {
+                        ts: [DATE_MINUS_THIRTY_ONE.getTime() / 1000],
+                        metadata: {
+                            active: true,
                         }
-                    }
-                },
+                    }]
+                ]),
             );
             expect(tracker.countActiveUsers(DATE_NOW)).toEqual({
                 allUsers: 1,
@@ -170,7 +170,7 @@ describe("userActivity", () => {
         });
         it("should mark user as private if metadata specifies it", () => {
             const tracker = new UserActivityTracker(
-                UserActivityTrackerConfig.DEFAULT,
+                instantConfig,
                 emptyDataSet(),
             );
             tracker.updateUserActivity(USER_ONE, { private: true}, DATE_MINUS_TWO);
@@ -183,7 +183,7 @@ describe("userActivity", () => {
         });
         it("should handle multiple users", () => {
             const tracker = new UserActivityTracker(
-                UserActivityTrackerConfig.DEFAULT,
+                instantConfig,
                 emptyDataSet(),
             );
             tracker.updateUserActivity(USER_ONE, { private: true }, DATE_MINUS_TWO);
@@ -199,4 +199,28 @@ describe("userActivity", () => {
             });
         });
     })
+    describe("debouncing", () => {
+        it("should only send one update in a specified period", async () => {
+            const updates = [];
+            const tracker = new UserActivityTracker(
+                {
+                    ...UserActivityTrackerConfig.DEFAULT,
+                    debounceTimeMs: 100,
+                },
+                emptyDataSet(),
+                (changes) => updates.push(changes),
+            );
+            tracker.updateUserActivity(USER_ONE, undefined, DATE_MINUS_ONE);
+            tracker.updateUserActivity(USER_TWO, undefined, DATE_MINUS_ONE);
+            tracker.updateUserActivity(USER_ONE, undefined, DATE_MINUS_ONE);
+            await new Promise(resolve => setTimeout(resolve, 200));
+            expect(updates.length).toEqual(1);
+            expect(updates[0].changed.sort()).toEqual([USER_ONE, USER_TWO]);
+
+            tracker.updateUserActivity(USER_ONE, undefined, DATE_NOW);
+            await new Promise(resolve => setTimeout(resolve, 200));
+            expect(updates.length).toEqual(2);
+            expect(updates[1].changed.length).toEqual(1);
+        });
+    });
 })
diff --git a/src/components/user-activity-store.ts b/src/components/user-activity-store.ts
@@ -53,14 +53,14 @@ export class UserActivityStore extends BridgeStore {
 
     public async getActivitySet(): Promise<UserActivitySet> {
         return this.select({}).then((records: any[]) => {
-            const users: {[mxid: string]: any} = {};
+            const userActivity: UserActivitySet = new Map();
             for (const record of records) {
-                users[record.mxid] = {
+                userActivity.set(record.mxid, {
                     ts:       record.ts,
                     metadata: record.metadata,
-                };
+                });
             }
-            return { users } as UserActivitySet;
+            return userActivity;
         });
     }
 }
diff --git a/src/components/user-activity.ts b/src/components/user-activity.ts
@@ -26,16 +26,7 @@ interface UserActivityMetadata {
     active?: true;
 }
 
-export interface UserActivitySet {
-    users: {[userId: string]: UserActivity};
-}
-
-// eslint-disable-next-line @typescript-eslint/no-namespace,no-redeclare
-export namespace UserActivitySet {
-    export const DEFAULT: UserActivitySet = {
-        users: {}
-    };
-}
+export type UserActivitySet = Map<string, UserActivity>;
 
 export interface UserActivity {
     ts: number[];
@@ -45,13 +36,15 @@ export interface UserActivity {
 export interface UserActivityTrackerConfig {
     inactiveAfterDays: number;
     minUserActiveDays: number;
+    debounceTimeMs: number;
 }
 
 // eslint-disable-next-line @typescript-eslint/no-namespace,no-redeclare
 export namespace UserActivityTrackerConfig {
     export const DEFAULT: UserActivityTrackerConfig = {
         inactiveAfterDays: 31,
         minUserActiveDays: 3,
+        debounceTimeMs:    60 * 1000, // 1 minute
     };
 }
 
@@ -78,14 +71,17 @@ const ONE_DAY = 24 * 60 * 60 * 1000;
  * to not interfere with UserActivityTracker's operations.
  */
 export class UserActivityTracker {
+    private debounceTimer: NodeJS.Timeout|undefined;
+    private debouncedChangedSet = new Set<string>();
+
     constructor(
         private readonly config: UserActivityTrackerConfig,
         private readonly dataSet: UserActivitySet,
         private readonly onChanges?: ChangesCallback,
     ) { }
 
     public updateUserActivity(userId: string, metadata?: UserActivityMetadata, dateOverride?: Date): void {
-        let userObject = this.dataSet.users[userId];
+        let userObject = this.dataSet.get(userId);
         if (!userObject) {
             userObject = {
                 ts: [],
@@ -115,15 +111,20 @@ export class UserActivityTracker {
             }
         }
 
-        this.dataSet.users[userId] = userObject;
-        setImmediate(() => {
-            log.debug("Notifying the listener of RMAU changes");
-            this.onChanges?.({
-                changed: [userId],
-                dataSet: this.dataSet,
-                activeUsers: this.countActiveUsers().allUsers,
-            });
-        });
+        this.dataSet.set(userId, userObject);
+        this.debouncedChangedSet.add(userId);
+        if (!this.debounceTimer) {
+            this.debounceTimer = setTimeout(() => {
+                log.debug(`Notifying the listener of RMAU changes`);
+                this.onChanges?.({
+                    changed: Array.from(this.debouncedChangedSet),
+                    dataSet: this.dataSet,
+                    activeUsers: this.countActiveUsers().allUsers,
+                });
+                this.debounceTimer = undefined;
+                this.debouncedChangedSet.clear();
+            }, this.config.debounceTimeMs);
+        }
     }
 
     /**
@@ -136,7 +137,7 @@ export class UserActivityTracker {
         let allUsers = 0;
         let privateUsers = 0;
         const activeSince = ((dateNow?.getTime() || Date.now()) - this.config.inactiveAfterDays * ONE_DAY) / 1000;
-        for (const user of Object.values(this.dataSet.users)) {
+        for (const user of this.dataSet.values()) {
             if (!user.metadata.active) {
                 continue;
             }
@@ -151,7 +152,7 @@ export class UserActivityTracker {
         return {allUsers, privateUsers};
     }
 
-    public getUserData(userId: string): UserActivity {
-        return this.dataSet.users[userId];
+    public getUserData(userId: string): UserActivity|undefined {
+        return this.dataSet.get(userId);
     }
 }
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		Add debouncing support to UserActivityTracker (defaults to 60 seconds).