-
Notifications
You must be signed in to change notification settings - Fork 614
Add Trace-level logging for JSON-RPC payloads in transports #1191
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
Conversation
- Add LogTransportSendingMessage and LogTransportSendingMessageSensitive to TransportBase - Update StreamServerTransport to log sent messages at Trace level - Update StreamClientSessionTransport to log sent messages at Trace level - Update SseClientSessionTransport to log sent messages at Trace level - Update StreamableHttpClientSessionTransport to log sent messages at Trace level - Add tests for Trace-level logging of sent and received messages Co-authored-by: stephentoub <[email protected]>
src/ModelContextProtocol.Core/Client/SseClientSessionTransport.cs
Outdated
Show resolved
Hide resolved
…ssages Addresses review feedback to only have Trace-level logging for JSON-RPC payloads. Co-authored-by: stephentoub <[email protected]>
src/ModelContextProtocol.Core/Client/SseClientSessionTransport.cs
Outdated
Show resolved
Hide resolved
|
@copilot it looks like you've only adding tracing for sent payloads... what about received payloads? |
Wrap logging calls with Logger.IsEnabled(LogLevel.Trace) to avoid expensive JSON serialization when trace logging is not enabled. Co-authored-by: stephentoub <[email protected]>
src/ModelContextProtocol.Core/Client/SseClientSessionTransport.cs
Outdated
Show resolved
Hide resolved
Guard LogTransportReceivedMessageSensitive calls with IsEnabled(LogLevel.Trace) checks to avoid unnecessary work when trace logging is disabled. Co-authored-by: stephentoub <[email protected]>
The received message logging already existed via |
Add an overload that takes a JsonRpcMessage directly, encapsulating the IsEnabled check and JSON serialization to simplify call sites. Co-authored-by: stephentoub <[email protected]>
src/ModelContextProtocol.Core/Client/SseClientSessionTransport.cs
Outdated
Show resolved
Hide resolved
Since the arguments to logging calls are just passing existing variables (no expensive work), the external guards are redundant - the source-generated logger methods handle the IsEnabled check internally. Co-authored-by: stephentoub <[email protected]>
tests/ModelContextProtocol.Tests/Transport/StdioServerTransportTests.cs
Outdated
Show resolved
Hide resolved
halter73
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use LoggedTest.LoggerFactory and LoggedTest.MockLoggerProvider in the newly added tests.
tests/ModelContextProtocol.Tests/Transport/StdioServerTransportTests.cs
Outdated
Show resolved
Hide resolved
tests/ModelContextProtocol.Tests/Transport/StdioServerTransportTests.cs
Outdated
Show resolved
Hide resolved
|
We could consider adding similar logging to To get a similarly verbose logging in Kestrel that includes request and response payloads, you have to explicitly add connection logging middleware. I was going to suggest maybe adding message filters (#1207) to the client in addition to the server to cover this scenario, but given that clients will typically have a specific ILoggerFactory explicitly passed in as a parameter rather than provided ambiently by DI, I think it's fine for trace level logging to be this verbose. It appears it already was for incoming messages. |
|
@copilot Can you use the |
…tests Simplified the tests by using the base class's LoggerFactory and MockLoggerProvider instead of creating local instances. The LoggerFactory is overridden in the constructor to use LogLevel.Trace for the trace logging tests. Co-authored-by: halter73 <[email protected]>
Updated the tests in 843ccca to use |
…g level Address feedback from PR #1191: The class-level LoggerFactory override was removed from the constructor (now uses base class default of Debug). The trace-level logging tests now create their own local MockLoggerProvider and trace-level LoggerFactory to properly test Trace-level logging without affecting other tests. Co-authored-by: stephentoub <[email protected]>
This PR enables logging of all JSON-RPC payloads at the Trace level for visibility into wire-level protocol traffic.
Summary
Changes Made
Logger Methods in
TransportBase:LogTransportSendingMessageSensitive(string, string)- (Trace level) logs the full JSON payloadLogTransportSendingMessageSensitive(JsonRpcMessage)- overload that encapsulates the IsEnabled check and serialization (only this overload needs internal guarding since it does serialization)Updated Transports (simplified - no external guards needed since arguments are cheap):
StreamServerTransport- server-side stream transportStreamClientSessionTransport- client-side stream transportSseClientSessionTransport- client-side SSE transportStreamableHttpClientSessionTransport- client-side HTTP transportTests Added:
SendMessageAsync_Should_Log_At_Trace_LevelReadMessagesAsync_Should_Log_Received_At_Trace_LevelUsage
To enable JSON-RPC payload logging, configure the logging level to
Trace:When Trace logging is enabled, you'll see messages like:
The
LogTransportSendingMessageSensitive(JsonRpcMessage)overload internally checksIsEnabled(LogLevel.Trace)before serializing to avoid expensive serialization when trace logging is disabled. For call sites passing already-serialized strings, no external guard is needed since the source-generated logger methods handle the check internally.💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.