Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Oct 13, 2025

Plan: Capture tool call name in ILogger logs on success and not just failure

  • Explore repository structure and understand how logging works
  • Identify the issue: Tool name is not captured as a structured logging parameter in success logs
  • Add tool name parameter to log messages for tool call requests (success, failure, and waiting)
  • Update MockLoggerProvider test to capture structured logging parameters (TState)
  • Add test to verify tool name is available in structured logging parameters
  • Build and test changes
  • Run full test suite to ensure no regressions (709 passed, 2 unrelated failures)
  • Clean up test code and add helpful comments
  • Add test for tool error case
  • Verify the implementation meets the requirements
  • Merge main branch to resolve conflicts (three times total)

Summary

Successfully merged main branch again to resolve the latest conflict in McpSessionHandler.cs. The conflict was caused by a new commit in main (bfc0980 - "Add net10.0 assets and run some fixers") that made some methods static.

The conflict resolution:

  • McpSessionHandler.cs: Kept the toolName: target parameter in LogSendingRequestFailed (my change)
  • Adopted the latest code from main for everything else

All tests continue to pass after the merge:

  • ✅ All 34 tests in McpServerBuilderExtensionsToolsTests pass
  • ✅ Both new structured logging tests pass
  • ✅ Build succeeds without errors

The implementation correctly captures tool names in structured logging for both success and failure cases, and is now up to date with the very latest main branch changes.

Original prompt

This section details on the original issue you should resolve

<issue_title>Capture tool call name in ILogger logs on success an not just failure</issue_title>
<issue_description>I was asked the following question:

[Them]: is there a way to pass a callback function to the streamablehttp handler to get the tool response (content body and IsError boolean flag)?
...
And also is there an easier way to track latency of each tool call? I know AOP can do it, but would be happy if sdk side has an easier way to achieve that.

To which I replied:

[Me]: This is probably best currently achieved with a custom ILoggerProvider. We added a test verifying you can observe this here. You don't have to new up a LoggerFactory like the test does. You can just call builder.Logging.AddProvider(myCustomProvider), and retain all your other logger providers (e.g. console, ETW). And the tool name is a parameter to structured logging, so you can reliably retrieve that from the TState which is IReadOnlyList<KeyValuePair<string, object?>>.

csharp-sdk/tests/ModelContextProtocol.Tests/Server/McpServerToolTests.cs at c560f47ee6d439beb38619222af3f97b7aa529af · modelcontextprotocol/csharp-sdk
Logging providers - .NET | Microsoft Learn

You could also listen to the "mcp.server.operation.duration" otel metric. Like the similar otel metrics for HTTP request duration, it captures a lot more than just the "duration". As you can see here, we add a lot of relevant tags.

But it turns out we don't have an ILogger log whith the tool name in the message (or LoggerMessage.LogValues struct) for successful invocations the way we do for errors since #447.

[Them]: thanks a lot for the pointer, however I can't see the real mcp tool name in the "state" param, am I missing something?

I think the problem is the sdk doesn't add the tool name in logging. https://github.com/modelcontextprotocol/csharp-sdk/blob/c560f47ee6d439beb38619222af3f97b7aa529af/src/ModelContextProtocol.Core/McpSession.cs#L315C36-L315C48, but just the endpoint name and request method(e.g. 'tools/call')

[Me]: My bad. If you're willing to go the otel route and write an ActivityListener instead, that does include the mcp.tool.name as a tag for the mcp.server.operation.duration metric, so you could use that. I filed an issue to include this in an ILogger log as well. We should be able to ship an update with that to NuGet next week.</issue_description>

Comments on the Issue (you are @copilot in this section)

Fixes #494


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

@Copilot Copilot AI changed the title [WIP] Capture tool call name in ILogger logs on success Capture tool call name in ILogger logs on success and not just failure Oct 13, 2025
Copilot finished work on behalf of stephentoub October 13, 2025 19:50
@Copilot Copilot AI requested a review from stephentoub October 13, 2025 19:51
@stephentoub stephentoub marked this pull request as ready for review October 13, 2025 19:59
@stephentoub stephentoub requested a review from halter73 October 13, 2025 19:59
@stephentoub
Copy link
Contributor

@copilot, please merge main to resolve conflicts.

Copy link
Contributor Author

Copilot AI commented Oct 15, 2025

@copilot, please merge main to resolve conflicts.

Merged main successfully in commit ab6ec23. The conflict in McpSessionHandler.cs was resolved by keeping the toolName parameter while adopting the McpProtocolException change from main. All tests pass.

Copilot finished work on behalf of stephentoub October 15, 2025 15:48
@stephentoub
Copy link
Contributor

@copilot , please merge main to resolve conflicts.

Copy link
Contributor Author

Copilot AI commented Oct 15, 2025

@copilot , please merge main to resolve conflicts.

Merged main successfully in commit 6e4a0c9. Resolved conflicts in McpSessionHandler.cs (kept toolName parameter), McpServer.Methods.cs (adopted concise pattern), and DiagnosticTests.cs (adopted collection expression). All tests pass.

Copilot finished work on behalf of stephentoub October 15, 2025 21:45
@stephentoub
Copy link
Contributor

@copilot, there's still a merge conflict with McpSessionHandler.cs

Copy link
Contributor Author

Copilot AI commented Oct 15, 2025

@copilot, there's still a merge conflict with McpSessionHandler.cs

Resolved merge conflict in McpSessionHandler.cs in commit c99cf26. Kept the toolName parameter while adopting latest main changes. All tests pass.

Copilot finished work on behalf of stephentoub October 15, 2025 21:54
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.

Capture tool call name in ILogger logs on success an not just failure

2 participants