Repository navigation
Conversation
MCP tools/call requests carried no trace context, so a tool server could not join the trace of the request that triggered it, and inbound W3C baggage was dropped everywhere because only the trace-context propagator was installed. - Install a composite TraceContext + Baggage propagator, so baggage extracted from the inbound request is re-injected on the stateful upstream path. - Inject the calling span's context into `params._meta` of each tools/call. The HTTP request is sent by a transport task spawned when the pooled connection opened, which does not run inside the calling span, so Span::current() there cannot supply it. `_meta` is where stdio servers read it; the HTTP client copies traceparent/tracestate/baggage from it into request headers. Tests: - mcp_trace_context_test: a recording HTTP MCP server sees the caller's trace id and baggage in headers and in _meta; removing either half fails it. - telemetry_trace_context_test: three requests each propagate their own inbound trace id upstream, with one exported server span apiece (a regression guard for vllm-project#459, which does not reproduce on main or v0.9.0), and baggage reaches the upstream on the store:true path; removing the baggage propagator fails it. Closes vllm-project#460 Refs vllm-project#459 Signed-off-by: fengjiaqi <enchiigo@gmail.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 567b4b0367
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mut params = CallToolRequestParams::new(name.to_owned()); | ||
| params.arguments = arguments; | ||
| let mut meta = rmcp::model::Meta::new(); | ||
| crate::executor::telemetry::stages::inject_context_into_meta(&mut meta.0); |
There was a problem hiding this comment.
McpClient::call_tool and bounded_http::propagation_headers now reach from tool/ into crate::executor::telemetry::stages for MCP-specific metadata and header behavior. This reverses the repository's required dependency direction—tool/ owns tool execution and executor/ coordinates it—so future tool or telemetry changes become coupled; move the helper and field contract into the MCP/tool owner or a genuinely shared neutral module.
AGENTS.md reference: AGENTS.md:L119-L123
Useful? React with 👍 / 👎.
| for (name, value) in propagation_headers(&message) { | ||
| request = request.header(name, value); |
There was a problem hiding this comment.
When a public/configured MCP headers map already contains traceparent, tracestate, or baggage, apply_custom_headers adds that value first and reqwest's RequestBuilder::header here appends the injected value rather than replacing it. The resulting request contains two traceparent values (invalid for W3C), while common HeaderMap extractors see the stale configured value first for all three fields, so that tool call does not reliably join the active span; reserve/remove these keys or insert propagation values into a HeaderMap before applying it.
Useful? React with 👍 / 👎.
…MCP servers The previous commit sent the caller's trace context and baggage to every MCP server, including ones whose URL the request itself declared. Baggage can hold values an operator's edge added to the inbound request (tenant or routing IDs, for example), and a request-declared server may belong to anyone, so that leaked operator data to third parties. McpClient now propagates only when built with with_trace_context_propagation, which McpClientPool::from_config (operator-configured servers) sets and from_params (request-declared servers) does not. Request-declared servers get exactly what they got before: no traceparent, tracestate, baggage, or _meta. The test covers both sides; propagating to every server again fails it. Signed-off-by: fengjiaqi <enchiigo@gmail.com>
maralbahari
left a comment
maralbahari
left a comment
There was a problem hiding this comment.
Thanks for the work! I left inline comments
| } | ||
| request = apply_custom_headers(request, custom_headers)?; | ||
| for (name, value) in propagation_headers(&message) { | ||
| request = request.header(name, value); |
There was a problem hiding this comment.
Inbound baggage: tenant=acme;note=%0Afoo causes a configured HTTP MCP call to fail before sending. The locked OpenTelemetry propagator decodes the property's %0A and reinjects it as a literal newline. Passing that string to .header() poisons the request builder with InvalidHeaderValue.
I reproduced this through McpClient::call_tool. Please validate propagation values with HeaderValue::try_from before adding them and skip invalid values, as the existing HeaderInjector does, so telemetry cannot prevent tool execution. Add a regression covering encoded baggage properties.
| request = apply_custom_headers(request, custom_headers)?; | ||
| for (name, value) in propagation_headers(&message) { | ||
| request = request.header(name, value); | ||
| } |
There was a problem hiding this comment.
apply_custom_headers can already install a configured traceparent, and reqwest's .header() appends rather than replaces it. This sends two trace parents: the HTTP server can read the configured value while _meta contains the calling span's value.
I reproduced this by setting McpServerEntry::Http.headers["traceparent"]; the existing caller-trace assertion then fails. Please merge propagation headers with replacement semantics and clear stale tracestate when replacing its trace parent. Apply this replacement policy to propagation-enabled calls while preserving explicitly supplied headers for request-declared servers. Cover conflicting configured headers in the transport test.
| } | ||
|
|
||
| /// W3C propagation fields carried in an MCP request's `params._meta`. | ||
| pub(crate) const MCP_META_PROPAGATION_FIELDS: [&str; 3] = ["traceparent", "tracestate", "baggage"]; |
There was a problem hiding this comment.
Please remove the separate MCP_META_PROPAGATION_FIELDS string list and make a typed W3C propagation structure the authoritative representation of the supported fields. Metadata conversion and HTTP-header construction should use that structure so the field list cannot drift independently from the implementation.
The concern is maintaining a separate schema alongside untyped JSON handling; this immutable const is not a mutable-global-state issue. Moving the same list into home configuration would still leave two representations to synchronize.
If operator control is needed, expose propagation or baggage policy through typed MCP settings in the existing ~/.agentic-api/config.toml configuration, respecting AGENTIC_API_HOME. Those would be new configuration options; protocol field names should remain owned by the typed implementation.
|
|
||
| struct MetaInjector<'a>(&'a mut serde_json::Map<String, serde_json::Value>); | ||
|
|
||
| impl Injector for MetaInjector<'_> { |
There was a problem hiding this comment.
Please model the supported propagation metadata as a typed Rust structure with named fields for traceparent, tracestate, and baggage, rather than using serde_json::Map<String, serde_json::Value> as the internal representation. Renaming self.0 to a named map field would not address the typing concern.
For example:
use serde::{Deserialize, Serialize};
#[derive(Debug, Default, Serialize, Deserialize)]
pub(crate) struct W3cPropagation {
#[serde(default, skip_serializing_if = "Option::is_none")]
traceparent: Option<String>,
#[serde(default, skip_serializing_if = "Option::is_none")]
tracestate: Option<String>,
#[serde(default, skip_serializing_if = "Option::is_none")]
baggage: Option<String>,
}Implement Injector on this structure and capture the calling span's context before the pooled transport takes ownership of the message. Keep conversion to rmcp's open-ended Meta representation at the MCP serialization boundary. On the HTTP side, restore the typed structure and validate values before inserting them into a HeaderMap with replacement semantics.
This explicitly models the supported W3C contract. Supporting another propagation format should be a deliberate extension rather than silently placing arbitrary fields in _meta that HTTP forwarding does not support.
There was a problem hiding this comment.
nit: Please move this dependency into a top-level use import instead of referencing crate::executor::telemetry::stages::MCP_META_PROPAGATION_FIELDS inline.
This branch has not been deployed
2 participants
Summary
Fixes #460. MCP
tools/callrequests now carry the calling span's trace context, and inbound W3Cbaggageis propagated.TraceContextPropagatorwas installed, so inboundbaggagewas dropped wherever the gateway builds its own request. The global propagator is now a composite ofTraceContextPropagatorandBaggagePropagator.inject_contextwas never called undertool/, but calling it inBoundedMcpHttpClient::post_messagewould not work either. MCP connections are pooled, and rmcp sends each request from a transport task spawned when the connection opened, soSpan::current()there is not the calling span. The context therefore travels in the message:McpClient::call_toolinjects it intoparams._meta(traceparent,tracestate,baggage), which is where stdio servers can read it._metafields into request headers, so HTTP servers and proxies see standard W3C headers.McpClientPool::from_config). Servers whose URL a request declares (from_params) get notraceparent,tracestate,baggageor_meta, which is the same as before this PR. Baggage can carry values an operator's edge added to the inbound request, and a server declared in a request may belong to anyone.docs/deploying/observability.mddescribes all of this.#459: I could not reproduce it. Running the issue's exact repro (real binary,
curl, an OTLP receiver, and a header-logging upstream) against both currentmainandv0.9.0, every request'straceparentreached the upstream with its own trace id, and the server spans were exported. Details are on #459. This PR adds a regression test for that behaviour but does not close the issue.Test Plan
agentic-server-core/tests/mcp_trace_context_test.rs: a recording streamable-HTTP MCP server receives the caller's trace id andbaggage: tenant=acme, both as headers and in_meta, while a request-declared server receives none of it. Removing the_metainjection, the header copy, or the trust gate makes it fail.agentic-server/tests/telemetry_trace_context_test.rs(uses the realtelemetry::init):POST /v1/responsesspan apiecestore: truepath, inbound baggage reaches the upstream; removing the baggage propagator makes this failcargo test --workspace(111 test binaries),cargo clippy --all-targets -- -D warnings,cargo fmt -- --check, andpre-commit run --all-filesall pass.