perf(rpc): batch goals bulkAnalytics ClickHouse queries for unfiltered goals - #680
perf(rpc): batch goals bulkAnalytics ClickHouse queries for unfiltered goals#680FindMalek wants to merge 2 commits into
Conversation
…d goals bulkAnalytics fired 2 ClickHouse queries per goal (completions + denominator) inside a Promise.all, so a website with 20 goals issued ~40 concurrent round-trips on every dashboard load. Goals with no filters at all (request-level or goal-level) now get grouped by their effective start date and counted in one batched query per date bucket via processGoalsConversionCountsBatch, plus one shared getTotalWebsiteUsers call, instead of 2 queries per goal. Any goal with a filter keeps the original one-query-per-goal path unchanged: buildIdentifiedEventStream only threads filter conditions through the step at array index 0 (correct for its funnel-entry-filter use case), so batching filtered goals together would silently apply one goal's filter to another's count. Scoping the batch to the zero-filter case keeps that shared query builder untouched. Fixes databuddy-analytics#679
|
@izadoesdev this is ready for review whenever you have a chance, CI should be green aside from the Vercel preview checks, which need a team member to authorize the deploy (outside my permissions as an external contributor) |
|
@FindMalek is attempting to deploy a commit to the Databuddy OSS Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR batches filter-free goal conversion queries by effective start date while retaining the original individual path for filtered goals and adding per-goal fallback after batch failures.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains. The 255-goal chunking keeps generated step identifiers within the UInt8 range, and failed batches now retry each goal independently without dropping healthy sibling results. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[bulkAnalytics request] --> B{Request or goal filters?}
B -->|Yes| C[Run goal individually]
B -->|No| D[Group by effective start date]
D --> E[Split into chunks of at most 255]
E --> F[Shared users query and batched completion query]
F -->|Success| G[Map per-step counts back to goals]
F -->|Failure| H[Fall back to isolated per-goal queries]
C --> I[Return analytics by goal ID]
G --> I
H --> I
Reviews (2): Last reviewed commit: "fix(rpc): cap goal batch size and isolat..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Fixes two issues Greptile flagged on the batching PR: processGoalsConversionCountsBatch encoded each goal as a ClickHouse UInt8 step number, which wraps past 255 and silently merges unrelated goals' completion counts on unlimited-plan websites with large goal counts. Grouping and chunking is now capped at 255 goals per query and extracted into groupGoalsForBulkAnalytics, a pure function with its own tests, instead of inline router logic. A failed batched query previously marked every goal in that date bucket as failed. It now falls back to the original per-goal queries for just that bucket, so one bad query no longer takes down otherwise healthy sibling goals.
|
Fixed both — capped batches at 255 goals (extracted the grouping into a tested pure function), and combined-query failures now fall back to per-goal queries for that bucket instead of failing every goal in it. |
|
@greptile review |
izadoesdev
left a comment
izadoesdev
left a comment
There was a problem hiding this comment.
Maintainer review — APPROVE
I traced every changed line against the ClickHouse query engine, the UInt8 encoding in buildIdentifiedEventStream, and the original per-goal code path. This is safe to merge to staging.
What I verified
ClickHouse SQL correctness ✔️ — processGoalsConversionCountsBatch passes [] filters to buildIdentifiedEventStream, which means the if (index === 0 && filters.length > 0) branch in the step-case builder never fires. Every step’s match condition is purely its base match (event name or normalized path), independent of array position. The GROUP BY step_num + uniqExact(vid) correctly counts distinct visitors per goal, including when a single event matches multiple goals’ targets (the arrayJoin/arrayFilter pattern produces one row per matched step).
Result mapping ✔️ — buildIdentifiedEventStream assigns step numbers as index + 1 internally (ignoring step_number on the input). The router maps goals with chunkGoals.map((goal, index) => ({ step_number: index + 1, ... })) and reads results with completionsByStep.get(index + 1) ?? 0. The ?? 0 correctly handles goals with zero completions (they won’t appear in the ClickHouse result set since there are no rows to aggregate).
UInt8 cap ✔️ — BATCH_CHUNK_SIZE = 255 ensures step numbers 1–255, all fitting in UInt8. The chunking in groupGoalsForBulkAnalytics slices correctly with goalsList.slice(i, i + chunkSize).
Fallback isolation ✔️ — On batch failure, only the failed chunk’s goals are retried via runGoalIndividually(goal, []). Other chunks in the same date bucket, and other date buckets, are unaffected. runGoalIndividually has its own try/catch so individual failures during fallback still produce proper error results rather than crashing the entire request.
Filter routing ✔️ — combinedFilters = [...requestFilters, ...goalFilters] — if non-empty, the goal goes to individualGoals. The batch path only processes zero-filter goals. This correctly avoids the buildIdentifiedEventStream issue where filters only apply to step index 0.
endDate handling ✔️ — Both the batch path and the individual path append 23:59:59 consistently. getTotalWebsiteUsers also normalizes date-only strings internally, so the denominator query is aligned.
getEffectiveStartDate extraction ✔️ — Identical logic to the original, just moved to a shared module. Both getAnalytics (single) and bulkAnalytics import the same function.
Minor nits (all non-blocking)
See inline comments for:
- Redundant
toUInt8(step)cast in the batch query forEachvsfor...ofper project rules- No test for the fallback orchestration path (batch fails → per-goal retry)
There was a problem hiding this comment.
suggestion (non-blocking): The batch query and grouping logic are well-tested, but there's no test covering the fallback path — i.e., processGoalsConversionCountsBatch throws, and the router retries each goal individually via runGoalIndividually. That orchestration lives in the router so it would be an integration-level test, but a focused unit test that mocks processGoalsConversionCountsBatch to reject and asserts each goal still gets a result (via the per-goal fallback) would close the gap. Not blocking since the fallback delegates to the original per-goal code path which is already exercised.
|
|
||
| const query = `WITH ${visitorIdentityCtes}, | ||
| ${buildIdentifiedEventStream(steps, [], params)} | ||
| SELECT toUInt8(step) AS step_num, uniqExact(vid) AS completions |
There was a problem hiding this comment.
nit: step is already toUInt8 from the events CTE (buildIdentifiedEventStream casts each step case via toUInt8(stepNumber) / toUInt8(0)). The extra toUInt8(step) here is harmless — ClickHouse will no-op it — but it makes it look like there's a type-narrowing concern that doesn't actually exist. step AS step_num would be clearer.
| ), | ||
| ]); | ||
|
|
||
| chunkGoals.forEach((goal, index) => { |
There was a problem hiding this comment.
style (non-blocking): Project rules and Ultracite prefer for...of over Array.forEach. Since you need the index here, for (const [index, goal] of chunkGoals.entries()) would satisfy the rule.
2 participants
Closes #679
Problem
goalsRouter.bulkAnalyticsfires 2 ClickHouse queries per goal (a completion count + agetTotalWebsiteUsersdenominator) inside aPromise.all. A website with 20 goals loading the goals dashboard issues ~40 concurrent ClickHouse round-trips on every request.Fix
Goals with no filters at all (neither request-level dashboard filters nor a goal-specific filter) get grouped by their effective start date and counted together:
getTotalWebsiteUserscall per date bucket (previously one per goal)processGoalsConversionCountsBatchquery per date bucket that matches every batched goal's step condition and returns per-goal completion counts viaGROUP BY(previously oneprocessGoalConversionCountquery per goal)Any goal carrying a filter keeps the exact original one-query-per-goal path, unchanged.
Why the scope stops at "no filters"
I traced
buildIdentifiedEventStream(the shared query builder also used by the funnels feature) and found it only threads filter conditions into the match condition for the step at array index 0 — correct for its actual use case (a funnel's entry filter gating step 1), but if I'd batched filtered goals together as "steps," any goal past index 0 would silently have its filter dropped, corrupting its completion count.With an empty filter list that branch never fires, so every step's match condition is identical in shape regardless of array position — which is what makes batching provably safe in that case. I didn't modify
buildIdentifiedEventStreamitself, since it's shared with funnels and a subtle bug there would have a much larger blast radius than this fix is worth.Testing
packages/rpc/src/lib/analytics-utils-goals-batch.test.ts: asserts the batched query issues exactly one ClickHouse call for N goals (instead of N), correctly maps results back by position, handles zero goals, and asserts the generated SQL never carries per-step filter gating.packages/rpc/package.json's explicit test file list.packages/rpcsuite: 182 pass, 0 fail, 14 skip.tsc --noEmitandbiome checkclean on all touched files.turbo run check-typesandturbo run test(triggered by the pre-push hook) both pass.Summary by cubic
Closes #679 by batching ClickHouse queries for unfiltered goals in
goalsRouter.bulkAnalytics. Previously each goal fired 2 queries (a completion count and the users denominator), so 20 goals meant ~40 round-trips per dashboard load. Goals with no filters now get grouped by effective start date, chunked by 255 goals per query, and counted in one query per chunk with a shared denominator call; a failed batched query falls back to the original per-goal path for just that chunk. Goals with filters keep the original per-goal behavior unchanged.Behavior notes
UInt8in ClickHouse and wrap past that limit; grouping and chunking live in the puregroupGoalsForBulkAnalyticshelper.buildIdentifiedEventStreamonly threads filters into the first step; batching filtered goals would silently corrupt their counts, so that shared query builder is left untouched.Written for commit dcef465. Summary will update on new commits.