Skip to content

chore: allow logging unredacted messages MCP-103 #420

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

Merged
merged 5 commits into from
Aug 6, 2025
Merged

chore: allow logging unredacted messages MCP-103 #420

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e3507e5
chore: allow logging unredacted messages
nirinchev Aug 4, 2025
27ae546
fix composite logger not redacting correctly
nirinchev Aug 4, 2025
7eb0143
clarify default unredacted
nirinchev Aug 4, 2025
c0b8d7a
add tests
nirinchev Aug 4, 2025
cc01ac5
Merge branch 'main' into ni/logger-changes
nirinchev Aug 6, 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
30 changes: 15 additions & 15 deletions src/common/atlas/accessListUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,25 +30,25 @@ export async function ensureCurrentIpInAccessList(apiClient: ApiClient, projectI
params: { path: { groupId: projectId } },
body: [entry],
});
logger.debug(
LogId.atlasIpAccessListAdded,
"accessListUtils",
`IP access list created: ${JSON.stringify(entry)}`
);
logger.debug({
id: LogId.atlasIpAccessListAdded,
context: "accessListUtils",
message: `IP access list created: ${JSON.stringify(entry)}`,
});
} catch (err) {
if (err instanceof ApiClientError && err.response?.status === 409) {
// 409 Conflict: entry already exists, log info
logger.debug(
LogId.atlasIpAccessListAdded,
"accessListUtils",
`IP address ${entry.ipAddress} is already present in the access list for project ${projectId}.`
);
logger.debug({
id: LogId.atlasIpAccessListAdded,
context: "accessListUtils",
message: `IP address ${entry.ipAddress} is already present in the access list for project ${projectId}.`,
});
return;
}
logger.warning(
LogId.atlasIpAccessListAddFailure,
"accessListUtils",
`Error adding IP access list: ${err instanceof Error ? err.message : String(err)}`
);
logger.warning({
id: LogId.atlasIpAccessListAddFailure,
context: "accessListUtils",
message: `Error adding IP access list: ${err instanceof Error ? err.message : String(err)}`,
});
}
}
12 changes: 10 additions & 2 deletions src/common/atlas/apiClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,11 @@ export class ApiClient {
};
} catch (error: unknown) {
const err = error instanceof Error ? error : new Error(String(error));
logger.error(LogId.atlasConnectFailure, "apiClient", `Failed to request access token: ${err.message}`);
logger.error({
id: LogId.atlasConnectFailure,
context: "apiClient",
message: `Failed to request access token: ${err.message}`,
});
}
return this.accessToken;
}
Expand All @@ -197,7 +201,11 @@ export class ApiClient {
}
} catch (error: unknown) {
const err = error instanceof Error ? error : new Error(String(error));
logger.error(LogId.atlasApiRevokeFailure, "apiClient", `Failed to revoke access token: ${err.message}`);
logger.error({
id: LogId.atlasApiRevokeFailure,
context: "apiClient",
message: `Failed to revoke access token: ${err.message}`,
});
}
this.accessToken = undefined;
}
Expand Down
6 changes: 5 additions & 1 deletion src/common/atlas/cluster.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,11 @@ export async function inspectCluster(apiClient: ApiClient, projectId: string, cl
return formatFlexCluster(cluster);
} catch (flexError) {
const err = flexError instanceof Error ? flexError : new Error(String(flexError));
logger.error(LogId.atlasInspectFailure, "inspect-cluster", `error inspecting cluster: ${err.message}`);
logger.error({
id: LogId.atlasInspectFailure,
context: "inspect-cluster",
message: `error inspecting cluster: ${err.message}`,
});
throw error;
}
}
Expand Down
111 changes: 86 additions & 25 deletions src/common/logger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,44 +50,93 @@ export const LogId = {
streamableHttpTransportCloseFailure: mongoLogId(1_006_006),
} as const;

interface LogPayload {
id: MongoLogId;
context: string;
message: string;
noRedaction?: boolean | LoggerType | LoggerType[];
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: i find no variables difficult to read and prone to difficult statements over the code. Not super used to a single variable having so many different types too. WDYT about the below?

Suggested change
noRedaction?: boolean | LoggerType | LoggerType[];
redaction?: {
enabled: boolean;
exceptFor?: LoggerType[];
};

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree - I also tend to prefer positively-named variables - in this case, I chose the negative because I want all messages to be redacted by default. If we had this as redaction/redacted, the implication would be that if we don't explicitly specify redaction: true, messages would be unredacted.

Copy link
Collaborator

Choose a reason for hiding this comment

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

good point, I threw this into an LLM and got another suggestion

