Skip to content

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.

Sign up for GitHub

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

Open
KKonstantinov wants to merge 12 commits into
base: main
Choose a base branch
from
+91 −0
Open

MCP SDK: SDK internal logging & customizable logger #1034

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
2042afd
add sdk internal logging PoC
KKonstantinov Oct 17, 2025
a78b866
fix comment
KKonstantinov Oct 17, 2025
d216768
fix comment
KKonstantinov Oct 17, 2025
7521991
fix comment
KKonstantinov Oct 17, 2025
3d7aca7
prettier
KKonstantinov Oct 17, 2025
df2bc7a
Merge branch 'main' into feature/sdk-internal-logging
KKonstantinov Oct 18, 2025
cb948fb
refactor to use syslog numbering of severity
KKonstantinov Oct 18, 2025
c513241
Merge branch 'feature/sdk-internal-logging' of github.com:KKonstantin…
KKonstantinov Oct 18, 2025
92b9c3c
prettier
KKonstantinov Oct 18, 2025
9f3cdf8
Merge branch 'main' into feature/sdk-internal-logging
KKonstantinov Oct 25, 2025
f661a22
Merge branch 'main' of github.com:modelcontextprotocol/typescript-sdk…
KKonstantinov Oct 25, 2025
0ab3a3f
Merge branch 'feature/sdk-internal-logging' of github.com:KKonstantin…
KKonstantinov Oct 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/server/streamableHttp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import getRawBody from 'raw-body';
import contentType from 'content-type';
import { randomUUID } from 'node:crypto';
import { AuthInfo } from './auth/types.js';
import { consoleLogger, Logger } from '../shared/logger.js';

const MAXIMUM_MESSAGE_SIZE = '4mb';

Expand Down Expand Up @@ -108,6 +109,11 @@ export interface StreamableHTTPServerTransportOptions {
* Default is false for backwards compatibility.
*/
enableDnsRebindingProtection?: boolean;

/**
* A custom logger to use for SDK internal logs.
*/
logger?: Logger;
}

/**
Expand Down Expand Up @@ -160,6 +166,7 @@ export class StreamableHTTPServerTransport implements Transport {
private _allowedHosts?: string[];
private _allowedOrigins?: string[];
private _enableDnsRebindingProtection: boolean;
private _logger: Logger;

sessionId?: string;
onclose?: () => void;
Expand All @@ -175,6 +182,7 @@ export class StreamableHTTPServerTransport implements Transport {
this._allowedHosts = options.allowedHosts;
this._allowedOrigins = options.allowedOrigins;
this._enableDnsRebindingProtection = options.enableDnsRebindingProtection ?? false;
this._logger = options.logger ?? consoleLogger;
}

/**
Expand Down Expand Up @@ -727,6 +735,7 @@ export class StreamableHTTPServerTransport implements Transport {
const standaloneSse = this._streamMapping.get(this._standaloneSseStreamId);
if (standaloneSse === undefined) {
// The spec says the server MAY send messages on the stream, so it's ok to discard if no stream
this._logger.warning(`No standalone SSE stream found, discarding message: ${message.method}`);
return;
}

Expand Down
53 changes: 53 additions & 0 deletions src/shared/logger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/**
* LogLevel - SysLog RFC5424 compliant log levels
*
* @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 •
edited
Loading

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 •
edited
Loading

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.

Copy link
Contributor

@mattzcarey mattzcarey Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not log if not passed in :) logs can add substantial cost in observability platforms so it would be better to be opt in.


/**
* Logger - SysLog RFC5424 compliant logger type
*
* @see RFC5424: https://tools.ietf.org/html/rfc5424
*/
export type Logger = {
[Level in LogLevel]: (message: string, extra?: Record<string, unknown>) => void;
};

/**
* Console logger implementation of the Logger interface, to be used by default if no custom logger is provided.
*
* @remarks
* The console logger will log to the console.
*
* The console logger will log at the following levels:
* - log (alias for console.debug)
* - info
* - error
*/
export const consoleLogger: Logger = {
debug: (message, extra) => {
console.log(message, extra);
Copy link
Member

@cliffhall cliffhall Oct 18, 2025 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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

@KKonstantinov KKonstantinov Oct 25, 2025

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 •
edited
Loading

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

Copy link
Contributor

@mattzcarey mattzcarey Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo stdioLogger should write to a file.

Copy link
Contributor

@mattzcarey mattzcarey Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Respecting log levels on console is important.

},
info: (message, extra) => {
console.info(message, extra);
},
notice: (message, extra) => {
console.info(message, extra);
},
warning: (message, extra) => {
console.warn(message, extra);
},
error: (message, extra) => {
console.error(message, extra);
},
critical: (message, extra) => {
console.error(message, extra);
},
alert: (message, extra) => {
console.error(message, extra);
},
emergency: (message, extra) => {
console.error(message, extra);
}
};
9 changes: 9 additions & 0 deletions src/shared/protocol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
} from '../types.js';
import { Transport, TransportSendOptions } from './transport.js';
import { AuthInfo } from '../server/auth/types.js';
import { consoleLogger, Logger } from './logger.js';

/**
* Callback for progress notifications.
Expand All @@ -52,6 +53,11 @@ export type ProtocolOptions = {
* e.g., ['notifications/tools/list_changed']
*/
debouncedNotificationMethods?: string[];

/**
* A custom logger to use for SDK internal logs.
*/
logger?: Logger;
};

/**
Expand Down Expand Up @@ -184,6 +190,7 @@ export abstract class Protocol<SendRequestT extends Request, SendNotificationT e
private _progressHandlers: Map<number, ProgressCallback> = new Map();
private _timeoutInfo: Map<number, TimeoutInfo> = new Map();
private _pendingDebouncedNotifications = new Set<string>();
private readonly _logger: Logger;

/**
* Callback for when the connection is closed for any reason.
Expand All @@ -210,7 +217,9 @@ export abstract class Protocol<SendRequestT extends Request, SendNotificationT e
fallbackNotificationHandler?: (notification: Notification) => Promise<void>;

constructor(private _options?: ProtocolOptions) {
this._logger = _options?.logger ?? consoleLogger;
this.setNotificationHandler(CancelledNotificationSchema, notification => {
this._logger.debug('Received a cancelled notification', { notification });
const controller = this._requestHandlerAbortControllers.get(notification.params.requestId);
controller?.abort(notification.params.reason);
});
Expand Down