Skip to content

Conversation

@KKonstantinov
Copy link
Contributor

@KKonstantinov KKonstantinov commented Oct 17, 2025

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

@KKonstantinov KKonstantinov requested a review from a team as a code owner October 17, 2025 15:49
*
* @see RFC5424: https://tools.ietf.org/html/rfc5424
*/
type LogLevel = 'debug' | 'info' | 'notice' | 'warning' | 'error' | 'critical' | 'alert' | 'emergency';
Copy link
Member

@cliffhall cliffhall Oct 18, 2025

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.

Copy link
Contributor Author

@KKonstantinov KKonstantinov Oct 18, 2025

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.

*/
export const consoleLogger: Logger = {
debug: (message, extra) => {
console.log(message, extra);
Copy link
Member

@cliffhall cliffhall Oct 18, 2025

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

Copy link
Contributor Author

@KKonstantinov KKonstantinov Oct 23, 2025

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?

Copy link
Member

@cliffhall cliffhall Oct 23, 2025

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

Copy link
Contributor Author

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?

Copy link
Member

@cliffhall cliffhall Oct 30, 2025

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

@m-paternostro
Copy link
Contributor

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.

@felixweinberger felixweinberger linked an issue Oct 21, 2025 that may be closed by this pull request
@m-paternostro
Copy link
Contributor

m-paternostro commented Oct 27, 2025

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).

/**
 * The possible levels for the Logger, inspired by RFC5424, with an additional 'trace' level for ultra-detailed logging.
 *
 * Level descriptions:
 *
 * ```
 * emergency - System is unusable (e.g., complete system failure)
 * alert     - Action must be taken immediately (e.g., data corruption detected)
 * critical  - Critical conditions (e.g., system component failures)
 * error     - Error conditions (e.g., operation failures)
 * warning   - Warning conditions (e.g., deprecated feature usage)
 * notice    - Normal but significant events (e.g., configuration changes)
 * info      - General informational messages (e.g., operation progress updates)
 * debug     - Detailed debugging information (e.g., function entry/exit points)
 * trace     - Extremely detailed debugging information (e.g., per-iteration or per-call tracing)
 * ```
 *
 * @see RFC5424: https://tools.ietf.org/html/rfc5424
 */
export type LogLevel = 'emergency' | 'alert' | 'critical' | 'error' | 'warning' | 'notice' | 'info' | 'debug' | 'trace';

/**
 * Returns the value for the specified log level, defaulting to the 'trace' value if level is unknown.
 *
 * @param level
 * @returns The log value.
 */
export const toLogLevelValue = (level: LogLevel): number => {
    switch (level) {
        case 'emergency':
            return 0;
        case 'alert':
            return 1;
        case 'critical':
            return 2;
        case 'error':
            return 3;
        case 'warning':
            return 4;
        case 'notice':
            return 5;
        case 'info':
            return 6;
        case 'debug':
            return 7;
        case 'trace':
            return 8;

        default:
            return 8;
    }
};

/**
 * Returns the log level for the specified value, defaulting to 'trace' if the value is not on the expected range.
 *
 * @param value
 * @returns The log level.
 */
export const toLogLevel = (value: number): LogLevel => {
    switch (value) {
        case 0:
            return 'emergency';
        case 1:
            return 'alert';
        case 2:
            return 'critical';
        case 3:
            return 'error';
        case 4:
            return 'warning';
        case 5:
            return 'notice';
        case 6:
            return 'info';
        case 7:
            return 'debug';
        case 8:
            return 'trace';

        default:
            return 'trace';
    }
};

type RFC5424Logger = {
    /**
     * Emits a log entry with the level specified by the method name.
     *
     * @param message - The message to log.
     * @param args - The arguments to log.
     */
    readonly [Level in LogLevel]: (message: string, ...args: unknown[]) => void;
};

/**
 * Logger - SysLog RFC5424 compliant logger type
 *
 * @see RFC5424: https://tools.ietf.org/html/rfc5424
 */
export interface Logger extends RFC5424Logger {
    /**
     * The "maximum" log level that is emitted by this logger, or 'off' to indicate that this logger does not
     * emit any log entries.
     *
     * For example, if `this.level` is 'error', only log entries with levels 'error', 'critical',
     * 'alert', and 'emergency'.
     *
     * @default 'info'
     */
    readonly level: LogLevel | 'off';

    /**
     * Emits a log entry with the specified level.
     *
     * Implementors are encouraged to use this method as the sink to emit the log entry for all levels.
     *
     * @param level - The level to log the message at.
     * @param message - The message to log.
     * @param args - The arguments to log.
     */
    readonly log: (level: LogLevel, message: string, ...args: unknown[]) => void;
}

/**
 * Base class for logger implementations.
 *
 * Implementors must provide the code for the `emit` method.
 */
export abstract class BaseLogger implements Logger {
    public readonly level: LogLevel | 'off';

    constructor(level: LogLevel | 'off' = 'info') {
        this.level = level;
    }

    emergency(message: string, ...args: unknown[]): void {
        this.log('emergency', message, ...args);
    }

    alert(message: string, ...args: unknown[]): void {
        this.log('alert', message, ...args);
    }

    critical(message: string, ...args: unknown[]): void {
        this.log('critical', message, ...args);
    }

    error(message: string, ...args: unknown[]): void {
        this.log('error', message, ...args);
    }

    warning(message: string, ...args: unknown[]): void {
        this.log('warning', message, ...args);
    }

    notice(message: string, ...args: unknown[]): void {
        this.log('notice', message, ...args);
    }

    info(message: string, ...args: unknown[]): void {
        this.log('info', message, ...args);
    }

    debug(message: string, ...args: unknown[]): void {
        this.log('debug', message, ...args);
    }

    trace(message: string, ...args: unknown[]): void {
        this.log('trace', message, ...args);
    }

    log(level: LogLevel, message: string, ...args: unknown[]): void {
        if (this.level === 'off') {
            return;
        }

        if (toLogLevelValue(level) > toLogLevelValue(this.level)) {
            return;
        }

        this.emit(level, message, ...args);
    }

    /**
     * Emits the log entry.
     *
     * This method is invoked after evaluating if the log entry is expected to be emitted and as such should
     * not perform any other level-related check.
     *
     * @param level
     * @param message
     * @param args
     */
    protected abstract emit(level: LogLevel, message: string, ...args: unknown[]): void;
}

/**
 * A logger instance that never emits.
 *
 * This logger is useful to avoid conditional checks when a logger is not provided.
 *
 * @example
 * ```ts
 * function start(logger?: Logger) {
 *   const localLogger = logger ?? OFF_LOGGER;
 *   localLogger.info('starting');
 * }
 * ```
 */
export const OFF_LOGGER: Logger = Object.freeze({
    level: 'off',
    emergency: () => {},
    alert: () => {},
    critical: () => {},
    error: () => {},
    warning: () => {},
    notice: () => {},
    info: () => {},
    debug: () => {},
    trace: () => {},
    log: () => {}
});

type LogEmitter = BaseLogger['emit'];

/**
 * Logger implementation that emits log entries to a given function.
 *
 * @example
 * ```ts
 * const logger = new EmitterLogger('debug', console.log);
 * logger.info('info message');
 * ```
 */
export class EmitterLogger extends BaseLogger {
    private readonly emitter: LogEmitter;

    constructor(level: LogLevel | 'off', emitter: LogEmitter) {
        super(level);
        this.emitter = emitter;
    }

    protected emit(level: LogLevel, message: string, ...args: unknown[]): void {
        this.emitter(level, message, ...args);
    }
}

/**
 * Logger implementation that emits log entries to the `console.log` function.
 *
 * @example
 * ```ts
 * const logger = new ConsoleLogLogger();
 * logger.info('info message'); // outputs to console.log
 * ```
 */
export class ConsoleLogLogger extends EmitterLogger {
    constructor(level: LogLevel | 'off' = 'info') {
        super(level, console.log);
    }
}

/**
 * Logger implementation that emits log entries to the `console.error` function.
 *
 * @example
 * ```ts
 * const logger = new ConsoleErrorLogger();
 * logger.warning('warning message'); // outputs to console.error
 * ```
 */
export class ConsoleErrorLogger extends EmitterLogger {
    constructor(level: LogLevel | 'off' = 'info') {
        super(level, console.error);
    }
}

/**
 * Logger implementation that emits log entries to the `console.{error|warn|log|debug}` functions based on the
 * log entry level.
 *
 * @example
 * ```ts
 * const logger = new ConsoleLogger('debug');
 * logger.debug('debug message');  // outputs to console.debug
 * logger.info('info message');  // outputs to console.log
 * logger.error('error message'); // outputs to console.error
 * ```
 */
export class ConsoleLogger extends EmitterLogger {
    constructor(level: LogLevel | 'off' = 'info') {
        super(level, (level, message, ...args) => {
            switch (level) {
                case 'emergency':
                case 'alert':
                case 'critical':
                case 'error':
                    console.error(message, ...args);
                    break;

                case 'warning':
                    console.warn(message, ...args);
                    break;

                case 'notice':
                case 'info':
                    console.log(message, ...args);
                    break;

                case 'debug':
                case 'trace':
                    console.debug(message, ...args);
                    break;

                default:
                    console.debug(message, ...args);
                    break;
            }
        });
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug Report-1: Missing Cancellation Logging Implementation

3 participants