Filter Bencher alerts to tracked measures (drop orphaned p90 false positives) (#3829)

justin808 · claude · web-flow · commit 6bf27ed05818 · 2026-06-08T17:16:28.000-10:00
## Problem ~99% of recent benchmark "performance-regression" alerts on `main` are false positives from a single **orphaned server-side Bencher threshold**. `p90_latency` (and `p99_latency`) were intentionally dropped from `track_benchmarks.rb` `THRESHOLDS` as too noisy ("tail noise can't meet the 1/20 target"). But Bencher thresholds are persistent server-side objects keyed on (branch, testbed, measure) — removing a measure from the CLI `--threshold-*` args stops *updating* the threshold, it never *deletes* it. The p90 metric is still submitted (for the dashboard), so the orphaned p90 threshold keeps evaluating p90 tail noise and firing alerts on nearly every run. These alerts are doubly bad: - They file a `performance-regression` issue (exit≠0), and - They are **invisible in the summary table** — the p90 column has no `:direction`, so it is never flagged 🔴. The filed issue names nothing actionable (this is the #3782 / #3795 "🔴 only appears in the legend" symptom). ### Evidence (public Bencher API, project `react-on-rails-t8a9ncxo`) - 255 most-recent active alerts: **248 p90-latency, 4 rps, 3 p50-latency**. - `main` branch: **164 active alerts → 162 p90-latency, 2 rps** (98.8% p90). - #3782 (`dec4b8c`) filed on exactly two p90 alerts — `/client_side_hello_world: Core` p90 `24.20 > upper 23.61` and `/rendered_html: Core` p90 — with no rps/p50/failed_pct alert. - `main`/`github-actions` carries thresholds for `rps, p50-latency, p90-latency, p99-latency, failed-pct` — two more than the code manages. (p99 is dormant: no p99 metric is submitted.) ## Fix Thread the measures we actually track (`THRESHOLDS` names) into `BencherReport`. An active alert on any **other** measure is classified as *filtered* rather than a regression. This reuses #3822's existing `filtered_alert?` exit-normalization, so a p90-only run no longer writes a candidate or files an issue. The fix is fully in-repo and makes the orphaned threshold harmless regardless of its server-side state. - Tracked measures (`rps`, `p50_latency`, `failed_pct`) are unaffected — including the "hidden" `failed_pct` regressions #3822 added coverage for. - Measure-less and benchmark-less alerts keep their existing fail-safe (still counted). - `tracked_measures` defaults to `nil` (track every measure) so non-production callers (`BenchmarkTable`, specs) are unchanged. ## Validation - `bundle exec rspec benchmarks/spec` — **234 examples, 0 failures** (5 new specs for tracked-measure filtering). - `bundle exec rubocop benchmarks/...` — no offenses. - **End-to-end against the real #3782 reports** (fetched from the Bencher API): `regression?` flips `true → false` (`filtered_alert? = true`) for both the Core and Pro p90 alerts, while rps/p50 alerts are untouched. - `script/ci-changes-detector origin/main` → Benchmark scripts → Lint (Ruby + JS). ## Companion cleanup (separate, manual — not in this PR) This neutralizes the orphaned threshold in code. To also stop it polluting the Bencher dashboard (162 cosmetic active alerts) and being cloned to every new branch, delete the server-side thresholds (needs `BENCHER_API_TOKEN`): - `main` p90 `51fb6a47-0083-4e84-a745-60ee42e3bba4`, p99 `6faa7a68-1835-4cd7-96dc-959220737172` - `master` p90 `d4ad2066-74cb-41f7-93bb-f0885358c56c`, p99 `61bf5ae6-77fa-474f-b6d8-ded630bd0c20` ## Relationship to other work Completes the noise fix that #3810 (fresh-runner confirmation) and #3822 (stale-alert filtering) started: #3822 only filters alerts whose metric *recovered*; a **live** p90 crossing (the dominant case) still passed through. Substantially removes the p90 tail noise behind #3169 and the issue explosion researched in #3755. Refs #3755, #3169, #3795 🤖 Generated with [Claude Code](https://claude.com/claude-code)  --- > [!NOTE] > **Medium Risk** > Changes which Bencher alerts count as regressions and can affect main-push candidate filing and CI exit behavior, though scoped to benchmark reporting with explicit specs and a nil default for other callers. > > **Overview** > Adds optional **`tracked_measures`** to `BencherReport.parse` / `#initialize` so active Bencher alerts on measures the repo no longer tracks (e.g. orphaned server-side **p90_latency** thresholds) are moved to **`filtered_alert?`** instead of **`regression?`**, reusing the existing exit-code normalization for filtered-only runs. > > **`track_benchmarks.rb`** now passes `THRESHOLDS.map(&:first)` when parsing the JSON report, so p90-only false positives no longer write regression candidates or file issues the summary table cannot flag. Tracked measures, measure-less fail-safe alerts, and callers that omit `tracked_measures` stay backward compatible. > > Specs cover orphaned p90 filtering, slug/name normalization, and a small fix for `BencherReport.new` with a Hash root in a perf-links test. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 7693f64. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup>   ## Summary by CodeRabbit ## Release Notes * **Bug Fixes** * Fixed false regression alerts for measures no longer tracked in performance benchmarks. The system now filters regression reports to only flag alerts for actively monitored metrics, preventing issue creation based on orphaned or retired threshold measurements that are no longer part of the current tracking configuration.  Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
diff --git a/benchmarks/lib/bencher_report.rb b/benchmarks/lib/bencher_report.rb
@@ -15,7 +15,7 @@
 # loud rather than mis-report), while purely informational alert fields are read
 # leniently (see #parse_alerts). The job fails loudly rather than silently rendering
 # garbage or missing a regression.
-class BencherReport
+class BencherReport # rubocop:disable Metrics/ClassLength
   class FormatError < StandardError; end
 
   # One benchmark+measure's value and its prediction interval. baseline/limits are
@@ -74,15 +74,22 @@ def mirror(limit)
 
   Alert = Struct.new(:benchmark, :measure, :limit, keyword_init: true)
 
-  def self.parse(json_string)
-    new(JSON.parse(json_string))
+  def self.parse(json_string, tracked_measures: nil)
+    new(JSON.parse(json_string), tracked_measures:)
   rescue JSON::ParserError => e
     raise FormatError, "Bencher report is not valid JSON: #{e.message}"
   end
 
-  def initialize(raw)
+  # tracked_measures: the measures the caller actually tracks (e.g. track_benchmarks.rb's
+  # THRESHOLDS names). When given, an active alert on any *other* measure is treated as
+  # filtered rather than a regression — this drops alerts from orphaned server-side Bencher
+  # thresholds (e.g. a p90_latency threshold the code stopped passing but never deleted),
+  # which otherwise file an issue that the summary table can't even flag. nil = track every
+  # measure (backward-compatible default for callers that only need parsing/significance).
+  def initialize(raw, tracked_measures: nil)
     raise FormatError, "report is not a JSON object (got #{raw.class})" unless raw.is_a?(Hash)
 
+    @tracked_measures = tracked_measures&.map { |measure| normalize(measure) }
     @boundaries = index_boundaries(raw)
     @alerts, @filtered_alerts = parse_alerts(raw).partition { |alert| current_regression_alert?(alert) }
     # Per-benchmark perf links are informational (they only decide whether a name links
@@ -190,6 +197,7 @@ def parse_alerts(raw)
   # rubocop:disable Metrics/CyclomaticComplexity
   def current_regression_alert?(alert)
     return true unless alert.benchmark
+    return false if untracked_measure_alert?(alert)
 
     direction = { "lower" => :lower, "upper" => :upper }[normalize(alert.limit)]
     return true unless direction
@@ -212,6 +220,16 @@ def current_regression_alert?(alert)
 
   def threshold_side?(boundary, direction) = direction == :lower ? boundary.lower_limit : boundary.upper_limit
 
+  # An active alert on a measure the caller does not track (an orphaned server-side
+  # threshold). Only applies when tracked_measures was given; a measure-less alert can't be
+  # classified here, so it falls through to the existing benchmark-level fail-safe.
+  def untracked_measure_alert?(alert)
+    return false unless @tracked_measures
+    return false unless alert.measure
+
+    !@tracked_measures.include?(normalize(alert.measure))
+  end
+
   def normalize(key)
     key.to_s.downcase.gsub(/[-\s]+/, "_")
   end
diff --git a/benchmarks/spec/bencher_report_spec.rb b/benchmarks/spec/bencher_report_spec.rb
@@ -207,6 +207,74 @@ def rps_regression_result(name: "/foo: Core", value: 80.0)
     end
   end
 
+  # An orphaned server-side threshold (e.g. p90_latency, which the code dropped from
+  # THRESHOLDS but never deleted in Bencher) keeps firing alerts on a measure the code
+  # no longer tracks. Those alerts are invisible in the summary table (the p90 column has
+  # no :direction) yet would still file a regression issue. `tracked_measures` lets the
+  # caller pass the measures it actually tracks so an alert on any other measure is treated
+  # as filtered, not a regression. See the orphaned-p90-threshold investigation.
+  describe "tracked-measure filtering" do
+    def p90_report(tracked_measures: :unset)
+      json = report_json(
+        results: [[benchmark_result(
+          name: "/foo: Core",
+          measures: [measure_entry(slug: "p90-latency", name: "p90_latency", value: 24.2,
+                                   baseline: 17.9, lower_limit: nil, upper_limit: 23.6)]
+        )]],
+        alerts: [alert(benchmark: "/foo: Core", measure_slug: "p90-latency", limit: "upper")]
+      )
+      tracked_measures == :unset ? described_class.parse(json) : described_class.parse(json, tracked_measures:)
+    end
+
+    it "filters an active alert on an untracked measure (orphaned p90 threshold)" do
+      report = p90_report(tracked_measures: %w[rps p50_latency failed_pct])
+      expect(report.regression?).to be(false)
+      expect(report.alerts).to be_empty
+      expect(report.filtered_alert?).to be(true)
+    end
+
+    it "still reports a regression on a tracked measure when tracked_measures is given" do
+      report = described_class.parse(
+        report_json(
+          results: [[rps_regression_result]],
+          alerts: [alert(benchmark: "/foo: Core", measure_slug: "rps", limit: "lower")]
+        ),
+        tracked_measures: %w[rps p50_latency failed_pct]
+      )
+      expect(report.regression?).to be(true)
+      expect(report.alerts.first.measure).to eq("rps")
+    end
+
+    it "matches tracked measures regardless of - / _ / case (slug vs THRESHOLDS name)" do
+      report = described_class.parse(
+        report_json(
+          results: [[benchmark_result(
+            name: "/foo: Core",
+            measures: [measure_entry(slug: "p50-latency", name: "p50_latency", value: 7.0,
+                                     baseline: 5.0, lower_limit: nil, upper_limit: 6.0)]
+          )]],
+          alerts: [alert(benchmark: "/foo: Core", measure_slug: "p50-latency", limit: "upper")]
+        ),
+        tracked_measures: %w[rps p50_latency failed_pct]
+      )
+      expect(report.regression?).to be(true)
+    end
+
+    it "counts every measure when tracked_measures is not given (backward compatible)" do
+      expect(p90_report.regression?).to be(true)
+    end
+
+    it "keeps a measure-less alert fail-safe even with tracked_measures set" do
+      measureless_alert = alert(benchmark: "/foo: Core", measure_slug: "rps", limit: "lower")
+      measureless_alert["threshold"] = {}
+      report = described_class.parse(
+        report_json(results: [[rps_regression_result]], alerts: [measureless_alert]),
+        tracked_measures: %w[rps p50_latency failed_pct]
+      )
+      expect(report.regression?).to be(true)
+    end
+  end
+
   describe "#boundary" do
     let(:report) do
       described_class.parse(
@@ -422,7 +490,7 @@ def result_for(name)
     end
 
     it "is false when the report lists no benchmarks (nothing to link)" do
-      expect(described_class.new("results" => [], "alerts" => []).perf_links_unavailable?).to be(false)
+      expect(described_class.new({ "results" => [], "alerts" => [] }).perf_links_unavailable?).to be(false)
     end
   end
 
diff --git a/benchmarks/track_benchmarks.rb b/benchmarks/track_benchmarks.rb
@@ -178,7 +178,11 @@ def run_bencher(branch, start_point_args)
     # backtrace so the failure is triageable in CI logs. This covers both the initial
     # run and the start-point-hash retry, since both go through run_bencher.
     begin
-      report = BencherReport.parse(stdout)
+      # Pass the measures we actually track so an alert from an orphaned server-side Bencher
+      # threshold (a measure dropped from THRESHOLDS but never deleted in Bencher, e.g.
+      # p90_latency) is treated as filtered rather than a regression — it would otherwise file
+      # an issue the summary table can't even flag (p90 has no :direction column).
+      report = BencherReport.parse(stdout, tracked_measures: THRESHOLDS.map(&:first))
     rescue BencherReport::FormatError => e
       warn "::error::Bencher JSON report has an unexpected shape — re-verify against " \
            "benchmarks/spec/bencher_report_spec.rb before bumping the CLI pin. #{e.message}"
