feat(correlation): propagate request ID and trace context across all legs - #654
feat(correlation): propagate request ID and trace context across all legs#654cdoern wants to merge 8 commits into
Conversation
|
Unsigned commits: ca5fa2d. Please sign your commits. |
…legs A single AI request reaches the backend over two paths: the forwarded request the proxy passes upstream, and the delegated callouts the proxy originates itself while handling it (Files API metadata and content fetches). Neither carried trace context, so the two arrived as unrelated traffic and a slow or failing request could not be attributed to a layer. Add a shared TraceContext that resolves `x-request-id` and a W3C `traceparent` once per downstream request, and stamp both onto every leg: - New `trace_context` filter propagates them to the forwarded request. - Delegated callouts inject them unconditionally, outside the operator-configured `forward_headers` allowlist, since correlation that depends on per-filter configuration silently goes missing. Each leg draws its own span-id from the shared trace-id, keeping delegation latency separable from inference latency. Two details worth noting for review: Identifier resolution reads `extra_request_headers` as well as the downstream headers. The `request_id` builtin writes its generated ID there rather than back into `ctx.request.headers`, so a downstream-only lookup finds an ID only when the client happened to supply one. Agreement across hops is established through request extensions rather than header inspection. Delegated callouts can run in the StreamBuffer pre-read phase, ahead of header-phase filters, where injected values are not visible; whichever hop resolves first initializes the shared context and later hops reuse it regardless of filter order. Client-supplied `traceparent` values are validated before use and discarded when malformed, rather than forwarded into the telemetry backend unchecked. Building the callout header set once per request instead of once per callout also removes repeated allowlist copies for requests that resolve several file references. Trace context is propagated but not yet exported: no OpenTelemetry pipeline exists in this repo, so these headers make requests correlatable without producing spans. Delegated MCP calls and delegation-latency metrics remain to be covered. Signed-off-by: Charlie Doern <cdoern@redhat.com>
ca5fa2d to
881e6f6
Compare
praxis-bot
left a comment
There was a problem hiding this comment.
Review Summary
Clean, well-documented feature with thorough test coverage. The module-level doc in correlation/mod.rs is excellent -- the resolution precedence and the agreement-across-hops rationale are exactly what a future maintainer needs. Three medium-severity items below.
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 0 |
| Medium | 3 |
|
Good direction, thanks @cdoern |
Address praxis-bot review findings on praxis-proxy#654. Resolve the Files API callout headers once at the filter boundary and thread them through current-input resolution and both state-history paths, so every delegated callout of one request shares a span-id regardless of whether history references are cache hits. Guard the defensive ID sanitization against emitting the all-zero trace-id and span-id that W3C Trace Context section 2.2.2 forbids, and document the get_or_init precondition on TraceContext::from_filter_context with a debug log when a caller resolves without it.
Address praxis-bot review findings on praxis-proxy#654. Resolve the Files API callout headers once at the filter boundary and thread them through current-input resolution and both state-history paths, so every delegated callout of one request shares a span-id regardless of whether history references are cache hits. Guard the defensive ID sanitization against emitting the all-zero trace-id and span-id that W3C Trace Context section 2.2.2 forbids, and document the get_or_init precondition on TraceContext::from_filter_context with a debug log when a caller resolves without it. Signed-off-by: Charlie Doern <cdoern@redhat.com>
679b75d to
57c5d27
Compare
The file-search callout filter merged onto main independently of the correlation work, and the two combined into a semantic conflict that git resolved cleanly. Before correlation, `post_json_bytes` applied the operator's `forward_headers` allowlist itself via `build_header_map`, so passing the downstream header map straight in was correct. Correlation moved allowlist application out to the caller — the parameter became a pre-built header set that the client now clones verbatim — but the file-search path still passed `&ctx.request.headers`. The result was that vector-store searches ignored their own configured allowlist and shipped the client's entire downstream header map, credentials included, to the vector store, while carrying no correlation. Build the callout header set once per request at the filter boundary, as the Files API path already does, and thread it through the fan-out. The pending-call check moves ahead of planning so trace context is established only for requests that actually call out, and before the plan borrows the context. `SearchPlan::has_pending_calls` is replaced by a shared `has_pending_calls` predicate, so the boundary check and the plan cannot drift apart. The regression test asserts all three halves: allowlisted headers arrive, non-allowlisted ones do not, and correlation is present. Signed-off-by: Charlie Doern <cdoern@redhat.com>
1086e4d to
e1cf0a2
Compare
|
My AI review: Findings [MAJOR] request_id can overwrite the shared ID [MINOR] W3C future-version and reserved-flag handling is incomplete [NIT] New modules violate mandatory source ordering |
Address the review findings on praxis-proxy#654. The request_id core builtin reads only the client's headers, so running it after trace_context mints a second ID that wins on the forwarded request under last-write-wins while the delegated calls and the echoed response header keep the first. Document the ordering requirement, order the example config accordingly, and warn at response time when a later filter injects a competing ID, which is the earliest point a filter can observe one. Mask inbound trace-flags to the sampled bit rather than re-emitting reserved bits under version 00, and continue the base fields of a higher-version traceparent instead of dropping the trace when it carries extension fields. Lay both new modules out as CONTRIBUTING requires: section separators, constants first, tests last, alphabetized fields and methods, and test utilities at the end of the test modules. Signed-off-by: Charlie Doern <cdoern@redhat.com>
|
My first instinct here is that most of this should be part of core and just "work out of the box" when enabled. The call site shouldn't have to be involved for the regular tracing headers./context SubRequestClient already has FrameworkHeaders that can be used, but we're missing a request scoped context the SubRequestClient can fetch the trace context from. Maybe HttpFilterContext::execute_subrequest(), or ctx.subrequests().execute() or similar... ? |
…tion Signed-off-by: Charlie Doern <cdoern@redhat.com> # Conflicts: # apis/src/openai/responses/file_search_callout/mod.rs # filters/src/register.rs
praxis-bot
left a comment
There was a problem hiding this comment.
Re-review (7 new commits)
All three prior findings have been addressed:
- callout_headers resolved independently -- fixed: resolved once in
resolve_and_rewriteand threaded through all phases. - sanitize could produce all-zero trace-id -- fixed:
FALLBACK_TRACE_ID/FALLBACK_SPAN_IDconstants withis_all_zeroguards. from_filter_contextdoc misleading -- fixed: doc now states the precondition and adebug!emits on fallback.
One new finding below.
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 0 |
| Medium | 1 |
| }); | ||
|
|
||
| if let Some(InboundTrace { flags, trace_id }) = inbound { | ||
| return (trace_id, flags); |
There was a problem hiding this comment.
[Medium] tracestate is silently dropped when continuing a client trace.
resolve_trace extracts traceparent and discards the companion tracestate header. W3C Trace Context section 3.3.4 requires that when a tracing system continues a trace, it MUST propagate tracestate in the outgoing request. A downstream observability system that carries vendor-specific correlation data in tracestate (e.g. a sampling priority, tenant tag, or upstream span link) loses it at this hop.
Since the proxy does not maintain its own tracestate entry, passive forwarding is sufficient: capture the inbound tracestate alongside the traceparent in InboundTrace, store it in TraceContext, and emit it from Correlation::apply and TraceContext::headers_for_hop. When the proxy starts a new trace (no valid inbound traceparent), no tracestate is needed.
struct InboundTrace {
flags: String,
trace_id: String,
tracestate: Option<String>, // new
}Then in TraceContext:
pub struct TraceContext {
flags: String,
request_id: String,
trace_id: String,
tracestate: Option<String>, // forwarded from inbound
}And in headers_for_hop / Correlation::apply, include tracestate when Some.
Here is the enhancement and draft PR |
|
We haven't heard back from you on this in some time, so we're going to mark it closed for now. However, if you're still interested in pushing this forward please don't hesitate to re-open and update 🖖 |
Problem
A single AI request reaches the backend over two paths:
Neither carried trace context. The two legs arrived at the backend as unrelated traffic, so a slow or failing request could not be attributed to a layer.
Change
A shared
TraceContextresolvesx-request-idand a W3Ctraceparentonce per downstream request, and both are stamped onto every leg:trace_contextfilter propagates them to the forwarded request.forward_headersallowlist — correlation that depends on per-filter configuration silently goes missing.Each leg draws its own span-id from the shared trace-id, which is what keeps delegation latency separable from inference latency.
Notes for review
Identifier resolution reads
extra_request_headers, not just downstream headers. Therequest_idbuiltin writes its generated ID there rather than back intoctx.request.headers. A downstream-only lookup finds an ID only when the client happened to supply one — and silently no-ops otherwise, which is the common case. There is a test named for it.Agreement across hops goes through request extensions, not header inspection. Delegated callouts can run in the
StreamBufferpre-read phase, ahead of header-phase filters, where injected values are not visible. Whichever hop resolves first initializes the shared context; later hops reuse it regardless of filter order. An earlier header-based attempt produced two different trace-ids for one request, caught by the integration test in this PR.Client-supplied
traceparentvalues are validated and discarded when malformed rather than forwarded into the telemetry backend unchecked.One assertion was deliberately relaxed. The delegated callouts of one request share a span, because correlation is resolved once at the filter boundary — file resolution is one delegation hop regardless of how many files it fetches. The test asserts that instead of per-callout spans, and that the forwarded leg's span is disjoint from it. Happy to switch to per-callout spans if reviewers prefer.
Incidental improvement: the callout header set is now built once per request instead of once per callout, removing repeated allowlist copies for requests that resolve several file references.
Scope
Trace context is propagated but not yet exported — no OpenTelemetry pipeline exists in this repo, so these headers make requests correlatable without producing spans. Wiring an exporter, covering delegated MCP calls, and recording a delegation-latency metric remain to be done.
Testing
traceparentvalidation, including a table of malformed inputstrace_contextfilterexamples/configs/trace-context.yaml, generated filter docs, full workspace suite andclippy -D warningsclean