-
Notifications
You must be signed in to change notification settings - Fork 66
Implemented MCP logging specification with auto-injection #104
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?
Implemented MCP logging specification with auto-injection #104
Conversation
34ae3c3
to
035b75e
Compare
Why you use Yoda style? Is it necessary? |
I was only trying to follow the project convention and ensure the PR focuses on the feature, rather than refactoring. The Yoda style is not necessary, but that's what the project mostly uses at the moment. I believe refactoring is an implementation that is worthy of its own PR |
953aca7
to
30f8f42
Compare
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.
Hi @Adebayo120 - thanks a lot for giving this a try!
I did a first round of review, and didn't tackle all yet - since this is a larger feature, i guess we will need a couple of rounds and discussions - also with @CodeWithKyrian.
Just want to be transparent about that from the start, but we'll guide you through it, if you're up for that.
Please have a look at my comments, and also have a look at the examples here with Inspector:
at first glance i found two issues here:
- the logger appears as tool argument in the schema
- setting a higher log level raises errors
let me know if you have some questions!
|
||
return round($result, $this->config['precision']); | ||
$finalResult = round($result, $this->config['precision']); | ||
$logger->info('✅ Calculation completed successfully', [ |
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.
$logger->info('✅ Calculation completed successfully', [ | |
$logger->info('Calculation completed successfully', [ |
|
||
private ServerCapabilities $serverCapabilities; | ||
|
||
private bool $loggingMessageNotificationEnabled = false; |
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.
can just be
private bool $loggingMessageNotificationEnabled = false; | |
private bool $logging = true; |
and default on server side should be true
|
||
private bool $loggingMessageNotificationEnabled = false; | ||
|
||
private ?LoggingLevel $currentLoggingMessageNotificationLevel = null; |
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.
also good to have a shorter and with a default:
private ?LoggingLevel $currentLoggingMessageNotificationLevel = null; | |
private LoggingLevel $loggingLevel = LoggingLevel::Warning; |
/** | ||
* Enables logging message notifications for this registry. | ||
*/ | ||
public function enableLoggingMessageNotification(): void | ||
{ | ||
$this->loggingMessageNotificationEnabled = true; | ||
} | ||
|
||
/** | ||
* Checks if logging message notification capability is enabled. | ||
* | ||
* @return bool True if logging message notification capability is enabled, false otherwise | ||
*/ | ||
public function isLoggingMessageNotificationEnabled(): bool | ||
{ | ||
return $this->loggingMessageNotificationEnabled; | ||
} | ||
|
||
/** | ||
* Sets the current logging message notification level for the client. | ||
* | ||
* This determines which log messages should be sent to the client. | ||
* Only messages at this level and higher (more severe) will be sent. | ||
*/ | ||
public function setLoggingMessageNotificationLevel(LoggingLevel $level): void | ||
{ | ||
$this->currentLoggingMessageNotificationLevel = $level; | ||
} | ||
|
||
/** | ||
* Gets the current logging message notification level set by the client. | ||
* | ||
* @return LoggingLevel|null The current log level, or null if not set | ||
*/ | ||
public function getLoggingMessageNotificationLevel(): ?LoggingLevel | ||
{ | ||
return $this->currentLoggingMessageNotificationLevel; | ||
} |
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.
those methods should be shorter as well, please :)
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.
are the necessary in the interface as well? 🤔
Going through comments and updating, thank you so much @chr-hertel |
|
This PR implements the Model Context Protocol (MCP) logging specification, providing centralized logging capabilities with auto-injection support for capability handlers. This enables developers to debug across multiple MCP servers from a single client interface, addressing the challenge of distributed logging in MCP ecosystems.
Motivation and Context
Fixes #70
This implementation addresses the missing logging functionality as outlined in the MCP logging specification.
Problem solved:
Key benefits:
How Has This Been Tested?
Comprehensive test coverage:
Test scenarios:
McpLogger
into tools, resources, and promptsWorking examples:
examples/stdio-logging-showcase/
- Complete demonstration of auto-injectionBreaking Changes
None. This implementation maintains full backward compatibility:
->enableMcpLogging()
McpLogger
orLoggerInterface
parameters are declaredTypes of changes
Checklist
Additional context
Implementation highlights:
Core Components:
McpLogger
: PSR-3 compliant logger with MCP notification transportNotificationSender
: Handles delivery of log messages to clientsSetLogLevelHandler
: Processes client log level configuration requestsLoggingLevel
enum: RFC-5424 compliant severity levels with indexingAuto-injection mechanism: