posthog icon indicating copy to clipboard operation
posthog copied to clipboard

refactor(plugin-server): Unify event types

Open Twixes opened this issue 3 years ago • 3 comments

Problem

When working on #10572, I realized how messy our typing for events is. Five types were exposed, and it was really hard to say which is for what:

  1. Event – an obsolete shape based on the old Postgres posthog_event table
  2. ClickHouseEvent – a relevant shape, but built by extending of Event and lacking persons-on-events columns
  3. PreIngestionEvent – a shape used for some intra-plugin-service processing
  4. IngestionEvent – weirdly an alias of PreIngestionEvent
  5. ClickhouseEventKafka – basically ClickHouseEvent but slightly different for no good reason

Additionally there are two @posthog/plugin-scaffold types: PluginEvent and ProcessedPluginEvent.

Both used a lot internally, which is hampering #10572 – because internal code is so closely intertwined with the representation exposed to plugin developers, changing the external API is now painful.

Changes

This reworks the typing structure to something more closely resembling the real world. Work in progress, but at the moment the scheme looks like this:

  1. RawEvent – raw ClickHouse event row (i.e. datetimes and JSON are strings), in sync with the current schema
  2. Event – same as above, but processed for JavaScript (i.e. datetimes and JSON are parsed)
  3. PreIngestionEvent – mostly the same, except can't contain serialized timestamps anymore
  4. PostIngestionEvent – clearer alias of IngestionEvent

How did you test this code?

Relying on TypeScript and existing tests to catch errors.

Twixes avatar Jul 01 '22 17:07 Twixes

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

posthog-bot avatar Jul 13 '22 07:07 posthog-bot

This PR was closed due to 2 weeks of inactivity. Feel free to reopen it if still relevant.

posthog-bot avatar Jul 20 '22 07:07 posthog-bot

Sorry this is taking a while to get in - other work took priority.

Note that the work you did rebasing/updating this before was kind of unneeded - as mentioned in the original slack thread, taking over this PR and will try get it merged :)

macobo avatar Aug 10 '22 08:08 macobo

Got everything green and added most of the missing testing. See comments above for what changed.

I also introduced branded types to disambiguate between ISO and Clickhouse timestamps as I removed the clonable concept.

Will merge on Monday as on holiday for next 2 days and want to monitor for any unintended regressions.

macobo avatar Aug 10 '22 10:08 macobo