 redaction?: {
        mode: "auto" | "force" | "skip";
        skipFor?: LoggerType[];
    };

but I think it still adds another complexity type w/ the modes, so feel free to ignore since I don't have a strong opinion

}

export type LoggerType = "console" | "disk" | "mcp";

export abstract class LoggerBase {
abstract log(level: LogLevel, id: MongoLogId, context: string, message: string): void;
public log(level: LogLevel, payload: LogPayload): void {
const noRedaction = payload.noRedaction !== undefined ? payload.noRedaction : "mcp";

this.logCore(level, {
...payload,
message: this.redactIfNecessary(payload.message, noRedaction),
});
}

protected abstract type: LoggerType;

protected abstract logCore(level: LogLevel, payload: LogPayload): void;

private redactIfNecessary(message: string, noRedaction: LogPayload["noRedaction"]): string {
if (typeof noRedaction === "boolean" && noRedaction) {
// If the consumer has supplied noRedaction: true, we don't redact the log message
// regardless of the logger type
return message;
}

if (typeof noRedaction === "string" && noRedaction === this.type) {
// If the consumer has supplied noRedaction: logger-type, we skip redacting if
// our logger type is the same as what the consumer requested
return message;
}

if (
typeof noRedaction === "object" &&
Array.isArray(noRedaction) &&
this.type !== null &&
noRedaction.indexOf(this.type) !== -1
) {
// If the consumer has supplied noRedaction: array, we skip redacting if our logger
// type is included in that array
return message;
}

info(id: MongoLogId, context: string, message: string): void {
this.log("info", id, context, message);
return redact(message);
}

error(id: MongoLogId, context: string, message: string): void {
this.log("error", id, context, message);
info(payload: LogPayload): void {
this.log("info", payload);
}
debug(id: MongoLogId, context: string, message: string): void {
this.log("debug", id, context, message);

error(payload: LogPayload): void {
this.log("error", payload);
}
debug(payload: LogPayload): void {
this.log("debug", payload);
}

notice(id: MongoLogId, context: string, message: string): void {
this.log("notice", id, context, message);
notice(payload: LogPayload): void {
this.log("notice", payload);
}

warning(id: MongoLogId, context: string, message: string): void {
this.log("warning", id, context, message);
warning(payload: LogPayload): void {
this.log("warning", payload);
}

critical(id: MongoLogId, context: string, message: string): void {
this.log("critical", id, context, message);
critical(payload: LogPayload): void {
this.log("critical", payload);
}

alert(id: MongoLogId, context: string, message: string): void {
this.log("alert", id, context, message);
alert(payload: LogPayload): void {
this.log("alert", payload);
}

emergency(id: MongoLogId, context: string, message: string): void {
this.log("emergency", id, context, message);
emergency(payload: LogPayload): void {
this.log("emergency", payload);
}
}

export class ConsoleLogger extends LoggerBase {
log(level: LogLevel, id: MongoLogId, context: string, message: string): void {
message = redact(message);
protected type: LoggerType = "console";

protected logCore(level: LogLevel, payload: LogPayload): void {
const { id, context, message } = payload;
console.error(`[${level.toUpperCase()}] ${id.__value} - ${context}: ${message} (${process.pid})`);
}
}
Expand All @@ -97,6 +146,8 @@ export class DiskLogger extends LoggerBase {
super();
}

protected type: LoggerType = "disk";

static async fromPath(logPath: string): Promise<DiskLogger> {
await fs.mkdir(logPath, { recursive: true });

Expand All @@ -116,8 +167,8 @@ export class DiskLogger extends LoggerBase {
return new DiskLogger(logWriter);
}

log(level: LogLevel, id: MongoLogId, context: string, message: string): void {
message = redact(message);
protected logCore(level: LogLevel, payload: LogPayload): void {
const { id, context, message } = payload;
const mongoDBLevel = this.mapToMongoDBLogLevel(level);

this.logWriter[mongoDBLevel]("MONGODB-MCP", id, context, message);
Expand Down Expand Up @@ -149,20 +200,25 @@ export class McpLogger extends LoggerBase {
super();
}

log(level: LogLevel, _: MongoLogId, context: string, message: string): void {
type: LoggerType = "mcp";

protected logCore(level: LogLevel, payload: LogPayload): void {
// Only log if the server is connected
if (!this.server?.isConnected()) {
return;
}

void this.server.server.sendLoggingMessage({
level,
data: `[${context}]: ${message}`,
data: `[${payload.context}]: ${payload.message}`,
});
}
}

class CompositeLogger extends LoggerBase {
// This is not a real logger type - it should not be used anyway.
protected type: LoggerType = "composite" as unknown as LoggerType;

private loggers: LoggerBase[] = [];

constructor(...loggers: LoggerBase[]) {
Expand All @@ -178,11 +234,16 @@ class CompositeLogger extends LoggerBase {
this.loggers = [...loggers];
}

log(level: LogLevel, id: MongoLogId, context: string, message: string): void {
public log(level: LogLevel, payload: LogPayload): void {
// Override the public method to avoid the base logger redacting the message payload
for (const logger of this.loggers) {
logger.log(level, id, context, message);
logger.log(level, payload);
}
}

protected logCore(): void {
throw new Error("logCore should never be invoked on CompositeLogger");
}
}

const logger = new CompositeLogger(new ConsoleLogger());
Expand Down
16 changes: 10 additions & 6 deletions src/common/session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,11 @@ export class Session extends EventEmitter<SessionEvents> {
await this.serviceProvider.close(true);
} catch (err: unknown) {
const error = err instanceof Error ? err : new Error(String(err));
logger.error(LogId.mongodbDisconnectFailure, "Error closing service provider:", error.message);
logger.error({
id: LogId.mongodbDisconnectFailure,
context: "session",
message: `Error closing service provider: ${error.message}`,
});
}
this.serviceProvider = undefined;
}
Expand All @@ -84,11 +88,11 @@ export class Session extends EventEmitter<SessionEvents> {
})
.catch((err: unknown) => {
const error = err instanceof Error ? err : new Error(String(err));
logger.error(
LogId.atlasDeleteDatabaseUserFailure,
"atlas-connect-cluster",
`Error deleting previous database user: ${error.message}`
);
logger.error({
id: LogId.atlasDeleteDatabaseUserFailure,
context: "session",
message: `Error deleting previous database user: ${error.message}`,
});
});
this.connectedAtlasCluster = undefined;
}
Expand Down
40 changes: 20 additions & 20 deletions src/common/sessionStore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,18 +47,18 @@ export class SessionStore {
private sendNotification(sessionId: string): void {
const session = this.sessions[sessionId];
if (!session) {
logger.warning(
LogId.streamableHttpTransportSessionCloseNotificationFailure,
"sessionStore",
`session ${sessionId} not found, no notification delivered`
);
logger.warning({
id: LogId.streamableHttpTransportSessionCloseNotificationFailure,
context: "sessionStore",
message: `session ${sessionId} not found, no notification delivered`,
});
return;
}
session.logger.info(
LogId.streamableHttpTransportSessionCloseNotification,
"sessionStore",
"Session is about to be closed due to inactivity"
);
session.logger.info({
id: LogId.streamableHttpTransportSessionCloseNotification,
context: "sessionStore",
message: "Session is about to be closed due to inactivity",
});
}

setSession(sessionId: string, transport: StreamableHTTPServerTransport, mcpServer: McpServer): void {
Expand All @@ -68,11 +68,11 @@ export class SessionStore {
}
const abortTimeout = setManagedTimeout(async () => {
if (this.sessions[sessionId]) {
this.sessions[sessionId].logger.info(
LogId.streamableHttpTransportSessionCloseNotification,
"sessionStore",
"Session closed due to inactivity"
);
this.sessions[sessionId].logger.info({
id: LogId.streamableHttpTransportSessionCloseNotification,
context: "sessionStore",
message: "Session closed due to inactivity",
});

await this.closeSession(sessionId);
}
Expand All @@ -95,11 +95,11 @@ export class SessionStore {
try {
await session.transport.close();
} catch (error) {
logger.error(
LogId.streamableHttpTransportSessionCloseFailure,
"streamableHttpTransport",
`Error closing transport ${sessionId}: ${error instanceof Error ? error.message : String(error)}`
);
logger.error({
id: LogId.streamableHttpTransportSessionCloseFailure,
context: "streamableHttpTransport",
message: `Error closing transport ${sessionId}: ${error instanceof Error ? error.message : String(error)}`,
});
}
}
delete this.sessions[sessionId];
Expand Down
Loading
Loading