fix(bench): handle Body::Structured records in harness + C1 (PR-K4) - #61
Merged
Conversation
Surfaced by the PR-K3 OTLP-fixture CI trigger: the harness crashed with a contract violation, and the Parquet writer rejected the fixture's kvlist body + non-empty attributes. Both gaps trace to the deferred RFC 0005 §3.3 canonicalisation PR — fixed here for the surfaces the bench owns, documented for the surfaces it doesn't. **Harness (the immediate contract violation):** RFC 0001 §6.1 assigns `Body::Structured` records a sentinel template id outside the Drain tree, so `cluster.templates_for(...)` correctly returns no leaf for the sentinel — but the harness's `want_snapshot` predicate assumed every non-lossy / non-`NO_TEMPLATE` record had one, raising `BenchError::Pipeline`. The predicate now narrows to `BodyKind::String`; structured records get `snapshot = None` like lossy ones do. **C1 (symmetric):** structured records are excluded from C1's denominator too. Per RFC 0001 §6.4 / RFC 0003 §6.4, structured reconstruction is the storage-layer `AnyValue`-bytes round-trip, *not* template + params, so the template-based equality C1 measures doesn't apply to them. Structured ≠ lossy (independent axes — a structured record can be high-confidence). **OTLP loader (bench-side workarounds for the Parquet writer's canonicalisation gaps):** - `attributes` / `resource_attributes` stripped to empty `Vec`s in `map_log_record`, with a TODO pointing at RFC 0005 §3.3 and the loader test pinning the strip so the revert PR has to update it (closing the loop). The receiver-amendment PR restores the 1:1 mapping when canonicalisation lands. - The kvlist body record removed from the committed fixture so the workflow can actually run on `crates/ourios-bench/tests/data/otlp` today (the writer rejects `Body::Structured` rows for the same RFC 0005 §3.3 reason). Loader coverage of the structured-body mapping moved to an inline-synthetic test, so the loader still pins the `Body::Structured(AnyValue)` round-trip per RFC 0003 §6.4 — it just doesn't sit in a fixture the writer chokes on. **RFC 0006 §3.4.2 amended** to document the C1 structured-body exclusion alongside the existing lossy exclusion. **Test plan:** ran `ourios-bench --corpus crates/ourios-bench/tests/data/otlp` locally; result was 3 records, **C1 PASS (3/3 non-lossy)**, A1 FAIL (expected on a small corpus), C2 ABSTAIN. +1 colocated test `c1::tests::structured_body_record_is_excluded_from_denominator` pins the C1 exclusion. fmt + clippy + 286 tests + mdbook all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR excludes BodyKind::Structured records from C1 non-lossy reconstruction metrics, refines the harness to capture snapshots only for String bodies, strips attributes from OTLP-loaded records, and updates tests, fixtures, and RFC documentation accordingly. ChangesStructured Body Exclusion and OTLP Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the bench harness to handle OTLP structured bodies and current Parquet writer limitations without failing the end-to-end bench path.
Changes:
- Skips template snapshot lookup and C1 denominator accounting for
BodyKind::Structuredrecords. - Temporarily strips OTLP log/resource attributes in the bench loader until RFC 0005 §3.3 canonicalization lands.
- Removes the structured-body row from the committed OTLP fixture while preserving structured-body loader coverage via an inline synthetic test.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
docs/rfcs/0006-bench-harness.md |
Documents the structured-body C1 exclusion and matching harness snapshot behavior. |
crates/ourios-bench/tests/data/otlp/sample.jsonl |
Keeps the committed OTLP fixture limited to Parquet-writable string-body records. |
crates/ourios-bench/src/harness.rs |
Avoids templates_for() snapshot lookup for structured-body records. |
crates/ourios-bench/src/corpus.rs |
Strips attributes/resource attributes for now and adds inline structured-body mapping coverage. |
crates/ourios-bench/src/c1.rs |
Excludes structured-body records from C1 reconstruction accounting and adds regression coverage. |
💡 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/corpus.rs`:
- Around line 427-442: Update the function/module contract comments to reflect
that attributes and resource_attributes are currently stripped (assigned to
Vec::new()) rather than preserved 1:1; locate the docstrings around the code
that mentions 1:1 attribute/resource mapping and change them to state that
attributes and resource attributes are intentionally dropped (and reference
dropped_attributes_count is preserved) until canonical-JSON KeyValue support is
added, and mention that the loader test pins this behavior so changes must
update the test.
In `@crates/ourios-bench/src/harness.rs`:
- Around line 138-154: The snapshot predicate was tightened to require
BodyKind::String (let want_snapshot = !record.lossy_flag && record.template_id
!= NO_TEMPLATE && matches!(record.body_kind, BodyKind::String)), so update the
run rustdoc examples and the non_lossy_callbacks_carry_a_template_snapshot test
to reflect that non-lossy records with non-String BodyKind now produce snapshot
= None; change assertions to check record.body_kind == BodyKind::String before
expecting Some(snapshot), update any explanatory text in the rustdoc to describe
the new BodyKind gate, and add/relocate unit tests next to harness.rs verifying
both paths (String → Some, Structured/other → None) as per the crate's colocated
test guideline.
🪄 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: e143df83-72b4-467f-8265-0c513e885f1a
📒 Files selected for processing (5)
crates/ourios-bench/src/c1.rscrates/ourios-bench/src/corpus.rscrates/ourios-bench/src/harness.rscrates/ourios-bench/tests/data/otlp/sample.jsonldocs/rfcs/0006-bench-harness.md
💤 Files with no reviewable changes (1)
- crates/ourios-bench/tests/data/otlp/sample.jsonl
…R-K4) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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/corpus.rs`:
- Around line 407-413: Update the ingest_otlp_jsonl rustdoc to match the current
stripped-attributes contract: clarify that ingest_otlp_jsonl does not perform a
strict 1:1 mapping of the OTLP envelope into OtlpLogRecord but instead strips
per-record attributes/resource_attributes (consistent with map_log_record and
the module docs) until RFC 0005 §3.3 canonicalisation is applied; reference
ingest_otlp_jsonl and map_log_record by name and explicitly mention attributes
and resource_attributes behavior so the documentation no longer claims a 1:1
mapping.
🪄 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: adfca398-4ad2-4dd3-9754-9eebb85b90a9
📒 Files selected for processing (2)
crates/ourios-bench/src/corpus.rscrates/ourios-bench/src/harness.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/ourios-bench/src/harness.rs
…R-K4) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/ourios-bench/src/harness.rs (1)
365-372: ⚡ Quick winTighten test assertions to exact counts.
The fixture contains exactly 2 records (1 string-body, 1 structured-body), so the assertions can be more precise. Current
>= 1assertions would pass even if both records received snapshots or neither did, though thestructured_seen == 1+ line 352 assertion provides partial coverage.✨ Proposed refinement
- assert!( - snapshots_present >= 1, - "the string-body record gets a snapshot" - ); - assert!( - snapshots_absent >= 1, - "the structured-body record's `None` is counted" - ); + assert_eq!( + snapshots_present, 1, + "exactly one record (the string body) gets a snapshot" + ); + assert_eq!( + snapshots_absent, 1, + "exactly one record (the structured body) gets None" + );🤖 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/src/harness.rs` around lines 365 - 372, Tighten the two snapshot-count assertions to require exact counts: change the assert! checks on snapshots_present and snapshots_absent to assert_eq!(snapshots_present, 1, "...") and assert_eq!(snapshots_absent, 1, "...") (keeping or updating the message) so the test enforces that exactly one string-body record and exactly one structured-body `None` snapshot were recorded; locate the variables snapshots_present and snapshots_absent in the same test around the structured_seen assertion and replace the >= 1 checks with equality checks.
🤖 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/src/harness.rs`:
- Around line 365-372: Tighten the two snapshot-count assertions to require
exact counts: change the assert! checks on snapshots_present and
snapshots_absent to assert_eq!(snapshots_present, 1, "...") and
assert_eq!(snapshots_absent, 1, "...") (keeping or updating the message) so the
test enforces that exactly one string-body record and exactly one
structured-body `None` snapshot were recorded; locate the variables
snapshots_present and snapshots_absent in the same test around the
structured_seen assertion and replace the >= 1 checks with equality checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 82ba9f2d-e080-46cb-b7c1-4657b020584d
📒 Files selected for processing (3)
crates/ourios-bench/src/c1.rscrates/ourios-bench/src/corpus.rscrates/ourios-bench/src/harness.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/ourios-bench/src/corpus.rs
- crates/ourios-bench/src/c1.rs
This was referenced May 29, 2026
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.
Context
Surfaced by triggering the PR-K3 workflow with
corpus_dir=crates/ourios-bench/tests/data/otlp: the run failed twice, exposing two latent gaps the plain-text path never exercised.BenchError::Pipeline—templates_for()returned no matching leaf for(template_id=4, template_version=1). Real cause: RFC 0001 §6.1 assignsBody::Structuredrecords a sentinel template id outside the Drain tree, so the lookup correctly returns nothing — but the harness assumed every non-lossy record had a leaf.attributes/resource_attributesandBody::Structuredrows are blocked by the deferred RFC 0005 §3.3 KeyValue / AnyValue canonicalisation PR.Fixes
Harness + C1 (the surfaces the bench owns)
harness::runnarrowswant_snapshottoBodyKind::String. Structured records getsnapshot = Nonelike lossy ones do — no contract violation.c1::recordskipsBodyKind::Structuredfrom both numerator and denominator. Per RFC 0001 §6.4 / RFC 0003 §6.4, structured reconstruction is the storage-layerAnyValue-bytes round-trip, not template + params, so the template-based equality C1 measures doesn't apply. Structured ≠ lossy (independent axes).OTLP loader (workarounds for the Parquet writer's canonicalisation gaps)
attributesandresource_attributesstripped to emptyVecs inmap_log_record. TODO comment points at RFC 0005 §3.3; the loader test pins the strip so the revert PR has to update it (closing the loop).Body::Structuredrows for the same reason). Loader coverage for structured-body mapping moved to an inline-synthetic test — the loader still pins theBody::Structured(AnyValue)round-trip per RFC 0003 §6.4; it just doesn't sit in a fixture the writer chokes on.Local end-to-end (the failure mode now fixed)
Test plan
cargo fmt --all --checkcargo clippy --all-targets --all-features -- -D warningscargo test --all-features— 286 passed / 19 ignored (+1 vs main: the colocatedc1::tests::structured_body_record_is_excluded_from_denominator)mdbook buildourios-bench --corpus crates/ourios-bench/tests/data/otlp— succeeds end-to-end, C1 PASSFollow-ups (out of scope here)
The real unblocker for richer OTLP corpora is the RFC 0005 §3.3 canonicalisation PR — KeyValue canonical-JSON encoding +
Body::Structured-bytes canonicalisation in the Parquet writer. Once it lands, three things flip back: the loader stops stripping attributes, the kvlist record returns to the fixture, and the harness/C1 exclusions stay (structured reconstruction is still semantically out of scope for C1, but at least it'd be Parquet-writable for A1).🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation