feat(transcode): propagate W3C trace-context and request deadlines to gRPC#32
Conversation
Carry request context across the REST→gRPC boundary so traces stay connected and client deadlines reach the upstream: - Trace-context: forward an incoming traceparent (and tracestate) into gRPC metadata, or synthesize a fresh one (random 16-byte trace-id + 8-byte span-id, sampled) when absent, so a non-instrumented client still produces a single joinable trace. - Deadline: parse a gRPC-format grpc-timeout header (<int><unit>, units H/M/S/m/u/n) and set it as the upstream call timeout; malformed or overflowing values are ignored and the channel default applies. Both apply to the unary and server-streaming transcoding paths. Closes #31
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds context propagation across the REST↔gRPC boundary: W3C ChangesContext Propagation: W3C Trace-Context and Deadline
Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client
participant Handler as streaming_handler / transcode_handler
participant Meta as metadata module
participant gRPC as gRPC upstream
Client->>Handler: HTTP request (traceparent?, grpc-timeout?)
Handler->>Meta: http_headers_to_grpc_metadata(headers)
Note over Meta: Forward or synthesize traceparent/tracestate
Meta-->>Handler: MetadataMap with trace-context
Handler->>Meta: apply_request_deadline(&mut request, headers)
Note over Meta: Parse grpc-timeout → Duration
Meta-->>Handler: Option<Duration> applied to request timeout
Handler->>gRPC: tonic::Request with metadata + deadline
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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 |
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 `@src/transcode/metadata.rs`:
- Around line 241-259: The test case in the
grpc_timeout_rejects_malformed_and_overflow function has a misleading comment.
The value "99999999999999999999H" is too large to parse as u64, so it fails at
the parsing step rather than exercising the checked_mul overflow guard. Replace
this test case with a value that successfully parses as u64 but overflows when
multiplied by 3600 (the seconds multiplier for hours), such as a value slightly
above u64::MAX divided by 3600, to properly test the overflow protection
mechanism.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6f818547-b43a-4244-ae0d-4d4796e7251f
📒 Files selected for processing (3)
README.mdsrc/transcode/metadata.rssrc/transcode/mod.rs
|
| Filename | Overview |
|---|---|
| src/transcode/metadata.rs | Core of the PR: adds W3C traceparent forwarding/synthesis and grpc-timeout parsing. All three previously-flagged issues (zero-duration, 8-digit limit, all-zeros traceparent) are correctly fixed with tests. Logic is sound; one minor edge case around tracestate leaking via forwarded_headers when the incoming traceparent is invalid. |
| src/transcode/mod.rs | Wires apply_request_deadline into both the unary (transcode_handler) and server-streaming (streaming_handler) paths — one-liner addition in each, correct placement after metadata is built. |
| README.md | Moves context-propagation from the Roadmap section to Features, accurately reflecting the new capability. |
Reviews (3): Last reviewed commit: "fix(transcode): accept future W3C tracep..." | Re-trigger Greptile
- A zero grpc-timeout ("0S") parses to a zero deadline, which makes the
upstream call expire immediately with DEADLINE_EXCEEDED.
- grpc-timeout accepts more than the spec's 8-digit limit, letting a
client set an absurd multi-millennia timeout.
- An all-zeros traceparent (invalid per W3C 3.2.2) is forwarded verbatim
instead of being replaced with a fresh, valid one.
All three fail on current code; fixes follow.
Part of #31
- An incoming traceparent is forwarded only when well-formed per W3C 3.2.2 (correct structure, non-zero trace-id and parent-id); otherwise a fresh one is synthesized, so an all-zeros or malformed value from a buggy client can't break trace continuity upstream. tracestate now only travels alongside a forwarded (valid) traceparent. - grpc-timeout is capped at the spec's 8 digits and a zero duration is rejected (falls back to the channel default) instead of setting an already-expired deadline that fails every call with DEADLINE_EXCEEDED. With the digit cap the unit multiplications provably cannot overflow, so they no longer need checked arithmetic. Part of #31
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 `@src/transcode/metadata.rs`:
- Around line 66-75: The is_valid_traceparent function currently rejects
traceparent headers with any version other than "00" by using version == "00" in
the validation logic, which violates the W3C Trace Context specification. Update
the version validation to accept any valid 2-character hexadecimal version
except "ff" instead of requiring an exact match to "00", while keeping all other
validation checks for trace_id, parent_id, and flags unchanged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: be4f9a83-c080-4d71-90f1-3c50b63a279a
📒 Files selected for processing (1)
src/transcode/metadata.rs
The validator only accepts version "00", so a W3C-valid versioned traceparent (e.g. "01-...") is dropped and replaced with a synthesized one, breaking trace continuity for clients using future versions. Fails on current code; fix follows. Part of #31
The validator only accepted version "00", so a W3C-valid versioned traceparent was dropped and resynthesized, breaking trace continuity for clients using newer versions. It now accepts any 2-hex version except the reserved "ff", keeps the baseline "00" at exactly four fields (future versions may append more), and retains the trace-id / parent-id integrity and non-zero checks. Part of #31
Summary
Request context now crosses the REST→gRPC boundary, so distributed traces stay connected and client deadlines reach the upstream. Both apply to the unary and server-streaming transcoding paths.
What's added
traceparent(andtracestate) is forwarded verbatim into gRPC metadata. When notraceparentis present, a fresh one is synthesized (random 16-byte trace-id + 8-byte span-id,-01sampled) so even a non-instrumented client produces a single joinable trace across the boundary.grpc-timeoutheader (<int><unit>, unitsH/M/S/m/u/n) is parsed and set as the upstream call timeout via tonic. Malformed or overflowing values are ignored and the channel default applies.Design
http_headers_to_grpc_metadatanow also injects trace-context;apply_request_deadlinereturns the appliedDuration(testable wiring). Parsing and trace-id generation are pure, unit-tested helpers. Timeout math uses checked arithmetic (overflow → ignored, not saturated).Testing
12 metadata tests: traceparent forward vs synthesize (valid W3C shape + uniqueness), tracestate forwarding,
grpc-timeoutparsing per unit + malformed/overflow rejection, deadline applied/no-op.cargo nextest run --features redis: 100 passed. clippy (all-features) + fmt clean.Docs
README: trace-context + deadline propagation moved from Roadmap to Features.
Closes #31