feat(rfc0024): green — P1/P2/P3 pipeline properties + canonical decode fix - #363
Conversation
…e fix
Discharges RFC0024.3/.4/.5: generated OTLP batches (both modes)
through MinerCluster → RFC 0005 writer → reader. P2 found a real
decode gap on its first run — the canonical codec rejected its own
encoder's output for empty AnyValues ({}, proto3 oneof-unset) and
explicit "value":null kvlist entries; the fallback decoder now
accepts both spellings, pinned by two new unit tests. P1 also
surfaced #362 (absent-body records wedge their partition buffer) —
pinned as a documented loud rejection until the RFC 0005 amendment.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 51 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)
📝 WalkthroughWalkthroughModifies canonical JSON decoding in ourios-core to treat null and empty objects as absent AnyValue/KeyValue values, with new codec tests. Adds ourios-testgen (and ourios-miner) dev-dependencies to ourios-miner and ourios-parquet, replacing ignored RFC0024 test stubs with active property-based tests and a proptest regression seed file. ChangesOTLP Absent-Value Decoding
RFC0024 Property Tests (Miner & Parquet)
Estimated code review effort: 4 (Complex) | ~60 minutes 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 |
There was a problem hiding this comment.
Pull request overview
This PR implements RFC 0024’s third “green slice” by turning the P1/P2/P3 pipeline property tests from stubs into live proptest suites, and fixes a production canonical JSON decode gap in ourios-core so the decoder accepts legal “empty” OTLP AnyValue spellings that can appear on the wire.
Changes:
- Implemented RFC0024 property tests: P1 (parquet write→read round-trip fidelity), P2 (no silent merge), and P3 (RFC0023 bounds under tiny caps) using
ourios-testgenadversarial + calibrated generators. - Widened
ourios_core::otlp::canonicaldecoding to accept{}andnullas legal “no value” spellings forAnyValue, with new unit tests to pin the behavior. - Added dev-dependencies (
ourios-testgen,ourios-miner) and committed.proptest-regressionsseeds per RFC 0024 guidance.
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/ourios-parquet/tests/rfc0024_properties.rs | Implements P1 round-trip fidelity through miner → writer → reader with OTLP-level body fidelity assertions. |
| crates/ourios-parquet/Cargo.toml | Adds dev-deps needed to run the parquet-side RFC0024 property suite. |
| crates/ourios-miner/tests/rfc0024_properties.rs | Implements P2 (no silent merge) and P3 (bounds) property tests using generated OTLP batches. |
| crates/ourios-miner/tests/rfc0024_properties.proptest-regressions | Adds saved proptest counterexample seeds for deterministic regression coverage. |
| crates/ourios-miner/Cargo.toml | Adds ourios-testgen dev-dependency for the miner-side property tests. |
| crates/ourios-core/src/otlp.rs | Fixes canonical decode to accept legal empty/unset AnyValue spellings and adds unit tests. |
| Cargo.lock | Updates lockfile for newly added workspace/dev dependencies. |
💡 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.
🧹 Nitpick comments (2)
crates/ourios-core/src/otlp.rs (1)
825-887: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage; consider pinning the decoder's byte-level contract directly.
The round-trip tests exercise the new
null/{}acceptance indirectly, via whatever bytes the fast (opentelemetry-proto) serializer happens to emit. Adding a direct test that feeds literal bytes (b"null",b"{}", and[{"key":"k","value":null}]) intodecode_any_value/decode_attributeswould pin the decoder's accepted wire contract independently of any future change to the fast-path encoder.♻️ Example additional test
#[test] fn decode_any_value_accepts_literal_null_and_empty_object() { for bytes in [b"null".as_slice(), b"{}".as_slice()] { let back = decode_any_value(bytes).expect("decode"); assert_eq!(back, AnyValue { value: None }); } } #[test] fn decode_attributes_accepts_literal_null_value() { let bytes = br#"[{"key":"k","value":null}]"#; let back = decode_attributes(bytes).expect("decode"); assert_eq!( back, vec![KeyValue { key: "k".to_string(), value: None, ..Default::default() }] ); }🤖 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-core/src/otlp.rs` around lines 825 - 887, Add direct decoder contract tests in the otlp round-trip test module so acceptance of literal wire forms is pinned independently of the serializer. Update the test area around decode_any_value and decode_attributes to feed raw bytes for null, {}, and a JSON attribute entry with value:null, then assert the decoded AnyValue and KeyValue results match the expected empty/absent values. Keep the existing round-trip tests, but add these byte-level cases to explicitly verify the decoder behavior.crates/ourios-miner/tests/rfc0024_properties.rs (1)
96-136: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAsymmetric divert/lossy assertions across body kinds.
The
Some(Body::String(s))arm assertslossy_flagand verbatimmined.bodyretention when diverted toNO_TEMPLATE(lines 107-115), but theStructuredandNonearms have no equivalent check. Reconstruction equality partially covers the merge invariant for these kinds, but explicitly asserting the divert/lossy contract for structured/absent bodies too would tighten P2 coverage and catch regressions iningest_structured/absent-body handling that reconstruction alone might miss (e.g. a wrongly-clearedlossy_flagon a correctly-reconstructing row).🤖 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-miner/tests/rfc0024_properties.rs` around lines 96 - 136, The divert/lossy contract is only asserted for Body::String, leaving Body::Structured and None under-tested. In the property test loop in rfc0024_properties.rs, extend the existing NO_TEMPLATE checks in the match over original.body so structured and absent bodies also verify the expected lossy_flag and retained payload behavior via mined.template_id, mined.lossy_flag, and mined.body. Use the same reconstruct/mined assertions pattern already present in the Some(Body::String) arm to tighten coverage for ingest_structured and absent-body handling.
🤖 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-core/src/otlp.rs`:
- Around line 825-887: Add direct decoder contract tests in the otlp round-trip
test module so acceptance of literal wire forms is pinned independently of the
serializer. Update the test area around decode_any_value and decode_attributes
to feed raw bytes for null, {}, and a JSON attribute entry with value:null, then
assert the decoded AnyValue and KeyValue results match the expected empty/absent
values. Keep the existing round-trip tests, but add these byte-level cases to
explicitly verify the decoder behavior.
In `@crates/ourios-miner/tests/rfc0024_properties.rs`:
- Around line 96-136: The divert/lossy contract is only asserted for
Body::String, leaving Body::Structured and None under-tested. In the property
test loop in rfc0024_properties.rs, extend the existing NO_TEMPLATE checks in
the match over original.body so structured and absent bodies also verify the
expected lossy_flag and retained payload behavior via mined.template_id,
mined.lossy_flag, and mined.body. Use the same reconstruct/mined assertions
pattern already present in the Some(Body::String) arm to tighten coverage for
ingest_structured and absent-body handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a5ff590-e304-4966-b58f-70f2d9377455
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
crates/ourios-core/src/otlp.rscrates/ourios-miner/Cargo.tomlcrates/ourios-miner/tests/rfc0024_properties.proptest-regressionscrates/ourios-miner/tests/rfc0024_properties.rscrates/ourios-parquet/Cargo.tomlcrates/ourios-parquet/tests/rfc0024_properties.proptest-regressionscrates/ourios-parquet/tests/rfc0024_properties.rs
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
What
Third RFC 0024 green slice: the three pipeline properties, discharging RFC0024.3 (P1,
ourios-parquet), RFC0024.4 (P2,ourios-miner), RFC0024.5 (P3,ourios-miner) — stub → live. Generated OTLP batches (adversarial + calibrated, per §3.2) run through the real pipeline:MinerClusterwith aSharedRecordSink→ RFC 0005Writer→Reader.MinedRecordstruct equality, plus OTLP-level body fidelity on the read-back rows (string bodies reconstruct bit-identically, structured bodies decode canonical-JSON equal). The writer's two documented loud rejections — §3.2 timestamp overflow on either column, andUnsupportedAbsentBody— are pinned as rejections, never silent drops.max_templates=3,max_node_children=2,max_line_tokens=8); leaf count checked after every ingest, over-long lines always divert, diverted rows keep their bodies. Per-node fan-out has no cluster-level observation surface — it stays pinned by the tree-level fan-out property and RFC0023.3 (noted in the test doc).The suite paid for itself on its first run — two real findings
ourios-core). The canonical codec rejected its own encoder's output for two legal spellings: an emptyAnyValuenested in arrays/kvlists ({}, proto3oneof-unset —opentelemetry-proto's deserializer errors "no known keys found") and explicit"value": nullkvlist entries in the hand-built fallback path. Stored bytes containing either were unreadable on the query path. The fallback decoder now accepts both, pinned byempty_any_value_round_trips_at_every_nestingandattributes_with_absent_and_empty_values_round_trip. Encoder bytes are unchanged (the exact-bytes pin still passes), so no stored data is affected — this widens what the reader accepts.BodyKind::Absent) → encode, which rejects it permanently — and the ingest sink retains the buffer on flush error, wedging that partition forever. The schema-level fix is an RFC 0005 amendment (§3.5 migration plan), so this PR pins the current rejection loudly and defers the representation change to the RFC process.Both findings are exactly RFC 0024 §2's thesis: "an input shape we didn't anticipate" as a generated, repeatable test class.
Invariants / hazards
ourios-coredecode fix touches a production path: it is strictly widening (decode-only, encoder bytes unchanged), covered by new unit tests plus the existing exact-bytes pin..proptest-regressionsfiles are committed per RFC 0024 §6 (counterexamples become permanent regression cases).Verification
cargo fmt --all --checkcleancargo clippy --all-targets --all-features -- -D warningsclean (workspace)cargo test --all-features— 123 suites, 912 tests, 0 failuresRemaining for the RFC: slice (d) — the P4 query oracle (RFC0024.6) + the adversarial umbrella (RFC0024.7).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes