-
Notifications
You must be signed in to change notification settings - Fork 78
Add logging support
#103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add logging support
#103
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require "json_rpc_handler" | ||
|
|
||
| module MCP | ||
| class LoggingMessageNotification | ||
| LOG_LEVEL_SEVERITY = { | ||
| "debug" => 0, | ||
| "info" => 1, | ||
| "notice" => 2, | ||
| "warning" => 3, | ||
| "error" => 4, | ||
| "critical" => 5, | ||
| "alert" => 6, | ||
| "emergency" => 7, | ||
| }.freeze | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Intuitively I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had the same thought, but I prioritized meeting the specifications of RFC 5424's Numerical Code explicitly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's more intuitive. Fixed. |
||
|
|
||
| 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 | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
| require_relative "../json_rpc_handler" | ||
| require_relative "instrumentation" | ||
| require_relative "methods" | ||
| require_relative "logging_message_notification" | ||
|
|
||
| module MCP | ||
| class Server | ||
|
|
@@ -31,7 +32,7 @@ def initialize(method_name) | |
|
|
||
| include Instrumentation | ||
|
|
||
| attr_accessor :name, :title, :version, :instructions, :tools, :prompts, :resources, :server_context, :configuration, :capabilities, :transport | ||
| attr_accessor :name, :title, :version, :instructions, :tools, :prompts, :resources, :server_context, :configuration, :capabilities, :transport, :logging_message_notification | ||
|
|
||
| def initialize( | ||
| name: "model_context_protocol", | ||
|
|
@@ -62,6 +63,7 @@ def initialize( | |
| validate! | ||
|
|
||
| @capabilities = capabilities || default_capabilities | ||
| @logging_message_notification = nil | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understand correctly, the Python SDK uses "info" level as the default. What do you think about doing the same? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's true. There's no need to set something that's not explicitly specified in the specification. |
||
|
|
||
| @handlers = { | ||
| Methods::RESOURCES_LIST => method(:list_resources), | ||
|
|
@@ -74,12 +76,12 @@ def initialize( | |
| Methods::INITIALIZE => method(:init), | ||
| Methods::PING => ->(_) { {} }, | ||
| Methods::NOTIFICATIONS_INITIALIZED => ->(_) {}, | ||
| Methods::LOGGING_SET_LEVEL => method(:logging_level=), | ||
|
|
||
| # No op handlers for currently unsupported methods | ||
| Methods::RESOURCES_SUBSCRIBE => ->(_) {}, | ||
| Methods::RESOURCES_UNSUBSCRIBE => ->(_) {}, | ||
| Methods::COMPLETION_COMPLETE => ->(_) {}, | ||
| Methods::LOGGING_SET_LEVEL => ->(_) {}, | ||
| } | ||
| @transport = transport | ||
| end | ||
|
|
@@ -140,6 +142,18 @@ def notify_resources_list_changed | |
| report_exception(e, { notification: "resources_list_changed" }) | ||
| end | ||
|
|
||
| def notify_log_message(data:, level:, logger: nil) | ||
| return unless @transport | ||
| return unless logging_message_notification&.should_notify?(level) | ||
|
|
||
| params = { data:, level: } | ||
| params[:logger] = logger if logger | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems there are no test cases for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As you said, I couldn't test it. Added. |
||
|
|
||
| @transport.send_notification(Methods::NOTIFICATIONS_MESSAGE, params) | ||
| rescue => e | ||
| report_exception(e, { notification: "log_message" }) | ||
| end | ||
|
|
||
| def resources_list_handler(&block) | ||
| @handlers[Methods::RESOURCES_LIST] = block | ||
| end | ||
|
|
@@ -232,6 +246,7 @@ def default_capabilities | |
| tools: { listChanged: true }, | ||
| prompts: { listChanged: true }, | ||
| resources: { listChanged: true }, | ||
| logging: {}, | ||
| } | ||
| end | ||
|
|
||
|
|
@@ -252,6 +267,19 @@ def init(request) | |
| }.compact | ||
| end | ||
|
|
||
| def logging_level=(request) | ||
| if capabilities[:logging].nil? | ||
| raise RequestHandlerError.new("Server does not support logging", request, error_type: :internal_error) | ||
| end | ||
|
|
||
| logging_message_notification = LoggingMessageNotification.new(level: request[:level]) | ||
| unless logging_message_notification.valid_level? | ||
| raise RequestHandlerError.new("Invalid log level #{request[:level]}", request, error_type: :invalid_params) | ||
| end | ||
|
|
||
| @logging_message_notification = logging_message_notification | ||
| end | ||
|
|
||
| def list_tools(request) | ||
| @tools.values.map(&:to_h) | ||
| end | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm slightly concerned about the circular reference between
lib/json_rpc_handler.rbandlib/mcp/server.rb, but I consider it to be fundamentally tied to the mechanism that handles server requests. Therefore, I think it's acceptable to be aware of RequestHandlerError.