feat(logging): add support for send log message notifications and implemented the SessionWithLogging interface on streamableHttpSession
#484
Conversation
- Introduced `LoggingLevel.ShouldSendTo` for log level comparison - Added `SendLogMessageToSpecificClient` to send log notifications to a specific client - Added `SendLogMessageToClient` to send log notifications to the current session client - Implemented `SessionWithLogging` interface for `streamableHttpSession` - Added `sessionLogLevelsStore` to manage per-session log levels - Updated `StreamableHTTPServer` to support setting and retrieving log levels - Added tests covering log notification sending logic and format validation
WalkthroughThis update introduces per-session logging level management and log message notification functionality. It adds log-level filtering, methods for sending log notifications to clients, and a thread-safe store for session log levels. The notification sending logic is refactored for modularity, and comprehensive tests are added for logging-related server behavior. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/session.go (1)
250-250: Address linter suggestion for field accessThe static analysis tool suggests using the promoted field directly for cleaner code.
- method := notification.Notification.Method + method := notification.Method
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
mcp/types.go(1 hunks)server/session.go(6 hunks)server/session_test.go(1 hunks)server/streamable_http.go(11 hunks)server/streamable_http_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
server/session_test.go (2)
Learnt from: octo
PR: mark3labs/mcp-go#149
File: mcptest/mcptest.go:0-0
Timestamp: 2025-04-21T21:26:32.945Z
Learning: In the mcptest package, prefer returning errors from helper functions rather than calling t.Fatalf() directly, giving callers flexibility in how to handle errors.
Learnt from: ezynda3
PR: mark3labs/mcp-go#461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
server/streamable_http.go (1)
Learnt from: ezynda3
PR: mark3labs/mcp-go#461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
server/session.go (1)
Learnt from: ezynda3
PR: mark3labs/mcp-go#461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
🧬 Code Graph Analysis (1)
server/streamable_http.go (2)
mcp/types.go (2)
LoggingLevel(751-751)LoggingLevelError(758-758)server/session.go (2)
SessionWithTools(32-40)SessionWithLogging(23-29)
🪛 GitHub Check: lint
server/session.go
[failure] 250-250:
QF1008: could remove embedded field "Notification" from selector (staticcheck)
🪛 GitHub Actions: golangci-lint
server/session.go
[error] 250-250: golangci-lint (staticcheck): could remove embedded field "Notification" from selector (QF1008)
🔇 Additional comments (15)
mcp/types.go (1)
764-783: LGTM!The logging level severity mapping and comparison logic are correctly implemented. The severity ordering follows the syslog standard (RFC-5424) as documented, and the
ShouldSendTomethod safely handles unknown levels by returning false.server/streamable_http_test.go (1)
728-795: LGTM!The test thoroughly validates the session logging functionality. Good use of hooks to capture the session context and proper synchronization with mutex for thread-safe access.
server/session_test.go (4)
1131-1222: Well-structured test coverage for log message filtering.The test properly validates that log messages are filtered based on the session's configured log level, ensuring that only messages at or above the minimum severity are sent.
1224-1344: Comprehensive test for sending logs to specific clients.Good coverage of edge cases including non-existent sessions and uninitialized sessions. The test properly validates error types using
errors.Is.
1346-1431: Excellent test for mixed session types.This test effectively validates the behavior when sessions have different capabilities (logging-supported, logging-unsupported, and uninitialized). The error assertions correctly verify the expected error types.
1433-1509: Thorough validation of log data format preservation.The test ensures that various data types (strings, maps, errors, nil) are correctly preserved in log notifications. Good coverage of different data formats that might be logged.
server/streamable_http.go (3)
516-547: Thread-safe implementation of session log levels store.The
sessionLogLevelsStorecorrectly implements thread-safe operations with proper mutex usage. The default log level ofLoggingLevelErroris a reasonable choice.
132-140: Proper integration of logging levels throughout the session lifecycle.The session log levels store is correctly initialized, passed to sessions, and cleaned up on deletion. The lifecycle management ensures no memory leaks.
Also applies to: 260-260, 370-370, 473-473
586-597: Clean implementation of SessionWithLogging interface.The
streamableHttpSessionproperly implements theSessionWithLogginginterface by delegating to the shared store. The interface assertions confirm the implementation.Also applies to: 617-624, 635-638
server/session.go (6)
99-113: LGTM!The
buildLogNotificationmethod correctly constructs a JSON-RPC notification from the logging message parameters, properly mapping all required fields.
115-128: Well-structured log notification implementation!The method properly validates the session, checks for logging support, and implements log level filtering before sending notifications. Good defensive programming with appropriate error handling.
184-201: LGTM!The implementation correctly handles session lookup, validation, and log level filtering before sending notifications to specific clients.
217-232: Good refactoring!The separation of notification building from the sending logic improves code organization and reduces duplication.
266-286: Clean refactoring!The method now properly delegates to the core notification logic, reducing code duplication while maintaining the same public interface.
289-312: LGTM!The refactoring successfully reduces code duplication by delegating to the helper method.
- Replaced `notification.Notification.Method` with `notification.Method` to resolve QF1008 from staticcheck - No functional change; improves code clarity and eliminates linter warning
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
server/session.go (2)
146-146: Fix field access inconsistencyFor consistency with the rest of the codebase, access the Method field through the Notification embedded field.
- "method": notification.Method, + "method": notification.Notification.Method,
175-175: Fix field access inconsistencyFor consistency with the rest of the codebase, access the Method field through the Notification embedded field.
- "method": notification.Method, + "method": notification.Notification.Method,
🧹 Nitpick comments (1)
server/session.go (1)
250-250: Consider consistency with field access pattern.For consistency with the codebase pattern, consider accessing the Method field through the Notification embedded field, similar to the pattern used elsewhere in error handling.
- method := notification.Method + method := notification.Notification.Method
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/session.go(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
server/session.go (1)
Learnt from: ezynda3
PR: mark3labs/mcp-go#461
File: server/sampling.go:22-26
Timestamp: 2025-06-30T07:13:17.052Z
Learning: In the mark3labs/mcp-go project, the MCPServer.capabilities field is a struct value (serverCapabilities), not a pointer, so it cannot be nil and doesn't require nil checking. Only pointer fields within the capabilities struct should be checked for nil.
🔇 Additional comments (6)
server/session.go (6)
22-29: Well-designed interface extension.The
SessionWithLogginginterface cleanly extendsClientSessionwith logging capabilities, following Go's composition patterns correctly.
99-113: Clean notification builder implementation.The
buildLogNotificationmethod properly constructs JSON-RPC notifications fromLoggingMessageNotificationobjects with correct field mapping.
115-128: Robust log message sending with proper validation.The method correctly validates session state, logging support, and log level filtering before sending notifications. The use of
ShouldSendTofor level filtering is appropriate.
184-201: Comprehensive validation and log level filtering.The method properly validates session existence, initialization, logging support, and applies log level filtering before sending notifications. Error handling is appropriate for each validation step.
235-264: Well-extracted core notification logic.The
sendNotificationCoremethod effectively consolidates notification sending logic, including SSE upgrade handling and proper error handling for blocked channels. Good refactoring to eliminate code duplication.
266-286: Improved notification sending with better error handling.The refactored method now uses the centralized
sendNotificationCorefor consistent behavior and error handling across all notification sending methods.
LoggingLevel.ShouldSendTofor log level comparisonSendLogMessageToSpecificClientto send log notifications to a specific clientSendLogMessageToClientto send log notifications to the current session clientSessionWithLogginginterface forstreamableHttpSessionsessionLogLevelsStoreto manage per-session log levelsStreamableHTTPServerto support setting and retrieving log levelsDescription
This PR adds support for sent structured log message notifications in MCP sessions.
This enables server-to-client structured logging over the MCP notification channel, respecting the client’s configured verbosity.
For example:
Type of Change
Checklist
MCP Spec Compliance
Additional Information
This PR lays the foundation for server-driven structured logging toward clients via MCP protocol. It supports log filtering by level and ensures format consistency for different
data. The mechanism respects client preferences and ensures graceful degradation in unsupported or uninitialized cases.Summary by CodeRabbit
New Features
Bug Fixes
Tests