Repository navigation
perf(cdp): skip parsing events the consumer has no work for - #127164
Conversation
🤖 CI report✅ Trunk lane — non-backend laneThis PR is assigned to the non-backend lane. It does not run backend Python tests and may merge in parallel with PRs in other lanes.
|
| First copy | Second copy | Lines | Tokens |
|---|---|---|---|
posthog/api/integration.py:2736 |
posthog/api/user_integration.py:573 |
19 | 85 |
✅ Duplication (TypeScript) — clean
New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.
🚨 Comment density — 17% of added code lines are comments (22 of 128)
This section warns when comments are more than 3% of the code lines a PR adds, and alerts above 6%. Before agent-assisted PRs, the typical share was about 2%. Only full-line comments count. Docstrings, generated files, snapshots, migrations, and workflow files are left out.
Comments that restate the code, record how the change came about, or narrate the next line add noise for the next reader. Keep the comments that explain a reason the code cannot show, and remove the rest. See .agents/skills/writing-code-comments/SKILL.md for the house rules.
Files with the most added comment lines:
| File | Comment lines | Added lines |
|---|---|---|
nodejs/src/cdp/consumers/message-parsing.ts |
8 | 20 |
nodejs/src/cdp/consumers/cdp-events.consumer.ts |
7 | 34 |
nodejs/src/cdp/consumers/cdp-events-consumer.test.ts |
5 | 36 |
nodejs/src/cdp/consumers/message-parsing.test.ts |
2 | 29 |
This check does not block merging. It updates on every push and clears when the share drops.
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 063baa8: 1 Must fix, 3 Should fix.
This is the only automatic review while this pull request is a draft. Comment @posthog review to review new changes sooner. Otherwise, a review of the new changes runs when you mark it ready for review.
See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.
|
Risk: No findings The only change since the last review removes a test-only app-metrics flush-interval override (with its comment) from two beforeEach blocks in cdp-events-consumer.test.ts. No production code in this PR changed since the last review, so no new security surface was introduced. Sentinel reviewed |
8b6e9d5 to
7ce5342
Compare
There was a problem hiding this comment.
PostHog Review
Found 1 should fix.
Findings on the changed lines
nodejs/src/common/services/app-metrics-aggregator.ts:97
P2 · Serialize timed drains with outstanding flushes
When Kafka acknowledgments stall beyond the interval, each timer tick starts another produce while earlier drains remain pending. Because flush() clears the buffer before awaiting delivery, batch flushes return without backpressure. Pending messages accumulate and exhaust the producer queue, dropping metrics; track the in-flight flush and make subsequent callers await it.
1d4778b to
fe1247b
Compare
342901d to
54721ae
Compare
54721ae to
fb8cf2c
Compare
fb8cf2c to
225c245
Compare
225c245 to
3e6ff36
Compare
The events consumer parsed every event body to find its team, then dropped the event when that team had no destination and no workflow, which is most of them. The team id now comes from the Kafka header, and the body is parsed inside toGlobals, which only runs once a team turns out to have work. A message with no usable header keeps the old path and reads the team from the body, so nothing depends on the header being there. The header decides which team's destinations load and the body carries the event data, so an event whose body names a different team is parked rather than built. Otherwise one team's event would reach another team's destinations. cdp_events_team_id_source counts which path each message took, so the rollout is visible. One behavior change: a malformed body for a team with no destination and no workflow is now skipped instead of parked, because nothing reads it. CDP would have produced nothing for that event either way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3e6ff36 to
ae6899c
Compare
|
/trunk merge |
|
😎 Stack merged successfully - details. |
Master now reads the team id from a Kafka header before it parses the body (#127164). The invalid team id skip and the not_an_event shape check move into the shared parseEvent, so the body fallback keeps them. The out-of-int4 row of the parks table on master is the case this branch reverses, so it stays out of that table. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Top layer of #127337. Reads the header #127163 adds.
Problem
Changes
toGlobals, which runs only once a team turns out to have a destination or a workflow.cdp_events_team_id_sourcecounts which path each message took, so the rollout is visible.Warning
A malformed body now reaches the dead-letter topic only when its team has a destination or a workflow. For any other team nothing reads the body, and CDP would have produced nothing for that event anyway.
Numbers
Parsing the event body accounted for about 6.5% of main-thread samples in a merged wall profile of the consumer, with roughly 4.5 points of that inside
JSON.parse. Most events belong to a team with neither a destination nor a workflow, so most of that work was spent on events the consumer then dropped.cdp_events_team_id_sourcegives the header-versus-body split once this ships, and the ratio of parsed to consumed events is what the saving tracks.Not yet measured in production: nothing is deployed, so there is no after-profile.
How did you test this code?
Test rationale:
readTeamIdHeadergets a case table, because a header value that is not a usable team id must never become one: the consumer would load another team's destinations. Two consumer cases follow: a header naming a team with no function plus an unparseable body, where a parked record would prove the body was read, and a header and body naming different teams, which must park. Existing cases in that file carry no header, so they already cover the fallback.Not run: jest, oxlint and oxfmt.
pnpm installcannot reach the npm registry in this sandbox.tsc --noEmitreports no error in the changed files.Release status
Docs update
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code, Opus 5
Skills:
/writing-code-comments,/writing-tests,/writing-pr-descriptions,/stacking-prs,/debugging-ci-failuresMore context for AI agents
toGlobalsrather than behind a separate flag, becauseparseBatchalready runstoGlobalsonly for a team that has work.cdp_events_team_id_sourceshows when every message carries the header; the body fallback can go once it does.cdp-events.consumer.ts.🤖 Generated with Claude Code