feat(bench): RFC 0031 — three-point selectivity curve in the indicative run - #480
Conversation
|
Warning Review limit reached
Next review available in: 46 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 (1)
📝 WalkthroughWalkthroughThe RFC0031 comparative harness now selects deterministic clean time windows, measures multiple Ourios and Loki query pairs, validates per-pair equivalence, and reports additional Loki byte metrics. The workflow captures a larger report section in the GitHub job summary. ChangesRFC0031 calibration flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ComparativeRun
participant Ourios
participant Loki
participant JobSummary
ComparativeRun->>ComparativeRun: build_pair_specs
ComparativeRun->>Ourios: query each PairSpec
ComparativeRun->>Loki: query_range each PairSpec
Loki-->>ComparativeRun: rows and byte metrics
Ourios-->>ComparativeRun: expected rows
ComparativeRun->>ComparativeRun: compare per-pair multisets
ComparativeRun->>JobSummary: write expanded report
Possibly related PRs
🚥 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 |
…ve run Run #6's storage-side result (5.95x on a 1-row answer) is a single point at the extreme-selectivity end, where Ourios's fixed per-query footer/ metadata reads dominate. This extends the indicative run to a curve: the severity pair (L2 family) plus two time-window slices (~100 and ~2000 rows on the picked service, L6 family, reported under f_l6), all measured against ONE container and ONE corpus replay. Window pairs are chosen with clean edges (no shared timestamp at the start, >=2 ns gap past the end) so [a, b) selects identical rows on both systems regardless of range-end inclusivity, and windows containing any zero-time_unix_nano row's observed-fallback timestamp are rejected — both systems would return such a row with DIFFERENT answer timestamps, a guaranteed equivalence mismatch. Locally proven before spending a run: picker edge/poison/centring unit tests, a fixture pass for the timestamp collector, and a fixture-store test that the bare-service DSL + now/window mapping slices exactly [start, end). Equivalence stays asserted per pair; gates stay REPORTED under the provisional §7 margins. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
There was a problem hiding this comment.
Pull request overview
Extends the RFC 0031 indicative comparative run (in ourios-bench) from a single extreme-selectivity measurement to a three-point selectivity curve (1 row + ~100 rows + ~2000 rows) while keeping equivalence asserted per pair and bytes gates reported under provisional margins. This improves the benchmark’s diagnostic value by measuring how fixed per-query costs amortize as result sizes grow.
Changes:
- Add corpus rescanning helpers to collect per-service timestamps and deterministically pick “clean-edge” time windows (including poison timestamp avoidance) for k-row slices.
- Refactor the indicative run into a set of
PairSpecs and run/verify/report each pair against one Loki container + one full-corpus replay. - Expand the workflow job-summary grep window to include the multi-section report.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/ourios-bench/tests/rfc0031_comparative.rs | Refactors the comparative run to measure three pairs and adds helper logic + unit tests for deterministic clean window selection and poison avoidance. |
| .github/workflows/comparative-bench.yml | Widens the summary extraction (grep -A60) to capture the expanded multi-pair report block. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f8f8a93 to
494fe09
Compare
The [start, end) mapping only holds for the time-window slices; the severity pair windows the full corpus and lets the predicate select. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
The +1 was provably safe (valid() already gated it), but the checked form keeps that safety local if the two sites ever decouple. Co-Authored-By: Claude Fable 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.
🧹 Nitpick comments (2)
crates/ourios-bench/tests/rfc0031_comparative.rs (2)
938-991: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a unit test for
build_pair_specsitself.The function is pure and fully testable without a corpus/network dependency, but only
pick_window_pair/collect_service_timestamps(its dependencies) have dedicated tests. The only exerciser of the spec-assembly logic itself (margin assignment,now/windowmapping, label text) is the#[ignore]drfc0031_indicative_comparative_run, which needsOURIOS_COMPARATIVE_CORPUSand a live Loki container — it won't run in normal CI. A regression here (e.g. swappedm_l2/f_l6, or a brokennow/windowformula) would go undetected until the dispatch workflow runs.Consider a test that builds a small
SelectivePairplus syntheticclean_ts/poison_ts, callsbuild_pair_specs, and asserts onmargin,start/end, and thenow/windowinvariant per spec.As per coding guidelines: "Unit tests must be next to the code and are mandatory for anything non-trivial".
🤖 Prompt for 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. In `@crates/ourios-bench/tests/rfc0031_comparative.rs` around lines 938 - 991, Add a focused unit test alongside build_pair_specs that constructs a small SelectivePair and synthetic clean_ts/poison_ts inputs, then verifies the generated specs’ count, margins, labels, start/end values, expected_rows, and now/window mappings, including the now == end and window == end - start invariant for window specs. Keep the test independent of corpus, network, or ignored comparative-run infrastructure.Source: Coding guidelines
649-665: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winService-name matching is inconsistent with
pick_selective_pair's extraction.
pick_selective_pairbuckets resources with noservice.nameattribute under the empty-string service key (.unwrap_or_default()). Here,matches_serviceshort-circuits tofalsewhenever the attribute chain resolves toNone, so it can never matchservice == "". If the severity-pair picker ever selects the empty-string bucket, this function silently returns empty clean/poison lists, and the laterpick_window_paircall panics inbuild_pair_specsinstead of surfacing the real cause.Extracting a shared helper (used by both
pick_selective_pairandcollect_service_timestamps) that returns the resolved service string (defaulting to"") and then compares by equality would close this gap and remove the duplicated attribute-lookup logic.♻️ Proposed fix sketch
+fn resource_service_name( + rl: &opentelemetry_proto::tonic::logs::v1::ResourceLogs, +) -> String { + rl.resource + .as_ref() + .and_then(|r| r.attributes.iter().find(|kv| kv.key == "service.name")) + .and_then(|kv| kv.value.as_ref()) + .and_then(|v| v.value.as_ref()) + .and_then(|v| match v { + opentelemetry_proto::tonic::common::v1::any_value::Value::StringValue(s) => { + Some(s.clone()) + } + _ => None, + }) + .unwrap_or_default() +} ... - let matches_service = rl - .resource - .as_ref() - .and_then(|r| r.attributes.iter().find(|kv| kv.key == "service.name")) - .and_then(|kv| kv.value.as_ref()) - .and_then(|v| v.value.as_ref()) - .is_some_and(|v| { - matches!( - v, - opentelemetry_proto::tonic::common::v1::any_value::Value::StringValue(s) - if s == service - ) - }); - if !matches_service { + if resource_service_name(rl) != service { continue; }🤖 Prompt for 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. In `@crates/ourios-bench/tests/rfc0031_comparative.rs` around lines 649 - 665, Unify service-name extraction for pick_selective_pair and collect_service_timestamps by introducing a shared helper that returns the service attribute value, defaulting to an empty string when absent or non-string. Replace the matches_service attribute chain with equality against that resolved string so the empty-string bucket is matched consistently.
🤖 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.
Nitpick comments:
In `@crates/ourios-bench/tests/rfc0031_comparative.rs`:
- Around line 938-991: Add a focused unit test alongside build_pair_specs that
constructs a small SelectivePair and synthetic clean_ts/poison_ts inputs, then
verifies the generated specs’ count, margins, labels, start/end values,
expected_rows, and now/window mappings, including the now == end and window ==
end - start invariant for window specs. Keep the test independent of corpus,
network, or ignored comparative-run infrastructure.
- Around line 649-665: Unify service-name extraction for pick_selective_pair and
collect_service_timestamps by introducing a shared helper that returns the
service attribute value, defaulting to an empty string when absent or
non-string. Replace the matches_service attribute chain with equality against
that resolved string so the empty-string bucket is matched consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f7205c6f-46ae-47a3-b871-8f25e92e6f24
📒 Files selected for processing (2)
.github/workflows/comparative-bench.ymlcrates/ourios-bench/tests/rfc0031_comparative.rs
…hmetic Copilot caught a real latent hazard: the severity pair queries the full corpus window, but the picker only EXCLUDED zero-time_unix_nano rows from its bands — it never checked whether the picked predicate could select one. A zero-time row with severity >= threshold (DSL side) or carrying the pair's text at any severity (LogQL side) is returned by both systems with different answer timestamps: a deterministic equivalence failure. Runs #5/#6 passed by corpus luck, not by construction. Zero-time rows are now tallied into per-service POISON bands and any candidate whose predicate could select one is disqualified, on both directions of the predicate. Unit-tested directly on hand-built bands. Also: checked_add on the two flagged window-end computations so a corrupted corpus fails loudly instead of wrapping in release mode. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 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:763
build_pair_specsassumes the service picked for the severity pair can also supply clean window slices of k=100 and k=2000. Howeverselect_pair_candidates/pick_selective_paircurrently select purely on the rare-severity band (1..=4000 rows) and do not ensure the chosen service has >=2000 clean records; in that case the dispatch-only run willpanic!("no clean 2000-row window …")even though a different service/threshold could work. Consider filtering candidates (or services) up-front to require at least the largest slice size worth of clean rows, or repicking whenpick_window_pairreturns None.
for (svc, (bands, poison)) in per_service {
if !safe(svc) {
continue;
}
for &threshold in bands.keys() {
What
Extends the RFC 0031 indicative comparative run from a single measured pair to a three-point selectivity curve, all against one Loki container and one full-corpus replay:
m_l2(10)f_l6(3)f_l6(3)Why
Run #6's refined storage-side number — 5.95× on a 1-row answer (vs 182.4× on decompressed
totalBytesProcessed) — is a single point at the extreme-selectivity end, where Ourios's fixed per-query footer/metadata reads dominate the bytes figure. The testable prediction: that fixed cost amortizes as the result set grows while Loki's chunk scan grows with it. This PR measures the curve instead of arguing about the point. Gates stay REPORTED, not asserted (§7 margins provisional); equivalence stays asserted per pair.How the window pairs stay honest
[a, b)is only picked if no earlier record sharesaand the next record past the window sits ≥ 2 ns beyondb = last + 1— so both systems select identical rows regardless of range-end inclusivity semantics.time_unix_nanorow's observed-fallback timestamp is rejected. Both systems would return such a row (Ourios windows the RFC 0005 effective timestamp; Loki stamps observed time), but with different answer timestamps — a guaranteed equivalence mismatch, so the picker refuses rather than letting the run die late.Locally proven before spending a dispatch run
pick_window_pairunit tests: centring, dirty-edge avoidance, poison avoidance, insufficient-inputNone.collect_service_timestampsfixture pass (clean/poison split, unknown service empty).window_pair_dsl_slices_the_fixture: builds the fixture store and proves the bare-service DSL parses and thenow = end / window = end − startmapping slices exactly[start, end)against the querier'sts ≥ now − window ∧ ts < nowfilter.Invariants / hazards
Bench-harness + workflow-summary change only; no hot path, no schema, no miner change. The Loki config deviations are unchanged from #478 (all ingest-side, all in Loki's favour, documented at the flag site). The job-summary grep widens (
-A8→-A60) for the multi-section report.Checks run
cargo fmt --all --check,cargo clippy --all-targets --all-features -- -D warnings,cargo nextest run -p ourios-bench(120 passed, container tests#[ignore]d as designed). The dispatch run itself is the CI-only validation, like the prior six.🤖 Generated with Claude Code
https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
Summary by CodeRabbit
New Features
Bug Fixes
Tests