MetaMask / MetaMask/metamask-extension
[P2] CV-adaptive threshold widening switches off above CV_ADAPTIVE_MAX, tightening the ceiling for the least stable metrics
- Dominant language
- TypeScript
- Stars
- 13.2k
- Forks
- 5.6k
- Avg merge
- 2d 5h
- Merged PRs (30d)
- 451
Description
**Parent:** [#7207 Statistical Sanitization](https://github.com/MetaMask/MetaMask-planning/issues/7207)
**File:** `test/e2e/benchmarks/utils/statistics.ts`
**Size:** S | **Hours:** 2–4
---
## Problem
`computeCvAdjustment` returns `undefined` outside a fixed band, so CV-adaptive threshold widening has two discontinuities and each caller decides for itself what `undefined` means. Verbatim at `main` `0350a01878e0d5e0eb1553b18ca7d653b4157cbb`, [`statistics.ts` L421-L437](https://github.com/MetaMask/metamask-extension/blob/0350a01878e0d5e0eb1553b18ca7d653b4157cbb/test/e2e/benchmarks/utils/statistics.ts#L421-L437):
```ts
export const CV_ADAPTIVE_MIN = 25;
export const CV_ADAPTIVE_MAX = 50;
export const computeCvAdjustment = (cv?: number): number | undefined => {
if (cv === undefined || cv < CV_ADAPTIVE_MIN || cv > CV_ADAPTIVE_MAX) {
return undefined;
}
return 1 + cv / 200;
};
```
A metric at cv 49 gets `1.245×`; at cv 24 it gets `1.0×`; with no derivable cv it gets `1.0×`. A 1-point change in measured CV across either boundary moves the effective ceiling by up to 24.5%, which is a cliff rather than a taper.
### What actually happens above the cap
Above `CV_ADAPTIVE_MAX` the metric is not gated at a tighter ceiling. It is dropped from validation, by a separate check in each of the two callers:
- [`validateResultThresholds`](https://github.com/MetaMask/metamask-extension/blob/0350a01878e0d5e0eb1553b18ca7d653b4157cbb/test/e2e/benchmarks/utils/statistics.ts#L636-L639) — the path the PR gate takes, via `compareBenchmarkEntries` — derives `cv` from `results.mean` and `results.stdDev` and `continue`s on `cv > CV_THRESHOLDS.POOR` (50) before any percentile is checked.
- [`validateThresholds`](https://github.com/MetaMask/metamask-extension/blob/0350a01878e0d5e0eb1553b18ca7d653b4157cbb/test/e2e/benchmarks/utils/statistics.ts#L591-L593) — the `TimerStatistics` path used by the runner — `continue`s on `stats.dataQuality === 'unreliable'`, which [`assessDataQuality`](https://github.com/MetaMask/metamask-extension/blob/0350a01878e0d5e0eb1553b18ca7d653b4157cbb/test/e2e/benchmarks/utils/statistics.ts#L249-L259) returns for `cv >= 50`.
So the `cv > CV_ADAPTIVE_MAX` branch of `computeCvAdjustment` is unreachable through either caller wherever a CV is derivable, and the demotion that option 3 below proposes is de facto already the behavior — implemented twice, outside the function that owns the policy, in two forms that do not agree.
**They disagree at exactly cv = 50.** `validateResultThresholds` uses strict `>`, so cv = 50 passes through and receives 1.25× widening; the code comment there states this is deliberate. `assessDataQuality` classifies cv = 50 as `unreliable`, so `validateThresholds` skips it. One measured value, two opposite outcomes depending on which entry point ran.
### The reachable defect is at the other end, and where no CV exists
Two cases reach `getEffectiveThreshold` with no widening and are still gated:
- **cv below `CV_ADAPTIVE_MIN`.** Intended per the doc comment ("below 25% the metric is already stable enough that widening would mask real regressions"), and still a 12.5% step at the boundary rather than a taper to 1.0×.
- **cv not derivable at all.** Neither skip fires on `undefined`, and `computeCvAdjustment(undefined)` returns `undefined`, so the metric is gated at `base × ciMultiplier` with no widening and no record that no adjustment was possible. [`performance-benchmarks.ts` L614-L620](https://github.com/MetaMask/metamask-extension/blob/0350a01878e0d5e0eb1553b18ca7d653b4157cbb/development/metamaskbot-build-announce/performance-benchmarks.ts#L614-L620) constructs a per-metric `BenchmarkResults` carrying only `p75` and `p95`, so every metric validated through that path takes this branch.
`undefined` therefore encodes three different situations — too stable to widen, too unstable to gate, and unmeasurable — and each caller resolves the ambiguity differently.
### Observed, not hypothetical
`onboardingNewWallet.total` landed on both sides of a boundary in two runs three days apart, same metric, same flow, same browser:
| run | widening | effective p75 ceiling | severity |
|---|---|---|---|
| [31787838711](https://github.com/MetaMask/metamask-extension/actions/runs/31787838711/job/94733708923) (PR #45432 iteration logging, `cd0c0ee28a4`) | none — cv outside band | 4200 × 1.3 = **5460ms** | 🔴 `Fail` |
| [31798462138](https://github.com/consensys-test/metamask-extension-test-majorlift/actions/runs/31798462138/job/94767086391) (fork arm, `2fd60be`) | 1.2315× | **6724ms** | 🟡 `Warn` |
The implied CV backs out identically from both percentiles in the second run — `6724/(4200×1.3) = 1.231502` → 46.30%, and `7364/(4600×1.3) = 1.231438` → 46.29% — so the widening is confirmed rather than inferred. Both base values are current: `onboardingNewWallet.total` is p75 fail 4200 / p95 fail 4600 at [`thresholds.ts` L124-L128](https://github.com/MetaMask/metamask-extension/blob/0350a01878e0d5e0eb1553b18ca7d653b4157cbb/test/e2e/benchmarks/utils/thresholds.ts#L124-L128), `CI_MULTIPLIER.TIER_1` = 1.3.
Which boundary the first run fell outside is not recoverable: its job (`run-benchmarks / quality-gate`, completed `failure` at 2026-08-14T09:50:37Z) is past GitHub's log retention, and `GET /actions/jobs/94733708923/logs` returns `BlobNotFound` — as does the same call against a job completed on 2026-09-02, so the failure says nothing about that run in particular. Given the skip behavior above, `cv > 50` would have produced no violation at all rather than a `Fail`, which leaves `cv < 25` and no-derivable-cv as the candidates.
The consequence is that a metric's ceiling is not stable between runs, and the instability is largest for the metrics already hardest to gate. For a bimodal metric whose CV straddles a boundary, which side a run lands on is itself a coin flip, stacked on top of the coin flip in the measurement.
---
## Solution
Make the adjustment continuous and monotone in CV over its whole domain, and give `undefined` one meaning. Options, in rough order of preference:
1. **Route the three cases separately.** Return a discriminated result — `widen(x)`, `no-widening` (below the floor), `not-gateable` (above the cap), `unmeasurable` (no cv) — so callers stop each inventing their own skip and the two paths cannot disagree at cv = 50. This subsumes option 3 and is the only option that removes the duplicated policy.
2. **Clamp instead of disabling.** Above `CV_ADAPTIVE_MAX`, return the adjustment at the cap (`1 + CV_ADAPTIVE_MAX/200 = 1.25×`) rather than `undefined`. Smallest change, but it makes the function's return value contradict the skips that fire first.
3. **Taper.** Continue widening above the cap at a reduced slope, with a hard ceiling on total adjustment.
Whichever is chosen, `CV_ADAPTIVE_MIN` needs the same decision: below 25 the adjustment is also `undefined`, which is correct only if 1.0× is genuinely intended there, and the boundary is a step either way. The no-derivable-cv case needs an explicit answer too, since it is the one that gates a metric on an unwidened ceiling without saying so.
---
## Acceptance Criteria
- [ ] `computeCvAdjustment` is continuous across `CV_ADAPTIVE_MAX` and `CV_ADAPTIVE_MIN` — no discontinuity in effective ceiling for a 1-point CV change at either boundary
- [ ] The adjustment is monotone non-decreasing in CV over its whole domain
- [ ] Behavior above the cap is explicitly chosen and documented (clamp / taper / route-to-demotion), not a fall-through, and the two callers' skips agree with it — including at exactly cv = 50, where they do not
- [ ] A metric with no derivable CV is handled explicitly: either skipped or gated with the absence recorded on the violation, never silently unwidened
- [ ] Unit tests cover cv just below, at, and just above both `CV_ADAPTIVE_MIN` and `CV_ADAPTIVE_MAX`, plus `cv === undefined`, asserting no cliff and one consistent outcome per value across both entry points
- [ ] Given the same measured CV, a gated metric's effective ceiling is reproducible across runs and across entry points
---
## Dependencies
**Related:** [metamask-extension#45266 onboarding benchmarks are bimodal](https://github.com/MetaMask/metamask-extension/issues/45266) — open; the slow path behind the straddling CV. [metamask-extension#45205 restore benchmark gate signal](https://github.com/MetaMask/metamask-extension/issues/45205) — open; recalibration. [metamask-extension#45444 demote the three bimodal onboarding metrics](https://github.com/MetaMask/metamask-extension/pull/45444) — merged 2026-08-28; its argument that no threshold value is correct against a bimodal null is strengthened by this cliff, and it has already taken `onboardingNewWallet.total` off the allowlist, so the run pair above can no longer block a PR. [metamask-extension#45935 the gate's only blocking metric has overlapping pass and fail ranges](https://github.com/MetaMask/metamask-extension/issues/45935) — open; the same CV-dependent ceiling measured on the one metric that still blocks
Contributor guide
Research direction
Start in test/e2e/benchmarks/utils/statistics.ts at computeCvAdjustment, validateThresholds, validateResultThresholds, and assessDataQuality; compare their handling of CV boundaries and undefined values. Also inspect development/metamaskbot-build-announce/performance-benchmarks.ts and test/e2e/benchmarks/utils/thresholds.ts. Done means the chosen policy is documented, both entry points agree, and boundary and undefined-CV tests pass.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- performance, testing-qa
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100