Add debouncing support to UserActivityTracker #448
Conversation
src/components/user-activity.ts
Outdated
| }); | ||
| if (!this.debounceTimer) { | ||
| this.debounceTimer = setTimeout(() => { | ||
| log.debug("Notifying the listener of RMAU changes"); |
There was a problem hiding this comment.
| log.debug("Notifying the listener of RMAU changes"); | |
| log.debug(`Notifying the listener of RMAU changes for ${Object.keys(this.dataSet).length} users`); |
Though, I wonder if we should make users a Map due to:
- prototype pollution scares
- giving us a
.sizeproperty?
There was a problem hiding this comment.
That makes sense, yes – though it'll be a breaking API change, since we expose it in the onChanges, and IRC and Slack rely on it when storing user activity numbers.
There was a problem hiding this comment.
Switched it to a proper Map in 141695f – but for the logline change we'd probably want "changes for ${changed.length} users" anyway?
There was a problem hiding this comment.
Actually, this debounce logic breaks it completely, since it always submit a single-element array, so future calls will be ignored if different userIds are used. Time to write some more tests.
There was a problem hiding this comment.
Fixed now, should be all good.
src/components/user-activity.ts
Outdated
| } | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-namespace,no-redeclare | ||
| export namespace UserActivityTrackerConfig { | ||
| export const DEFAULT: UserActivityTrackerConfig = { | ||
| inactiveAfterDays: 31, | ||
| minUserActiveDays: 3, | ||
| debounceTimeMs: 0, |
There was a problem hiding this comment.
I have a fear this is still a severe footgun, and wonder if we should default it to something like 100ms to be on the safe side. Honestly I feel the logic here doesn't need to be close to real-time though, we could easily up this to a minute and not have a massively noticeable difference.
| @@ -9,13 +9,17 @@ const USER_THREE = "@charlie:example.com"; | |||
| const ONE_DAY = 24 * 60 * 60 * 1000; | |||
There was a problem hiding this comment.
Just to note, I think these can be .ts files now
This allows us to throttle how often active user calculations are allowed to run – by default they run on every event sent, which for large deployments means both multiple times per second (which is completely unnecessary), and very costly (since we need to check every user against the current timestamp).
Currently it's defaulting to "as often as you want to" keeping backwards compatibility – but I'd rather make the default somewhat sane by default – possibly auto-scaled with the number of users in the dataset? It's okay to run every second if we have ten users but for 10k users once a minute would be both sensible and acceptable, so I'm tempted to put some "smart default" in place, but I'm curious of others' opinions.