Fix: Handle logging/setLevel request for server #358
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements logging level filtering for the Model Context Protocol Kotlin SDK. When a client sets a logging level, the server now filters outgoing log messages based on severity, sending only messages at or above the configured level.
- Added server-side logging level filtering based on client-set level
- Added test coverage for logging level request handling and filtering behavior
- Minor refactoring to remove redundant type parameter in client request
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| kotlin-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerSession.kt | Implements logging level filtering logic with severity-based comparison and request handler registration |
| kotlin-sdk-test/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/client/ClientTest.kt | Adds test validating that server filters messages based on client-set logging level |
| kotlin-sdk-client/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/client/Client.kt | Removes redundant type parameter from request call |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kotlin-sdk-test/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/client/ClientTest.kt
Outdated
Show resolved
Hide resolved
...-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerSession.kt
Outdated
Show resolved
Hide resolved
kpavlov
left a comment
kpavlov
left a comment
There was a problem hiding this comment.
Let's make currentLoggingLevel thread-safe and use LoggingLevel#ordinal instead of calculating the severity level in a loop for every ServerSession instance, and improve the test.
...-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerSession.kt
Outdated
Show resolved
Hide resolved
...-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerSession.kt
Outdated
Show resolved
Hide resolved
...-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerSession.kt
Outdated
Show resolved
Hide resolved
...-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/ServerSession.kt
Show resolved
Hide resolved
kotlin-sdk-test/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/client/ClientTest.kt
Outdated
Show resolved
Hide resolved
| ) | ||
|
|
||
| // Only warning and error should be received | ||
| assertEquals(2, receivedMessages.size, "Should receive only 2 messages (warning and error)") |
There was a problem hiding this comment.
Shouldn't awaitility be used here?
It will be needed for sure to verify rate-limiting
There was a problem hiding this comment.
it's common code, I used delay
kotlin-sdk-test/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/client/ClientTest.kt
Outdated
Show resolved
Hide resolved
| val serverSession = serverSessionResult.await() | ||
|
|
||
| // Set logging level to warning | ||
| val result = client.setLoggingLevel(LoggingLevel.warning) |
There was a problem hiding this comment.
It's easy to write a parametrized test verifying all logging levels
kotlin-sdk-test/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/client/ClientTest.kt
Outdated
Show resolved
Hide resolved
devcrocod
force-pushed
the
devcrocod/logging-levels
branch
from
a5dd29e to
f6b6ce4
Compare
devcrocod
force-pushed
the
devcrocod/logging-levels
branch
from
f6b6ce4 to
d8f972c
Compare
| ) | ||
| } | ||
|
|
||
| delay(100) |
There was a problem hiding this comment.
awaitility is more correct than delay
There was a problem hiding this comment.
it's common code, I used delay
kpavlov
force-pushed
the
devcrocod/logging-levels
branch
from
d8f972c to
1162179
Compare
Fix #293
Also fixed a bug with initialization: properties must be declared before the init block; otherwise, the initialization order is violated and an NPE will occur in the init block.
Motivation and Context
After initialization, MCP clients send a
logging/setLevelrequest if the logging capability is enabled. This request was previously not handled, leading to an error that blocked the use of the MCP server in the inspector and other clients.How Has This Been Tested?
Added integration test
Breaking Changes
None
Types of changes
Checklist