Skip to content

fix(bench): RFC 0031 — generalize the pair picker (v8 has no ERROR logs) - #475

Merged
jensholdgaard merged 7 commits into
mainfrom
rfc0031-pair-picker-generalize
Jul 11, 2026
Merged

fix(bench): RFC 0031 — generalize the pair picker (v8 has no ERROR logs)#475
jensholdgaard merged 7 commits into
mainfrom
rfc0031-pair-picker-generalize

Conversation

@jensholdgaard

@jensholdgaard jensholdgaard commented Jul 11, 2026

Copy link
Copy Markdown
Owner

What

Run #1 of the indicative comparative (29160983634) failed fast and honestly (12 s, before any container): the scan found zero ERROR-severity rows in the entire 4.9 M-record v8 capture. The calibration manifest confirms the corpus's complete severity distribution: INFO ×2,192,261, "Information" ×2,756,331, WARN ×4 — the demo's adFailure/paymentFailure flags surface in traces/metrics, not logs. The picker refused to fabricate a pair rather than compare nonsense — exactly its job.

Fix

pick_error_pairpick_selective_pair: instead of a hardcoded ERROR band, scan per-service (severity_number, severity_text) row counts and pick any (service, threshold T, text t) where every row with number ≥ T carries the single text t, count 1..=4000. The consistency requirement still makes DSL severity >= T ↔ LogQL severity_text="t" the same question; fewest rows wins (ties → lowest threshold → smallest service).

  • On v8 this selects the WARN band (≥ 13, 4 rows) — an extremely selective, fully honest L2 pair.
  • On error-bearing corpora it selects ERROR exactly as before — the fixture unit test now pins that (threshold 17, text "ERROR").

After merge

Re-dispatch comparative-bench.yml → run #2 proceeds past the picker into the store build, the 4.9 M-record Loki replay, equivalence at scale, and the first number.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Tests
    • Expanded comparative calibration coverage to support any single severity-text band, including WARN-only services.
    • Improved selection validation across severity thresholds and text labels.
    • Updated dispatch comparisons to use dynamically selected severity criteria for consistent Ourios and Loki results.

Run #1 of the indicative comparative (29160983634) failed fast (12 s,
before any container) in exactly the designed way: the corpus scan found
ZERO severity>=17 rows in the whole 4.9 M-record otel-demo v8 capture —
the calibration manifest confirms the corpus's complete severity
distribution is INFO x2.19M, "Information" x2.76M, WARN x4 (the demo's
adFailure/paymentFailure flags surface in traces/metrics, not logs) —
and the picker refused to fabricate a pair.

pick_error_pair → pick_selective_pair: instead of a hardcoded ERROR
band, scan per-service (severity_number, severity_text) row counts and
pick any (service, threshold T, text t) where EVERY row with number>=T
carries the single text t and the count is 1..=4000. The consistency
requirement still makes DSL `severity >= T` and LogQL
`severity_text="t"` the same question. On v8 this selects the WARN band
(>=13, 4 rows — an extremely selective honest L2); on error-bearing
corpora it selects ERROR exactly as before, which the fixture unit test
now pins (threshold 17, text ERROR).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@jensholdgaard
jensholdgaard requested a review from Copilot July 11, 2026 17:22
@coderabbitai

coderabbitai Bot commented Jul 11, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

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

Next review available in: 52 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: d9b4a3dd-fa26-401f-8bd9-595b4f891624

📥 Commits

Reviewing files that changed from the base of the PR and between 291fbfa and e1fb684.

📒 Files selected for processing (1)
  • crates/ourios-bench/tests/rfc0031_comparative.rs
📝 Walkthrough

Walkthrough

The RFC0031 comparative test now dynamically selects a consistent severity-text band, derives its severity threshold, and applies the selected values to both Ourios DSL and Loki LogQL comparisons. Picker tests cover ERROR and WARN bands.

Changes

RFC0031 selective-pair calibration

Layer / File(s) Summary
Corpus counting and pair model
crates/ourios-bench/tests/rfc0031_comparative.rs
Introduces SelectivePair and generic per-service, severity-number, and severity-text counting while preserving timestamp filtering and window tracking.
Selective candidate enumeration
crates/ourios-bench/tests/rfc0031_comparative.rs
Derives valid threshold bands, enforces row-count and text-consistency rules, and filters unsafe service and text names.
Comparative query integration and tests
crates/ourios-bench/tests/rfc0031_comparative.rs
Uses the selected threshold and text in Ourios and Loki queries, updates diagnostics, and tests ERROR and WARN fixture selections.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant RFC0031 comparative run
  participant pick_selective_pair
  participant Ourios DSL
  participant Loki LogQL
  RFC0031 comparative run->>pick_selective_pair: select service, threshold, and text
  pick_selective_pair-->>RFC0031 comparative run: return rows and time window
  RFC0031 comparative run->>Ourios DSL: query severity >= threshold
  RFC0031 comparative run->>Loki LogQL: query severity_text = text
  Ourios DSL-->>RFC0031 comparative run: return matching rows
  Loki LogQL-->>RFC0031 comparative run: return matching rows
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: generalizing the benchmark pair picker beyond ERROR logs.
Description check ✅ Passed The description explains the change and motivation well, but it does not follow the template's exact Summary, Related, and Checklist sections.
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-pair-picker-generalize

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 AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the RFC0031 comparative benchmark “pair picker” to handle real-world corpora that may have no ERROR logs (e.g., otel-demo v8), by dynamically selecting a highly selective (service, severity threshold, severity text) pair that should produce a small, exactly-equivalent result set across Ourios (DSL) and Loki (LogQL).

Changes:

  • Replace pick_error_pair with pick_selective_pair, scanning per-service severity bands to choose the smallest consistent band under the 5000-line cap.
  • Update the indicative comparative run to parameterize both the DSL threshold and LogQL severity_text based on the picked pair.
  • Update/rename the unit test to assert the fixture still selects the ERROR band (threshold == 17, text == "ERROR").

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

Comment thread crates/ourios-bench/tests/rfc0031_comparative.rs Outdated
Comment thread crates/ourios-bench/tests/rfc0031_comparative.rs Outdated
Comment thread crates/ourios-bench/tests/rfc0031_comparative.rs Outdated
Copilot review, all three:
- REVERSE consistency (real equivalence hole): the check guaranteed all
  num>=T rows share text t, but not that no LOWER-severity rows carry t
  — LogQL's text filter would return those too and break equivalence at
  container time. A candidate now also requires the service's total
  count for text t to equal the selected rows.
- Nested map (service -> number -> text -> count): thresholds are the
  deduped number keys (no recomputation per (num,text) pair).
- Severity texts clone only on FIRST occurrence per (service, number)
  via get_mut-then-insert — not one clone per record across millions.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread crates/ourios-bench/tests/rfc0031_comparative.rs
Comment thread crates/ourios-bench/tests/rfc0031_comparative.rs
pick_selective_pair hit 106/100 after the reverse-consistency addition;
extract select_pair_candidates. No behavior change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread crates/ourios-bench/tests/rfc0031_comparative.rs
Copilot review:
- pair.service / pair.text are interpolated into quoted DSL and LogQL
  string literals; a `"` or `\` (legal in OTLP attributes) would break or
  change either query. Rather than implement escaping for two query
  languages, a candidate whose names fall outside a conservative charset
  is simply skipped.
- Doc: candidate thresholds are the OBSERVED severity numbers, which is
  complete — a gap threshold selects exactly the same rows as the next
  observed number above it, adding no new candidates.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread crates/ourios-bench/tests/rfc0031_comparative.rs
A v8-shaped miniature fixture (INFO-dominated + one WARN, zero ERROR)
pins that the picker selects the WARN band — the exact path run #1
surfaced.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread crates/ourios-bench/tests/rfc0031_comparative.rs Outdated
Copilot review: a zero-ts record can't be returned by either side's
time-windowed query, so counting it into a band would fail the pair's
expected-count check at run time. Skip it for bands (still counted in
the total-records report figure).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread crates/ourios-bench/tests/rfc0031_comparative.rs Outdated
Copilot review: the effective-timestamp fallback means BOTH systems can
return a time=0/observed-set record (Ourios windows the RFC 0005 §3.2
effective column; Loki's OTLP ingest falls back to observed) — but with
DIFFERENT answer timestamps (Ourios keeps time_unix_nano=0, Loki stamps
observed), so their LineKeys can never match. Exclusion is required for
key identity, not window reachability; the comment now says so.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

@jensholdgaard
jensholdgaard merged commit 01069ad into main Jul 11, 2026
26 checks passed
@jensholdgaard
jensholdgaard deleted the rfc0031-pair-picker-generalize branch July 11, 2026 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants