Skip to content

chore(experiments): extract metric value and predicate semantics#60787

Merged
andehen merged 2 commits into
masterfrom
refactor/exp-qb-3-metric-values
Jun 2, 2026
Merged

chore(experiments): extract metric value and predicate semantics#60787
andehen merged 2 commits into
masterfrom
refactor/exp-qb-3-metric-values

Conversation

@andehen
Copy link
Copy Markdown
Contributor

@andehen andehen commented May 30, 2026

Problem

This is PR 3 of an 8-PR stacked, behavior-preserving refactor that breaks the ~3000-line experiment_query_builder.py god class into cohesive modules, following the team's hybrid refactor plan. It isolates the metric value and predicate semantics — conversion-window predicates, the metric predicate, the value expression, and the value-aggregation expression (sum/avg/min/max/unique-sessions/DAU/unique-groups/HogQL-aggregation/nullable/winsorization) — into a dedicated module so the builder no longer owns this self-contained slice of SQL-construction logic.

Changes

  • Added posthog/hogql_queries/experiments/experiment_metric_values.py with seven module-level functions taking fully explicit inputs (team, source, date_range_query, conversion_window_seconds, table_alias, events_alias, column_name, value_expr, etc.): get_conversion_window_seconds, build_conversion_window_predicate, build_session_conversion_window_predicate, build_conversion_window_predicate_for_events, build_metric_predicate, build_value_expr, and build_value_aggregation_expr.
  • ExperimentQueryBuilder keeps a thin delegating wrapper of the same name and signature for each of the seven methods, so every caller — the mean/ratio/funnel/retention builders, internal callers in experiment_query_runner.py, and the ExperimentFunnelActorsQueryBuilder subclass — is unchanged.
  • Removed now-unused imports from experiment_query_builder.py (get_source_value_expr, the hogql_aggregation_utils block, and ExperimentMetricMathType) that now live only in the new module.
  • Mechanical, behavior-preserving move: generated ClickHouse SQL is unchanged (experiment snapshots stable, no .ambr diffs); no SQL or logic was edited; all comments preserved verbatim.

How did you test this code?

Authored by an AI agent (Claude Code) in an automated multi-agent workflow — no manual testing was performed. The following automated tests were run locally and pass, with no snapshot (.ambr) changes:

  • posthog/hogql_queries/experiments/test/experiment_query_runner/test_mean_metric.py
  • posthog/hogql_queries/experiments/test/experiment_query_runner/test_ratio_metric.py

(48 passed, 1 pre-existing skip, 47 snapshots passed; no .ambr files changed.)

Automatic notifications

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

Docs update

Add the skip-inkeep-docs label — internal refactor with no user-facing docs impact.

🤖 Agent context

Authored by Claude Code via a stacked-PR refactor workflow; PR 3 of 8, stacked on refactor/exp-qb-2-exposure. Mechanical, behavior-preserving extraction — generated SQL is unchanged (experiment snapshot tests pass with no .ambr diffs) and delegating wrappers were kept on ExperimentQueryBuilder so ExperimentFunnelActorsQueryBuilder and internal callers keep working. The seven extracted functions take fully explicit args; the builder wrappers retain the source=None resolution and the ExperimentMeanMetric/metric is required asserts since some callers invoke them externally. PR 2 used a class-based exposure builder; per this phase's plan the metric-value helpers are module-level functions. Agent-authored and requires human review — do not self-merge or auto-approve.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 30, 2026

🎭 Playwright report · View test results →

⚠️ 2 flaky tests:

  • Redirect to appropriate place after login (chromium)
  • Save an insight, make changes, discard them, and save a copy (chromium)

These issues are not necessarily caused by your changes.
Annoyed by this comment? Help fix flakies and failures and it'll disappear!

@andehen andehen force-pushed the refactor/exp-qb-2-exposure branch from 7b6791d to 1c27829 Compare June 2, 2026 06:20
Base automatically changed from refactor/exp-qb-2-exposure to master June 2, 2026 12:27
Move conversion-window, metric-predicate, value, and value-aggregation
helpers out of ExperimentQueryBuilder into experiment_metric_values.py as
module-level functions with explicit inputs. The builder keeps delegating
wrappers of the same name and signature, so all callers (including the
funnel actors subclass) are unchanged. Pure mechanical move, no SQL change.
@andehen andehen force-pushed the refactor/exp-qb-3-metric-values branch from 57e897c to 653d88f Compare June 2, 2026 12:30
@andehen andehen marked this pull request as ready for review June 2, 2026 12:30
@andehen andehen requested a review from a team June 2, 2026 12:30
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 2, 2026

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

---

### Issue 1 of 3
posthog/hogql_queries/experiments/experiment_metric_values.py:150-156
These docstrings were copied verbatim from the class methods and still say "uses `self.metric.source` by default" — but this module-level function has no `self` and requires `source` as an explicit argument. Any new reader of this file will be confused about whether the function falls back to something automatically.

```suggestion
def build_value_expr(source: MetricSource, apply_coalesce: bool = True) -> ast.Expr:
    """
    Extracts the value expression from the metric source configuration.
    For ratio metrics, pass the specific source (numerator or denominator).
    For mean metrics, pass the resolved metric source explicitly.

    Args:
```

### Issue 2 of 3
posthog/hogql_queries/experiments/experiment_metric_values.py:199-202
Same stale `self.metric.source` reference in the docstring for `build_value_aggregation_expr`. The function requires `source` explicitly; the "uses self.metric.source by default" text is left over from the class context and no longer applies here.

```suggestion
    """
    Returns the value aggregation expression based on math type.
    For ratio metrics, pass the specific source (numerator or denominator) and events_alias.
    For mean metrics, pass the resolved metric source explicitly with "metric_events" alias.
```

### Issue 3 of 3
posthog/hogql_queries/experiments/experiment_metric_values.py:106-110
Same stale `self.metric.source` reference in `build_metric_predicate`'s docstring — no `self` here either.

```suggestion
    """
    Builds the metric predicate as an AST expression.
    For ratio metrics, pass the specific source (numerator or denominator) and table_alias.
    For mean metrics, pass the resolved metric source explicitly with "events" alias.
    """
```

Reviews (1): Last reviewed commit: "chore(experiments): extract metric value..." | Re-trigger Greptile

Comment thread posthog/hogql_queries/experiments/experiment_metric_values.py
Comment thread posthog/hogql_queries/experiments/experiment_metric_values.py Outdated
Comment thread posthog/hogql_queries/experiments/experiment_metric_values.py
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
@andehen andehen enabled auto-merge (squash) June 2, 2026 12:52
@andehen andehen merged commit 7797052 into master Jun 2, 2026
267 checks passed
@andehen andehen deleted the refactor/exp-qb-3-metric-values branch June 2, 2026 13:20
@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 14:17 UTC Run
prod-us ✅ Deployed 2026-06-02 14:32 UTC Run
prod-eu ✅ Deployed 2026-06-02 14:35 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.

2 participants