Add McpLogger/McpLoggerProvider for server logs #71
Conversation
Adds a new ILoggerProvider to the McpServer's logging factory which can then relay server logs which occur within the System.Extensions.Logging pattern back to the client via a notification message as per the spec.
| var json = JsonSerializer.Serialize(message); | ||
| var element = JsonSerializer.Deserialize<JsonElement>(json); | ||
|
|
||
| await mcpServer.SendLogNotificationAsync(new LoggingMessageNotificationParams |
There was a problem hiding this comment.
Just to call out one of the issues I hit and which needs to be addressed first, this ends up possibly completing asynchronously, which then means normal logging can result in erroneous concurrent use of these types.
There was a problem hiding this comment.
Is the issue to address first here making McpJsonRpcEndpoint.SendMessageAsync and SendRequestAsync thread-safe? I think this serves as a good example why thread safety is desirable for this API. Sending things like notifications from a background thread often doesn't require any app-level synchronization. This is why SignalR's HubConnectionContext uses a SemaphoreSlim _writeLock.
Right now, it looks like McpJsonRpcEndpoint is already using ConcurrentDictionary and Interlocked to track pending requests, so as long as ITransport.SendMessageAsync is thread-safe, it should be too aside for request handler registration. SseResponseStreamTransport.SendMessageAsync should be thread-safe, since it passes all calls through a multi-writer channel, but I understand not wanting to put that burden on a transport and instead have McpJsonRpcEndpoint using something like SignalR's _writeLock.
There was a problem hiding this comment.
Is the issue to address first here making McpJsonRpcEndpoint.SendMessageAsync and SendRequestAsync thread-safe?
Yes. We should do so for both client and server.
There was a problem hiding this comment.
It looks like logging level is set in a server notification handler with the method name of the client side notification and not the request handler for setting logging level?
The test I wrote for the raw handlers should be useful as reference.
| @@ -140,6 +147,18 @@ options.GetCompletionHandler is { } handler ? | |||
| (request, ct) => Task.FromResult(new CompleteResult() { Completion = new() { Values = [], Total = 0, HasMore = false } })); | |||
| } | |||
|
|
|||
| private void AddLoggingLevelNotificationHandler(McpServerOptions options) | |||
There was a problem hiding this comment.
Sorry for the brief review comment earlier. I was in transit, and thought I had enough time to elaborate and I didn't.
This should be set based on "notifications/message". It should be set based in the "logging/setLevel" request handler. So SetLoggingLevelHandler should be called instead of this method.
https://spec.modelcontextprotocol.io/specification/2025-03-26/server/utilities/logging/
The notification handler only makes sense client side.
I wonder if we should actually throw an exception if registering a notification handler on the wrong side of Client/Server, in the cases where the spec clearly defines where to handle it?
@stephentoub
|
|
||
| [UnconditionalSuppressMessage("Trimming", "IL2026:Members annotated with 'RequiresUnreferencedCodeAttribute' require dynamic access otherwise can break functionality when trimming application code", Justification = "<Pending>")] | ||
| [UnconditionalSuppressMessage("AOT", "IL3050:Calling members annotated with 'RequiresDynamicCodeAttribute' may break functionality when AOT compiling.", Justification = "<Pending>")] | ||
| public async void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func<TState, Exception?, string> formatter) |
There was a problem hiding this comment.
Probably not for a first version but the spec states:
Servers SHOULD:
- Rate limit log messages
| => default; | ||
|
|
||
| public bool IsEnabled(LogLevel logLevel) | ||
| => logLevel.ToLoggingLevel() <= mcpServer.LoggingLevel; |
There was a problem hiding this comment.
I think this needs to check for null as well. If the client hasn't sent a logging level request, the server must not send notifications. See Message Flow
Adds a new ILoggerProvider to the McpServer's logging factory which can then relay server logs which occur within the System.Extensions.Logging pattern back to the client via a notification message as per the spec.
Motivation and Context
I started writing this code before I saw #53 was merged and now see #59 open, so thought I'd push this code up anyway even if it's ultimately just throwaway code.
How Has This Been Tested?
Not yet
Breaking Changes
Logging from the server now will all go back to the client, so it is more a behavioural change, but could be unexpected noise.
Types of changes
Checklist