feat(MCPServer): support logging/setlevel request
#276
Conversation
WalkthroughA new capability for configuring per-session logging levels was introduced. This includes a new MCP method ( Changes
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
✨ 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
🔭 Outside diff range comments (1)
server/hooks.go (1)
1-3:⚠️ Potential issueDirectly editing generated code is not recommended.
This file is marked as generated code with a clear "DO NOT EDIT" comment at the top, yet the changes have been made directly to it. This is likely why your pipeline is failing.
Instead of directly editing this file, you should:
- Modify the template file located at
server/internal/gen/hooks.go.tmpl- Run
go generate ./...to regenerate the code- Commit the regenerated code
The pipeline error confirms this: "Generated code is not up to date. Please run 'go generate ./...' and commit the changes."
🧹 Nitpick comments (4)
server/stdio.go (2)
56-56: Consistent field alignment in struct definition.The field alignment in the
stdioSessionstruct is inconsistent. The newloggingLevelfield uses tabs differently than the fields above it.- notifications chan mcp.JSONRPCNotification - initialized atomic.Bool - loggingLevel atomic.Value + notifications chan mcp.JSONRPCNotification + initialized atomic.Bool + loggingLevel atomic.Value
81-84: Consider adding nil check inGetLogLevel.The type assertion
level.(mcp.LoggingLevel)could panic ifloggingLevelhasn't been initialized. While theInitialize()method should set a default value, it would be safer to add a nil check.func(s *stdioSession) GetLogLevel() mcp.LoggingLevel { level := s.loggingLevel.Load() + if level == nil { + return mcp.LoggingLevelError + } return level.(mcp.LoggingLevel) }server/sse.go (1)
58-65: Thread-safe implementation of logging level methods.The
SetLogLevelandGetLogLevelmethods properly use atomic operations for thread safety.Similar to the implementation in
stdio.go, consider adding a nil check inGetLogLevel:func(s *sseSession) GetLogLevel() mcp.LoggingLevel { level := s.loggingLevel.Load() + if level == nil { + return mcp.LoggingLevelError + } return level.(mcp.LoggingLevel) }server/session_test.go (1)
662-708: Good test coverage for new logging functionality.The test properly verifies:
- Default logging level is set correctly
- The
logging/setLevelrequest works as expected- The logging level is updated atomically
Consider adding a test case for when logging capability is disabled to ensure proper error responses.
func TestMCPServer_SetLevelNotEnabled(t *testing.T) { // Create server without logging capability server := NewMCPServer("test-server", "1.0.0") // Create and initialize a session sessionChan := make(chan mcp.JSONRPCNotification, 10) session := &sessionTestClientWithLogging{ sessionID: "session-1", notificationChannel: sessionChan, } session.Initialize() // Register the session err := server.RegisterSession(context.Background(), session) require.NoError(t, err) // Try to set logging level when capability is disabled sessionCtx := server.WithContext(context.Background(), session) setRequest := map[string]any{ "jsonrpc": "2.0", "id": 1, "method": "logging/setLevel", "params": map[string]any{ "level": mcp.LoggingLevelCritical, }, } requestBytes, err := json.Marshal(setRequest) require.NoError(t, err) response := server.HandleMessage(sessionCtx, requestBytes) resp, ok := response.(mcp.JSONRPCResponse) assert.True(t, ok) // Verify we get a METHOD_NOT_FOUND error assert.NotNil(t, resp.Error) assert.Equal(t, mcp.METHOD_NOT_FOUND, resp.Error.Code) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
mcp/types.go(1 hunks)server/errors.go(1 hunks)server/hooks.go(4 hunks)server/request_handler.go(1 hunks)server/server.go(1 hunks)server/session.go(1 hunks)server/session_test.go(3 hunks)server/sse.go(3 hunks)server/stdio.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
server/session.go (1)
mcp/types.go (1)
LoggingLevel(624-624)
server/request_handler.go (3)
mcp/types.go (5)
MethodSetLogLevel(52-52)SetLevelRequest(594-602)EmptyResult(255-255)METHOD_NOT_FOUND(242-242)INVALID_REQUEST(241-241)server/errors.go (1)
ErrUnsupported(10-10)server/server.go (1)
UnparsableMessageError(67-71)
server/stdio.go (2)
mcp/types.go (3)
JSONRPCNotification(210-213)LoggingLevelError(631-631)LoggingLevel(624-624)server/session.go (2)
ClientSession(11-20)SessionWithLogging(23-29)
server/session_test.go (4)
mcp/types.go (7)
JSONRPCNotification(210-213)LoggingLevelError(631-631)LoggingLevel(624-624)LoggingLevelCritical(632-632)JSONRPCResponse(216-220)Result(191-195)EmptyResult(255-255)server/session.go (3)
ClientSession(11-20)SessionWithTools(32-40)SessionWithLogging(23-29)server/server.go (2)
NewMCPServer(275-301)WithLogging(261-265)testdata/mockstdio_server.go (1)
JSONRPCResponse(18-26)
server/sse.go (2)
mcp/types.go (2)
LoggingLevelError(631-631)LoggingLevel(624-624)server/session.go (3)
ClientSession(11-20)SessionWithTools(32-40)SessionWithLogging(23-29)
server/hooks.go (1)
mcp/types.go (3)
SetLevelRequest(594-602)EmptyResult(255-255)MethodSetLogLevel(52-52)
🪛 GitHub Actions: go
server/request_handler.go
[error] 110-140: Generated code is not up to date. Please run 'go generate ./...' and commit the changes. Detected modifications in this file that are not committed.
server/hooks.go
[error] 67-100: Generated code is not up to date. Please run 'go generate ./...' and commit the changes. Detected modifications in this file that are not committed.
🔇 Additional comments (12)
mcp/types.go (1)
50-52: Good addition of the new logging method constant.The new
MethodSetLogLevelconstant is well-documented with a clear comment and specification URL reference. The naming follows the established pattern of other MCP method constants in this file.server/errors.go (1)
20-20: LGTM: Clear and consistent error message definition.The new error
ErrSessionDoesNotSupportLoggingfollows the established naming convention and provides a clear message about the failure condition.server/session.go (1)
22-29: Well-designed interface extension for logging functionality.The
SessionWithLogginginterface follows good design principles by:
- Extending the base
ClientSessioninterface- Providing clear, focused methods with good documentation
- Following the existing pattern of interface extensions (
SessionWithTools)This approach allows for easy implementation by various session types while maintaining backward compatibility.
server/stdio.go (3)
67-69: Initialization of logging level looks good.The default logging level is properly set during session initialization.
77-79: Thread-safe implementation ofSetLogLevel.Good use of atomic operations to ensure thread safety when updating the logging level.
86-89: Good use of interface assertions.The interface assertions ensure at compile-time that
stdioSessionimplements both theClientSessionandSessionWithLogginginterfaces.server/sse.go (3)
31-31: Adding atomic logging level storage.Good use of
atomic.Valuefor thread-safe storage of the logging level.
49-51: Proper initialization of logging level.The default logging level is correctly set during session initialization.
92-94: Updated interface assertions correctly.Good practice to explicitly verify at compile-time that the implementation satisfies all required interfaces.
server/session_test.go (2)
102-136: Comprehensive test client implementation.The
sessionTestClientWithLoggingimplementation is thorough and properly implements theSessionWithLogginginterface with thread-safe operations.
137-142: Updated interface assertions in tests.Good practice to explicitly verify at compile-time that test implementations satisfy all required interfaces.
server/hooks.go (1)
70-71: New hook implementation follows the correct pattern.The implementation of the
SetLevelhooks follows the same pattern as other hooks in the codebase, which is good. The types, struct fields, and methods are all consistent with the existing pattern.Also applies to: 105-106, 319-345
There was a problem hiding this comment.
This file is autogenerated (as is request_handler.go), you need to update the associated template file and re-run go generate ./....
There was a problem hiding this comment.
emm.... one thing that confuses me is that in the added case mcp.MethodSetLogLevel: statement , we may need statement if s.capabilities.logging == false to check whether MCPServer supports logging and for code style consistency, but the template file will generate statement if s.capabilities.logging == nil instead, which incurs a syntax error since serverCapabilities.logging is of type bool in our code now.
So may we change the template file or the type of s.capabilities.logging or move this checking into func MCPServer.handleSetLevel ?
There was a problem hiding this comment.
We could change it to be something like this:
diff --git a/server/server.go b/server/server.go
index 33ab4c3866..40225526ae 100644
--- a/server/server.go
+++ b/server/server.go
@@ -161,7 +161,7 @@
tools *toolCapabilities
resources *resourceCapabilities
prompts *promptCapabilities
- logging bool
+ logging *bool
}
// resourceCapabilities defines the supported resource-related features
@@ -260,7 +260,7 @@
// WithLogging enables logging capabilities for the server
func WithLogging() ServerOption {
return func(s *MCPServer) {
- s.capabilities.logging = true
+ s.capabilities.logging = mcp.ToBoolPtr(true)
}
}
@@ -289,7 +289,7 @@
tools: nil,
resources: nil,
prompts: nil,
- logging: false,
+ logging: mcp.ToBoolPtr(false),
},
}
@@ -512,7 +512,7 @@
}
}
- if s.capabilities.logging {
+ if *s.capabilities.logging == true {
capabilities.Logging = &struct{}{}
}
What do you think? With ^ changes, local tests pass for me (so it's a "safe" change). It doesn't seem to impact users (since they'd still just be calling WithLogging), and I think it would allow the logic you are looking for?
If that doesn't work, your other suggestion seemed totally fine too (make the generator to specifically handle this case).
There was a problem hiding this comment.
Sorry for my late response. I think it's a great and neat solution for this problem. I have adopted it and made a little adjustment:
- initialize
loggingto benilby default in funcNewMCPServer:
Because our template file will generateif s.capabilities.logging == nilto check whether MCPServer supports logging. If we uselogging: mcp.ToBoolPtr(false)as the default setting ,s.capabilities.loggingwill not benilwhen MCPServer does not support logging and theifstatement will not work as expected.
For this situation, I also added a new Testcase namedTestMCPServer_SetLevelNotEnabledin filesession_test.go - add
nilcheck in funchandleInitializesince I setloggingto benilby default:
if s.capabilities.logging != nil && *s.capabilities.logging
But this solution relies on one assumption: logging should be a pointer either to nil or true , and should not be a pointer to false which couldn't be detected by the generated if statement. And our design now ensures this assumption, because we set logging to be nil by default and only func WithLogging() will update logging to be a pointer to true
What do you think? Should we make more adjustments?
This PR supports
logging/setlevelfunctionality according to the specification:Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores