feat(core): propagate request-scoped TraceContext on subrequests - #1052
feat(core): propagate request-scoped TraceContext on subrequests#1052VaishnaviHire wants to merge 1 commit into
Conversation
|
Unsigned commits: b4df497. Please sign your commits. |
|
PR too large: 839 lines added (limit: 500, excludes Cargo files, tests, docs, examples, and benchmarks). Please split into smaller PRs. |
praxis-bot
left a comment
praxis-bot
left a comment
There was a problem hiding this comment.
PR Review
Summary: Adds request-scoped TraceContext to correlate forwarded requests and subrequests under a shared trace-id. Each outbound hop mints a fresh span-id while preserving the trace-id, enabling end-to-end correlation. The implementation moves traceparent parsing to praxis-core and extends HttpFilterContext with helper methods for automatic trace propagation.
Overall: The architecture is sound and the refactoring improves code reuse. The core logic correctly implements W3C Trace Context semantics including flag masking, all-zero ID rejection, and future-version tolerance. However, there are gaps in test coverage for error paths and the new context helper methods.
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 3 |
| Medium | 3 |
Findings without inline placement
-
[Large] Integration test needed: Add an end-to-end integration test that exercises a filter making a subrequest and verify that both the forwarded request and the subrequest share the same trace-id but have different span-ids. The existing tests only verify the filter in isolation.
-
[Medium] Request ID resolution precedence is complex but undocumented:
resolve_request_id()prefers pending filter values over inbound headers, warns on conflicts, and falls back to generation. This precedence should be documented in thetrace_contextfilter's user-facing documentation (the .md file) and in the function's rustdoc, not just buried in the implementation.
| http::header::HeaderValue::from_static("congo=t61rcWkgMzE,rojo=00f067aa0ba902b7"), | ||
| http::header::HeaderValue::from_static("02-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01-extra"), | ||
| ); | ||
| let mut ctx = crate::test_utils::make_filter_context(&req); |
There was a problem hiding this comment.
[Medium] ensure_extra_header() behavior is not tested: When a competing header already exists with a different value, this function warns but does NOT add the new value. This "warn and keep existing" behavior should have a dedicated test verifying that the original value is preserved and the new value is not added to extra_request_headers.
|
|
||
| let action = filter.on_request(&mut ctx).await.unwrap(); | ||
| assert!(matches!(action, FilterAction::Continue)); | ||
|
|
There was a problem hiding this comment.
[Medium] Missing test for request ID resolution with conflicting inbound header: resolve_request_id() has a branch (lines 324-330) that warns when pending and inbound values differ, but there's no test exercising this specific case. The test reuses_pending_request_id_from_earlier_filter verifies pending-only, but not pending vs inbound conflict.
b2232f0 to
7050f80
Compare
praxis-bot
left a comment
praxis-bot
left a comment
There was a problem hiding this comment.
PR Review (re-review)
Summary: Revisited after 1 new commit. Previous findings around error path tests (set_request_id, set_traceparent, set_correlation), context method tests (execute_subrequest, send_streaming_subrequest), ensure_extra_header behavior, and conflicting request ID resolution are now covered by new tests. One new finding on the reuse path.
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 0 |
| Medium | 1 |
7050f80 to
b2f1a54
Compare
Store TraceContext in extensions from the trace_context filter, inject x-request-id and traceparent for upstream hops, and apply the same correlation via FrameworkHeaders on the normal subrequest path so call sites do not set those headers manually. Signed-off-by: VaishnaviHire <vhire@redhat.com>
b2f1a54 to
1546bf6
Compare
3 participants
What does this PR do?
A single client request can produce more than one outbound call: the
forwarded upstream request, plus subrequests made while handling it.
Today those legs do not share correlation headers, so a slow or failed
subrequest cannot be joined back to the original request.
This PR stores request-scoped
TraceContext(x-request-id+trace-id) once, injects it on the forwarded hop, and passes thatsame context onto the default subrequest path. Each
outbound hop gets a fresh span-id under the shared trace-id.
Which issue(s) does this relate to?
Fixes #1051
Checklist
git commit -s)make lint && make testpasses locallyDoes this introduce a breaking change?