feat(bench): RFC 0031 L4 — wire into the live dispatch loop - #536
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesThe RFC0031 harness now selects bucket-aligned L4 frequency pairs, measures Loki through matrix queries with diagnostics, and compares grouped counts using a 90% completeness margin while rejecting phantom cells and over-counts. L4 comparative evaluation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PairPicker
participant L4Measurement
participant Loki
participant AggregationComparator
PairPicker->>L4Measurement: Select bucket-aligned L4 specification
L4Measurement->>Loki: Poll matrix query with bucket-width step
Loki-->>L4Measurement: Grouped samples and byte statistics
L4Measurement->>AggregationComparator: Submit grouped-count maps
AggregationComparator-->>L4Measurement: Accept within margin or record failure
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Live-wires PairClass::L4 into rfc0031_indicative_comparative_run, the #[ignore]d container-based dispatch test. The previous slice proved the L4 machinery (ourios_aggregate_answer, parse_loki_matrix, pick_frequency_pair, compare_aggregations) only at the fixture level, against a hand-built Loki matrix response — this slice makes it real against a running Loki container and the actual corpus. L4 is picked and measured as its own step, kept OUT of the `Picks`/`specs: Vec<PairSpec>` pipeline the L1/L2/L3/L6 classes share: an aggregation's (bucket, group) -> count map is not a LineKey multiset, and forcing it through OuriosAnswer/compare_lines would misrepresent the state rather than model it (the same "make invalid states unrepresentable" reasoning the miner/parquet layers already follow). Concretely: pick_frequency_pair runs post-store-build like pick_template_pair; its PairSpec is built with the exact dsl/logql shape the fixture-level test already pinned; loki_query_matrix issues a real query_range metric call with `step` pinned to the bucket width so evaluation instants land on parse_loki_matrix's documented bucket-alignment convention (t = bucket_start + width); loki_measure_frequency_pair polls it to completeness the same way loki_measure_pair does for line-returning pairs. Both share the same Loki container and corpus replay as the existing pairs. Equivalence-required-but-bytes-unasserted: RFC0031.1 (result-set equivalence) is never optional, so run_l4_pair asserts compare_aggregations(...).is_equal() unconditionally — an L4 mismatch fails the run exactly like every other class's equivalence check. Only the bytes RATIO stays unasserted (M_L4 is still §7-DEFERRED, no frozen margin to gate against yet): print_l4_report reuses print_pair_bytes_gates, which already prints L4's ratio with no verdict. L4 is measured, equivalence-checked, and reported LAST — after the L1-L3/L6 evidence has printed and their frozen gates have already asserted — so an L4-only failure cannot destroy that evidence (the same run #11 salvage lesson the rest of the harness follows). A missing candidate is reported loudly at pick time, never silently skipped. Purely additive: class_pair_specs, build_pair_specs, frozen_gate_failures, print_pair_bytes_gates, print_indicative_report, PairSpec, and PairClass are unchanged — no frozen-gate behavior for L1/L2/L3/L6 is touched. Verification: cargo fmt --all --check, cargo clippy --all-targets --all-features -- -D warnings (workspace), cargo nextest run -p ourios-bench (165 passed, 7 skipped) and cargo test -p ourios-bench --all-features all green, including the untouched fixture-level rfc0031_5_l4_frequency_aggregation_bytes. The corpus-scale dispatch test itself needs Docker + OURIOS_COMPARATIVE_CORPUS, neither available in this sandbox — its first live proof is the comparative-bench dispatch workflow, same as every other slice in this harness's history. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
jensholdgaard
force-pushed
the
rfc0031-l4-live-wiring
branch
from
July 15, 2026 10:46
d6f4416 to
f18c610
Compare
The dispatch's first-ever run failed: capture_regex's own Go RE2 escapes (\s+, \S+) were embedded inside a double-quoted LogQL string literal, which tried to interpret those backslashes as its own escape sequences (\s is not a valid one) and Loki rejected the query with "invalid char escape" before the pattern reached the regex engine. Fixed by switching to a backtick-delimited (LogQL/Go raw string) regexp argument, which passes the pattern through literally. Extracted the duplicated PairSpec-construction block (present independently in the fixture test and the live-wiring loop) into one shared l4_pair_spec helper, closing the drift risk and centralizing the fix. Added a backtick guard: a capture_regex containing a backtick (regex_escape does not escape backticks) would prematurely close the raw string, so the candidate is now rejected loudly instead of emitting a malformed query. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
…lure asserts The second dispatch run failed on the pre-existing, documented L3 Loki-side flake (0 of 9 rows before timeout) — but the run never even attempted L4: the failures.is_empty() assert for L1-L6's own salvaged measurement failures sat textually BEFORE the L4 measurement/report code, so any earlier pair's failure aborted the test before L4 was ever reached. This inverted the design intent (an L4-only failure should not destroy L1-L6 evidence, not the other way around). Moved L4's measurement to run immediately after the report prints, before the gate/failures assertions. run_l4_pair now pushes a Loki-side measurement failure (flake) into the same failures vec the other classes salvage into, instead of panicking immediately — so a flaky L4 measurement no longer aborts before the L1-L6 evidence is captured, symmetric with the fix for the reverse direction. A genuine L4 equivalence MISMATCH still hard-panics immediately, unchanged: RFC0031.1 equivalence is never optional, matching L1-L6's own compare_lines assertion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
The third dispatch got past the control-flow fix and genuinely measured L4 — but the picked candidate (a service's dominant, near-catch-all template) summed to ~971K matching rows, and Loki returned only 811,775 of them before the 300s poll deadline (the same budget every other class's loki_measure_pair uses). L4_MIN_ROWS was a floor with no ceiling, so the picker had no reason to prefer a smaller, still-meaningful candidate. Added L4_MAX_ROWS=100_000 (comfortable margin at the observed ~2.7K rows/s Loki throughput) to frequency_shape_rejection, so the picker moves on to a candidate the poll can actually finish measuring. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
Run #4's L4 pair plateaued at 11,053/11,523 rows across every 10s poll instead of climbing to completeness. The picker's row ceiling (run #3's fix) had already ruled out "too large to finish in time" — the count never moved at all, which points at a cache serving the same stale answer on every retry rather than a slow ingest. Loki's bundled local-config.yaml enables the embedded results cache for query_range's metric/matrix path (L4's loki_query_matrix), keyed by the query+start+end+step tuple that loki_measure_frequency_pair repolls unchanged. The first (still-incomplete) response gets cached and echoed back on every subsequent poll. Plain log queries (loki_query_range, used by L1-L3/L6) aren't extent-cached the same way, so they self-heal across polls untouched by this. -query-range.cache-results=false trades Loki's own query latency for correctness of the harness's completeness poll — in Loki's favour, same as the other operator-tuning flags already on this container. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
Contributor
There was a problem hiding this comment.
Pull request overview
This PR wires the RFC 0031 L4 “frequency aggregation” pair into the live, container-based comparative dispatch loop in ourios-bench, enabling first real Loki query_range matrix measurements for L4 alongside existing L1–L3/L6 pairs.
Changes:
- Refactors L4 fixture construction into a shared
l4_pair_specbuilder and adds validation for LogQL raw-string safety. - Adds an L4 row-count ceiling (
L4_MAX_ROWS) enforced by the picker to keep the Loki poll within the shared measurement deadline. - Introduces Loki metric/matrix querying (
loki_query_matrix), polling measurement (loki_measure_frequency_pair), and L4 reporting/dispatch integration (run_l4_pair+print_l4_report), with targeted tests.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Run #5 never got past container startup: `-query-range.cache-results` doesn't exist ("flag provided but not defined"), so Loki's /ready check timed out on a container that failed to start at all. Checked the pinned v3.5.3 source directly instead of guessing again: queryrangebase.Config.CacheResults is registered under the `querier.` flag prefix in roundtrip.go, not `query-range.`. Correct flag is -querier.cache-results=false. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
Run #6 (with the corrected -querier.cache-results=false flag from the prior commit) proved the results-cache theory wrong: L1-L3/L6 all measured cleanly, but L4 still plateaued — 10752/11523 rows (93.3%), even slightly worse than run #4's 95.9% pre-fix, and the shortfall varies run to run rather than repeating a fixed cached answer. That points at genuine, variable completion time rather than a bug: L4's LogQL runs a `| regexp` capture over every candidate line before grouping and counting, a real per-line cost the other classes' plain stream/count queries never pay. Widened loki_measure_frequency_pair's deadline from 300s to 900s — well inside the CI job's unset (360 min default) timeout given the whole run has taken ~95-100 min so far. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
…line theory Runs #4/#6/#7 all converged L4 to ~93-96% of expected rows, independent of poll deadline (300s vs 900s made no measurable difference) — ruling out both a results-cache echo (already disabled in #fe5915a) and a "just needs more time" theory (the deadline widening from the prior commit). A stable, time-independent shortfall points at something being permanently excluded, not merely delayed. Pulled the frozen otel-demo-v8 corpus locally and checked every log line matching the L4 pair's needle ("Wrote producer snapshot at offset") against its capture regex directly: all 11,525 matches parse cleanly. The regex/content isn't the problem — some matching lines are never being scanned at all. That points at Loki's default -validation.max-entries-limit (5000): count_over_time with a |regexp stage has to scan every raw kafka log line in a query-frontend split before the line filter narrows it down, and kafka's per-split volume exceeds 5000 lines often enough to silently truncate the scan before every matching line is reached. Raised the limit well past the corpus's noisiest single template's volume (~971K rows). Reverted the 900s deadline back to 300s (matching loki_measure_pair) — the widened deadline never addressed the actual bottleneck, and keeping it would misattribute the fix in a way that'd mislead the next reader. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
…gone Run #8 (max-entries-limit raised) moved L4 from a hard ~93% plateau to 97.1% (11192/11523) — real progress, and unlike runs #4/#6/#7 the remaining gap now plausibly behaves like genuine ingest settle time rather than a fixed ceiling, since the artificial cap that made the prior 300s vs 900s test inconclusive is gone. Widened the deadline to 600s to test that directly before assuming a third factor is at play. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ 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 |
…gnostics Run #9 (600s) measured 96.5% (11123/11523), statistically the same as run #8's 97.1% at 300s — deadline widening does nothing here, so the remaining shortfall after the entries-limit fix is a second stable cap, not settle time. Reverted the deadline back to 300s to match loki_measure_pair rather than keep an unjustified change. Wired the existing dump_loki_diagnostics helper (already used by loki_measure_pair on a deadline miss) into loki_measure_frequency_pair too — it's built around spec.logql + stats parsing, which is query-shape-agnostic, so it works unmodified for the matrix path. If L4 still falls short, the next run's failure carries the raw Loki stats (chunk-fetch counts, any warnings) instead of another guess. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
…iage loki_query_range_uncapped used expect()/assert!() internally, but it runs on the L4 deadline-miss diagnostic path inside the same runtime.block_on that gathers L1-L3/L6's evidence — a panic there (a real Loki error response, a malformed body) would unwind the whole async block and lose all of it, defeating the print-before-assert salvage design (Copilot). Converted to return Result<u64, String> instead of panicking, matching the already-panic-free sibling diagnostics (dump_loki_diagnostics et al.). Also: fix a test comment that said "one row under the ceiling" for a fixture that actually lands exactly at the ceiling; fix an unreachable! message's imprecise invariant claim (the real gating condition is l4_spec.is_some() implies l4_loki.is_some(), not "iff frequency is Some"); document loki_query_matrix's whole-second/bucket-alignment precondition and verify it against l4_pair_spec, its only caller; reorder loki_measure_frequency_pair's deadline-miss diagnostics to run only when the completeness margin is actually missed, not on every deadline-miss regardless of outcome; fix two fixture comments claiming "~300s average spacing" that don't match their own timestamps (actually ~580s) and a comment attributing the L4 shortfall to ingest-side dedup after that theory was directly disproven elsewhere in the same file. Verified the remaining ~40 accumulated review threads (mostly a recurring "shared 300s deadline" doc/code mismatch and the run_l4_pair ordering claim, duplicated across many review rounds) against current code: all already correct, superseded by earlier commits in this investigation. 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: 1
🤖 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/src/comparative.rs`:
- Line 305: Move the margin validation assertion to the start of the enclosing
benchmark function, before calling phantom_cells or any data-dependent early
return. Ensure invalid margins always panic according to the public contract,
while preserving the existing phantom/over-count logic for valid margins in both
affected locations.
🪄 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: a9cdf3fa-2049-41e2-9aa2-9d7f9393bc76
📒 Files selected for processing (3)
crates/ourios-bench/src/comparative.rscrates/ourios-bench/tests/rfc0031_comparative.rsdocs/rfcs/0031-comparative-evaluation-loki.md
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/rfcs/0031-comparative-evaluation-loki.md
- crates/ourios-bench/tests/rfc0031_comparative.rs
Validate margin at function entry rather than after the phantom/overcount checks, so an invalid margin always panics per the documented contract instead of potentially returning a data-shaped mismatch first (CodeRabbit). Fix the doc comment paragraph still describing the superseded floor-based tolerance (Copilot). Reword the L4-skip diagnostic and failure message to name both reasons l4_spec can be None (picker bounds vs a backtick in the capture regex) and to make clear the skip fails the dispatch rather than reading as benign (Copilot, two sites). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
Two Copilot findings on the previous commit, both verified genuine: The cardinality-1 exemption applied at any margin, so a caller passing margin = 1.0 (exact completeness) would still accept Loki returning 0 of 1 for an n=1 key — the exemption now only applies to a genuinely fractional margin, with a regression test covering both directions at 1.0. Bit-identical behavior at the harness's 0.90. loki_query_matrix still used expect/assert internally, so a transient transport error, 5xx, or torn body during the L4 poll — which runs LAST in the same async block holding every other pair's already-collected measurement — would panic and unwind all of it. Converted to Result<L4Measured, String>; the poll loop now retries an Err until its deadline exactly like an incomplete answer, then surfaces it as the pair's failure. Extracted the below-margin shortfall diagnostics into dump_l4_shortfall_diagnostics to stay under clippy's function-length limit. Neither change alters the measured semantics run #23 is currently confirming (the comparator formula is untouched; at margin 0.90 the exemption gating is unchanged). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
Copilot's latest pass proposed weakening phantom detection from (bucket, group_key) cells to bare group_keys so a boundary-exact record shifting into an empty adjacent bucket can't read as phantom. Declined: a systematic bucket-decode error (every cell shifted one width — the run #11 bug class) leaves every per-key total intact, so the cell-level check is the only guard that catches it, while the false positive it risks requires a nanosecond-exact bucket-boundary timestamp that no real dispatch has ever produced. Documented the trade-off on phantom_cells instead of changing behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
crates/ourios-bench/tests/rfc0031_comparative.rs:2888
- The
loki_measure_frequency_pairrustdoc currently overstates what the fallback probes prove ("whether Loki never stored the missing lines at ingest") and implies the poll only succeeds onceexpected_rowsis reached. In reality, the poll can accept a below-expected_rowsresult if overall completeness is withinL4_COMPLETENESS_MARGIN, and the plain line-filter probe still goes throughquery_rangeso it can only distinguish metric-aggregation-specific loss vs plain-query loss (not ingest vs query). Updating the doc here would keep it consistent with the later, more precise explanation indump_l4_shortfall_diagnostics.
/// The L4 pair's Loki measurement: poll the matrix `query_range` until
/// ingest has caught up to the expected total row count — the
/// aggregation counterpart of [`loki_measure_pair`]. Returns `Err`
/// instead of panicking on a deadline miss, so an L4 failure cannot
/// destroy the already-measured/printed evidence for the other pairs
This was referenced Jul 17, 2026
jensholdgaard
added a commit
that referenced
this pull request
Jul 18, 2026
…539) * test(bench): rfc 0031 l4 — property tests for the margin comparator Issue #538 item 1: compare_aggregations_within_margin accumulated five design iterations during PR #536, each edge case discovered via a real 2h dispatch or a reviewer counterexample. These seven proptest properties pin the rules those shapes are instances of — identical answers equal at any margin, the maximal admitted undercount accepted, one row past it rejected, per-key overcounts and phantom cells never tolerated, margin=1.0 agreeing exactly with compare_aggregations, and the examples cap respected — so the next formula change is caught in milliseconds instead of a dispatch. The suite is mutation-tested: reintroducing each of the three historical bug classes (the margin=1.0 exemption leak, a disabled phantom check, and run #19's ceil().max(1) tolerance formula) makes at least one property fail. The first mutation pass exposed a generator gap — a uniform 1..500 count draw made cardinality-1 keys too rare to exercise the exemption gate — fixed by biasing half the count draws into 1..4, with the rationale documented on the generator. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * test(bench): rfc 0031 l4 — review triage hardens the property suite All three PR #539 review findings were genuine, and fixing the third uncovered a real latent flaw in the suite itself: Cap property could pass vacuously when every generated key was cardinality-1 (Copilot) — now runs at margin = 1.0 where the exemption never applies and an empty Loki answer is guaranteed to mismatch, with the Equal arm removed entirely. Generator size bound was exclusive, contradicting its own doc (Copilot) — now 1..=12. The rejection property undercut every key, so a grand-total-only comparator would also have rejected its shapes (CodeRabbit) — now only the victim key drops below margin while every other key stays complete, which is the actual per-key claim. Tightening that last property exposed a float-boundary bug in the min_admitted_total helper: ceil(380.0 * 0.55) = 210 in f64 (the product computes to 209.00000000000003), but the true bound is exactly 209, which the comparator correctly admits via its epsilon — so the rejection property constructed 209 expecting rejection and spuriously failed (the pre-review shape had the same latent flaw, just never drew the input). The helper now walks down from the ceil using the comparator's own epsilon predicate, making min admitted and min - 1 rejected by construction. Mutation check re-run: the historical ceil().max(1) formula is now caught by three properties instead of two. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
jensholdgaard
added a commit
that referenced
this pull request
Jul 18, 2026
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
jensholdgaard
added a commit
that referenced
this pull request
Jul 18, 2026
…class filter (#542) * refactor(bench): rfc 0031 — split the comparative harness + class filter 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 * fix(bench): rfc 0031 — class filter applies during spec construction 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 * fix(bench): rfc 0031 — panic-safe line-pair measurement poll 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 --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
jensholdgaard
added a commit
that referenced
this pull request
Jul 18, 2026
* docs(bench): §9.17 — the L4 frequency-aggregation measurement record The PR #536 arc's outcome folded into the thesis-gates doc: the completeness-shortfall investigation summary (upstream grafana/loki #10658, every harness-side mechanism ruled out, RFC 0031 §7 margin amendment), the measured pair, and the 4-run 3.69-3.73× storage / 86.6-87.1× processed band with per-run completeness. M_L4 stays §7-deferred; the proposed freeze shape lives on #498. Closes the §9-record step for the last unmeasured must-win class. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * docs(bench): §9.17 review fixes — code-span template, run id, L2 precedent All three review findings were genuine: the template renders as inline code instead of escaped angle brackets; the first measurement row now carries its workflow id (29573249312) so the four-run series is fully auditable; and the M_L4 freeze framing mis-stated the L2 precedent — L2's freeze is processed-primary at 10x PLUS a frozen 1.1x storage-side floor, not storage-informational, and L4's 3.69-3.73x storage band would clear a similar floor with headroom. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * docs(bench): §9.17 — single-date header, first run's completeness figure Both Copilot findings valid: every listed run is 2026-07-17 (the 07-18 work was follow-on hardening, not measurement), and the first row now carries its real completeness — 1167/1197 = 97.5%, pulled from run 29573249312's job log — instead of a status word in a metric column. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * docs(bench): §9.17 — attribute the mutation-check evidence to PR #539 "Mutation-tested" read as if a standing mutation-testing harness exists in-repo; the check was a one-time manual verification during #539 (re-introduce each historical comparator bug, confirm a property catches it) whose evidence trail is that PR's record. Reworded to say exactly that. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * docs(bench): §9.17 — run #22's ratios recovered from its log, ID casing Both findings valid, and the better fix for the second was recovering the data: run 29608796312's L4 report DID print before the unrelated L3 panic (storage 3.698x, processed 86.469x, from its job log), so the row now carries real ratios instead of dashes — which also corrects the processed band's floor to 86.5x. Header casing workflow id -> ID. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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
Wires
PairClass::L4into the actual dispatch loop (rfc0031_indicative_comparative_run, the container-based test real runs execute) — the last L4 slice before a dispatch can measure frequency aggregation against Loki. Base:rfc0031-l4-harness(#534, merged).pick_frequency_pairruns post-store-build (same timing aspick_template_pair) and builds aPairSpecfrom the exact DSL/LogQL shaperfc0031_5_l4_frequency_aggregation_bytesalready pins. It's a first-fit search — the first(template_id, param)in ascending order clearing every shape floor, includingL4_MIN_AVG_INTERVAL_SECONDS— not an exhaustive ranking of every candidate in the corpus by frequency; see "18 dispatches" below for why that floor exists, and the code review section for why first-fit is the deliberate choice over a full ranking.loki_query_matrix: a realquery_rangemetric-query HTTP call withsteppinned to the bucket width, so eval instants land onparse_loki_matrix's documentedt = bucket_start + widthconvention. Kept separate fromloki_query_range/loki_query_with_stats(both streams-specific) rather than overloading them.l4_pair_specepoch-aligns the query window'sstart/endto bucket boundaries, matching Ourios's ownbucket(width)semantics.loki_measure_frequency_pairpolls to completeness likeloki_measure_pair, sharing the corpus replay + Loki container the other pairs use — with extensive on-failure diagnostics (Loki's own container logs, itsloki_discarded_samples_totalmetrics, a plain unaggregated line-count cross-check) built up over the course of this PR's investigation, all still live for any future flake; the expensive diagnostics only run when the completeness margin is actually missed, not on every deadline-miss (the margin check is now the first thing evaluated). If the picker finds no viable candidate at all, that's now a hard dispatch failure (not just a logged skip) — L4 is a must-win class and can't silently pass with zero L4 evidence.run_l4_pair: re-runsourios_aggregate_answer, asserts equivalence viacompare_aggregations_within_margin— a documented, narrowly-scoped completeness margin (RFC 0031 §7, 2026-07-17), checked pergroup_key(not a single grand total — see code review below for why), not the unconditional exact-match originally planned.class_pair_specs,build_pair_specs,frozen_gate_failures,print_pair_bytes_gates,PairSpec,PairClassare all byte-for-byte unchanged for the existing classes — L4 is additive.Why equivalence needed a documented margin
18 real dispatches. The first several fixed genuine harness bugs (LogQL escaping, a control-flow ordering bug, a missing row ceiling on the picker). What followed was a sustained investigation into a persistent, structural completeness shortfall that no harness-side fix ever fully closed — every mechanism checkable from this side came back clean:
query_rangecall).(timestamp, body)collisions found via direct corpus analysis against the frozenotel-demo-v8release (rules out Loki's documented ingester dedup — the leading theory until disproven directly).push_corpus_to_loki/push_otlpread end to end — no drop path, andpartial_success.rejected_log_recordsis asserted clean on every push, every run.level=warn/level=errorin Loki's own container logs (bar one harmless startup transient).loki_discarded_samples_total/loki_discarded_bytes_total— its dedicated accounting for silent discards, incremented even when nothing is logged.This matches an open, unresolved upstream Loki issue (grafana/loki#10658): wide-time-range queries silently missing a small, consistent percentage of lines, no error, no discard signal, no maintainer-identified root cause. A documented, external, currently-unfixable characteristic of the comparison partner — not an Ourios or harness defect.
Decision (RFC 0031 §7, full evidence trail there):
L4_COMPLETENESS_MARGIN = 0.90, real headroom over the observed 3.9–4.4% loss band.compare_aggregations_within_marginstill hard-fails, at any margin, on a phantom cell (one Loki reports that Ourios's own answer doesn't contain at all) or anygroup_keywhose total across all its buckets exceeds Ourios's — the signals that would actually indicate a query-construction or Ourios-side bug. Only aggregate under-counting, per key, up to the margin, is tolerated.Code review hardening (post run #18)
Copilot + CodeRabbit's re-review after run #18 passed surfaced one substantive gap and several real documentation/robustness issues, all verified against current code before fixing:
group_keywhile under-counting another by the same amount and still read as 100% complete: Ourios{A: 100, B: 100}vs Loki{A: 190, B: 10}sums to a "complete" 200/200 while hidingAbeing fabricated to compensate forBbeing nearly lost. Fixed by aggregating both sides bygroup_keyfirst (summing each key across every bucket it appears in), then applying the phantom/overcount/margin checks per-key — this still tolerates run feat(miner): add Drain prefix tree skeleton (RFC 0001 §6.2 step 3) #17's exact shape (a single bucket's +1 doesn't change a key's own total across its buckets) while rejecting the redistribution a pure grand-total check missed. Two new regression tests cover both shapes.examples: Vec::new()on both paths despite the function acceptingexamples_cap).#[allow(clippy::cast_precision_loss)]justification comment, added.Further review hardening (2026-07-17, post run #21)
A second review pass (Copilot + CodeRabbit) after run #21 passed surfaced one more real bug and several accumulated documentation gaps, all re-verified against current code:
ceil(o*(1-margin)).max(1)) loosened the margin for every small-but-not-1 total, not justn = 1(o = 2at 90%: tolerance 1 permits 50% completeness). Replaced with a direct, epsilon-guarded comparison —loki_total >= ourios_total * margin— which also fixed a second, self-introduced bug the first attempt at a fix (plainfloor()) had:1.0 - 0.9isn't exactly0.1inf64, sofloor(40.0 * (1.0 - 0.9))truncated to 3 instead of 4, tightening the tolerance at exact-margin boundary cases — caught by the existingmargin_comparison_tolerates_undercount_within_margintest before it ever reached CI.expected_rows) no longer pays for several seconds of unnecessary HTTP round trips.run_l4_pair's own rustdoc still claimed L4 runs after the frozen §7 gates assert, contradicting both this description and the call site — fixed at the source (it had only ever been corrected in this description across ~10 prior review rounds, never in the code comment itself).query_rangecall, so a shortfall there rules out "specific to the metric-aggregation path," not "proves Loki never stored the lines") fixed at their remaining live sites.M_L4staying deferred (it states the target contract, not something currently gated); and a comment addressing the reviewed step-grid bucket-alignment concern (the first evaluated instant decodes to an empty phantom bucket that Loki never returns a sample for, not a lost real bucket — verified againstl4_pair_spec's only caller).Measured
Run #18 (
template_id=60"Periodic task",param(0),bucket(12h), 1197 rows) was L4's first clean pass. Run #21 (2026-07-17, latest before this hardening batch) landed on the same candidate and passed again: Loki returned 1164/1197 (97.2%), comfortably inside the margin.Both reported only —
M_L4stays §7-deferred, not frozen off a handful of runs (same precedent asM_L2's deferral before RFC 0033). Run #21 passed under the pre-hardening tolerance formula; 97.2% clears the 90% margin by a wide enough gap that neither the old nor the new formula would flip the verdict for this specific pair, but a fresh dispatch is still queued to confirm the corrected formula end to end before merge, since a noisier pair in the future could land in the gap between the two.Checks run
cargo fmt --all --check; workspacecargo clippy --all-targets --all-features -- -D warnings;ourios-benchlib tests (126 passed) + localrfc0031_comparativeintegration tests (40 passed);mdbook build. All green on the latest commit (5899054). Real dispatch: run #21, green (pre-hardening-batch code).🤖 Generated with Claude Code
https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
Summary by CodeRabbit
New Features
Documentation
Summary by CodeRabbit
group_keyunder-count allowance, while always rejecting phantom records, per-group_keyover-counts, and cross-key redistribution.M_L4gating remains deferred.