Skip to content

feat(nimbus): Include ration in the calculation for the SRM ratios alert#16011

Open
yashikakhurana wants to merge 8 commits into
mainfrom
16010
Open

feat(nimbus): Include ration in the calculation for the SRM ratios alert#16011
yashikakhurana wants to merge 8 commits into
mainfrom
16010

Conversation

@yashikakhurana

Copy link
Copy Markdown
Contributor

Because

  • Experiments with intentionally unequal branch ratios (e.g. 95% delivery / 5%) were triggering false SRM mismatch alerts because the check assumed equal distribution across branches

This commit

  • Passes each experiment's configured branch ratios to the SRM check so the chi-square test compares actual enrollment against the correct expected ratio, not 50/50

Fixes #16010

@yashikakhurana

Copy link
Copy Markdown
Contributor Author

@jaredlockhart Fixed two things that were causing false SRM alerts:

  • The check assumed equal branch ratios even for intentional splits like 95/5 — now it uses the actual configured ratios
  • With large populations, even a 0.2% difference was triggering the alert — added a minimum 5% deviation so it only fires when something is actually wrong

@jaredlockhart jaredlockhart left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Oh yes good, taking the designed ratio into account good catch! And actually yeah checking telemetry shows that 5% is probably exactly what we want. One thing though I think this only updates slack, don't we also need to pass the ratios in here:

is_srm, srm_p_value = check_srm_mismatch(self.monitoring_data)

To fix it in the UI?

yashikakhurana and others added 2 commits June 19, 2026 10:29
The UI path (monitoring_summary) called check_srm_mismatch without the
configured branch ratios, so experiments with intentional unequal splits
(e.g. 95/5) still showed false SRM mismatches in the UI even after the
Slack path was fixed. Pass the already-in-scope branch_ratios so the
chi-square test compares against the configured distribution.
@yashikakhurana

Copy link
Copy Markdown
Contributor Author

Oh yes good, taking the designed ratio into account good catch! And actually yeah checking telemetry shows that 5% is probably exactly what we want. One thing though I think this only updates slack, don't we also need to pass the ratios in here:

is_srm, srm_p_value = check_srm_mismatch(self.monitoring_data)

To fix it in the UI?

ahh very good catch @jaredlockhart

Both callers (Slack alerts and monitoring_summary) always pass branch
ratios now, so drop the branch_ratios=None defaults on compute_srm_p_value,
compute_max_ratio_deviation, and check_srm_mismatch. Tests that exercised
the equal-ratio fallback pass {} explicitly.
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.

Adjust SRM ration for the alert

2 participants