feat(ingester): add OTLP/JSON decode + RFC0003.6 (encoding-rule conformance) - #131
Conversation
…rmance) Second green slice of the OTLP receiver (RFC 0003 §6.2). Adds the `application/json` decode path and flips RFC0003.6 live. `receiver::decode::decode_json` decodes an `ExportLogsServiceRequest` from OTLP/JSON via the proto types' `with-serde` `Deserialize` (serde_json); `DecodeError` gains the `Json` variant (the #[non_exhaustive] reservation from the prior slice). Unknown fields are ignored (serde default). RFC0003.6 verifies the OTLP-JSON deviations from plain proto3-JSON by asserting the actual encoded/decoded shape (these asserts ARE the verification of with-serde's behaviour, not assumptions): hex traceId/spanId (not base64), integer enums (not names), decimal-string 64-bit ints accepted as number-or-string, lowerCamelCase keys, and unknown-field tolerance — all confirmed compliant. A JSON<->protobuf equivalence proptest (reusing the RFC0003.5 strategy, now extracted to a shared tests/otlp_strategy module) covers strings/bools/ints/bytes/ arrays/kvlists. Finding (#130): with-serde's `double` JSON formatter is not shortest-round-trippable — ~12% of arbitrary f64 drift 1-2 ULP. Per the OTLP/JSON spec this is a precision limitation, not a syntax violation (the spec constrains double syntax, not binary64 round-trip fidelity), so doubles are excluded from the strict equivalence proptest (export_request_json_safe) and covered by a targeted test that pins "nice" doubles round-trip exactly + documents the limitation. The drift itself is not asserted (that would break when upstream improves). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 52 minutes and 15 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR implements OTLP/HTTP JSON decoding support for the ingester, extending the existing protobuf-only wire decoding with a new ChangesOTLP/HTTP JSON Decoding Implementation and RFC0003.6 Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Implements the OTLP/JSON decode path for the ingester’s OTLP receiver slice (RFC 0003 §6.2), and flips RFC0003.6 “OTLP/JSON encoding-rule conformance” tests live alongside the existing protobuf equivalence checks.
Changes:
- Add
receiver::decode_json(serde-based OTLP/JSONExportLogsServiceRequestdecoding) and extendDecodeErrorwith aJsonvariant. - Replace the RFC0003.6 red-gate stub with live equivalence + spec-asserting JSON shape tests (incl. unknown-field tolerance and 64-bit int number-or-string acceptance).
- Extract the shared proptest strategy into
tests/otlp_strategy/and reuse it for RFC0003.5 and RFC0003.6.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ourios-ingester/tests/rfc0003_6_json_protobuf_equivalence.rs | Adds live RFC0003.6 proptest + spec-asserting OTLP/JSON shape tests. |
| crates/ourios-ingester/tests/rfc0003_5_grpc_http_protobuf_equivalence.rs | Switches to the shared OTLP proptest strategy module. |
| crates/ourios-ingester/tests/otlp_strategy/mod.rs | New shared proptest strategies for OTLP request generation (JSON-safe and full). |
| crates/ourios-ingester/src/receiver/decode.rs | Adds decode_json and DecodeError::Json. |
| crates/ourios-ingester/src/receiver.rs | Re-exports decode_json from the receiver module. |
| crates/ourios-ingester/Cargo.toml | Adds serde_json dependency for OTLP/JSON decoding. |
| Cargo.lock | Locks the new serde_json dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -24,12 +24,16 @@ use prost::Message; | |||
| pub enum DecodeError { | |||
There was a problem hiding this comment.
Fixed in 442f5e9 — the #[non_exhaustive] note no longer claims a future Json variant (it's in this PR); it now explains the general forward-compat reservation for future transports/encodings.
| pub mod decode; | ||
|
|
||
| pub use decode::{DecodeError, decode_protobuf}; | ||
| pub use decode::{DecodeError, decode_json, decode_protobuf}; |
There was a problem hiding this comment.
Fixed in 442f5e9 — the module doc now says the decode layer covers "protobuf + OTLP/JSON" rather than "protobuf today; OTLP/JSON next".
![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-ingester/tests/rfc0003_6_json_protobuf_equivalence.rs (2)
77-127: ⚡ Quick winConsider testing non-finite doubles (NaN, Infinity) to verify proto3-JSON string serialization.
The current "nice doubles" test covers finite values that serialize as JSON numbers. However, non-finite
f64values (NaN,Infinity,-Infinity) follow a different code path: they serialize as JSON strings per proto3-JSON spec. Testing these would verify thewith-serdestring serialization/deserialization behavior documented in the codebase learnings.📝 Example test addition
// Add after line 97 for non_finite in [f64::NAN, f64::INFINITY, f64::NEG_INFINITY] { let back = roundtrip_double_via_otlp_json(non_finite); // NaN is special: NaN != NaN, so check bit pattern if non_finite.is_nan() { assert!(back.is_nan(), "NaN must round-trip as NaN via OTLP/JSON"); } else { assert_eq!( back, non_finite, "non-finite double {non_finite} must round-trip via OTLP/JSON string encoding", ); } }🤖 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-ingester/tests/rfc0003_6_json_protobuf_equivalence.rs` around lines 77 - 127, Add a new loop in the rfc0003_6_nice_doubles_roundtrip_through_otlp_json test that rounds-trip non-finite f64 values (f64::NAN, f64::INFINITY, f64::NEG_INFINITY) via roundtrip_double_via_otlp_json and asserts correct behavior: for NaN use is_nan()/bit-check to verify NaN round-trips as NaN, and for +/-Infinity assert equality with the original value; reuse the existing roundtrip_double_via_otlp_json helper and keep failure messages consistent with the existing assertions.Source: Learnings
129-179: 💤 Low valueConsider testing decoder accepts uppercase hex
traceId/spanId.The encoder test uses
to_lowercase()(lines 141, 149) when verifying hex format, suggesting case-insensitivity is expected. However, the decoder test on line 188-189 only uses lowercase hex. Testing uppercase hex would provide more thorough coverage of the case-insensitivity requirement.📝 Example test addition
// Add a new test after rfc0003_6_decoder_accepts_compliant_otlp_json #[test] fn rfc0003_6_decoder_accepts_uppercase_hex_trace_span_ids() { let uppercase_hex = br#"{"resourceLogs":[{"scopeLogs":[{"logRecords":[{ "timeUnixNano":"1700000000000000000", "traceId":"0102030405060708090A0B0C0D0E0F10", "spanId":"0102030405060708" }]}]}]}"#; let req = decode_json(uppercase_hex).expect("uppercase hex trace/span IDs accepted"); let rec = &req.resource_logs[0].scope_logs[0].log_records[0]; assert_eq!(rec.trace_id, (1u8..=16).collect::<Vec<u8>>()); assert_eq!(rec.span_id, (1u8..=8).collect::<Vec<u8>>()); }🤖 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-ingester/tests/rfc0003_6_json_protobuf_equivalence.rs` around lines 129 - 179, Add a decoder test that verifies uppercase hex traceId/spanId are accepted: create a new test function (e.g., rfc0003_6_decoder_accepts_uppercase_hex_trace_span_ids) that constructs an OTLP/JSON request with traceId and spanId using uppercase hex (matching values used in rfc0003_6_json_encoder_emits_otlp_json_deviations), call decode_json(...) and assert the resulting request's resource_logs[0].scope_logs[0].log_records[0].trace_id and .span_id equal the expected byte vectors (1..=16 and 1..=8 respectively); place this test after rfc0003_6_decoder_accepts_compliant_otlp_json and reuse the same decode_json helper so the decoder handling of case-insensitive hex is covered.
🤖 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-ingester/tests/rfc0003_6_json_protobuf_equivalence.rs`:
- Around line 77-127: Add a new loop in the
rfc0003_6_nice_doubles_roundtrip_through_otlp_json test that rounds-trip
non-finite f64 values (f64::NAN, f64::INFINITY, f64::NEG_INFINITY) via
roundtrip_double_via_otlp_json and asserts correct behavior: for NaN use
is_nan()/bit-check to verify NaN round-trips as NaN, and for +/-Infinity assert
equality with the original value; reuse the existing
roundtrip_double_via_otlp_json helper and keep failure messages consistent with
the existing assertions.
- Around line 129-179: Add a decoder test that verifies uppercase hex
traceId/spanId are accepted: create a new test function (e.g.,
rfc0003_6_decoder_accepts_uppercase_hex_trace_span_ids) that constructs an
OTLP/JSON request with traceId and spanId using uppercase hex (matching values
used in rfc0003_6_json_encoder_emits_otlp_json_deviations), call
decode_json(...) and assert the resulting request's
resource_logs[0].scope_logs[0].log_records[0].trace_id and .span_id equal the
expected byte vectors (1..=16 and 1..=8 respectively); place this test after
rfc0003_6_decoder_accepts_compliant_otlp_json and reuse the same decode_json
helper so the decoder handling of case-insensitive hex is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b9dc52a-f5ae-4045-9245-de42366f2350
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/ourios-ingester/Cargo.tomlcrates/ourios-ingester/src/receiver.rscrates/ourios-ingester/src/receiver/decode.rscrates/ourios-ingester/tests/otlp_strategy/mod.rscrates/ourios-ingester/tests/rfc0003_5_grpc_http_protobuf_equivalence.rscrates/ourios-ingester/tests/rfc0003_6_json_protobuf_equivalence.rs
Both doc comments were written in the RFC0003.5 slice when OTLP/JSON was still upcoming. This PR implements it, so: the `DecodeError` `#[non_exhaustive]` note no longer promises a future `Json` variant (it's here) and instead explains the general forward-compat reservation, and the receiver module doc says the decode layer covers "protobuf + OTLP/JSON", not "protobuf today; OTLP/JSON next". Surfaced in review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
What
Second green slice of the OTLP receiver (RFC 0003 §6.2): the OTLP/JSON decode path, flipping RFC0003.6 live. Builds on the wire-decode layer (#129).
Changes
receiver::decode::decode_json— decodes anExportLogsServiceRequestfrom OTLP/JSON via the proto types'with-serdeDeserialize(serde_json).DecodeErrorgains theJsonvariant (the#[non_exhaustive]reservation from feat(ingester): OTLP wire-decode layer + RFC0003.5 (protobuf equivalence) #129); unknown fields are ignored (serde default).tests/otlp_strategy/and reused by RFC0003.5 + RFC0003.6.RFC0003.6 — verified, not assumed
Per the maintainer's OTel-spec guidance, the tests assert the actual OTLP/JSON shape
with-serdeemits/accepts (the asserts are the verification — no speculation about crate internals). All confirmed compliant:traceId/spanId(not base64), integer enums (not names), decimal-string 64-bit ints accepted as number or string, lowerCamelCase keys, unknown-field tolerance;Finding:
with-serdelossy doubles (#130)The equivalence proptest surfaced that
with-serde'sdoubleJSON formatter is not shortest-round-trippable — ~12% of arbitraryf64drift 1–2 ULP (serde_jsonitself is exact, so it's awith-serdecustom formatter). Per the OTLP/JSON spec this constrains double syntax, not binary64 round-trip fidelity, so it's a precision limitation, not a conformance violation (confirmed via the maintainer's OpenTelemetry-spec review). Handling:export_request_json_safe);Verification
cargo test -p ourios-ingester✓ — RFC0003.5 + RFC0003.6 live (equivalence stable at 4096 cases); 13rfc0003_*scenarios remain ignored.cargo fmt --all --check✓ ·cargo clippy --all-targets --all-features -- -D warnings✓ (workspace)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests