feat: New ingestion consumer #27668
feat: New ingestion consumer #27668
Conversation
# Conflicts: # plugin-server/src/main/pluginsServer.ts
benjackwhite
changed the title
There was a problem hiding this comment.
This is a really good step forward, the new IngestionConsumer is much cleaner than what I've seen in Mr Blobby.
I haven't done a super-detailed review of the ingestion code, as it would take me a bit more time to properly understand the old code and compare it with the new implementation. Let me know if you want me to do that.
I left some comments in a couple of places, they're not super critical, so I'll approve the PR and leave it to you what to do with them.
|
|
||
| beforeEach(async () => { | ||
| const now = new Date(2025, 1, 1).getTime() | ||
| jest.spyOn(Date, 'now').mockReturnValue(now) |
There was a problem hiding this comment.
issue: This looks like it's gonna break when you run it in a different timezone – in the snapshot it's already a different date. I assume we don't want to mock the timers using jest because it's an integration test and it might be running timers somewhere in the middle of the stack. Could we do the same as we do with UUIDs or completely mask out the timestamps in the snapshots?
There was a problem hiding this comment.
Yeah I tried a few things and this seemed to be the best option. I'll circle back and see if I can make it nicer
| }) | ||
|
|
||
| describe('dropping events', () => { | ||
| describe.each(['headers', 'payload'] as const)('via %s', (kind) => { |
There was a problem hiding this comment.
note: For me it's quite a bit of effort to understand those tests – there's quite a lot of back-and-forth:
- to understand why the payload tests pass, I had to scroll up quite a bit to find that there's a default token in the payload
- in the header tests the payloads still have the tokens – the name of the describe block suggest that we're testing events with tokens only in the headers
The tests below are short enough (and very readable on their own) that they could be duplicated and the createEvent function could be explicit on where the token lands, instead of relying on addMessageHeaders filtering by the kind of test.
There was a problem hiding this comment.
Cool point taken. I think its too early for this to be reviewed tbh as I'm mostly trying to understand the existing logic myself...
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
benjackwhite
requested review from
meikelmosby and
pl
and removed request for
pl and
meikelmosby
# Conflicts: # frontend/__snapshots__/replay-player-success--second-recording-in-list--dark.png
There was a problem hiding this comment.
Found some bugs here - will pull out to a separate PR
Problem
This PR is the first in a few steps to start refactoring the plugin server to be much cleaner and more minimal
Changes
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
How did you test this code?