fix: Webhooks groups properties

[tiina303]

Problem

When we stopped writing groups on events (which was the right thing to do as it's not used in analytics queries and used up resources unnecessarily), then it broke webhooks using group properties.

This is innefficient as the comment says, but we don't want to spend too much time improving a legacy system that's going away soon.

Also made sure we don't accidentally break this functionality by adding a functional test for it.

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?

[mariusandra]

When we stopped writing groups on events (which was the right thing to do as it's not used in analytics queries and used up resources unnecessarily), then it broke webhooks using group properties.

Which were never enabled because nobody told us we can, despite groups being a first class citizen. You were still getting persons on events. However which would actually be great to do in analytics, as queries with groups can get slow for our biggest and most important customers.

Fixed that for you. "The right thing" is extremely subjective.

[tiina303]

@mariusandra this came from Product Analytics side, see #22764

Group properties on events never went anywhere, having only been partially implemented (certainly not at all in HogQL), and I don't think we want to prioritize that idea now.
(I'm a tiny bit worried that group properties behaving differently from person ones might be confusing for users, but this just doesn't seem important.)

[mariusandra]

I don't see how these two things are at odds. The conclusion is different. You concluded "let's kill it". I would've concluded "let's finish it (next month?) cause it'll be a nightmare to re-enable and backfill, so no action now". Notably missing is an analysis on either option.

[tiina303]

Unfortunately it would have required data fixing / backfill anyway, which I called this out in #22962

There was at some point a problem with data written to group properties, so they aren't fully in a good state anyway.

[mariusandra]

Don't forget, we're serving the customer here. The customer is using "frontend apps" like Product Analytics or Feature Flags to do what they need to do. Whatever would be great for users of Product Analytics (and the other products) should take priority over what inconveniences a team that effectively serves other teams, like Pipeline.

PoE clearly was done to help end users with performance. GoE was discussed, but put on the back burner as the next step. Having it would obviously help with any query that uses group properties (enough users to matter, I've seen reports). We have also always said Groups should be a "first class citizen", whatever that means, so removing features from Groups seems like a steps backwards.

In any case, please own up and fix the regression from webhooks (and CDP?), and/or communicate with the users why groups are not a first class citizen anymore if you choose that.

[tiina303]

fix the regression from webhooks

I'm confused, this is exactly what this PR does

[mariusandra]

Yes, thank you. If you need help with reviewing, let me know. It was still draft when I opened it earlier.

[bretthoerner]

I don't know much about these code paths, and since this is undoing a breaking change and people are talking about backfills it makes me nervous to breeze through this.

I didn't review #22962 but is this just a revert of that change, or something more complicated?

[bretthoerner]

Just noting for others that the previous group code used a Redis cache before Postgres: https://github.com/PostHog/posthog/pull/22962/files#diff-0769dccc3458a7eb730036617ac32002072330fcdc5ec5a1d11364769d5dd5b6L488

Since this is just for Webhooks now and we're using a PG read replica this seems fine for now. If anything, it would be nice to do one query for all of them rather than a query each...