-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Add richer OTel MCP span attributes #2387
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Kludex
wants to merge
5
commits into
main
Choose a base branch
from
codex/add-otel-mcp-attributes
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 build_client_span_attributes has no session_id parameter, so client spans can never carry the mcp.session.id attribute while server spans for the same session can — creating an observability asymmetry. The PR description's claim that 'mcp.session.id is surfaced on the client side through the streamable HTTP write stream wrapper' is factually incorrect: commit 67ca7fe in this same branch explicitly removed that wrapper without adding a replacement. To fix this, add a session_id parameter to build_client_span_attributes and thread the session ID from ClientStreamableHTTPTransport.session_id through BaseSession.send_request.
Extended reasoning...
What the bug is and how it manifests
build_client_span_attributes (src/mcp/shared/_otel.py lines 40-57) accepts only method, request_id, and params — there is no session_id parameter. This means that no matter what the calling code does, client OTel spans can never carry the mcp.session.id attribute. By contrast, build_server_span_attributes (lines 60-84) accepts an explicit session_id parameter and conditionally sets attributes["mcp.session.id"] when it is non-None. The two functions are structurally asymmetric for session ID support.
The specific code path that triggers it
The streamable HTTP client transport (src/mcp/client/streamable_http.py, ClientStreamableHTTPTransport) maintains self.session_id and includes it in every outgoing request via the mcp-session-id HTTP header. However, this value is never passed to BaseSession.send_request (src/mcp/shared/session.py:276), which calls build_client_span_attributes with only method, request_id, and the request params dict. There is no pathway to supply a session_id to the client span builder.
Why existing code does not prevent it
Commit 67ca7fe ('Remove client OTel session-id wrapper') in this same PR branch deliberately removed the _SessionAwareWriteStream wrapper class and the _get_transport_session_id() mechanism that previously solved this problem. Commit 2c4b51a ('Restore OTel span attribute helpers') then re-added build_client_span_attributes without the session_id parameter. The TODO comment left by the author in client/streamable_http.py acknowledges the removal was intentional ('I have dropped the get_session_id callback because it breaks the Transport protocol') but offers no replacement. The PR description was not updated to reflect this change, and instead continues to claim the old mechanism still works.
What the impact would be
In distributed tracing across real deployments, client and server processes are separate. An operator querying their tracing backend (e.g. Jaeger, Honeycomb) by mcp.session.id to correlate all activity for a given session would find server spans but miss every client span, making the trace incomplete and the session timeline unreconstructable. This is the core use case for session ID on spans.
How to fix it
Add a session_id: str | None = None parameter to build_client_span_attributes with the same conditional-set logic as build_server_span_attributes. Then thread the session ID from the transport into BaseSession.send_request — either by passing it as a parameter to send_request, or by making BaseSession aware of the transport's session ID via a callback or property on the write stream (the original approach, now removed).
Step-by-step proof