Skip to content

Collapse results and alerts by default in the reports list API#926

Draft
epompeii wants to merge 1 commit into
develfrom
claude/collapse-report-list
Draft

Collapse results and alerts by default in the reports list API#926
epompeii wants to merge 1 commit into
develfrom
claude/collapse-report-list

Conversation

@epompeii

@epompeii epompeii commented Jul 8, 2026

Copy link
Copy Markdown
Member

Summary

BREAKING CHANGE: GET /v0/projects/{project}/reports now omits the results and alerts for each report by default. One console reports page could be large JSON, fully buffered by Dropshot before send, driving server memory spikes and a large amount of edge egress. Set the new expand query param to true (bencher report list --expand) to restore the previous full-report behavior; the console UI does not use it.

A new counts field is always present on every report response and mirrors the collapsed fields:

"counts": {
  "results": [
    { "benchmarks": 5, "measures": 2 },
    { "benchmarks": 5, "measures": 2 }
  ],
  "alerts": { "total": 3, "active": 1 }
}

Changes

  • JsonReport.results and .alerts are now Option with skip_serializing_if; new counts: JsonReportCounts field and expand query param
  • QueryReport::into_json takes a ReportMode { Full, Collapsed } enum so the compiler forces every call site to choose; only the list endpoint defaults to Collapsed (GET-one, POST, and bencher run stay Full)
  • Collapsed counts come from one GROUP BY aggregate over report_benchmark JOIN metric plus a status-only alert query; the full path computes counts in memory from the loaded data, and a test asserts both paths agree
  • Console: ReportRow renders from counts; the perf Reports tab lazily fetches GET-one when a report is expanded; the perf page default plot bootstrap fetches the first report via GET-one
  • CLI: new bencher report list --expand flag; bencher run reads its alert count from counts; bencher_comment handles the Option fields
  • Changelog entry with the breaking-change callout; regenerated openapi.json and bencher.ts

Compatibility

An older CLI's bencher report list cannot parse the new default list response (its generated type requires results). Everything else, including older bencher run, keeps working.

Testing

  • 4 new model unit tests (per-iteration counts, empty report, alert counts, full/collapsed agreement) and 4 new endpoint integration tests (collapsed by default, expand=true, GET-one and POST stay full)
  • Seed test asserts the collapsed list, --expand, and alert counts end to end
  • Browser smoke: public perf page default plot bootstrap, expandable report card, and branch page Recent Reports card all render correctly against a seeded server; collapsed list page was expanded even on tiny 5-benchmark seed reports

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

🤖 Claude Code Review

PR: #926
Base: devel
Head: claude/collapse-report-list
Commit: 480fcc9e12f8fc23537c5a3b4a50b1b774cf7a48


I've reviewed the full diff. Here is my assessment.

Review: Collapse results and alerts by default in the reports list API

Overall this is a well-crafted, well-tested change. It follows the project's TDD methodology (unit tests in report/mod.rs, integration tests in reports.rs, and an end-to-end assertion in seed_test.rs), uses a strong ReportMode enum instead of a bool, destructures on conversion, and is documented in the endpoint docs and changelog.

Correctness

The core risk in this design is that the two count paths disagree:

  • Full (report_counts) derives counts from the already-materialized results/alerts.
  • Collapsed (get_report_counts) uses aggregate SQL (COUNT(DISTINCT ...) grouped by iteration).

I traced both against the full-results builder (into_report_results_json) and get_report_alerts:

  • Both exclude benchmarks with no metrics (full inner-joins the metric_boundary view; collapsed inner-joins metric), so empty iterations drop from both. ✅
  • Benchmark counts agree even with a duplicate (iteration, benchmark) because full merges by UUID → 1 and collapsed uses COUNT(DISTINCT benchmark_id) → 1. ✅
  • Measures: full dedups via HashSet on measure UUID; collapsed uses COUNT(DISTINCT measure_id). ✅
  • Alerts total/active: get_report_alerts returns all alerts (no status filter), so full's total matches collapsed's SUM(count); active set once because the GROUP BY status yields one row per status. ✅
  • Iteration ordering is iteration ASC in both, so the positional counts.results arrays line up (the arrays are positional and carry no iteration index, which is fine given consistent ordering).

