Skip to content

fix(experiments): add absolute floor to precompute canary correctness check - #87880

Merged
trunk-io[bot] merged 2 commits into
masterfrom
experiments/canary-absolute-floor
Aug 24, 2026
Merged

trunk-io[bot] merged 2 commits into
masterfrom
experiments/canary-absolute-floor

Conversation

@jurajmajerik

@jurajmajerik jurajmajerik commented Aug 24, 2026 •

Copy link
Copy Markdown
Contributor

Problem

The precompute canary's correctness check uses a relative tolerance only. On a sparse metric, a gap of a few events between the precomputed and the live read exceeds any relative tolerance, so expected live-vs-frozen drift (person merges after a window froze, late-arriving events) pages as a divergence. Most of the divergences flagged in the last week were this shape - the worst, reported at 27%, was a 10-conversion gap on an experiment with millions of exposures.

Changes

  • A correctness sum deviation now only counts when the per-variant difference also exceeds 100 (MIN_CORRECTNESS_SUM_DELTA).
  • Exposure counts are never floored: a beyond-tolerance exposure-set divergence pages regardless of experiment size, and every false positive observed so far was sum-driven.
  • The floor is applied where deviations are computed, so the verdict, the Slack alert, the divergence log, and the Prometheus gauge report the same number.
  • The stability check is unchanged.

No user-visible changes; this only affects canary alerting.

How did you test this code?

  • New parameterized case with last week's false-positive shape (a 10-event gap at 27% relative on a sparse funnel) - catches the floor being removed.
  • New parameterized case with a 3% exposure-count divergence of only 90 users - catches the floor being extended to exposure counts, which would hide real cache breaks on small experiments.
  • New test_sub_floor_correctness_wiggle_is_not_reported - catches sub-floor drift leaking back into the reported deviation and the Prometheus gauge.
  • Ran products/experiments/backend/temporal/test_canary_logic.py locally.

Automatic notifications

  • Publish to changelog?

Docs update

None - the canary runbook lives in the module docstring, updated here.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Written with Claude Code after a root-cause investigation of last week's canary divergences (Loki divergence logs via the Grafana MCP). Skills invoked: analyzing-experiment-precompute-canary, writing-tests, writing-code-comments, writing-pr-descriptions. Design choices: the floor sits inside _max_deviation rather than in a separate verdict check, so sub-floor wiggles are excluded from every reporting surface at once instead of passing the verdict while still inflating the gauge. The floor was initially applied to both sums and exposure counts; scoped to sums only after review, since a 100-user floor could hide a genuine exposure-set break on a small experiment.

@jurajmajerik jurajmajerik self-assigned this Aug 24, 2026
@trunk-io

trunk-io Bot commented Aug 24, 2026 •

Copy link
Copy Markdown

😎 Merged successfully - details.

@github-actions

Copy link
Copy Markdown
Contributor

🤖 CI report

⚠️ Trunk lane — backend Python lane

This PR is assigned to the backend Python lane. It runs backend Python tests and may merge in parallel with PRs in other lanes.

@jurajmajerik
jurajmajerik marked this pull request as ready for review August 24, 2026 14:23
@pr-assigner-resolver-posthog
pr-assigner-resolver-posthog Bot requested a review from a team August 24, 2026 14:24
@jurajmajerik jurajmajerik added the stamphog Request AI approval (no full review) label Aug 24, 2026
@greptile-apps

greptile-apps Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor
Prompt To Fix All With AI
### Issue 1
products/experiments/backend/temporal/canary_logic.py:257-258
**Raw floor breaks metric semantics**

When a mean or ratio metric uses property-value or numerator units, this applies the event-oriented `MIN_CORRECTNESS_DELTA` directly to its raw `sum`. High-valued metrics can therefore page on drift from only a few users, while discrepancies spanning more than 100 events can be hidden when their summed values remain below 100.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (1): Last reviewed commit: "fix(experiments): add absolute floor to ..." | Re-trigger Greptile

@trunk-io

trunk-io Bot commented Aug 24, 2026 •

Copy link
Copy Markdown

Static Badge   Static Badge   Static Badge

View Full Report ↗︎ ⋅ Docs

stamphog[bot]
stamphog Bot previously approved these changes Aug 24, 2026

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small, well-tested change to internal canary alerting logic by a STRONG-familiarity owning-team author; no risky territory (no schema/API/auth/CI changes), no outstanding concerns.

  • Author wrote 100% of the modified lines and has 7 merged PRs in these paths (familiarity STRONG).
  • 👍 on the PR from greptile-apps[bot].
Gate mechanics and policy version
Gate Result
prerequisites ✓ all clear
deny-list ✓ no deny categories matched
size ✓ 23L, 1F substantive, 48L/2F incl. docs/generated/snapshots — within ceiling
tier ✓ T1-agent / T1b-small (48L, 2F, single-area, fix)
stamphog 2.0.0b4 .stamphog/policy.yml @ f0c5caf · reviewed head f0c5caf

@stamphog
stamphog Bot dismissed their stale review August 24, 2026 14:34

A new stamphog review started for this PR — the fresh verdict replaces this approval.

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Contained fix to an internal precompute-canary alerting threshold, well tested, author has strong familiarity and owns the code; greptile's earlier semantic concern about applying the floor to non-event-count metrics is tagged outdated and was engaged with via the updated docstring rationale, with a subsequent 👍 from greptile on the current head — not a security/data-loss concern that survives unaddressed.

  • Author wrote 100% of the modified lines and has 7 merged PRs in these paths (familiarity STRONG).
  • 👍 on the PR from greptile-apps[bot].
  • Greptile flagged (P1, now outdated) that the raw MIN_CORRECTNESS_SUM_DELTA floor is applied uniformly to mean/ratio metric sums, which are in arbitrary value units rather than event counts — small-scale value-sum metrics could have real drift masked below the 100-unit floor. The author addressed the rationale in the docstring but didn't add a metric-type-aware threshold or a test covering a small-scale mean/ratio case; worth a follow-up if this canary continues to miss drift on non-funnel metrics.
Gate mechanics and policy version
Gate Result
prerequisites ✓ all clear
deny-list ✓ no deny categories matched
size ✓ 21L, 1F substantive, 56L/2F incl. docs/generated/snapshots — within ceiling
tier ✓ T1-agent / T1b-small (56L, 2F, single-area, fix)
stamphog 2.0.0b4 .stamphog/policy.yml @ 6f41878 · reviewed head 6f41878

@trunk-io
trunk-io Bot merged commit 777c2ad into master Aug 24, 2026
287 checks passed
@trunk-io
trunk-io Bot deleted the experiments/canary-absolute-floor branch August 24, 2026 15:48
@deployment-status-posthog

deployment-status-posthog Bot commented Aug 24, 2026 •

Copy link
Copy Markdown

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-08-24 16:21 UTC Run
prod-us ✅ Deployed 2026-08-24 16:54 UTC Run
prod-eu ✅ Deployed 2026-08-24 23:15 UTC Run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stamphog Request AI approval (no full review)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant