Skip to content

Commit e63cc09

Browse files
justin808claude
andauthored
docs: document benchmark gate tuning plan (#3234)
## Summary - extend the library benchmarking strategy with a main-gate re-enable plan - capture the baseline dependency on PR #3148 and the need for about 30 clean main runs - define threshold, sample-size, overlap, and runner-noise acceptance criteria before restoring a hard gate Refs #3169 ## Test Plan - `pnpm exec prettier --check internal/planning/library-benchmarking.md` - `git diff --check` - pre-commit/pre-push hooks <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Low risk: changes are documentation plus a link-checker exclusion for Bencher URLs; no runtime or CI behavior is modified. > > **Overview** > Adds a detailed **“Main Gate Re-Enablement Plan”** to `internal/planning/library-benchmarking.md`, defining baseline prerequisites (post-PR-3148 history), an alert-overlap based tuning sequence, and acceptance criteria/false-positive targets before switching Bencher regressions from warnings back to a hard CI gate. > > Updates `.lychee.toml` to exclude authenticated Bencher dashboard/perf URLs from link checking to avoid CI false failures. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit dfd4250. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added a "Main Gate Re-Enablement Plan" in internal planning: describes baseline run requirements, a stepwise tuning sequence for alert sensitivity, guidance to escalate to dedicated benchmarking runners if noise persists, and acceptance criteria (consecutive-run checks, induced-regression verification, and false-positive targets) for restoring the CI main gate. * No end-user facing changes in this release. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 2d02884 commit e63cc09

2 files changed

Lines changed: 125 additions & 0 deletions

File tree

.lychee.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ exclude = [
3232
'^https://invite\.reactrails\.com/?$', # Slack invite endpoint returns 403 for automated requests
3333
'^https://docs\.google\.com', # Private docs require auth
3434
'^https://claude\.ai', # Returns 403 for automated requests
35+
'^https://bencher\.dev/(console/projects|perf)/react-on-rails-t8a9ncxo(/.*)?$', # Dashboard and perf URLs require auth; return 403
3536
'^https://(www\.)?110grill\.com/?$', # Returns 403 for automated requests
3637

3738
# ============================================================================

internal/planning/library-benchmarking.md

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,3 +59,127 @@ Options to improve accuracy if needed:
5959
2. **Adaptive rate** - Quick max-rate probe, then benchmark at 70% capacity
6060
3. **Per-route fixed rates** - Maintain target RPS config (high maintenance burden)
6161
4. **Dedicated benchmark runners** - Reduce CI noise with consistent hardware
62+
63+
## Main Gate Re-Enablement Plan
64+
65+
The benchmark workflow currently treats main-regression alerts as warnings because single-run Bencher alerts on
66+
GitHub-hosted runners have been dominated by environmental noise. The goal is to make a fired alert much more likely
67+
to represent a real regression before restoring a hard gate, where CI fails the job instead of posting a warning.
68+
69+
Standing instructions while this plan is in effect:
70+
71+
- **Wait for history before tuning.** Do not tune thresholds before the full 30-run window exists; sparse history trains
72+
on noise rather than signal.
73+
- **Fallback if the gate flips back.** If the hard gate has to return to warning mode, re-tune from the existing baseline
74+
window and overlap data first rather than starting over.
75+
- **Baseline reset exception.** Start a fresh 30-run baseline window only after benchmark workflow or runner changes
76+
invalidate the old history.
77+
- **Archive when done.** Once the hard gate has been restored and held for 30 or more qualifying runs without breaching
78+
the false-positive target, delete the entire `## Main Gate Re-Enablement Plan` section (this heading and every
79+
subsection through the end of the file, including the trailing Issue 3169 link) in a follow-up commit; the executed
80+
plan then lives only in git history.
81+
82+
### Baseline Dependency
83+
84+
The Bencher reporting baseline fix from [PR 3148](https://github.com/shakacode/react_on_rails/pull/3148) landed on
85+
2026-04-23. Do not re-enable the hard gate until at least 30 successful `Benchmark Workflow` runs on `main` have built
86+
fresh history. Count only completed `benchmark` jobs triggered after that merge; exclude pre-merge runs, branch runs,
87+
reruns of any kind (manual workflow reruns or automatic GitHub retries), and docs-only pushes skipped by
88+
[`script/ci-changes-detector`](../../script/ci-changes-detector). Record each counted run ID and timestamp in
89+
[Issue 3169](https://github.com/shakacode/react_on_rails/issues/3169).
90+
91+
### Current Bencher Configuration
92+
93+
The current Bencher invocation lives in `.github/workflows/benchmark.yml` inside the `run_bencher` function:
94+
95+
- `BOUNDARY=0.95`
96+
- `MAX_SAMPLE=64`
97+
- `--err` causes Bencher regression alerts to return a non-zero exit code
98+
- each threshold uses `--threshold-test t_test` and `--threshold-max-sample-size $MAX_SAMPLE`
99+
- `rps` uses `--threshold-lower-boundary $BOUNDARY` and `--threshold-upper-boundary _` (which disables the upper bound)
100+
because higher RPS is better; a regression is a drop below the lower bound
101+
- `p50_latency`, `p90_latency`, `p99_latency`, and `failed_pct` use `--threshold-lower-boundary _` (which disables the
102+
lower bound) and `--threshold-upper-boundary $BOUNDARY` because lower latency and failure rate are better; a regression
103+
is a rise above the upper bound
104+
- on main, `run_bencher` captures Bencher's non-zero exit as `BENCHER_EXIT_CODE`; the
105+
`Warn if Bencher detected regression on main` step emits `::warning::` for regression alerts instead of exiting
106+
- a separate main-branch step creates or updates a GitHub issue labeled `performance-regression` and links to the
107+
regression run
108+
- operational Bencher failures already hard-fail via `Fail on non-regression Bencher error on main`, so only regression
109+
alerts are soft while the gate is in warning mode
110+
- restoring the hard gate means changing the warning step to exit with `$BENCHER_EXIT_CODE` after the false-positive
111+
target is met, not removing `--err`
112+
113+
> **Note:** The values above are a snapshot of the workflow at the time of writing and capture only the tuning-relevant
114+
> flags; operational flags such as `--quiet` and `--format html` are intentionally omitted because they do not affect
115+
> threshold behavior. Verify against `.github/workflows/benchmark.yml` (`run_bencher` function) before tuning because the
116+
> workflow is the source of truth.
117+
118+
### Tuning Sequence
119+
120+
1. Keep the gate in warning mode while gathering the new baseline.
121+
2. Compare adjacent qualifying main runs by shared `(benchmark, measure)` alert pairs:
122+
- **Data source:** Use the Bencher HTML report in the workflow run summary. If browser access to Bencher is available,
123+
the history URL is `https://bencher.dev/perf/react-on-rails-t8a9ncxo`; otherwise use the overlap-comparison method
124+
tracked in [Issue 3169](https://github.com/shakacode/react_on_rails/issues/3169).
125+
- **Qualifying run:** A push that modifies at least one file that
126+
[`script/ci-changes-detector`](../../script/ci-changes-detector) does not classify as docs-only.
127+
- **Jaccard overlap formula:** For adjacent alert sets `A` and `B`, compute `|A intersect B| / |A union B|`. For
128+
example, two shared pairs across 10 total unique alert pairs gives `2 / 10 = 20%`.
129+
- **When to start:** Begin overlap analysis once at least 5 adjacent qualifying-run pairs exist (i.e., at least 6
130+
qualifying runs because each pair shares one run with its neighbor).
131+
- **Noise floor (`0.20`):** Overlap below this means runner noise is still dominating; keep collecting runs.
132+
- **Signal threshold (`0.40`):** Proceed to step 3 only after the full 30-run baseline window exists **and** overlap is
133+
at least `0.40` for 3 consecutive adjacent qualifying-run pairs. The gap between `0.20` and `0.40` avoids flip-flopping
134+
between noise and signal states; the thresholds were chosen empirically from the alert-overlap evidence in Issue 3169
135+
and should be revisited if the alert distribution changes significantly.
136+
- **Small-sample caveat:** If a comparison has fewer than 5 unique alert pairs, record it in Issue 3169 and keep
137+
collecting runs.
138+
139+
3. Prefer threshold changes that require stronger evidence before failure:
140+
- widen the Bencher boundary from `0.95` toward `0.99`
141+
- keep `--threshold-max-sample-size $MAX_SAMPLE` aligned with the available history; add a minimum-sample rule only
142+
if Bencher supports that flag for the configured threshold type
143+
- require manual tracking in Issue 3169 to see the same `(benchmark, measure)` pair alert on at least 2 consecutive
144+
runs before filing or failing (restated as Acceptance Criterion 2 below — the same gate, viewed from the tuning side)
145+
146+
If overlap remains below `0.40` after boundary widening, collect 5 more qualifying runs and re-evaluate from step 2
147+
before escalating to step 4. Escalate to step 4 unconditionally after 2 such re-evaluation cycles, which means at
148+
least 10 extra qualifying runs beyond the initial 30-run window, if overlap has not reached `0.40`.
149+
150+
4. If shared-runner noise remains high, move benchmark jobs to larger GitHub-hosted runners or dedicated runners before
151+
restoring the hard gate.
152+
153+
### Acceptance Criteria
154+
155+
1. Before restoring the hard gate, verify it can detect real regressions: add a temporary controller delay to a benchmarked
156+
SSR route large enough to cause at least 20% degradation versus the current baseline median for that route, confirm an
157+
alert fires under the tuned settings, then revert the delay. The 20% magnitude reflects the project's stated detection
158+
goal (see "Why We Chose Max Rate" above) and the fact that GitHub-hosted runner noise under `BOUNDARY=0.95` with the
159+
t-test masks smaller deltas; recalibrate this floor downward if the gate later moves to dedicated runners or boundary
160+
widening reduces the noise floor. If no alert fires, re-tune before proceeding.
161+
2. As a pre-condition for starting the 5-run clean-run count below, the tuned settings must require manual tracking in
162+
Issue 3169 to show the same `(benchmark, measure)` pair alerting on at least 2 consecutive runs before filing or
163+
failing; a single noisy run does not trigger the gate. This is a manual gate: Bencher still alerts on the first run,
164+
and the requirement is that a reviewer confirms recurrence in Issue 3169 before acting on it. (This is the same gate
165+
stated in Tuning Sequence step 3; restated here because it is also a pre-condition for the 5-run clean-run count.)
166+
3. Only after steps 1 and 2 pass, at least 5 consecutive qualifying main `Benchmark Workflow` runs complete with no
167+
Bencher regression alert; that means `BENCHER_HAS_ALERT` stays `0` with the current code (a value of `1` is what
168+
triggers the warning step and the regression-issue update). A run qualifies when the
169+
triggering push modifies at least one file that [`script/ci-changes-detector`](../../script/ci-changes-detector) does
170+
not classify as docs-only. Track the running count in [Issue 3169](https://github.com/shakacode/react_on_rails/issues/3169).
171+
4. Restore the hard gate once criteria 1-3 pass; those checks establish the project false-positive target of no more than
172+
1 noisy failure in 20 successful main `Benchmark Workflow` runs whose triggering commits do not intentionally change
173+
benchmark performance. Criterion 5 below defines who tracks this rate after re-enabling and the review cadence that
174+
triggers reverting to warning mode if the target is breached.
175+
5. After re-enabling, record each main gate failure in Issue 3169 with a noisy/real classification. Treat an alert as noisy
176+
when it does not recur for the same `(benchmark, measure)` pair in the next qualifying run and has no matching
177+
performance-sensitive code change. The 1-in-20 window is rolling: count the most recent 20 such qualifying runs (the
178+
same cohort defined in criterion 4), and exclude intentional-perf-change commits from both the numerator (noisy
179+
failures) and the denominator (the 20-run total) rather than counting them as either real or noisy.
180+
Review the running rate after every 5 gate-triggering runs or at least monthly, whichever comes first. If the gate later
181+
exceeds the 1-in-20 noisy-failure rate on main, revert it to warning mode and re-tune thresholds from the existing
182+
baseline window and overlap data before trying to re-enable it again.
183+
184+
See [Issue 3169](https://github.com/shakacode/react_on_rails/issues/3169) for the tracking discussion and historical
185+
alert-overlap evidence.

0 commit comments

Comments
 (0)