MetaMask / MetaMask/metamask-extension

[P2] CV-adaptive threshold widening switches off above CV_ADAPTIVE_MAX, tightening the ceiling for the least stable metrics

Open
#45,544 0 comments 0 reactions 0 assignees View on GitHub
INVALID-ISSUE-TEMPLATE ta-bot-triage
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.