MCP SDK: SDK internal logging & customizable logger

[KKonstantinov]

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 Logger only in a few places for demonstrating purposes. If this gets traction, will update the PR for the Logger to 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 console has been provided. (However, we could opt-in for no logging if no Logger is provided as opposed to defaulting to console)

How Has This Been Tested?

Tested passing a Winston logger mapped to the Logger type.

Breaking Changes

Logger is optional. No breaking changes.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the MCP Documentation
• My code follows the repository's style guidelines
• New and existing tests pass locally
• I have added appropriate error handling
• I have added or updated documentation as needed

[cliffhall]

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.

[KKonstantinov]

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.

[cliffhall]

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);
}

[m-paternostro]

A few suggestions if I may:

• Design: add a generic log(level, message, extra) to make it easier for dynamic logging (mapping, shared level, ...)
• Implementation: all level specific methods (info(...), debug(...), ...) delegate to log(level, ...), making it very easy for extending the log behavior ("overwrite one method and done")
• Design: Perhaps use ...args: unknown[] instead of Record<string, unknown> for extra? For example, one cannot pass an instance of Error as a Record (it would require wrapping the error with a record).

For reference, I maintain a small, related library that I use on my projects.