feat(blobby): overflow detection updates the redis set with keys that trigger #21154
Conversation
| const key = `${team_id}-${session_id}` | ||
| // TODO: use this for session key too if it's safe to do so | ||
| const overflowKey = `${team_id}:${session_id}` |
There was a problem hiding this comment.
does the comment mean whether we should use the same for key and overflowKey?
i think safe to do that
There was a problem hiding this comment.
Thanks! I'll do it in a separate no-op PR, as making this PR pass tests might be complicated already, and I don't want to add noise
| await this.sessions[key]?.add(event) | ||
| await Promise.allSettled([ | ||
| this.sessions[key]?.add(event), | ||
| this.overflowDetection?.observe(overflowKey, event.metadata.rawSize, event.metadata.timestamp), |
There was a problem hiding this comment.
you could void this promise instead of waiting on it since it's ok for some to fail maybe, but it should be fast enough that that's bike-shedding and we'll see the impact if it isn't fast enough so ignore as nit-picking 🤣
There was a problem hiding this comment.
The only async part is the redis ZADD when triggering on the first time, the detection itself does not yield, so I don't think voiding it would help throughput 🤔
I'd say let's rollout and re-evaluate if there's impact. When merging, overflow will be disabled, it'll be re-enabled when rolling out the new CAPTURE_CONFIG_REDIS_HOST
Problem
Follow-up to #21046 to implement consumer-side overflow detection. See https://github.com/PostHog/product-internal/pull/573 for spec.
Changes
OverflowDetectionis promoted intoOverflowManagerwith the responsibility to update Redis when triggeredCAPTURE_CONFIG_REDIS_HOSTenvvar to point to the Redis that capture looks at (common django Redis for now). We'll use the same envvar when implementing the same detection for analytics. Not allowing port & creds to be configured, as this was only needed for the public chartDoes this work well for both Cloud and self-hosted?
Not documented nor supported on hobby
How did you test this code?