The into_json_full_and_collapsed_agree test locks this invariant in, and the integration test cross-checks counts against expanded results. Good defensive coverage. The u32::try_from(...).unwrap_or(u32::MAX) saturation avoids panics on the i64 → u32 narrowing.

Notable: client-facing breaking change

JsonReport.results/alerts become Option, and the list endpoint now omits them. Older CLIs (whose generated JsonReport requires those fields) will fail to parse bencher report list. This is correctly flagged BREAKING CHANGE in the changelog with mitigation ("update the CLI"; bencher run unaffected). Worth making sure this lands on a minor/major boundary as the changelog's v0.6.9 implies. No action needed, just the highest-impact thing for reviewers to be aware of.

Minor observations (non-blocking)

  • N+1 in the collapsed list path: each report still triggers two aggregate queries (get_report_counts). Lighter than the old full-row load, so no regression, but a page of N reports is 2N round-trips. If report-list latency matters at scale, a future batched query keyed by report_id would help. Fine for this PR.
  • resource_not_found_err!(ReportBenchmark, report_id) / (Alert, report_id): passing report_id as the "resource" identifier is slightly loose (these are aggregate queries, not a lookup of a specific row), but consistent with the surrounding style.
  • Frontend default-plot bootstrap (PerfPanel.tsx): the default perf plot now needs a second sequential fetch (list → full first report) before it can populate. Correct and Sentry-guarded, just a small added latency on first paint. The first_report_fetcher memo duplicates the effect's guard conditions; that's intentional (avoids the fetch when not needed) but does mean the condition list is maintained in two places.
  • ReportCard consumers: I confirmed every ReportCard/DeckCard usage is fed a full report (ReportsTab now fetches fullReport on expand; the deck view uses the single-report endpoint), so no component reads results/alerts off a collapsed payload.

Standards compliance

  • No emdashes; #[expect(...)] with reasons; strong types; OpenAPI/TS regenerated and committed; CLAUDE.md and changelog updated. ✅

Verdict: Approve. The full/collapsed count-agreement invariant is the load-bearing correctness property and it's both sound and test-covered. The only thing I'd want double-checked before merge is that the CLI-parse breaking change is acceptable for the targeted release, since it's documented as such.


Model: claude-opus-4-8

@epompeii epompeii force-pushed the claude/collapse-report-list branch from 978b032 to 210b67f Compare July 8, 2026 06:34
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

🐰 Bencher Report

Branchclaude/collapse-report-list
Testbedintel-v1
Click to view all benchmark results
BenchmarkLatencyBenchmark Result
microseconds (µs)
(Result Δ%)
Upper Boundary
microseconds (µs)
(Limit %)
Adapter::Json📈 view plot
🚷 view threshold
4.67 µs
(+0.98%)Baseline: 4.62 µs
4.90 µs
(95.21%)
Adapter::Magic (JSON)📈 view plot
🚷 view threshold
4.48 µs
(-0.42%)Baseline: 4.50 µs
4.72 µs
(94.86%)
Adapter::Magic (Rust)📈 view plot
🚷 view threshold
25.80 µs
(+1.04%)Baseline: 25.54 µs
26.64 µs
(96.87%)
Adapter::Rust📈 view plot
🚷 view threshold
3.52 µs
(+1.12%)Baseline: 3.49 µs
3.60 µs
(97.91%)
Adapter::RustBench📈 view plot
🚷 view threshold
3.53 µs
(+1.34%)Baseline: 3.48 µs
3.59 µs
(98.25%)
🐰 View full continuous benchmarking report in Bencher

BREAKING CHANGE: GET /v0/projects/{project}/reports now omits the results
and alerts for each report by default. One console reports page could be
~150 MB of JSON, fully buffered by Dropshot before send, driving server
memory spikes and edge egress. Set the new expand query param to true
(bencher report list --expand) to restore the previous behavior.

A new counts field summarizes each report: the number of benchmarks and
distinct measures per iteration and the total and active alerts. It is
always present, computed from the loaded data on the full path and with
cheap aggregate queries on the collapsed path.

The console now renders report rows from counts, lazily fetches the full
report when one is expanded on the perf page, and bootstraps the default
perf plot with a single get-one request.
@epompeii epompeii force-pushed the claude/collapse-report-list branch from 210b67f to 480fcc9 Compare July 8, 2026 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant