Skip to content

feat: Add logging handler to MCP Server #415

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

Closed
wants to merge 12 commits into from

Conversation

fegalves
Copy link

@fegalves fegalves commented May 14, 2025

This change aims to add the possibility to receive events from the MCP Server context, allowing you to log information about given requests/responses and possible exceptions that might have been thrown by tools/prompts and resources right before they're handled and sent as formal responses to the Client.

Motivation and Context

Based on #359 request, we've created a proposal for letting the developers to access specific message events that could help troubleshooting possible problems when calling MCP resources at the server.

How Has This Been Tested?

We're currently testing this version generating observability outputs to troubleshoot MCP Tool calls, allowing us to monitor possible unhandled exceptions that might occur and which parameters generated faulty scenarios.

Breaking Changes

There's no breaking changes associated with this change. This feature is meant to be fully optional.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

@fegalves fegalves marked this pull request as ready for review May 16, 2025 13:59
@fegalves fegalves changed the title wip: add logging handler to MCP Server feat: Add logging handler to MCP Server May 16, 2025
@fegalves
Copy link
Author

Hey @halter73, @stephentoub,

Would you mind to take a look in this PR, please?

@halter73
Copy link
Contributor

halter73 commented May 23, 2025

Thanks for your time on this. My feedback is basically the same feedback I left on the issue, so I would lean towards closing this PR until we design a full middleware layer that allows you to intercept and modify requests and responses perhaps short-circuiting the inner layers entirely. Given that functionality, a lot of this seems redundant. And a lot of this still seems redundant given we have structured logging and otel metrics.

I think this is similar to our Middleware! issue at #267.

But if you're strictly interested in observability, I'd also look into what you can do with a custom ILoggerProvider implementation that listens for ModelContextProtocol logs. Things like the log EventId and even the TState should be semi-stable. We don't promise not to break it, but we're not promising not to break APIs in these previews either. And they should be much more stable after 1.0.

If there's any information you need in the logs that we don't give to you, it's a much less of an ask for us to emit a new or more content-rich log message than to get something like an OnLog API added which is a bit unconventional.

With #447 merged, you can now capture any exceptions from tool calls by implementing an ILoggerProvider as demonstrated in ToolCallError_LogsErrorMessage. I don't think it makes sense to add new try/catches to log things like this PR does, but wherever we already catch user-unhandled exceptions, we should absolutely log these as an error using ILogger.

var errorLog = Assert.Single(mockLoggerProvider.LogMessages, m => m.LogLevel == LogLevel.Error);
Assert.Equal($"\"{toolName}\" threw an unhandled exception.", errorLog.Message);
Assert.IsType<InvalidOperationException>(errorLog.Exception);
Assert.Equal(exceptionMessage, errorLog.Exception.Message);

Furthermore, a lot of this could already be achieved with a delegating ITransport. It's not the simplest thing to set up, but it's already possible. Maybe we could add a Func<ITransport, ITransport> TransportFilter option to make that easier, but I don't think that's necessary. At least the TransportFilter would be less new API surface to maintain than this logging abstraction.

I'm interested in what @stephentoub and @eiriktsarpalis think too. If we do go forward on this, I do have thoughts about where we invoke the log handler, whether it needs to be a delegate, what properties belong on the context and what should be settable, etc., but I don't want to waste time on those things if we ultimately decide to close this PR.

@eiriktsarpalis
Copy link
Contributor

I concur with @halter73's view that this is something that could in principle be achieved using ILoggerProvider. Assuming the key requirements of #359 can be addressed either by improving what does get logged or via the middleware feature I think we should consider closing this PR.

@fegalves
Copy link
Author

fegalves commented May 27, 2025

Hey @halter73, @eiriktsarpalis, thanks for you considerations on this PR.
The idea of adding the try/catches was just to allow intercepting the exceptions at the Resource/Prompt contexts before it gets raised and provide pretty much the same behavior as in the tool context. Of course, we can always make the logging from inside each tool when an exception happens, but we have some scenarios where the tool didn't get called and we simply couldn't understand what went wrong. At least until the #447 PR was addressed.
As for the logging delegation, we'll try to move on with ILoggerProvider alternatives, as suggested, and see if we can achieve similar results. At least until we have an option to intercept via Middleware the calls and then do some custom handling from there.

I'll close this PR, then.

@fegalves fegalves closed this May 27, 2025
@eiriktsarpalis
Copy link
Contributor

Thank you regardless!

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.

3 participants