feat(ingester): OTLP wire-decode layer + RFC0003.5 (protobuf equivalence) - #129
Conversation
|
Warning Review limit reached
More reviews will be available in 1 minute and 31 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 ignored due to path filters (1)
📒 Files selected for processing (5)
✨ 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.
|
34118e6 to
afad4c0
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces the first “green slice” of the OTLP receiver (RFC 0003 §6.2) by adding a transport-agnostic protobuf wire-decode layer for ExportLogsServiceRequest, along with a property-based test that flips RFC0003.5 (gRPC ≡ HTTP/x-protobuf decode equivalence) live.
Changes:
- Added
receiver::decode::decode_protobufplus aDecodeErrorwrapper aroundprostdecode failures. - Implemented RFC0003.5 as a proptest-based round-trip + equivalence test over a bounded OTLP proto value space.
- Updated crate/module docs and added dependencies (
opentelemetry-proto,prost,proptest) to support wire decoding and tests.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ourios-ingester/src/receiver/decode.rs | New OTLP protobuf wire-decode module and DecodeError. |
| crates/ourios-ingester/src/receiver.rs | Exposes the new decode module and re-exports decode API. |
| crates/ourios-ingester/tests/rfc0003_5_grpc_http_protobuf_equivalence.rs | Turns RFC0003.5 from ignored stub into a proptest-backed acceptance test. |
| crates/ourios-ingester/src/lib.rs | Updates crate-level docs to reflect receiver “greening” and the new decode slice. |
| crates/ourios-ingester/Cargo.toml | Adds opentelemetry-proto, prost, and proptest dependencies for decoding + tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[derive(Debug)] | ||
| pub enum DecodeError { | ||
| /// Protobuf bytes failed `prost` decode — a malformed wire payload. | ||
| Protobuf(prost::DecodeError), | ||
| } |
There was a problem hiding this comment.
Fixed in 0de2937 — DecodeError is now #[non_exhaustive], so the RFC0003.6 Json variant lands additively and downstream matches keep a wildcard arm.
| # `prost::Message::{decode, encode_to_vec}` for the protobuf payload. | ||
| prost = { version = "0.14", default-features = false } |
There was a problem hiding this comment.
Good catch — real fragility. prost now declares features = ["std"] explicitly (0de2937), rather than relying on workspace feature-unification to satisfy prost::DecodeError: std::error::Error for the source() chain.
| /// An `AnyValue` tree: scalars, plus up to one nesting level of array | ||
| /// and kvlist (the structured-body shapes RFC0003.7 will lean on). | ||
| fn any_value() -> impl Strategy<Value = AnyValue> { | ||
| scalar_value() | ||
| .prop_recursive(2, 12, 4, |inner| { |
There was a problem hiding this comment.
Fixed the doc in 0de2937 — prop_recursive(2, …) permits up to two nesting levels, so the comment now says "up to two nesting levels" (the depth was intentional for coverage; only the wording was wrong).
…valence) First green slice of the OTLP receiver (RFC 0003 §6.2). Lands the transport-agnostic decode layer and flips RFC0003.5 live. `receiver::decode::decode_protobuf` decodes the `ExportLogsServiceRequest` payload that both the gRPC and HTTP `application/x-protobuf` transports carry (they share one decoder), via `prost`. A hand-rolled `DecodeError` (no thiserror, `#[non_exhaustive]` so the RFC0003.6 `Json` variant is additive) wraps the prost failure with a source chain; a future transport handler maps it to the controlled error RFC0003.11 requires. `prost`'s `std` feature is explicit, since `source()` returns `prost::DecodeError` as `&dyn std::error::Error`. RFC0003.5 goes live: a proptest strategy over the OTLP proto value space (scalar + nested array/kvlist `AnyValue`s, multi-resource/scope/record requests) asserts decode is faithful (round-trips the original) and transport-agnostic (the same payload bytes decode identically whether "gRPC"- or "HTTP"-framed). The double strategy excludes NaN, since `NaN != NaN` would make the round-trip equality assertion spuriously fail — the decode contract, not float identity, is under test. Stable across repeated 512-case runs. API is grown incrementally per the maintainer decision: this slice adds only the decode layer + the opentelemetry-proto/prost deps. OTLP/JSON (RFC0003.6 — proto3-JSON encoding rules) and the live tonic/axum transports land in the next slices. No live listener yet, so the transports are tested at the shared decode boundary. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
afad4c0 to
0de2937
Compare
What
First green slice of the OTLP receiver (RFC 0003 §6.2) — the transport-agnostic wire-decode layer — flipping RFC0003.5 live. Follows the red gate (#128) and the maintainer decisions: start with wire decode, grow the API incrementally.
Changes
receiver::decode::decode_protobuf— decodes theExportLogsServiceRequestpayload that both the gRPC and HTTPapplication/x-protobuftransports carry (one shared decoder), viaprost. A hand-rolledDecodeError(nothiserror) wraps theprostfailure with asource()chain; a future transport handler maps it to the controlled error RFC0003.11 requires — never a panic.opentelemetry-proto(same version + features the workspace already pins) andprost. Notonic/axumyet — the live listeners are a later transport slice, so decode is specified + tested at the boundary the transports share.RFC0003.5 (now live)
A proptest strategy over the OTLP proto value space (scalar + one-level nested array/kvlist
AnyValues; multi-resource/scope/record requests) asserts the decoder is:decode_protobuf(req.encode_to_vec()) == req;The double strategy excludes
NaN: protobuf round-tripsNaNfaithfully, butNaN != NaNwould make the equality assertion spuriously fail — the decode contract, not float identity, is under test. Stable across repeated 512-case runs.Scope / next
tonic/axumtransports, and the WAL-before-ack path follow as their own slices.rfc0003_*tests stay#[ignore]'d.Verification
cargo test -p ourios-ingester✓ —rfc0003_5passes; the other 14 remain ignored.cargo fmt --all --check✓ ·cargo clippy --all-targets --all-features -- -D warnings✓ (workspace)🤖 Generated with Claude Code