-
Notifications
You must be signed in to change notification settings - Fork 599
Align telemetry with OpenTelemetry MCP semantic conventions #1139
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
Conversation
…en-telemetry/semantic-conventions#2083 - Change mcp.tool.name to gen_ai.tool.name - Change mcp.prompt.name to gen_ai.prompt.name - Change mcp.request.id to jsonrpc.request.id - Change rpc.jsonrpc.error_code to rpc.response.status_code - Update network.transport values from 'stdio' to 'pipe' and 'sse'/'http' to 'tcp' - Add gen_ai.operation.name attribute (value: 'execute_tool' for tools/call) - Update histogram bucket boundaries to match semantic conventions spec - Update tests to verify new attribute names Co-authored-by: stephentoub <[email protected]>
|
…t outer GenAI activity
- Fix span name to follow format "{mcp.method.name} {target}" per semantic conventions
- Add mcp.protocol.version attribute to telemetry after initialization
- Add network.protocol.name attribute for HTTP transports
- Implement outer GenAI activity detection for tool calls (avoid duplicate spans)
- Add NegotiatedProtocolVersion property to McpSessionHandler for telemetry
- Propagate protocol version from client/server impl to session handler
- Update tests to verify mcp.protocol.version attribute
Co-authored-by: stephentoub <[email protected]>
Addressed in eecac8d:
|
stephentoub
left a comment
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.
@copilot, please iterate through https://github.com/open-telemetry/semantic-conventions/blob/main/docs/gen-ai/mcp.md in detail, validating every statement / requirement against this implementation. You might want to first emit a file containing a todo list for every individual requirement in that spec, and then iterate through that todo list validating each; just make sure to delete that temporary todo list file when you're done.
- Change default network.transport from "pipe" to "unknown" for unsupported transports - Convert NegotiatedProtocolVersion to auto-property - Only look for outer GenAI activity on CLIENT side, not SERVER side - Move ExtractTargetFromMessage calls into ShouldInstrumentMessage blocks - Remove "Per semantic conventions" comments - Delete ShortSecondsBucketBoundaries, rename to ExplicitBucketBoundaries - Change TryGetOuterToolExecutionActivity to check activity.OperationName - Add test for FunctionInvokingChatClient span augmentation Co-authored-by: stephentoub <[email protected]>
Completed in 44cf972 - created a detailed todo checklist in /tmp/mcp-semantic-conventions-todo.md, validated each requirement against the implementation, made necessary fixes, and deleted the file. Key findings: Validated and implemented:
Noted for future work (marked as Recommended, not Required):
|
|
cc: @lmolkova |
…e comments, use pattern matching - Invert if statement in TryGetOuterToolExecutionActivity to avoid multiple return false - Remove "Per MCP" comment in McpSessionHandler - Use pattern matching `is "tcp"` instead of `== "tcp"` - Remove redundant DisplayName setting (StartActivity already sets it via CreateActivityName) Co-authored-by: stephentoub <[email protected]>
The parameter was only used for DisplayName setting which was removed as redundant. Co-authored-by: stephentoub <[email protected]>
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
|
@copilot there are two test failures in the CI. Looks the first is credible. CC @stephentoub |
tarekgh
left a comment
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.
Other than failing tests, LGTM!
Thanks. I've seen that failure before; we have some kind of non-determinism in our tests we need to root out. |
{mcp.method.name} {target}PR Feedback Addressed
is "tcp"instead of== "tcp"Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.