Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Jan 12, 2026

  • Update attribute names to match new MCP semantic conventions
  • Add missing attributes: mcp.protocol.version, network.protocol.name
  • Fix span name to follow format {mcp.method.name} {target}
  • Implement outer GenAI activity detection (client side only)
  • Update histogram bucket boundaries
  • Update tests

PR Feedback Addressed

  • Change default network.transport from "pipe" to "unknown"
  • Convert NegotiatedProtocolVersion to auto-property
  • Only detect outer GenAI activity on CLIENT side, not SERVER side
  • Move ExtractTargetFromMessage calls into if blocks
  • Remove all "Per MCP/semantic conventions" comments
  • Delete ShortSecondsBucketBoundaries, rename to ExplicitBucketBoundaries
  • Change TryGetOuterToolExecutionActivity to check activity.OperationName
  • Add test for FunctionInvokingChatClient span augmentation
  • Simplify TryGetOuterToolExecutionActivity with inverted if structure
  • Use pattern matching is "tcp" instead of == "tcp"
  • Remove redundant DisplayName setting
  • Remove unused usingOuterActivity parameter from AddTags
Original prompt

The pr @open-telemetry/semantic-conventions/pull/2083 adds a standard convention for mcp. This repo implements an older attempt at such a convention. Please go through the new standard convention in detail and ensure it's fully implemented in the Modelcontextprotocol.Core library with 100% compatibility and interoperability.


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

…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]>
Copilot AI changed the title [WIP] Add implementation of new MCP standard conventions Align telemetry with OpenTelemetry MCP semantic conventions Jan 12, 2026
Copilot AI requested a review from stephentoub January 12, 2026 18:25
@stephentoub
Copy link
Contributor

@copilot:

  • Is the span name correct? It looks like the activity is currently being created with the name of the method rather "Span name SHOULD follow the format {mcp.method.name} {target} where target SHOULD match {gen_ai.tool.name} or {gen_ai.prompt.name} when applicable. If there is no low-cardinality target available, the Span name SHOULD be {mcp.method.name}."
  • Where is mcp.protocol.version being set? Where is network.protocol.name being set? Etc. Please re-review https://github.com/open-telemetry/semantic-conventions/blob/main/docs/gen-ai/mcp.md#recording-mcp-transport and ensure that every property called out is either being set or that there's a good reason it's being excluded (which should be noted in a comment).
  • The spec calls out "If the MCP instrumentation can reliably detect that outer GenAI instrumentation is already tracing the tool execution, it SHOULD NOT create a separate span. Instead, it SHOULD add MCP-specific attributes to the existing tool execution span." If these tools are being used as AIFunctions, and are being invoked from a FunctionInvokingChatClient with Microsoft.Extensions.AI, then there may already be an active Activity for this. Please look for it and update that activity with the additional properties rather than creating a new one.

…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]>
Copy link
Contributor Author

Copilot AI commented Jan 12, 2026

@copilot:

  • Is the span name correct? It looks like the activity is currently being created with the name of the method rather "Span name SHOULD follow the format {mcp.method.name} {target} where tar...

Addressed in eecac8d:

  1. Span name: Fixed to follow format {mcp.method.name} {target} - the activity is now created with the correct name upfront via CreateActivityName(method, target).

  2. Missing attributes:

    • Added mcp.protocol.version - propagated from client/server after initialization to the session handler
    • Added network.protocol.name for HTTP transports (value: "http")
    • Other attributes like server.address/server.port noted with TODOs as they require transport-level information not currently available
  3. Outer GenAI activity detection: Added TryGetOuterToolExecutionActivity() that checks if Activity.Current has gen_ai.operation.name = execute_tool. When detected, MCP-specific attributes are added to that activity instead of creating a new span.

Copy link
Contributor

@stephentoub stephentoub left a 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]>
Copy link
Contributor Author

Copilot AI commented Jan 12, 2026

@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. Yo...

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:

  • Context propagation via params._meta ✓
  • Span naming format {mcp.method.name} {target}
  • All required/conditionally required attributes ✓
  • Outer GenAI activity detection (client-side only) ✓
  • Histogram bucket boundaries ✓

Noted for future work (marked as Recommended, not Required):

  • server.address/server.port and client.address/client.port require transport-layer information
  • network.protocol.version could be added for HTTP transports
  • jsonrpc.protocol.version is always "2.0" so not needed

@stephentoub stephentoub marked this pull request as ready for review January 12, 2026 22:58
@stephentoub
Copy link
Contributor

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]>
Copy link
Contributor

Copilot AI left a 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.

@tarekgh
Copy link
Collaborator

tarekgh commented Jan 13, 2026

@copilot there are two test failures in the CI. Looks the first is credible.

CC @stephentoub

[xUnit.net 00:01:57.26]     ModelContextProtocol.Tests.DiagnosticTests.Session_TracksActivities [FAIL]
Data collector 'Blame' message: All tests finished running, Sequence file will not be generated.
  Failed ModelContextProtocol.Tests.DiagnosticTests.Session_TracksActivities [135 ms]
  Error Message:
   Assert.Single() Failure: The collection did not contain any matching items
Expected:   (predicate expression)
Collection: [Activity { ActivityTraceFlags = Recorded, Baggage = [], Context = System.Diagnostics.ActivityContext, DisplayName = "initialize", Duration = 00:00:00.0006470, ··· }, Activity { ActivityTraceFlags = Recorded, Baggage = [], Context = System.Diagnostics.ActivityContext, DisplayName = "initialize", Duration = 00:00:00.0043991, ··· }, Activity { ActivityTraceFlags = Recorded, Baggage = [], Context = System.Diagnostics.ActivityContext, DisplayName = "notifications/initialized", Duration = 00:00:00.0001142, ··· }, Activity { ActivityTraceFlags = Recorded, Baggage = [], Context = System.Diagnostics.ActivityContext, DisplayName = "tools/list", Duration = 00:00:00.0001837, ··· }, Activity { ActivityTraceFlags = Recorded, Baggage = [], Context = System.Diagnostics.ActivityContext, DisplayName = "notifications/initialized", Duration = 00:00:00.0000079, ··· }, ···]
  Stack Trace:
     at ModelContextProtocol.Tests.DiagnosticTests.Session_TracksActivities() in /_/tests/ModelContextProtocol.Tests/DiagnosticTests.cs:line 50
--- End of stack trace from previous location ---
Results File: /home/runner/work/csharp-sdk/csharp-sdk/artifacts/testresults/_runnervmi13qx_2026-01-12_23_19_41.trx
Waiting 10 seconds for foreground threads to exit...
   at Xunit.v3.TestProcessLauncherAdapter.GetAssemblyInfo(ITestProcessLauncher launcher, XunitProjectAssembly projectAssembly)
   at Xunit.v3.TestProcessLauncherAdapter.GetAssemblyInfo(XunitProjectAssembly projectAssembly)
   at Xunit.Runner.v3.Xunit3..ctor(XunitProjectAssembly projectAssembly, ISourceInformationProvider sourceInformationProvider, IMessageSink diagnosticMessageSink, ITestProcessLauncher testProcessLauncher)
   at Xunit.Runner.v3.Xunit3.ForDiscoveryAndExecution(XunitProjectAssembly projectAssembly, ISourceInformationProvider sourceInformationProvider, IMessageSink diagnosticMessageSink, ITestProcessLauncher testProcessLauncher)
   at Xunit.XunitFrontController.Create(XunitProjectAssembly projectAssembly, ISourceInformationProvider sourceInformationProvider, IMessageSink diagnosticMessageSink, ITestProcessLauncher testProcessLauncher)
   at Xunit.Runner.VisualStudio.VsTestRunner.RunTestsInAssembly(IRunContext runContext, IFrameworkHandle frameworkHandle, LoggerHelper logger, TestPlatformContext testPlatformContext, RunSettings runSettings, IMessageSink reporterMessageHandler, AssemblyRunInfo runInfo)
   at Xunit.Runner.VisualStudio.VsTestRunner.RunTestsInAssembly(IRunContext runContext, IFrameworkHandle frameworkHandle, LoggerHelper logger, TestPlatformContext testPlatformContext, RunSettings runSettings, IMessageSink reporterMessageHandler, AssemblyRunInfo runInfo)
Data collector 'Blame' message: All tests finished running, Sequence file will not be generated.
Results File: D:\a\csharp-sdk\csharp-sdk\artifacts\testresults\runneradmin_runnervmybtk3_2026-01-13_17_03_41.trx

Copy link
Collaborator

@tarekgh tarekgh left a 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!

@stephentoub stephentoub enabled auto-merge (squash) January 14, 2026 01:57
@stephentoub stephentoub merged commit d52a7b1 into main Jan 14, 2026
20 of 24 checks passed
@stephentoub stephentoub deleted the copilot/implement-standard-convention-mcp branch January 14, 2026 02:07
@stephentoub
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants