OTEL: fix end-to-end MCP trace interop#3997
Conversation
|
CI failed due to two issues in |
🤖 Generated with Codex
🤖 Generated with Codex
🤖 Generated with Codex
031971c to
409c971
Compare
- Rename get_trace_context_carrier -> extract_propagation_keys_from_meta (carrier is OTEL jargon, new name is self-evident) - Rename _get_ambient_span_context -> _get_ambient_or_current_span_context (makes fallback to current span explicit in name) - Fix extract_trace_context docstring: merged -> propagated (precise) - Trim duplicate wording in _initialize_session_with_meta docstring
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9feca45fb1
ℹ️ 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".
| if not native_telemetry_enabled(): | ||
| yield get_noop_span() | ||
| return |
There was a problem hiding this comment.
Preserve extracted request context when telemetry is suppressed
When FASTMCP_TELEMETRY_MODE=propagation_only (or suppress_fastmcp_telemetry() is active), this early return skips _get_parent_trace_context() and never attaches the propagated MCP _meta context to the current execution context. In practice, server handlers then lose incoming trace/baggage (for example baggage.get_baggage(...) returns None) and any downstream inject_trace_context() call propagates the wrong context, which breaks the commit’s stated “propagation-only” interoperability contract for mixed/third-party clients.
Useful? React with 👍 / 👎.
|
Closing in favor of PR #4046 which has a clean single-commit history. |
FastMCP's OpenTelemetry support worked best when FastMCP owned the entire MCP exchange. In mixed setups, native FastMCP spans could compete with outer instrumentation for ownership of the MCP trace, server spans could prefer ambient transport context over the propagated MCP parent, and the FastMCP client still sent
initializeand discovery requests like an opaque client. The result was traces with the right raw events but the wrong hierarchy at the MCP boundary.This makes FastMCP compose cleanly as both client and server. FastMCP now has a supported
propagation_onlymode plussuppress_fastmcp_telemetry()for cases where another instrumentation layer should own MCP spans, server spans parent from propagated MCP context while linking ambient transport spans, baggage stays current during execution, and the FastMCP client now propagates_metaoninitialize,tools/list,resources/list,resources/templates/list, andprompts/list. The new interop tests cover mixed instrumentation, opaque third-party MCP clients, and the missing client handshake/discovery paths so this behavior is exercised end to end instead of inferred.Refs #3451. Closes #3993. Closes #3998.
🤖 Generated with Codex