feat(bench): per-service C2 decomposition + v8 B2 pricing (#444) - #445
Merged
Conversation
The v8 capture (§9.12) failed whole-corpus C2 (0.199) entirely because of one service — kafka's broker logs fragment on short offset-bearing lines while every application service converges cleanly. Reconstructing that by hand (split the corpus, re-run the gate 5×) is toil; make it a first-class bench diagnostic instead. C2Accumulator now attributes each template creation to the minting line's service.name. Creation is a globally-monotonic event, so this is O(services) memory — no per-service id set, preserving the module's whole memory-safety argument (a non-converging corpus is exactly where a HashSet would balloon). Per-service creations partition the whole-corpus end count exactly; prints the breakdown whenever a corpus carries >1 service. The gate itself is unchanged (whole-corpus) — the breakdown is additive, so #444's scope-C2- per-service option can be judged on real numbers without a code change. §9.12 also records the B2 pricing: the deployed time/column pruning floor is identical on the fragmented (kafka) and converged (cart) services — both prune 48/49 row groups at ~3.5 ms — so fragmentation costs template-exact query *precision*, not query *latency*. That reframes #444 as a query-capability decision, not a performance one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 21 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 (4)
📝 WalkthroughWalkthroughAdds a per- ChangesC2 per-service decomposition
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 extends the ourios-bench C2 convergence gate with an additive, per-service.name diagnostic breakdown to localize non-convergence in multi-service corpora (notably otel-demo v8 / #444), and records benchmark findings about fragmentation’s impact (precision vs latency) in docs/benchmarks.md.
Changes:
- Add per-service C2 decomposition to the streaming accumulator and serialize it on
C2Result(by_service,services_truncated). - Print the per-service C2 diagnostic in the
ourios-benchCLI summary when multiple services are present. - Document v8 per-service C2 attribution and “B2 pricing” findings in
docs/benchmarks.md.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/benchmarks.md | Adds narrative + table capturing v8 per-service C2 attribution and B2 query behavior under fragmentation. |
| crates/ourios-bench/src/report.rs | Updates a unit test to construct C2Result with the new fields. |
| crates/ourios-bench/src/main.rs | Prints per-service C2 diagnostic breakdown in CLI summary output. |
| crates/ourios-bench/src/lib.rs | Extends C2Result JSON/schema with per-service decomposition structs/fields. |
| crates/ourios-bench/src/c2.rs | Implements per-service attribution and computes per-service convergence results in C2Accumulator. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/lib.rs`:
- Around line 604-606: The `by_service` field documentation is inconsistent with
the actual `C2Accumulator::record()` path: plain-text input still gets bucketed
into `UNKNOWN_SERVICE`, so `by_service` is not empty. Update the doc comment on
`by_service` in `lib.rs` to reflect that missing `service.name` values are
accumulated under an `<unknown>`/`UNKNOWN_SERVICE` entry, and keep the wording
aligned with how the benchmark harness calls `record()` for each emitted line.
In `@docs/benchmarks.md`:
- Line 1141: The sentence in docs/benchmarks.md starts with the issue reference
"`#444`", which can be parsed ambiguously and trigger markdownlint heading-style
handling. Rephrase that line so it does not begin with "`#444`" while still
explicitly referencing the issue, and keep the wording readable in prose; update
the surrounding benchmark text in the same section if needed to preserve
meaning.
🪄 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: 2db5006b-86dd-4aa2-9edb-84f7885cd306
📒 Files selected for processing (5)
crates/ourios-bench/src/c2.rscrates/ourios-bench/src/lib.rscrates/ourios-bench/src/main.rscrates/ourios-bench/src/report.rsdocs/benchmarks.md
…ose fixes Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jensholdgaard
added a commit
that referenced
this pull request
Jul 10, 2026
* feat(bench): redefine C2 gate as per-service (#444) C2 is specified over "a corpus from a single stable service", so grading a multi-service OTLP capture with one whole-corpus ratio is a category error: it conflates a noisy infra broker (high-cardinality offset/path tokens) with clean application services, failing the gate even when every application service converges perfectly (v8 §9.12). Redefine the gate to fold over the per-service decomposition (#445): a corpus passes iff every service with >= 1M lines has ratio >= 0.5, fails if any such service is below 0.5, abstains when none reach 1M. A single-service corpus — including the plain-text <unknown> bucket — collapses to the whole-corpus verdict, so historical text-corpus rows are unchanged; only multi-service OTLP corpora differ. The whole-corpus ratio is retained as a diagnostic. Under this gate the v8 capture flips FAIL -> PASS: cart is the sole service clearing the 1M-line floor and it converges at ratio 1.000; kafka's fragmentation (14,608 templates on 137k lines) is a bounded diagnostic, not a gate failure. Token-level polishing of hostile infra logs is an OTel Collector concern (transform/redaction processor upstream), not the miner's — consistent with "format parsing is the Collector's job". RFC 0006 §3.4.3 pass condition amended (maintainer-approved 2026-07-10); docs/benchmarks.md C2 grain + §9.12 v8 record updated. Cargo.lock synced (main's lock still listed async-stream for ourios-ingester after the TLS work dropped it). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3 * docs(bench): sharpen C2 single-service collapse claim; abstain wording (#444) Review follow-ups on #451: - Copilot: the single-service gate is measured at the *exact* millionth service line (created_at_1m), not the sampled whole-corpus point, so the collapse is verdict-equivalence for historical converged corpora (ratio far from the 0.5 boundary), not bit-identity with the sampled whole-corpus diagnostic. Corrected the over-claim in gate_pass's doc, RFC 0006 §3.4.3, and benchmarks.md §C2 — no behaviour change; the exact-1M measurement is strictly more precise than the diagnostic, so special-casing len==1 to the diagnostic would regress it. - CodeRabbit: §9.12 no longer says "every application service converges essentially perfectly" — cart (the sole gated service) passes; the smaller services abstain below the 1M-line floor and are not graded, though their observed counts sit at the same near-flat convergence. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3 * fix(bench): gate every >=1M C2 service; zero-template service passes (#444) Copilot review on #451: - Correctness: gate_pass inferred the gated set via filter_map(|s| s.pass), assuming pass=Some iff a service reached 1M lines — but the per-service builder left pass=None when a >=1M service minted zero templates (every line NO_TEMPLATE), so gate_pass silently dropped it and `None` meant both "below 1M" and "degenerate >=1M". Fix: the builder now gates on lines>=1M unconditionally; a zero-template >=1M service passes trivially (flat count is the strongest convergence — C2's falsifier is *linear* growth) with an undefined ratio. `None` from gate_pass now unambiguously means "no service reached 1M lines". main.rs prints the new (ratio=None, pass=Some(true)) case. Regression test added. - Docs: the module-level "Pass" / ratio definitions and the attribute() comment still described the pre-#444 whole-corpus gate; updated to the per-service gate (whole-corpus ratio is diagnostic-only). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3 * docs(bench): convergence ratio range is [0, 1], not (0, 1] (#444) Copilot review on #451: count_at_1m can be 0 (with SS > 0) when the first template is minted only after the 1M-line mark, so the ratio's defined range includes 0. Corrected the module-level c2.rs doc and RFC 0006 §3.4.3 (a >=1M service that mints *zero* templates overall stays undefined/None, distinct from ratio 0.0). Doc-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3 * fix(bench): keep C2 whole-corpus diagnostic pair consistent; lean gate_pass (#444) Copilot review on #451 (round 4): - Correctness (report.rs contract): an all-NO_TEMPLATE >=1M corpus (SS=0) produced the mixed diagnostic pair (template_count_at_1m_lines=Some(0), convergence_ratio=None), which report.rs rejects as an inconsistent C2 result — report generation would error. This was latent pre-#444; the new zero-template test made the state reachable. Fix: finalize now gates the whole-corpus count on template_count_at_end > 0 too, so the pair is always both-Some or both-None. report.rs renders the SS=0 >=1M case as "n/a (SS 0 — no templates mined)", distinct from the sub-1M "corpus < 1 M lines" abstention (uses the existing corpus_at_least_1m flag). Two regression tests: the accumulator asserts the consistent (None, None) pair; report.rs asserts the render doesn't error. - Style: gate_pass no longer allocates a temporary Vec<bool> — a single pass that short-circuits on the first failing gated service. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3 * fix(bench): reject absent-measurement corruption; precise ratio-0 wording (#444) Copilot review on #451 (round 5): - report.rs: the (None,None) && corpus_at_least_1m arm rendered "no templates mined" without checking SS==0, so a corrupt ResultsFile with corpus_at_least_1m=true and SS>0 but no measurement would render a self-contradictory row instead of erroring (the surrounding contract's intent). Tightened: the arm now requires template_count_at_end==0; any >=1M/SS>0 (None,None) falls to the inconsistency error (message broadened to state the pair is set iff >=1M with SS>0). Regression test added. - Docs: reworded the ratio-0 condition from the causal "first template minted after 1M" to the observable "no template minted as of the sample nearest the 1M mark" — count_1m is the nearest sample, not the exact millionth line, so a sample landing after 1M can already include post-1M templates. c2.rs module doc + RFC 0006 §3.4.3. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3 * docs(rfc): document C2 SS==0 semantics in RFC 0006 §3.4.3 (#444) Copilot review on #451 (round 6) — propagate the SS==0 (zero-template) semantics through the RFC spec text so it matches the implementation: - Convergence ratio is defined only when SS > 0; undefined (null, paired with a null count_at_1m) for a >=1M corpus that mints no templates. - Per-service pass rule now states the zero-template trivial-PASS exception, and that each service's count_at_1m is its *exact* millionth line (not the whole-corpus nearest-sample diagnostic). - "Reported as" paragraph: the two whole-corpus fields are null for corpora <1M *or* SS==0, and form a matched pair (both null or both set) that report.rs enforces. Doc-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3 * fix(bench): label C2 diagnostic ratio in report; stale doc + MAX_SERVICES note (#444) Copilot review on #451 (round 7): - report.rs: the C2 status-table Measurement column rendered an unlabeled "ratio …" that reads like the gate metric, but it's the whole-corpus diagnostic now — on a multi-service corpus it can be sub-0.5 with a PASS Verdict (per-service). Labeled "whole-corpus ratio … — diagnostic; verdict is per-service", matching the CLI summary's phrasing. - c2.rs: gate_pass doc still referenced "the filter_map below" after the round-4 rewrite to a for-loop; updated to describe the fold. - RFC 0006 §3.4.3: documented the MAX_SERVICES=1024 cardinality cap and the <other> fold (c2.services_truncated) — beyond the cap the gate is no longer strictly per-service; the truncation is surfaced, never silent. Doc/label-only; no behaviour change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3 * docs(bench): note zero-template trivial-PASS in C2Result doc (#444) Copilot review on #451 (round 8): the C2Result doc said every >=1M service must pass "its own ratio >= 0.5" without the zero-template exception the code and RFC 0006 §3.4.3 now carry. Added it — a >=1M service that mints zero templates (SS=0, undefined 0/0 ratio) passes trivially. Doc-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3 * fix(bench): print C2 per-service breakdown for single-service corpora too (#444) Copilot review on #451 (round 9): the CLI printed the per-service breakdown only when by_service.len() > 1, so a single-service (or plain-text <unknown>) corpus showed only the whole-corpus *diagnostic* ratio, hiding the per-service exact-1M measurement that the verdict is actually based on. Now printed whenever any service bucket exists. Extracted the per-service status formatting into a testable per_service_status() helper with a unit test covering the ratio pass/fail, SS=0 trivial-pass, and abstain states. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3 * docs(bench): sync CLI-breakdown descriptions with round-9 behaviour (#444) Copilot review on #451 (round 10) — two stale descriptions after the round-9 change (breakdown now prints for single-service corpora too): - c2.rs: stable_curve_ratio_is_one's header comment said "ratio 1.0 → pass", but the test drives observe (no service.name) and asserts the per-service gate abstains (pass=None). Reworded to "whole-corpus diagnostic ratio 1.0", matching the body comment. - benchmarks.md §9.12: said the CLI prints the breakdown only for corpora with "more than one bucket"; it now prints whenever any service bucket exists. Updated (and relabeled the decomposition the gate, not just a diagnostic). Doc-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3 --------- Co-authored-by: Claude Opus 4.8 (1M context) <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.
Turns the v8 C2 finding (§9.12) into standing measurement infrastructure, and records the B2 pricing that reframes #444.
Per-service C2 diagnostic
The v8 capture failed whole-corpus C2 (ratio 0.199) entirely because of the kafka broker (14,608 templates on 2.8% of lines); every application service converges cleanly (cart PASSes at 2.76M lines with 2 templates). I found that by splitting the corpus and re-running the gate five times — this makes it automatic.
C2Accumulatorattributes each template creation to the minting line'sservice.name. Creation is a globally-monotonic event (RFC 0001 §6.1 id allocation), so this is O(services) memory — no per-service id set, which preserves the module's whole memory-safety argument (a non-converging corpus is exactly where aHashSetwould balloon).ourios-bench --gates c2prints the breakdown when a corpus carries >1service.name.MAX_SERVICES= 1024 →<other>bucket) mirrors §3.2's ethos;<unknown>bucket for the plain-text corpus form.B2 pricing (the reframing)
The deployed time/column pruning floor is identical regardless of fragmentation. So fragmentation costs template-exact query precision (kafka's dominant event is scattered across ~11,651 ids), not query latency — #444 is a query-capability decision, not a performance one. Recorded in §9.12.
Tests
6 c2 unit tests (5 pre-existing + the new partition-exactness and unknown-service arms); the partition test encodes the monotonic-id invariant the attribution relies on.
cargo clippy --all-targets --all-features -- -D warningsexit 0,mdbook build✓, and confirmed end-to-end on the real v8 corpus (breakdown reproduces the by-hand table in one pass).The miner-side de-fragmentation (the actual thesis-value fix) stays gated on the #444 direction call.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation