Skip to content

chore(legacy-webhook-service): remove unused group enrichment#60972

Merged
dmarchuk merged 2 commits into
masterfrom
dmarchuk/legacy-webhook-drop-group-enrichment
Jun 2, 2026
Merged

chore(legacy-webhook-service): remove unused group enrichment#60972
dmarchuk merged 2 commits into
masterfrom
dmarchuk/legacy-webhook-drop-group-enrichment

Conversation

@dmarchuk
Copy link
Copy Markdown
Contributor

@dmarchuk dmarchuk commented Jun 1, 2026

Problem

The legacy webhook service (cdp-legacy-on-event) enriches each event with group properties (event.groups) before processing — fetching group properties from personhog. But nothing in this service consumes that enriched output:

  • The fired webhook body is HookPayload (nodejs/src/types.ts), which has no groups field, and convertToHookPayload assembles the body by explicitly naming fields (no spread), so event.groups never reaches the wire.
  • The action matcher (nodejs/src/cdp/legacy-webhooks/action-matcher.ts) only handles event / person / element / cohort filter types — there is no group case, and none of those handlers read event.groups.

The enrichment was introduced in #59546 (batched to reduce personhog GetGroup tail latency), carried over from the legacy plugin-server port. On closer inspection it fetches group properties from personhog on every batch and throws them away — pure overhead.

Changes

Delete the group enrichment from the legacy webhook service rather than relocate it:

  • legacy-webhook-service.ts: drop the addGroupPropertiesToPostIngestionEventsBatch call (and its try/catch fallback) from processBatch; remove the now-unused groupTypeManager / groupRepository constructor params and imports.
  • legacy-webhooks/utils.ts: deleted entirely (both addGroupPropertiesToPostIngestionEvent and addGroupPropertiesToPostIngestionEventsBatch plus their makeGroupLookupKey / GroupLookupKey helpers were the only contents).
  • cdp-legacy-event.consumer.ts / server.ts / tests/helpers/cdp.ts: stop threading groupTypeManager into the consumer; the legacy consumer now uses CdpConsumerBaseDeps directly. This removes the personhog group lookups (fetchGroup / fetchGroupsByKeys) from this path entirely.

This is an alternative to #60513, which instead optimizes where the group fetch runs. Since nothing here consumes the enriched groups, deleting the fetch is simpler, removes the personhog round-trips outright, and also eliminates the "a single fetchGroupsByKeys failure fails the whole batch" path (flagged by review on #59546).

Trade-off: if we ever want group-property-based webhook matching in this service, it would need to be rebuilt — the action matcher never supported a group filter type, so no existing behavior is lost today.

How did you test this code?

I'm an agent (Claude Code). Automated tests only — no manual product testing claimed.

  • Rewrote legacy-webhook-service.test.ts: removed the obsolete enrichment cases (asserting processedEvent.groups, the enrichment-failure fallback, and the fetchGroupsByKeys-throws path) and the PersonHogGroupRepository compatibility block. Added two guards: one asserting processBatch never calls the group repository (fetchGroup / fetchGroupsByKeys), and a payload-contract regression test proving the POSTed HookPayload body is byte-for-byte identical whether or not group_analytics is enabled (no groups key; properties / person / elementsList intact). Deleted utils.test.ts along with utils.ts.
  • Payload-neutrality proof (master vs branch): ran the real service through processEvent twice for the same event — once with event.groups populated exactly as the deleted enrichment produced it (i.e. master's output), once without — and confirmed the POSTed webhook body is byte-for-byte identical and never contains a groups key. Since convertToHookPayload / postWebhook are unchanged vs master, this demonstrates the removal does not alter the wire payload.
  • Ran the full nodejs/src/cdp/legacy-webhooks/ suite (56 tests) and cdp-legacy-event.consumer.test.ts (13 tests) against a locally migrated Postgres + Redis — all green.
  • Confirmed every other consumer of group-type/feature data (process-groups-step, readonly-process-groups-step, groups-manager.service) constructs and queries GroupTypeManager / GroupRepository independently, so nothing relied on this service to warm a shared cache.
  • Typecheck (tsc --noEmit), prettier --check, and eslint all pass on the touched files; full CI green (all three Node.js test shards + code quality).

Automatic notifications

  • Publish to changelog?
  • Alert Sales and Marketing teams?

Docs update

🤖 Agent context

Authored by Claude Code (Agent tool). Approach: verified that the enriched event.groups has no consumer in this service — not the HookPayload wire body (no groups field; explicit field assembly in convertToHookPayload) and not the action matcher (no group filter case). Given that, chose to delete the fetch outright rather than relocate it (#60513's approach), removing the personhog group lookups from this path.

The enrichment originated in #59546; its author confirmed on this PR that deletion is the right call. The constructor-signature change rippled into the consumer, server.ts, and the tests/helpers/cdp.ts deps helper; since groupTypeManager was the only field the legacy consumer's deps interface added over the base, the CdpLegacyEventsConsumerDeps interface was removed and the consumer now takes CdpConsumerBaseDeps directly (a follow-up commit also dropped the now-redundant createCdpLegacyEventsConsumerDeps test helper). The key new test locks the payload contract by comparing the webhook body with group_analytics on vs off and asserting they're identical — the guard that proves the change is payload-neutral.

@dmarchuk dmarchuk marked this pull request as ready for review June 1, 2026 21:42
@assign-reviewers-posthog assign-reviewers-posthog Bot requested review from a team June 1, 2026 21:43
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 1, 2026

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
nodejs/tests/helpers/cdp.ts:49-51
`createCdpLegacyEventsConsumerDeps` is now a zero-value pass-through wrapper — it does nothing that calling `createCdpConsumerDeps` directly wouldn't do. Per the "no superfluous parts" principle, the wrapper should be removed and the one remaining caller (`cdp-legacy-event.consumer.test.ts`) updated to call `createCdpConsumerDeps` directly.

```suggestion
// Removed: createCdpLegacyEventsConsumerDeps — callers should use createCdpConsumerDeps directly
```

Reviews (1): Last reviewed commit: "chore(legacy-webhook-service): remove un..." | Re-trigger Greptile

Comment thread nodejs/tests/helpers/cdp.ts Outdated
@nickbest-ph
Copy link
Copy Markdown
Contributor

amazing, ship it!

Copy link
Copy Markdown
Contributor

@z0br0wn z0br0wn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you simply love to see it

Copy link
Copy Markdown
Collaborator

@meikelmosby meikelmosby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Double check please that this is 100% not used anywhere in the old plugin-world. Happy to be removing dead code!

@dmarchuk dmarchuk merged commit 75c9b4e into master Jun 2, 2026
174 of 179 checks passed
@dmarchuk dmarchuk deleted the dmarchuk/legacy-webhook-drop-group-enrichment branch June 2, 2026 09:59
@deployment-status-posthog
Copy link
Copy Markdown

deployment-status-posthog Bot commented Jun 2, 2026

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-06-02 10:38 UTC Run
prod-us ✅ Deployed 2026-06-02 10:51 UTC Run
prod-eu ✅ Deployed 2026-06-02 10:55 UTC Run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants