fix(bench): RFC 0031 — salvage per-pair reports + L3 timeout diagnostics by jensholdgaard · Pull Request #488 · jensholdgaard/ourios

jensholdgaard · 2026-07-12T09:01:17Z

What

Run #11's post-mortem hardening, two changes:

Salvage: loki_measure_pair returns Result instead of panicking at its deadline. The indicative run now prints the report for every successfully measured pair FIRST, then fails listing the broken ones — run feat(core): add MinerConfig — flips §3.1.1, §3.2.1, §3.2.2 #11's single L3 timeout destroyed three pairs' worth of a 40-minute run's measurements; that can't recur.
Diagnostics: a timed-out pair dumps a post-mortem to the run log before failing: the failing query's raw (truncated to 4 KB) response body and a filterless 3-entry sample of the same window. Run feat(core): add MinerConfig — flips §3.1.1, §3.2.1, §3.2.2 #11 told us only "0 of 9 rows"; run refactor(miner): split MaskTag from ParamType for exhaustiveness #12 will show WHY — whether structured metadata is absent on replayed old data, whether the response shape differs, or whether a limit bites — for a trace filter whose shape the fixture container test validates green.

The mystery being instrumented

The L3 fixture arm (same LogQL shape, same key name, live Loki container) passes in this PR's own loki-interop job, but the identical filter over the replayed v8 corpus returned 0 rows for a trace the picker verified has 9 eligible rows. The diagnostic dump is designed to discriminate the leading hypotheses in one dispatch.

Invariants / hazards

Bench harness only. The fail-loudly contract is preserved — the job still exits non-zero on any unmeasured pair; it just stops throwing away the neighbours' evidence.

Checks run

cargo fmt --all --check, cargo clippy --all-targets --all-features -- -D warnings (workspace-wide), cargo nextest run -p ourios-bench (133 passed, 13 skipped).

🤖 Generated with Claude Code

https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

Summary by CodeRabbit

Bug Fixes
- Indicative comparative runs now continue measuring remaining pairs when an individual Loki pair times out.
- Successful comparisons are reported independently from timed-out pairs.
- Timeout diagnostics now include additional response and sample information.
- Runs still clearly report and fail when any pair measurements remain unsuccessful.

Run #11's L3 timeout panicked mid-run and destroyed the three already- measured pairs' report. Pair measurements now return Result: the report prints for every measured pair, THEN the run fails listing the broken ones. A timed-out pair dumps a post-mortem first — the failing query's raw (truncated) response and a filterless sample of the same window — so the next dispatch diagnoses WHY Loki returned 0 rows for a trace the fixture-validated filter shape finds, instead of just reporting that it did. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

coderabbitai · 2026-07-12T09:01:33Z

Warning

Review limit reached

@jensholdgaard, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 14 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: f7f6579f-d812-40dc-82db-d12ef57855fc

📥 Commits

Reviewing files that changed from the base of the PR and between 3710df3 and 9acdbf2.

📒 Files selected for processing (1)

crates/ourios-bench/tests/rfc0031_comparative.rs

📝 Walkthrough

Walkthrough

The RFC0031 comparative run now treats Loki pair timeouts as per-pair failures, emits diagnostics, continues processing successful pairs, performs equivalence checks on those pairs, and asserts collected failures after reporting.

Changes

RFC0031 comparative timeout handling

Layer / File(s)	Summary
Loki measurement result handling `crates/ourios-bench/tests/rfc0031_comparative.rs`	`PairSpec` is cloneable, and `loki_measure_pair` returns successful measurements or diagnostic error strings instead of asserting immediately on timeout.
Measurement partitioning `crates/ourios-bench/tests/rfc0031_comparative.rs`	Measurement results are split into successful and failed pairs, with diagnostic output including Loki query responses and filterless samples.
Success-only comparison and deferred failures `crates/ourios-bench/tests/rfc0031_comparative.rs`	Equivalence checks and reporting use successful pairs, while collected failures are asserted after all measurements complete.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

jensholdgaard/ourios#466: Introduces the RFC0031 comparative test stubs that this harness extends.
jensholdgaard/ourios#467: Adds the compare_lines equivalence comparator used by the success-only checks.
jensholdgaard/ourios#479: Adds Loki fetched-byte handling in the same comparative measurement and reporting path.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description is informative but does not follow the required template and omits the Related section and checklist items.	Rewrite the PR text using ## Summary, ## Related, and ## Checklist, and include the required links and status items.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the benchmark hardening and timeout diagnostics change.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch rfc0031-l3-diagnostics

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

Copilot

Pull request overview

Hardens the RFC 0031 comparative bench harness so a single Loki timeout no longer discards already-measured per-pair results, and adds run-log diagnostics to explain why a Loki query timed out (especially for the L3 trace-filter case over replayed corpora).

Changes:

Change loki_measure_pair to return Result instead of panicking on deadline, enabling partial salvage of successful measurements.
Print an indicative report for all successfully measured pairs before failing the run, and list failed pairs afterward.
Add a Loki timeout post-mortem dump (truncated raw response + a broader-window sample) to stderr to aid debugging.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…ion doc Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

…ctly Same direction=forward and the same limit as the failing query, so the dump shows what the poll actually saw; the stream cap keeps it bounded. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

…rror bodies Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

…ump timeout Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

…icker diagnosed) (#490) The #488 diagnostics discriminated the L3 intermittency in one run: Loki's query_ingesters_within cutoff (default 3h) makes queries over the replayed corpus's weeks-old range skip the ingesters entirely (ingester.totalReached: 0 in the failing response), so rows still in unflushed low-volume chunks are invisible — visibility raced the flush loop. High-volume streams always flushed fast enough, which is why only the 9-row trace pair flickered while kafka pairs never did. -querier.query-ingesters-within=0 disables the cutoff: the query-side twin of reject-old-samples=false for frozen corpora, documented as the third in-Loki's-favour deviation. Also: 10 s poll interval (run #13's 2 s polling queued 321 s of engine time behind itself) and a partialSuccess assert on the OTLP push path so silently-rejected records fail at ingest, not as a downstream equivalence mystery. Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

jensholdgaard requested a review from Copilot July 12, 2026 09:01

Copilot started reviewing on behalf of jensholdgaard July 12, 2026 09:01 View session