Enhance HTTP and MCP session logging #608
Conversation
| "User-Agent", | ||
| McpSessionIdHeaderName, | ||
| "X-Request-ID", | ||
| "X-Correlation-ID" |
There was a problem hiding this comment.
I'm not sure if it should be the responsibility of the MCP SDK to log HTTP headers. Have you considered using ASP.NET Core's HTTP logging middleware instead? https://learn.microsoft.com/en-us/aspnet/core/fundamentals/http-logging/?view=aspnetcore-9.0#enable-http-logging
This allows each application developer to configure exactly what set of request and response headers to log, redact certain headers, log request and response bodies, and a lot more.
There was a problem hiding this comment.
Thanks for the pointer, that makes a lot of sense. Removed this change.
| @@ -774,4 +777,13 @@ private static TimeSpan GetElapsed(long startingTimestamp) => | |||
|
|
|||
| [LoggerMessage(Level = LogLevel.Trace, Message = "{EndpointName} sending message. Message: '{Message}'.")] | |||
| private partial void LogSendingMessageSensitive(string endpointName, string message); | |||
|
|
|||
| [LoggerMessage(Level = LogLevel.Debug, Message = "{EndpointName} session {SessionId} created with transport {TransportKind}")] | |||
| private partial void LogSessionCreated(string endpointName, string sessionId, string transportKind); | |||
There was a problem hiding this comment.
Can we make these trace level logs? The Streamable HTTP transport creates one of these per-request in Stateless mode, so this could get pretty noisy. I also don't think we need both LogSessionCreated and LogSessionConnected. Created and Disposed should be sufficient. "Connected" doesn't make much sense on the server side anyway.
Out of curiosity, what made you think that these events in particular were interesting? I wonder if moving these logs up a level to McpClient and McpServer would allow us to provide more informative log messages.
There was a problem hiding this comment.
I agree that this could be very noisy, moved it to TRACE level.
Out of curiosity, what made you think that these events in particular were interesting? I wonder if moving these logs up a level to McpClient and McpServer would allow us to provide more informative log messages.
We had encountered issues related to session (session ID not found IIRC). And it's very hard to debug without any logs related to session lifecycle.
I looked into McpServer and McpEndpoint where getting session and creating new sessions are involved, I'm wondering if there's any extra information we can get out of it.
| @@ -18,7 +18,7 @@ | |||
|
|
|||
| namespace ModelContextProtocol.AspNetCore; | |||
|
|
|||
| internal sealed class StreamableHttpHandler( | |||
| internal sealed partial class StreamableHttpHandler( | |||
There was a problem hiding this comment.
I don't see why this was made into a partial class.
There was a problem hiding this comment.
Made it a partial class because I wanted to use the LoggerMessage generation. Removed the Http header logs per comment.
Add debug and trace level log for:
Http request non-sensitive headersMotivation and Context
The added logs will help with server debugging.
How Has This Been Tested?
Log changes, no tests needed.
Breaking Changes
No
Types of changes
Checklist
Additional context