fix(ingest): accept proto3-JSON unset AnyValue on the OTLP/JSON paths - #550
Merged
Conversation
Found by #546's smoke capture (issue #549): the OTel-Demo (post-2.2.0) emits body-less event records whose file-exporter encoding is "body":{} — proto3-JSON's valid empty-message encoding of an unset AnyValue — and opentelemetry-proto's with-serde deserializer (0.32.0, current latest) rejects it ("Invalid data for Value, no known keys found"), along with "null", which its own serializer emits for the same state. The production OTLP/HTTP application/json receiver shared the deserializer, so a spec-compliant exporter would 400. ourios_core::otlp::lenient_json: direct parse first (hot path untouched for valid input); on failure, unset-AnyValue encodings at the Option-typed positions (LogRecord.body, KeyValue.value incl. kvlists) rewrite to the absent field and the parse retries once. Bounded, documented fidelity concession: the faithful decode is Some(AnyValue{value:None}) (what protobuf/prost preserves per RFC 0018), but the broken upstream deserializer leaves no encoding reaching that state — the shim concedes exactly the presence bit of an EMPTY value; for bodies even that is invisible (Body::from_any_value collapses both). Unknown-key objects still reject (no loosening); array-element unset stays a parse error (no absent encoding exists — upstream-fix territory). Wired at decode_json and the bench corpus loader; serde becomes a non-optional ourios-core dep (already transitive via serde_json). The RFC0003.6 equivalence suite gains the cross-transport case pinning the interim contract exactly — including the assert that flips when the upstream fix restores full fidelity — and the canonical round trip of both stored forms. C1's denominator gains the BodyKind::Absent exclusion (the twin of the existing Structured exclusion, simpler proof: no body arrived, no template allocated, nothing to reconstruct): the smoke corpus's three event records failed the gate on rows carrying no reconstruction obligation. End-to-end proof: the demo-main smoke capture (1,048 records incl. the events) now runs the full parse -> mine -> Parquet -> C1 pipeline at 1.000000, exit 0. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
|
Warning Review limit reached
Next review available in: 27 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 (15)
📝 WalkthroughWalkthroughThe PR adds lenient OTLP/JSON parsing for unset ChangesOTLP JSON unset AnyValue handling
C1 absent-body accounting
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant OTLP_JSON
participant decode_json
participant lenient_json
participant ExportLogsServiceRequest
OTLP_JSON->>decode_json: Submit JSON bytes
decode_json->>lenient_json: Parse with from_slice
lenient_json-->>decode_json: Return normalized request
decode_json-->>ExportLogsServiceRequest: Return decoded logs request
Possibly related issues
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes OTLP/JSON ingestion for spec-valid proto3-JSON encodings of an unset AnyValue ({} / null) by introducing a narrowly-scoped lenient JSON retry shim in ourios-core, wiring it into the OTLP/HTTP JSON receiver and the bench corpus loader, and adjusting the C1 reconstruction gate to exclude records with BodyKind::Absent.
Changes:
- Add
ourios_core::otlp::lenient_jsonto retry parsing after stripping unset-AnyValueencodings fromLogRecord.bodyandKeyValue.valueslots. - Route ingester
decode_jsonand bench JSONL corpus parsing through the lenient shim; add cross-transport equivalence + regression tests. - Exclude
BodyKind::Absentfrom C1’s reconstruction denominator and add a unit test for the exclusion.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ourios-ingester/tests/it/rfc0003_6_json_protobuf_equivalence.rs | Adds a regression/equivalence test covering unset-AnyValue behavior across JSON vs protobuf transports. |
| crates/ourios-ingester/src/receiver/decode.rs | Switches OTLP/HTTP JSON decoding to go through the lenient JSON shim (direct parse first, retry on failure). |
| crates/ourios-core/src/otlp.rs | Implements otlp::lenient_json and adds targeted tests for {} / null unset-AnyValue encodings. |
| crates/ourios-core/Cargo.toml | Makes serde non-optional for the shim’s DeserializeOwned bound; keeps derive behind oidc. |
| crates/ourios-bench/src/corpus.rs | Uses the lenient JSON shim when loading OTLP/JSONL corpora. |
| crates/ourios-bench/src/c1.rs | Excludes absent-body records from C1 denominator; adds a unit test for the new exclusion. |
💡 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: 1
🧹 Nitpick comments (1)
crates/ourios-core/src/otlp.rs (1)
959-1051: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for nested
kvlistValueunset-AnyValuenormalization. The doc comment onlenient_jsonexplicitly claims the shim handles unsetAnyValue"including insidekvlistValue", and the recursivestrip_unset_any_valuesdesign supports it generically, but no test anywhere in this PR exercises aKeyValuenested inside akvlistValuewith an unset{}/nullvalue.
crates/ourios-core/src/otlp.rs#L959-L1051: root-cause site — the recursion claims nestedkvlistValuesupport but has no dedicated regression test locking it in.crates/ourios-core/src/otlp.rs#L1183-L1240: add alenient_json-module unit test with abody/attributes[].valueshaped as{"kvlistValue":{"values":[{"key":"...","value":{}}]}}and assert the nested value normalizes to absent.crates/ourios-ingester/tests/it/rfc0003_6_json_protobuf_equivalence.rs#L234-L323: extend (or add alongside) the equivalence test with a nested-kvlistValue case to pin the same transport equivalence one level deeper.🤖 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 959 - 1051, The recursive lenient AnyValue normalization lacks regression coverage for nested kvlistValue entries. In crates/ourios-core/src/otlp.rs lines 1183-1240, add a lenient_json unit test using a body or attributes value shaped with kvlistValue.values containing a KeyValue whose value is {}, and assert it decodes with the nested value absent; in crates/ourios-ingester/tests/it/rfc0003_6_json_protobuf_equivalence.rs lines 234-323, extend the equivalence coverage with the same nested-kvlistValue case and verify JSON and protobuf transports remain equivalent. The root logic at lenient_json::strip_unset_any_values requires no direct change.Source: Coding guidelines
🤖 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-ingester/src/receiver/decode.rs`:
- Around line 71-84: Instrument decode_json with a Prometheus counter for the
lenient_json retry path, incrementing it when the fallback is used and
distinguishing successful retries from retries that still return
DecodeError::Json. Preserve the direct-parse result and existing error behavior,
and register/reuse the receiver’s established metrics infrastructure so the
counter is available for operational monitoring.
---
Nitpick comments:
In `@crates/ourios-core/src/otlp.rs`:
- Around line 959-1051: The recursive lenient AnyValue normalization lacks
regression coverage for nested kvlistValue entries. In
crates/ourios-core/src/otlp.rs lines 1183-1240, add a lenient_json unit test
using a body or attributes value shaped with kvlistValue.values containing a
KeyValue whose value is {}, and assert it decodes with the nested value absent;
in crates/ourios-ingester/tests/it/rfc0003_6_json_protobuf_equivalence.rs lines
234-323, extend the equivalence coverage with the same nested-kvlistValue case
and verify JSON and protobuf transports remain equivalent. The root logic at
lenient_json::strip_unset_any_values requires no direct change.
🪄 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: 7920ddd5-83f4-45db-b5ae-1875b2744424
📒 Files selected for processing (6)
crates/ourios-bench/src/c1.rscrates/ourios-bench/src/corpus.rscrates/ourios-core/Cargo.tomlcrates/ourios-core/src/otlp.rscrates/ourios-ingester/src/receiver/decode.rscrates/ourios-ingester/tests/it/rfc0003_6_json_protobuf_equivalence.rs
Both Copilot findings valid: the null-encodings test now covers both
unset spellings ({} and null) at both Option-typed positions instead
of claiming null-attribute coverage it lacked, and the absent-body C1
fixture uses the miner's real NO_TEMPLATE sentinel (cluster.rs emits
template id 0 for BodyKind::Absent) rather than an invented shape.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
CodeRabbit invoked §6.3 correctly: the lenient-retry path had no operator signal. Per the one-instrument-plus-attribute convention (the error.type shape), the batches counter gains the registry attribute ourios.ingest.json.lenient — new name through semconv/registry + weaver generate as required — threaded decode_json → ingest_bound → record_batch (grpc/protobuf batches pass false; a debug-level trace at the decode site keeps the log stream un-spammable at high lenient rates since the metric is the countable signal). lenient_json gains from_slice_flagged; the metrics test asserts the attribute lands on exactly the lenient batch. Copilot's three future-proofing findings applied: the core lenient tests now assert absent-or-unset (the one state Body::from_any_value maps both decodings to) instead of hard-pinning today's upstream failure mode, leaving the ingester's RFC0003.6 equivalence test as the single designated upstream-fix flip signal — which now also asserts the decode-path flag, so the flip is doubly visible. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
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.
What
Fixes #549: spec-valid OTLP/JSON carrying an unset
AnyValue("body": {}/"value": {}/null) no longer fails to parse — on the production OTLP/HTTPapplication/jsonreceiver or the bench corpus loader. Found by #546's smoke capture: the OTel-Demo (post-2.2.0) emits body-less event records at 35%-of-records scale, andopentelemetry-proto'swith-serdedeserializer (0.32.0, current latest) rejects both proto3-JSON encodings of the unset state — includingnull, which its own serializer emits.The shim (
ourios_core::otlp::lenient_json)Direct parse first — the hot path is untouched for valid input. On failure, unset-
AnyValueencodings at theOption-typed positions (LogRecord.body,KeyValue.value, incl. inside kvlists) rewrite to the absent field and the parse retries once. Unknown-key objects still reject (no loosening); array-element unset stays an error (no absent encoding exists — only the upstream fix resolves it).The fidelity call, made explicitly
The faithful decode of present-but-unset is
Some(AnyValue { value: None })— what protobuf/prost preserves, per RFC 0018's preserve-don't-correct rule, and the existing canonical round-trip tests pin that distinction. The broken upstream deserializer leaves no encoding that reaches that state, so the shim concedes exactly one thing: the presence bit of an empty value, on the JSON transport, until upstream is fixed. For bodies even that is invisible (Body::from_any_valuecollapses both states — proven). The interim contract is pinned by a new RFC0003.6 cross-transport equivalence test whose assert flips when the upstream fix lands — I first attempted the "cleaner" convergence (collapsing{}→Noneon canonical decode) and reverted it when it broke the RFC 0018 round-trip tests: those tests were right, the wire distinction is real, and this PR does not weaken them.The C1 finding
With parsing fixed, the smoke corpus exposed a second assumption: C1 counted
BodyKind::Absentrows in its reconstruction denominator (the harness predates body-less records) and failed on rows carrying no reconstruction obligation. Absent joins the existingStructuredexclusion — simpler proof: no body arrived, no template was allocated, nothing to reconstruct. Unit test added.End-to-end proof
The demo-
mainsmoke capture (1,048 records including all 9 event types) now runs the full parse → mine → Parquet → C1 pipeline: C1 = 1.000000 (1045/1045), exit 0.Hazards / invariants
Upstream: issue + patch PR to
opentelemetry-rustfollow next (per their contributing guidelines), with this PR's evidence.Checks run
cargo fmt --all --check; workspacecargo clippy --all-targets --all-features -- -D warnings; test suites forourios-core(62),ourios-ingester(68 incl. the new equivalence case),ourios-bench(135); the smoke-corpus end-to-end run above.🤖 Generated with Claude Code
https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
Summary by CodeRabbit
New Features
{}ornull.Bug Fixes
Tests