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.
| @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.
dak2
force-pushed
the
support-logging
branch
2 times, most recently
from
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? |
|
@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? |
|
@koic Oops, I missed it! That's true. This implementation doesn't meet a logging specification. |
|
I implemented the logic to determine whether or not to send a notification based on level. If the notification level does not match, it returns nil. |
dak2
force-pushed
the
support-logging
branch
2 times, most recently
from
cf8cfcf to
cf2e38c
Compare
| "notice" => 5, | ||
| "info" => 6, | ||
| "debug" => 7, | ||
| }.freeze |
There was a problem hiding this comment.
Intuitively I think debug with the lowest level of information should be ordered as 0 and emergency with the highest level of information as 7. In other words, the keys are in reverse order.
There was a problem hiding this comment.
I had the same thought, but I prioritized meeting the specifications of RFC 5424's Numerical Code explicitly.
However, looking at the java-sdk, it seems to be defined in reverse order.
Since that order is intuitive and easy to understand, I made the same correction.
There was a problem hiding this comment.
It seems my explanation was not clear. What is actually expected is the following.
LOG_LEVELS = {
"debug" => 0,
"info" => 1,
"notice" => 2,
"warning" => 3,
"error" => 4,
"critical" => 5,
"alert" => 6,
"emergency" => 7,
}.freezeThere was a problem hiding this comment.
That's more intuitive. Fixed.
lib/mcp/server.rb
Outdated
| report_exception(e, { notification: "resources_list_changed" }) | ||
| end | ||
|
|
||
| def notify_logging_message(notification_level: nil, logger: nil, data: nil) |
There was a problem hiding this comment.
notify_log_message looks a bit simple. Was there any SDK or other reference you based the naming on? Also, level keyword might be simpler for users than the notification_level keyword argument.
And, it looks like everything except the logger keyword is required. As for the ordering, the schema's data, level, logger sequence seems easier to understand.
https://modelcontextprotocol.io/specification/2025-06-18/schema#notifications%2Fmessage
There was a problem hiding this comment.
notify_log_message looks a bit simple. Was there any SDK or other reference you based the naming on?
That may be true. I also felt it was a little redundant when I implemented it. I referred to typescript-sdk implementation. (Although the prefix is send)
However, as you said, notify_log_message is simpler, so I changed it.
It looks like python-sdk uses the name send_log_message.
There was a problem hiding this comment.
Also, level keyword might be simpler for users than the notification_level keyword argument.
And, it looks like everything except the logger keyword is required. As for the ordering, the schema's data, level, logger sequence seems easier to understand.
Fixed them.
|
|
||
| test "should_notify? returns false when notification level is lower priority than threshold level" do | ||
| logging_message_notification = LoggingMessageNotification.new(level: "warning") | ||
| assert_not logging_message_notification.should_notify?(notification_level: "notice") |
There was a problem hiding this comment.
That's just my two cents. refute appears to be preferred to assert_not in this repository.
There was a problem hiding this comment.
That seems to be the case. I don't have any particular preferences, so I'll just go with the style of the project.
Fixed it.
lib/mcp/server.rb
Outdated
| return unless @transport | ||
| raise LoggingMessageNotification::NotSpecifiedLevelError unless logging_message_notification&.level | ||
|
|
||
| current_level = notification_level || logging_message_notification.level |
There was a problem hiding this comment.
current_level would be more precise as log_level. And log_level is a required parameter, I didn't understand the intention of falling back to the global logging_message_notification.level.
There was a problem hiding this comment.
The KW argument for level was implemented with a default value of nil, so it seems that it was implemented to falling back. I removed it because it was unnecessary.
lib/mcp/server.rb
Outdated
| return unless logging_message_notification.should_notify?(notification_level: current_level) | ||
|
|
||
| params = { level: current_level } | ||
| params[:logger] = logger if logger |
There was a problem hiding this comment.
It seems there are no test cases for the logger keyword. Can you add it?
There was a problem hiding this comment.
As you said, I couldn't test it. Added.
lib/mcp/server.rb
Outdated
| raise LoggingMessageNotification::NotSpecifiedLevelError unless logging_message_notification&.level | ||
|
|
||
| current_level = notification_level || logging_message_notification.level | ||
| return unless logging_message_notification.should_notify?(notification_level: current_level) |
There was a problem hiding this comment.
notification is already clear from the receiver name, so level alone might be sufficient.
| return unless logging_message_notification.should_notify?(notification_level: current_level) | |
| return unless logging_message_notification.should_notify?(level: log_level) |
There was a problem hiding this comment.
If it is defined as level name, I think it will be difficult to distinguish the KW argument name of should_notify? from the level instance variable name in lib/mcp/logging_message_notification.rb.
As an alternative, how about setting the KW argument name of should_notify? to log_level?
I think log_level is easier to understand because it indicates data received when notifying the log. What do you think?
There was a problem hiding this comment.
Ah, it doesn't really seem necessary for it to be a keyword argument in the first place. How about making it a positional argument instead?
There was a problem hiding this comment.
Thank you. You're absolutely right. Fixed.
|
User-facing documentation should be included in the README. |
dak2
force-pushed
the
support-logging
branch
9 times, most recently
from
8322249 to
aba1877
Compare
| private attr_reader(:level) | ||
|
|
||
| def initialize(level:) | ||
| @level = level | ||
| end | ||
|
|
||
| def valid_level? | ||
| LOG_LEVEL_SEVERITY.keys.include?(level) | ||
| end | ||
|
|
||
| def should_notify?(log_level) | ||
| LOG_LEVEL_SEVERITY[log_level] >= LOG_LEVEL_SEVERITY[level] | ||
| end |
There was a problem hiding this comment.
Can you use the instance variable instead of private attr_reader method.
| private attr_reader(:level) | |
| def initialize(level:) | |
| @level = level | |
| end | |
| def valid_level? | |
| LOG_LEVEL_SEVERITY.keys.include?(level) | |
| end | |
| def should_notify?(log_level) | |
| LOG_LEVEL_SEVERITY[log_level] >= LOG_LEVEL_SEVERITY[level] | |
| end | |
| def initialize(level:) | |
| @level = level | |
| end | |
| def valid_level? | |
| LOG_LEVEL_SEVERITY.keys.include?(@level) | |
| end | |
| def should_notify?(log_level) | |
| LOG_LEVEL_SEVERITY[log_level] >= LOG_LEVEL_SEVERITY[@level] | |
| end |
There was a problem hiding this comment.
| end | ||
|
|
||
| def should_notify?(log_level) | ||
| LOG_LEVEL_SEVERITY[log_level] >= LOG_LEVEL_SEVERITY[level] |
There was a problem hiding this comment.
This would raise an error when log_level is nil, right? If so, please add a test and guard against it.
There was a problem hiding this comment.
I guess it's fine to return false instead of throwing an error.
Since this method is called during the notifications/message phase, returning false means that there is no need to respond to the client. According to JSON-RPC 2.0 spec, Notifications must not respond to the client.
| **Key Features:** | ||
| - Supports 8 log levels (debug, info, notice, warning, error, critical, alert, emergency) based on https://modelcontextprotocol.io/specification/2025-06-18/server/utilities/logging#log-levels |
There was a problem hiding this comment.
| **Key Features:** | |
| - Supports 8 log levels (debug, info, notice, warning, error, critical, alert, emergency) based on https://modelcontextprotocol.io/specification/2025-06-18/server/utilities/logging#log-levels | |
| **Key Features:** | |
| - Supports 8 log levels (debug, info, notice, warning, error, critical, alert, emergency) based on https://modelcontextprotocol.io/specification/2025-06-18/server/utilities/logging#log-levels |
There was a problem hiding this comment.
dak2
force-pushed
the
support-logging
branch
7 times, most recently
from
79132ec to
bdb0056
Compare
README.md
Outdated
| | Level | Severity | Description | | ||
| |-------|----------|-------------| | ||
| | `debug` | 0 | Detailed debugging information | | ||
| | `info` | 1 | General informational messages | | ||
| | `notice` | 2 | Normal but significant events | | ||
| | `warning` | 3 | Warning conditions | | ||
| | `error` | 4 | Error conditions | | ||
| | `critical` | 5 | Critical conditions | | ||
| | `alert` | 6 | Action must be taken immediately | | ||
| | `emergency` | 7 | System is unusable | |
There was a problem hiding this comment.
The Severity values (i.e., from 0 to 7) appear to be an internal implementation detail and not something users need to be aware of. How about renaming the Level heading to Severity Level and removing the Severity column?
There was a problem hiding this comment.
README.md
Outdated
|
|
||
| For example, if the client sets the level to `"error"` (severity 4), the server will send messages with levels: `error`, `critical`, `alert`, and `emergency`. | ||
|
|
||
| For more details, see the [MCP Logging specification](https://modelcontextprotocol.io/specification/2025-06-18/server/utilities/logging). |
There was a problem hiding this comment.
FYI: #189
Can you update this URL to the following?
| For more details, see the [MCP Logging specification](https://modelcontextprotocol.io/specification/2025-06-18/server/utilities/logging). | |
| For more details, see the [MCP Logging specification](https://modelcontextprotocol.io/specification/latest/server/utilities/logging). |
There was a problem hiding this comment.
Oops, I wasn't paying attention. I fixed it.
https://github.com/modelcontextprotocol/ruby-sdk/compare/bdb005613864fc0ed2664b4e9fc0fb16e7bf8f2d..ac354523e320a9e5f39948bca653152437266119
|
|
||
| **Key Features:** | ||
|
|
||
| - Supports 8 log levels (debug, info, notice, warning, error, critical, alert, emergency) based on https://modelcontextprotocol.io/specification/2025-06-18/server/utilities/logging#log-levels |
There was a problem hiding this comment.
Ditto.
| - Supports 8 log levels (debug, info, notice, warning, error, critical, alert, emergency) based on https://modelcontextprotocol.io/specification/2025-06-18/server/utilities/logging#log-levels | |
| - Supports 8 log levels (debug, info, notice, warning, error, critical, alert, emergency) based on https://modelcontextprotocol.io/specification/latest/server/utilities/logging#log-levels |
There was a problem hiding this comment.
README.md
Outdated
| details: { host: "localhost", port: 5432 } | ||
| }, | ||
| level: "error", | ||
| logger: "DatabaseLogger" # Optional logger name |
There was a problem hiding this comment.
Nits.
| logger: "DatabaseLogger" # Optional logger name | |
| logger: "DatabaseLogger" # Optional logger name |
There was a problem hiding this comment.
README.md
Outdated
| The SDK supports 8 log levels with increasing severity: | ||
|
|
||
| | Severity Level | Description | | ||
| |-------|-------------| | ||
| | `debug` | Detailed debugging information | | ||
| | `info` | General informational messages | | ||
| | `notice` | Normal but significant events | | ||
| | `warning` | Warning conditions | | ||
| | `error` | Error conditions | | ||
| | `critical` | Critical conditions | | ||
| | `alert` | Action must be taken immediately | | ||
| | `emergency` | System is unusable | |
There was a problem hiding this comment.
Thanks for taking care of this. Since there are now only two items, a list seems sufficient. This also looks better from an accessibility perspective.
| The SDK supports 8 log levels with increasing severity: | |
| | Severity Level | Description | | |
| |-------|-------------| | |
| | `debug` | Detailed debugging information | | |
| | `info` | General informational messages | | |
| | `notice` | Normal but significant events | | |
| | `warning` | Warning conditions | | |
| | `error` | Error conditions | | |
| | `critical` | Critical conditions | | |
| | `alert` | Action must be taken immediately | | |
| | `emergency` | System is unusable | | |
| The SDK supports 8 log severity levels with increasing severity: | |
| - `debug` - Detailed debugging information | |
| - `info` - General informational messages | |
| - `notice` - Normal but significant events | |
| - `warning` - Warning conditions | |
| - `error` - Error conditions | |
| - `critical` - Critical conditions | |
| - `alert` - Action must be taken immediately | |
| - `emergency` - System is unusable |
There was a problem hiding this comment.
That makes sense! Applied the changes.
https://github.com/modelcontextprotocol/ruby-sdk/compare/ac354523e320a9e5f39948bca653152437266119..44b969a31a3b4f126776791c2f2d4911510963b8
lib/mcp/server.rb
Outdated
| }.compact | ||
| end | ||
|
|
||
| def logging_level=(request) |
There was a problem hiding this comment.
Hm, In Ruby, using a form like logging_level = request, where the attribute and the argument being assigned are not equivalent, feels unnatural. As an alternative, how about using a name like configure_logging_level(request)?
| def logging_level=(request) | |
| def configure_logging_level(request) |
There was a problem hiding this comment.
I initially used set_logging_level to stay consistent with the python-sdk.
However, since it was flagged by RuboCop's Naming/AccessorMethodName cop, I decided to go with configure_logging_level instead.
Thanks!
There was a problem hiding this comment.
Yeah, it is possible to disable the RuboCop rule and use set_logging_level, but the behavior expected from the name doesn't seem intuitive. In any case, since this is a private method, there is effectively no impact on users.
dak2
force-pushed
the
support-logging
branch
2 times, most recently
from
c015c74 to
5b244e7
Compare
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.
|
Let's take this as a starting point. Thank you! |
|
I appreciate your continued support. Thank you!! |
Some new features and fixes have been added, and it seems like a good time to cut a release. - #103 - #223 - #218 - #221 - #225 - #224 - #220 For now, the features completed so far can be included in this release, and subsequent feature proposals can continue to be incorporated into future releases.
4 participants
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?
Server
Client
Breaking Changes
None
Types of changes
Checklist
Additional context