refactor(bench): RFC 0031 — split the comparative harness + dispatch class filter - #542
Merged
Merged
Conversation
Issue #499 + issue #538 item 4. The split: rfc0031_comparative.rs (6,031 lines) becomes a directory target — same single binary, same nextest group, same test names except the two Docker tests (now interop::-qualified) — cut along the seams #499 proposed: main.rs (module doc, §5 scenario tests, the dispatch run + its reports), loki.rs (container plumbing, the shared LOKI_DISPATCH_FLAGS, HTTP + measurement polls, diagnostics), interop.rs (the two PR-gated Docker tests), pickers.rs (corpus scanners + pair pickers + shared fixtures), picker_tests.rs, harness.rs (specs, gate math, latency channel, template-map probe, results record). Largest file is now 1,403 lines. Every moved item is pub(crate); item bodies are byte-identical moves. The loki-interop CI job's --exact filters are updated to the qualified names, WITH a guard: a bare name would have matched nothing and still exited 0, silently hollowing out a required check — the job now greps "2 passed" from the tee'd log (pipefail keeps real failures fatal). The class filter: OURIOS_COMPARATIVE_CLASSES (workflow input `classes`, default "all") selects which taxonomy classes a dispatch measures, so a targeted re-run — re-verifying one flaky pair — skips the other classes' measurement polls and latency reps instead of paying the full ~2h. Skipping L4 also skips pick_frequency_pair (the one expensive picker pass) and downgrades the L4-missing must-win failure to a requested skip. The filter never touches what a measured pair asserts — equivalence and the frozen gates apply to everything that runs — and the results artifact records the filter (additive `class_filter` field, null for a full run) so the trend series can tell partial runs apart. Unknown class names panic loudly. Unit tests cover the parser, the subset/full behaviors, and an exhaustiveness anchor tying PairClass::ALL to the enum. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds the RFC0031 comparative benchmark stack: deterministic pair pickers, Ourios/Loki measurements, byte and latency gates, JSON artifacts, Docker interop tests, and workflow controls for selecting benchmark classes. ChangesRFC0031 comparative evaluation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Workflow
participant ComparativeTest
participant OuriosStore
participant LokiContainer
Workflow->>ComparativeTest: set selected benchmark classes
ComparativeTest->>OuriosStore: build store and measure pair specs
ComparativeTest->>LokiContainer: start container and push OTLP corpus
ComparativeTest->>LokiContainer: poll range and matrix queries
ComparativeTest->>ComparativeTest: evaluate gates and produce JSON results
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR restructures the RFC 0031 comparative benchmark harness in ourios-bench by splitting the previously monolithic rfc0031_comparative integration test into multiple modules while preserving the single test binary entrypoint, and adds a dispatch-time “taxonomy class” filter to allow targeted comparative re-runs (e.g., only L3 or only L4) without changing any equivalence/gate semantics for the pairs that do run.
Changes:
- Split the RFC 0031 comparative integration test into a directory of modules (
main.rs+harness.rs+pickers.rs+picker_tests.rs+loki.rs+interop.rs), keeping the consolidated test binary. - Add
OURIOS_COMPARATIVE_CLASSES/ workflow inputclassesto measure only selectedPairClasscategories and record that filter in the results artifact. - Harden CI’s
loki-interopjob against silent no-op--exactfilters by using module-qualified test names and grepping for “2 passed”.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
crates/ourios-bench/tests/rfc0031_comparative/main.rs |
New module entrypoint + scenario tests + dispatch-only comparative run; applies class filtering to measured pairs. |
crates/ourios-bench/tests/rfc0031_comparative/harness.rs |
PairSpec/gate machinery, latency channel, results artifact schema, and new ClassFilter parsing/serialization. |
crates/ourios-bench/tests/rfc0031_comparative/pickers.rs |
Corpus scanning + picker logic (selective/trace/template/frequency/window) and shared fixtures. |
crates/ourios-bench/tests/rfc0031_comparative/picker_tests.rs |
Unit tests for pickers and fixtures (moved out of the monolith). |
crates/ourios-bench/tests/rfc0031_comparative/loki.rs |
Loki container plumbing, OTLP push, query/poll loops, and diagnostics; shared dispatch flags. |
crates/ourios-bench/tests/rfc0031_comparative/interop.rs |
Docker-gated interop tests (equivalence + backdated wide-range) used by CI. |
.github/workflows/comparative-bench.yml |
Adds classes workflow input and wires it to OURIOS_COMPARATIVE_CLASSES. |
.github/workflows/ci.yml |
Updates loki-interop to use module-qualified test names and adds a grep guard to fail on empty filters. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Both Copilot findings on the split PR were genuine: ClassFilter::parse now de-duplicates tokens first-seen — "L1,L1" is one class, and a list naming every class with repeats is a FULL run (artifact_value keys off the count, so duplicates previously made a full run read as partial in the results artifact). The filter moves inside build_pair_specs/class_pair_specs instead of a post-hoc retain: an excluded class must impose no preconditions — concretely, a classes=L1 dispatch previously still died on the L6 window loop's no-clean-window panic for a class it never asked to measure. Skip messages now say "SKIPPED by OURIOS_COMPARATIVE_CLASSES" rather than the misleading no-eligible-candidate text. A new regression test pins both directions: excluded classes skip their preconditions, and a REQUESTED window class still enforces its clean-window requirement (the filter selects classes; it never softens a requested class's rules). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ourios-bench/tests/rfc0031_comparative/harness.rs`:
- Around line 289-311: Update the class parsing logic in the relevant parser
constructor and its artifact_value path to canonicalize duplicate class names or
reject repeated tokens before full-set detection; ensure repeated selections
cannot be treated as PairClass::ALL. Add parser unit coverage for repeated input
such as L1,L1,L1,L1,L1,L1, preserving existing validation for unknown and empty
selections.
- Around line 51-76: Update the L4 grid and decoding flow around start, end, and
parse_loki_matrix to handle timestamps exactly equal to a bucket boundary. Add a
boundary-timestamp fixture covering a row at start, then align the evaluation
grid and decoder so count_over_time’s first window maps to Ourios’s [start,
start + width) bucket; alternatively reject such timestamps explicitly. Ensure
the existing non-boundary bucket mapping remains unchanged.
In `@crates/ourios-bench/tests/rfc0031_comparative/loki.rs`:
- Around line 260-286: Update loki_query_with_stats to return
Result<(Vec<LineKey>, u64, LokiFetchedBytes), String> instead of panicking on
transport, HTTP status, body, or parsing failures. Replace the expect/assert
paths with descriptive String errors, then update its callers in
loki_measure_pair to propagate errors into the existing deadline retry loop,
matching loki_query_matrix while preserving previously collected measurements.
In `@crates/ourios-bench/tests/rfc0031_comparative/main.rs`:
- Around line 553-565: Apply the requested class filter before invoking pickers
or constructing specs: conditionally skip excluded L1/L4 picker flows, including
template-map publication and related measurements, and make build_pair_specs
omit excluded classes before requiring their candidate windows rather than
retaining afterward. Add a unit test for a targeted run where an excluded window
has no valid candidate, verifying the run succeeds without selecting that class.
In `@crates/ourios-bench/tests/rfc0031_comparative/pickers.rs`:
- Around line 925-929: The latest-version wildcard mapping in the grouped
comparison must not be applied indiscriminately across template versions. Update
the logic around the `latest` map and its regex-derived candidate selection to
use only wildcard ordinals stable across every version, or validate each derived
group against `answer.groups` before comparison. Add a multi-version regression
unit test covering earlier rows where the latest `param(n)` is absent or maps to
a different position.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b20ee6e2-8e90-403e-b8c9-8b440349d42e
📒 Files selected for processing (9)
.github/workflows/ci.yml.github/workflows/comparative-bench.ymlcrates/ourios-bench/tests/rfc0031_comparative.rscrates/ourios-bench/tests/rfc0031_comparative/harness.rscrates/ourios-bench/tests/rfc0031_comparative/interop.rscrates/ourios-bench/tests/rfc0031_comparative/loki.rscrates/ourios-bench/tests/rfc0031_comparative/main.rscrates/ourios-bench/tests/rfc0031_comparative/picker_tests.rscrates/ourios-bench/tests/rfc0031_comparative/pickers.rs
CodeRabbit caught that loki_query_with_stats — the one remaining Loki-query helper on a measurement poll path — still used expect/assert for transport, HTTP, body, and parse errors, so a single transient blip inside loki_measure_pair's poll would unwind the async block holding every pair's already-collected measurements. Converted to Result<_, String> with retry-until-deadline in the poll loop — the identical salvage pattern loki_query_matrix and loki_query_range_uncapped received in #536; this closes the set (every Loki query helper on a measurement or diagnostic path is now panic-free). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
This was referenced Jul 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Two long-deferred items in one structural PR: the #499 harness split and #538's last item (the dispatch class filter), sequenced together because the filter touches exactly the dispatch plumbing the split reorganizes.
The split (#499)
rfc0031_comparative.rs(6,031 lines) becomes a directory target — same single binary (RFC 0028 consolidation preserved:tests/rfc0031_comparative/main.rs), same nextest group — cut along the seams #499 proposed:main.rspickers.rspicker_tests.rsharness.rsloki.rsLOKI_DISPATCH_FLAGS, HTTP, measurement polls, diagnosticsinterop.rsItem bodies are byte-identical moves; visibility is
pub(crate). Test names are unchanged except the two Docker tests, which becomeinterop::-qualified — and that renaming exposed a trap worth naming: theloki-interopjob filters by--exact, and a stale bare name would have matched nothing and still exited 0, silently hollowing out a required check. The job now greps2 passedfrom the tee'd log (with pipefail), so an empty filter is a hard failure forever.The class filter (#538 item 4)
OURIOS_COMPARATIVE_CLASSES(workflow inputclasses, defaultall): a targeted re-run — say, re-verifying one flaky pair after an L3 flicker — measures only the requested classes, skipping the other classes' measurement polls and latency reps (~2 h → substantially less; ingest still dominates the floor). Skipping L4 also skipspick_frequency_pair(the one expensive picker pass) and downgrades the L4-missing must-win failure to a requested skip.Discipline boundaries, explicitly:
class_filterfield,nullfor a full run) so the completeness trend series distinguishes partial runs from full ones.PairClass::ALLto the enum so a future variant can't silently become unfilterable.Hazards / invariants
No production code. The §7 gate semantics, equivalence rules, and measurement paths are untouched (byte-identical moves + a measurement-selection layer). The one behavioral edge — L4-missing no longer failing a run that excluded L4 — is the filter's documented purpose, and full runs (the default) behave exactly as before.
Checks run
cargo fmt --all --check; workspacecargo clippy --all-targets --all-features -- -D warnings;ourios-benchlib (134) +rfc0031_comparativeintegration tests (45 = 42 + 3 filter tests; 7 ignored Docker/dispatch). This PR's ownloki interopjob exercises the renamed Docker tests + the empty-filter guard live.🤖 Generated with Claude Code
https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
Summary by CodeRabbit
New Features
Bug Fixes
Tests