feat(experiments): extend metric-events precomputation to count/sum mean metrics - #75903
Draft
jurajmajerik wants to merge 2 commits into
Draft
feat(experiments): extend metric-events precomputation to count/sum mean metrics#75903jurajmajerik wants to merge 2 commits into
jurajmajerik wants to merge 2 commits into
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Experiment precomputation has two halves: an exposures table (built for every metric type) and a metric-events table (built only for ordered funnel metrics). Mean metrics only get the exposures half, so every uncached load of a mean metric scans the team's raw events. At teams enrolled in precomputation this is the largest remaining source of slow, read-heavy experiment queries, while fully precomputed funnels at the same teams run interactively.
Changes
Extends the metric-events half to mean metrics for the simplest case: events or actions source, count (total) or sum math. No CUPED, no breakdowns, no data warehouse, no session properties. Everything ineligible keeps the existing direct-scan path.
_metric_events_precompute_applicable()now admits eligible mean metrics alongside ordered funnels.MeanQueryBuilder.get_mean_metric_events_query_for_precomputation()writes one row per matching metric event into the existingexperiment_metric_events_preaggregatedtable, using thenumeric_valuecolumn that was already in the schema. No schema change.metric_eventsCTE reads the preaggregated table (filtered by job ids, team, and experiment window) instead of scanning events. The rest of the query (conversion-window join, winsorization, threshold) is untouched, so results are identical between the two paths.GROUP BY entity_id, timestamp, event_uuid). Sum is the first aggregate on this table that is not idempotent under duplicate rows: exposures re-aggregate with argMin/min/max and funnel evaluation tolerates a doubled event, but a replayed build INSERT would double a sum. ReplacingMergeTree only collapses duplicates at merge time and the read does not use FINAL, so the read defends itself the way the exposures read does.get_metric_events_query_for_precomputation()on the query builder dispatches funnel vs mean; the runner's build orchestration, fallback, andexperiment_metric_events_pathtagging work unchanged.Design notes:
experiment_date_todeclared a sentinel, same as the funnel template. Reusing_build_metric_predicate()would bake the resolved dates into the query, so a running experiment's moving window end would change the job hash on every load and defeat cache reuse.Follow-ups (informed by a review of past precompute fix PRs):
How did you test this code?
products/experiments/backend/hogql_queries/test/experiment_query_runner/suite passes (278 tests). The existing parameterized mean tests' precomputed legs now route through the new build and read path end to end, asserting exact results for sum, count, conversion window, winsorization, ignore-zeros, and threshold. Their 7 ClickHouse query snapshots changed accordingly; ineligible math types kept their snapshots, confirming the gate.test_experiment_mean_metric_events_preaggregation.pycovers only what nothing else catches: a hash-stability test (a refactor that bakes the resolved date bounds into the mean build template would silently kill cache reuse without failing any result assertion), a parameterized gate test (a wrongly widened gate causes swallowed build failures, a wrongly narrowed one silently loses precompute, both invisible to result assertions because of the fallback), and a replay-tolerance test that duplicates every stored build row and asserts results still match the direct scan. I verified the replay test actually fails (sum doubles) when the read-side dedup is removed.mypy --cache-fine-grained .clean,hogli ci:preflight --fixgreen.Automatic notifications
Docs update
Updated
LAZY_COMPUTATION.mdwith a note on the metric-events half.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Implemented with Claude Code from a written brief. Skills invoked: /wt (worktree setup), /writing-tests. The plan considered including min/max/avg in the first PR since they are parity-safe for free, but we kept the allowlist to count/sum to match the brief's scope and keep the review surface small. CUPED support and CUPED usage measurement were explicitly deferred.
After the initial version, I (Claude) reviewed all precompute-related fix PRs merged this year (INSERT date constraints, insert settings parity, ReplacingMergeTree ORDER BY, hash stability, sync distributed inserts, TTL refold, byte-cap no-retry, GROUP BY spill, group-aggregation and cohort gates) and checked which lessons the mean path inherits via the shared executor versus which it must handle itself. That review produced the read-side dedup commit and the follow-ups list above.