-
Notifications
You must be signed in to change notification settings - Fork 1.4k
MCP SDK: SDK internal logging & customizable logger #1034
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?
MCP SDK: SDK internal logging & customizable logger #1034
Conversation
src/shared/logger.ts
Outdated
| * | ||
| * @see RFC5424: https://tools.ietf.org/html/rfc5424 | ||
| */ | ||
| type LogLevel = 'debug' | 'info' | 'notice' | 'warning' | 'error' | 'critical' | 'alert' | 'emergency'; |
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 think there is a problem with this implementation, since it is based on level names rather than level numbers.
I had previously thought that these stated level names were actually what SysLog RFC5424 compliant log levels meant. The MCP logging spec even indicates this, saying it uses the RFC5424 log levels, and then describes them by name, not number. I now realize that is not fully correct either. It uses the level names that RFC5424 uses as examples.
On closer reading of the PRI section of RFC5424, and upon inspecting the Winston logger level names, I realize the names are not the spec, but rather the numerical severity level.
RFC5424 only says Severity values MUST be in the range of 0 to 7 inclusive. It goes on to explain how to combine Severity with Facility value to produce the Priority value. It doesn't mention anything about the specific level names being required, they are apparently only for demonstrative purposes.
Winston (which you mention as being compatible with this logging feature) uses different names:
const levels = {
error: 0,
warn: 1,
info: 2,
http: 3,
verbose: 4,
debug: 5,
silly: 6
};So a logger.critical() call would have no implementation in Winston out of the box.
Notice Winston's implementation is centered around the numerical values that RFC5424 specifies (though it has one less severity level). Note that Winston allows you to have your own custom log level titles, since it's not the name, but rather the severity level that is important.
I think if we're going to try and implement a custom logging feature around RFC5424, we should take the approach Winston did, which is to base it around numeric severity value, letting the name just be associated.
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.
Thanks for the review!
Have updated implementation to match them.
I had previously opted in to follow rather the MCP logging spec naming (e.g. emergency vs emerg - the former being used in the MCP logging spec, and the latter being used in Winston).
I agree to question the MCP logging documentation - that it does not define priorities, but by RFC5424 it should, as the standard revolves around priorities and not about naming.
PS. The code quote you've pasted in above from Winston's README is actually its npm log levels and not their syslog levels, probably accidentally.
On another note, the goal here is not to be able to pass a winston logger and for this to work out of the box, but rather any logging library can be mapped to the Logger interface, similar to how it's done for console on the consoleLogger given as example.
Would also like some comment/viewpoint on whether we should provide a default consoleLogger behavior (and log in console by default), or rather take a "not log at all if logger isn't passed by the end user" approach.
…ov/typescript-sdk into feature/sdk-internal-logging
| */ | ||
| export const consoleLogger: Logger = { | ||
| debug: (message, extra) => { | ||
| console.log(message, extra); |
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 concerned about any implementation that uses anything but console.error, since it will not play well with an STDIO connection, where all messages written to stdout MUST be well-formed JSON-RPC.
In Node, console.log, console.info, and console.debug all write to stdout.
People might use it only during development and with StreamableHttp it's not an issue, but still, it's a footgun.
I could imagine an implementation that always uses console.error(), but encodes the level name in the message, e.g.
info: (message, extra) => {
`console.error(`[info] ${message}`, extra);
}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.
Maybe we have another default logger for STDIO specifically? That's absolutely possible, and won't add much overhead in terms of code implementation.
Not sure here... Because otherwise we limit every other transport to .error; I know for sure some systems (e.g. if it's a NextJS backend) will treat .error in a special way - so defaulting for that for info/debug/notice seems suspicious.
Alternatively, we can wholeheartedly just export the default console logger, a default stdio console logger, and let the end user of the SDK specify it if they wish. Otherwise, our default would be to not log anything?
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.
Doing two logger implementations, one that would work with STDIO and one that would work with other transports seems reasonable. Not sure how to name them, maybe defaultLogger and stdioLogger? Then streamableHttp.ts could use defaultLogger and stdio.ts could use stdioLogger.
However, you were planning to log in different places in the SDK, will they all have access to the logger that the transport has chosen? Server code that gets run before a transport is connected could still fail. What logger would it use? Maybe when creating a Server or McpServer instance, you could pass it in there? Since you have logger:Logger in ProtocolOptions, and since ServerOptions extends ProtocolOptions you should be able to send it in when creating the server:
import { stdioLogger as logger } from '@modelcontextprotocol/sdk/shared/logger.js';
...
const server = new McpServer(
{
name: "my-server",
version: "1.0.0",
},
{
logger,
capabilities: {
tools: {},
logging: {},
},
instructions,
},
);
const transport = new StdioServerTransport();
await server.connect(transport);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.
The problem I see is that McpServer might then use a defaultLogger, then upon connecting to Stdio, we could switch to stdioLogger (for that, we can use both error and warn since they both get written to stderr as opposed to stdout).
However, it still concerns me - I'm not sure if McpServer wrote to stdout just before the StdioServerTransport got connected, whether that would cause issues for stdio? I need to check/test.
Ultimately we might decide to not go for a default logger at all, and leave that to the implementor? Or alternatively, only have default for the HTTP transports?
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 was thinking the exact same thing, re: logging to stdio from the server prior to connection and then stderr from connection onward. I sort of think that would be ok. We'd want to make sure that the logger used prior to connection is replaced by the logger used afterward. That is, Server class would have a public logger property that could be set by the McpServer class and replaced once the Transport implementation once connected. McpServer would use the logger on its this.server reference, or better, by way of accessors to such. So
// This could return a Server or McpServer
// Any logging as a result of this call is
// happening on the server's default logger,
// which could be console.log
const {server} = createServer();
// Now we switch to a transport-appropriate logger
server.logger = new StdioTransportLogger()
const transport = new StdioServerTransport();
await server.connect(transport);|
A few suggestions if I may:
For reference, I maintain a small, related library that I use on my projects. |
… into feature/sdk-internal-logging
…ov/typescript-sdk into feature/sdk-internal-logging
|
I took the liberty to propose a logger implementation that, I believe is "simple but not simplistic". Let me know what you think. If you like the direction, we could put in the PR, add tests, and potentially add a formatter (to output the timestamp for example). |
Currently, the SDK does not have any internal logging, nor a way for external / end users of the SDK to provide their own custom loggers (such as https://github.com/winstonjs/winston, https://github.com/pinojs/pino, https://github.com/trentm/node-bunyan or others).
NOTE: Have added the
Loggeronly in a few places for demonstrating purposes. If this gets traction, will update the PR for theLoggerto exist in all classes (Server & Client-side).Motivation and Context
This proposed change introduces a type that can be optionally passed (no breaking change) to the MCP SDK classes, which will then lead to the provided logger being used to log.
SysLog - RFC5424 levels have been used for the
Logger.A default mapping of
consolehas been provided. (However, we could opt-in for no logging if noLoggeris provided as opposed to defaulting toconsole)How Has This Been Tested?
Tested passing a Winston logger mapped to the
Loggertype.Breaking Changes
Logger is optional. No breaking changes.
Types of changes
Checklist