Add logging support
#103
Add logging support
#103
Conversation
|
|
||
| module MCP | ||
| class LoggingMessageNotification | ||
| VALID_LEVELS = ["debug", "info", "notice", "warning", "error", "critical", "alert", "emergency"].freeze |
There was a problem hiding this comment.
Can you rename to the folliwng?
| VALID_LEVELS = ["debug", "info", "notice", "warning", "error", "critical", "alert", "emergency"].freeze | |
| LOG_LEVELS = ["debug", "info", "notice", "warning", "error", "critical", "alert", "emergency"].freeze |
There was a problem hiding this comment.
There was a problem hiding this comment.
I'd suggest to also use the string array directly for easier parsing:
LOG_LEVELS = %w[debug info notice warning error critical alert emergency].freezeThere was a problem hiding this comment.
You are right according to the Ruby style guide.
However, I don't think it needs to be fixed.
This repository depends on rubocop-shopify. It disables the Style/WordArray cop.
See: https://github.com/Shopify/ruby-style-guide/blob/0de5b278c576ad2f537f9f51f5897587a9b33841/rubocop.yml#L1405-L1407
Additionally, there don't seem to be any compelling reasons to enable it for this case in this repository.
| @@ -55,6 +56,7 @@ def initialize( | |||
| @server_context = server_context | |||
| @configuration = MCP.configuration.merge(configuration) | |||
| @capabilities = capabilities || default_capabilities | |||
| @logging_message_notification = nil | |||
There was a problem hiding this comment.
If I understand correctly, the Python SDK uses "info" level as the default. What do you think about doing the same?
https://github.com/modelcontextprotocol/python-sdk/blob/v1.12.3/src/mcp/server/fastmcp/server.py#L132
There was a problem hiding this comment.
I don't think it's necessary to set a default value, but what do you think?
The log_level literal specified in the MCP spec appears to be defined in mcp/types.py, and it seems that no default value has been set.
The log_level in fastmcp/server.py#L132 appears to set the default value for uvicorn's log_level.
However, if this literal is the same as the one specified in the MCP spec, I don't think it meets the logging specifications, as levels such as emergency and notice are not defined.
There was a problem hiding this comment.
That's true. There's no need to set something that's not explicitly specified in the specification.
Your current suggestion makes sense to me.
6f91a19 to
754f8cd
Compare
| @@ -0,0 +1,48 @@ | |||
| # typed: true | |||
There was a problem hiding this comment.
Wouldn't the test pass even without this # typed: true?
There was a problem hiding this comment.
Oh, it's probably a copy-paste mistake. Deleted.
|
@dak2 Oops, can you rebase with the latest master to resolve the conflicts? |
A server can send structured logging messages to the client. https://modelcontextprotocol.io/specification/2025-06-18/server/utilities/logging#logging Logging was specified in the 2024-11-05 specification, but since it was not supported in ruby-sdk, I implemented it. https://modelcontextprotocol.io/specification/2024-11-05/server/utilities/logging I also made it possible to output a simple notification message in the examples.
|
@koic |
|
Oops, sorry if my understanding of the specification and this implementation is different. As I understand it, with It seems to me that the current PR is not implemented in this way. Could you check? |
Motivation and Context
A server can send structured logging messages to the client. https://modelcontextprotocol.io/specification/2025-06-18/server/utilities/logging#logging
Logging was specified in the 2024-11-05 specification, but since it was not supported in ruby-sdk, I implemented it. https://modelcontextprotocol.io/specification/2024-11-05/server/utilities/logging
I also made it possible to output a simple notification message in the examples.
How Has This Been Tested?
Breaking Changes
None
Types of changes
Checklist
Additional context