-
Notifications
You must be signed in to change notification settings - Fork 211
perf(rpc): batch goals bulkAnalytics ClickHouse queries for unfiltered goals #680
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: staging
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| import { describe, expect, mock, test } from "bun:test"; | ||
|
|
||
| const chQueryMock = mock((_query: string, _params?: Record<string, unknown>) => | ||
| Promise.resolve([] as unknown[]) | ||
| ); | ||
|
|
||
| mock.module("@databuddy/db/clickhouse", () => ({ | ||
| chQuery: chQueryMock, | ||
| chCommand: mock(async () => undefined), | ||
| })); | ||
|
|
||
| const { buildGoalAnalyticsResult, processGoalsConversionCountsBatch } = | ||
| await import("./analytics-utils"); | ||
|
|
||
| describe("processGoalsConversionCountsBatch", () => { | ||
| test("returns an empty map and issues no query for an empty step list", async () => { | ||
| chQueryMock.mockClear(); | ||
|
|
||
| const result = await processGoalsConversionCountsBatch([], { | ||
| websiteId: "site_1", | ||
| startDate: "2026-01-01", | ||
| endDate: "2026-01-07 23:59:59", | ||
| }); | ||
|
|
||
| expect(result.size).toBe(0); | ||
| expect(chQueryMock).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test("counts every goal in a single ClickHouse round trip", async () => { | ||
| chQueryMock.mockClear(); | ||
| chQueryMock.mockImplementationOnce(() => | ||
| Promise.resolve([ | ||
| { step_num: 1, completions: 42 }, | ||
| { step_num: 2, completions: 7 }, | ||
| { step_num: 3, completions: 0 }, | ||
| ]) | ||
| ); | ||
|
|
||
| const result = await processGoalsConversionCountsBatch( | ||
| [ | ||
| { step_number: 1, type: "PAGE_VIEW", target: "/pricing", name: "Pricing" }, | ||
| { step_number: 2, type: "EVENT", target: "signup", name: "Signup" }, | ||
| { step_number: 3, type: "EVENT", target: "purchase", name: "Purchase" }, | ||
| ], | ||
| { | ||
| websiteId: "site_1", | ||
| startDate: "2026-01-01", | ||
| endDate: "2026-01-07 23:59:59", | ||
| } | ||
| ); | ||
|
|
||
| expect(chQueryMock).toHaveBeenCalledTimes(1); | ||
| expect(result.get(1)).toBe(42); | ||
| expect(result.get(2)).toBe(7); | ||
| expect(result.get(3)).toBe(0); | ||
| }); | ||
|
|
||
| test("never receives a non-empty filter list, which would make batching unsafe", async () => { | ||
| chQueryMock.mockClear(); | ||
| chQueryMock.mockImplementationOnce(() => Promise.resolve([])); | ||
|
|
||
| await processGoalsConversionCountsBatch( | ||
| [{ step_number: 1, type: "EVENT", target: "signup", name: "Signup" }], | ||
| { websiteId: "site_1", startDate: "2026-01-01", endDate: "2026-01-07" } | ||
| ); | ||
|
|
||
| const [query] = chQueryMock.mock.calls.at(-1) as [string]; | ||
| expect(query).not.toContain("browserFilter"); | ||
| expect(query).not.toContain("customFilter"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("buildGoalAnalyticsResult", () => { | ||
| test("computes conversion rate from completions and total entered users", () => { | ||
| const analytics = buildGoalAnalyticsResult("Signup", 25, 100); | ||
|
|
||
| expect(analytics.total_users_completed).toBe(25); | ||
| expect(analytics.total_users_entered).toBe(100); | ||
| expect(analytics.overall_conversion_rate).toBe(25); | ||
| expect(analytics.steps_analytics).toHaveLength(1); | ||
| expect(analytics.steps_analytics[0]?.step_name).toBe("Signup"); | ||
| }); | ||
|
|
||
| test("reports a zero conversion rate instead of dividing by zero", () => { | ||
| const analytics = buildGoalAnalyticsResult("Signup", 0, 0); | ||
|
|
||
| expect(analytics.overall_conversion_rate).toBe(0); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -855,6 +855,44 @@ ORDER BY step_num, date`; | |
| }; | ||
| }; | ||
|
|
||
| export const buildGoalAnalyticsResult = ( | ||
| stepName: string, | ||
| completions: number, | ||
| totalWebsiteUsers: number | ||
| ): FunnelAnalytics => ({ | ||
| overall_conversion_rate: pct(completions, totalWebsiteUsers), | ||
| total_users_entered: totalWebsiteUsers, | ||
| total_users_completed: completions, | ||
| avg_completion_time: 0, | ||
| avg_completion_time_formatted: "—", | ||
| biggest_dropoff_step: 1, | ||
| biggest_dropoff_rate: 0, | ||
| duration_available: false, | ||
| steps_analytics: [ | ||
| { | ||
| step_number: 1, | ||
| step_name: stepName, | ||
| users: completions, | ||
| total_users: totalWebsiteUsers, | ||
| conversion_rate: pct(completions, totalWebsiteUsers), | ||
| dropoffs: 0, | ||
| dropoff_rate: 0, | ||
| avg_time_to_complete: 0, | ||
| error_context_available: false, | ||
| error_count: 0, | ||
| error_rate: 0, | ||
| top_errors: [], | ||
| }, | ||
| ], | ||
| error_insights: { | ||
| available: false, | ||
| total_errors: 0, | ||
| sessions_with_errors: 0, | ||
| dropoffs_with_errors: 0, | ||
| error_correlation_rate: 0, | ||
| }, | ||
| }); | ||
|
|
||
| export const processGoalAnalytics = async ( | ||
| steps: AnalyticsStep[], | ||
| filters: Filter[], | ||
|
|
@@ -873,39 +911,37 @@ export const processGoalAnalytics = async ( | |
| abortSignal | ||
| ); | ||
|
|
||
| return { | ||
| overall_conversion_rate: pct(completions, totalWebsiteUsers), | ||
| total_users_entered: totalWebsiteUsers, | ||
| total_users_completed: completions, | ||
| avg_completion_time: 0, | ||
| avg_completion_time_formatted: "—", | ||
| biggest_dropoff_step: 1, | ||
| biggest_dropoff_rate: 0, | ||
| duration_available: false, | ||
| steps_analytics: [ | ||
| { | ||
| step_number: 1, | ||
| step_name: step.name, | ||
| users: completions, | ||
| total_users: totalWebsiteUsers, | ||
| conversion_rate: pct(completions, totalWebsiteUsers), | ||
| dropoffs: 0, | ||
| dropoff_rate: 0, | ||
| avg_time_to_complete: 0, | ||
| error_context_available: false, | ||
| error_count: 0, | ||
| error_rate: 0, | ||
| top_errors: [], | ||
| }, | ||
| ], | ||
| error_insights: { | ||
| available: false, | ||
| total_errors: 0, | ||
| sessions_with_errors: 0, | ||
| dropoffs_with_errors: 0, | ||
| error_correlation_rate: 0, | ||
| }, | ||
| }; | ||
| return buildGoalAnalyticsResult(step.name, completions, totalWebsiteUsers); | ||
| }; | ||
|
|
||
| export const processGoalsConversionCountsBatch = async ( | ||
| steps: AnalyticsStep[], | ||
| params: ClickhouseQueryParams, | ||
| abortSignal?: AbortSignal | ||
| ): Promise<Map<number, number>> => { | ||
| if (steps.length === 0) { | ||
| return new Map(); | ||
| } | ||
|
|
||
| const query = `WITH ${visitorIdentityCtes}, | ||
| ${buildIdentifiedEventStream(steps, [], params)} | ||
| SELECT toUInt8(step) AS step_num, uniqExact(vid) AS completions | ||
|
FindMalek marked this conversation as resolved.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: |
||
| FROM events | ||
| GROUP BY step_num`; | ||
| const rows = await chQuery<{ step_num: number; completions: number }>( | ||
| query, | ||
| params, | ||
| { abort_signal: abortSignal } | ||
| ); | ||
|
|
||
| const result = new Map<number, number>(); | ||
| for (const row of rows) { | ||
| result.set( | ||
| toFiniteNumber(row.step_num, 0), | ||
| toFiniteNumber(row.completions, 0) | ||
| ); | ||
| } | ||
| return result; | ||
| }; | ||
|
|
||
| // Referrer analytics — step matching in ClickHouse, referrer grouping in JS | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| import { describe, expect, test } from "bun:test"; | ||
| import { groupGoalsForBulkAnalytics } from "./goals-bulk-analytics-grouping"; | ||
|
|
||
| interface TestFilter { | ||
| field: string; | ||
| operator: string; | ||
| value: string; | ||
| } | ||
|
|
||
| interface TestGoal { | ||
| createdAt: Date | null; | ||
| filters: TestFilter[] | null; | ||
| id: string; | ||
| ignoreHistoricData: boolean; | ||
| } | ||
|
|
||
| const goal = (id: string, filters: TestFilter[] | null = null): TestGoal => ({ | ||
| id, | ||
| createdAt: null, | ||
| ignoreHistoricData: false, | ||
| filters, | ||
| }); | ||
|
|
||
| describe("groupGoalsForBulkAnalytics", () => { | ||
| test("splits a batch larger than chunkSize into multiple chunks", () => { | ||
| const goals = Array.from({ length: 5 }, (_, i) => goal(`g${i}`)); | ||
|
|
||
| const { batchChunks, individualGoals } = groupGoalsForBulkAnalytics( | ||
| goals, | ||
| [], | ||
| "2026-01-01", | ||
| 2 | ||
| ); | ||
|
|
||
| expect(individualGoals).toHaveLength(0); | ||
| expect(batchChunks).toHaveLength(3); | ||
| expect(batchChunks.map((chunk) => chunk.goals.length)).toEqual([2, 2, 1]); | ||
| expect(batchChunks.flatMap((chunk) => chunk.goals.map((g) => g.id))).toEqual( | ||
| ["g0", "g1", "g2", "g3", "g4"] | ||
| ); | ||
| }); | ||
|
|
||
| test("routes goals with a goal-level filter to individualGoals", () => { | ||
| const filtered = goal("filtered", [ | ||
| { field: "path", operator: "equals", value: "/pricing" }, | ||
| ]); | ||
| const unfiltered = goal("unfiltered"); | ||
|
|
||
| const { batchChunks, individualGoals } = groupGoalsForBulkAnalytics( | ||
| [filtered, unfiltered], | ||
| [], | ||
| "2026-01-01", | ||
| 255 | ||
| ); | ||
|
|
||
| expect(individualGoals).toEqual([ | ||
| { goal: filtered, combinedFilters: filtered.filters }, | ||
| ]); | ||
| expect(batchChunks).toHaveLength(1); | ||
| expect(batchChunks[0]?.goals).toEqual([unfiltered]); | ||
| }); | ||
|
|
||
| test("routes every goal to individualGoals when a request-level filter applies", () => { | ||
| const goals = [goal("a"), goal("b")]; | ||
| const requestFilters: TestFilter[] = [ | ||
| { field: "country", operator: "equals", value: "US" }, | ||
| ]; | ||
|
|
||
| const { batchChunks, individualGoals } = groupGoalsForBulkAnalytics( | ||
| goals, | ||
| requestFilters, | ||
| "2026-01-01", | ||
| 255 | ||
| ); | ||
|
|
||
| expect(batchChunks).toHaveLength(0); | ||
| expect(individualGoals).toHaveLength(2); | ||
| for (const entry of individualGoals) { | ||
| expect(entry.combinedFilters).toEqual(requestFilters); | ||
| } | ||
| }); | ||
|
|
||
| test("groups filter-free goals by effective start date into separate chunks", () => { | ||
| const recent = goal("recent"); | ||
| const backfilled: TestGoal = { | ||
| id: "backfilled", | ||
| createdAt: new Date("2026-01-15"), | ||
| ignoreHistoricData: true, | ||
| filters: null, | ||
| }; | ||
|
|
||
| const { batchChunks } = groupGoalsForBulkAnalytics( | ||
| [recent, backfilled], | ||
| [], | ||
| "2026-01-01", | ||
| 255 | ||
| ); | ||
|
|
||
| expect(batchChunks).toHaveLength(2); | ||
| const dates = batchChunks.map((chunk) => chunk.effectiveStartDate).sort(); | ||
| expect(dates).toEqual(["2026-01-01", "2026-01-15"]); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| export interface GoalForGrouping { | ||
| createdAt: Date | null; | ||
| filters: unknown; | ||
| id: string; | ||
| ignoreHistoricData: boolean; | ||
| } | ||
|
|
||
| export const getEffectiveStartDate = ( | ||
| requestedStartDate: string, | ||
| createdAt: Date | null, | ||
| ignoreHistoricData: boolean | ||
| ): string => { | ||
| if (!(ignoreHistoricData && createdAt)) { | ||
| return requestedStartDate; | ||
| } | ||
|
|
||
| const createdDate = new Date(createdAt).toISOString().split("T")[0]; | ||
| return new Date(requestedStartDate) > new Date(createdDate) | ||
| ? requestedStartDate | ||
| : createdDate; | ||
| }; | ||
|
|
||
| export interface BatchChunk<TGoal extends GoalForGrouping> { | ||
| effectiveStartDate: string; | ||
| goals: TGoal[]; | ||
| } | ||
|
|
||
| export interface GroupedGoalsForBulkAnalytics< | ||
| TGoal extends GoalForGrouping, | ||
| TFilter, | ||
| > { | ||
| batchChunks: BatchChunk<TGoal>[]; | ||
| individualGoals: { combinedFilters: TFilter[]; goal: TGoal }[]; | ||
| } | ||
|
|
||
| export function groupGoalsForBulkAnalytics< | ||
| TGoal extends GoalForGrouping, | ||
| TFilter, | ||
| >( | ||
| goalsList: TGoal[], | ||
| requestFilters: TFilter[], | ||
| startDate: string, | ||
| chunkSize: number | ||
| ): GroupedGoalsForBulkAnalytics<TGoal, TFilter> { | ||
| const batchGroups = new Map<string, TGoal[]>(); | ||
| const individualGoals: { combinedFilters: TFilter[]; goal: TGoal }[] = []; | ||
|
|
||
| for (const goal of goalsList) { | ||
| const filters = (goal.filters as TFilter[]) || []; | ||
| const combinedFilters = [...requestFilters, ...filters]; | ||
| if (combinedFilters.length > 0) { | ||
| individualGoals.push({ goal, combinedFilters }); | ||
| continue; | ||
| } | ||
|
|
||
| const effectiveStartDate = getEffectiveStartDate( | ||
| startDate, | ||
| goal.createdAt, | ||
| goal.ignoreHistoricData | ||
| ); | ||
| const group = batchGroups.get(effectiveStartDate) ?? []; | ||
| group.push(goal); | ||
| batchGroups.set(effectiveStartDate, group); | ||
| } | ||
|
|
||
| const batchChunks: BatchChunk<TGoal>[] = []; | ||
| for (const [effectiveStartDate, groupGoals] of batchGroups) { | ||
| for (let i = 0; i < groupGoals.length; i += chunkSize) { | ||
| batchChunks.push({ | ||
| effectiveStartDate, | ||
| goals: groupGoals.slice(i, i + chunkSize), | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| return { batchChunks, individualGoals }; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking): The batch query and grouping logic are well-tested, but there's no test covering the fallback path — i.e.,
processGoalsConversionCountsBatchthrows, and the router retries each goal individually viarunGoalIndividually. That orchestration lives in the router so it would be an integration-level test, but a focused unit test that mocksprocessGoalsConversionCountsBatchto 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.