chore: allow logging unredacted messages MCP-103 #420
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the logger interface to allow consumers to opt out of automatic redaction logic by introducing a noRedaction option. The change enables either a blanket opt-out or selective opt-out for specific logger types, addressing cases where redaction makes logs meaningless (e.g., HTTP transport startup messages).
- Migrated all logger calls from positional parameters to object-based payloads
- Added
noRedactionconfiguration option to control redaction behavior per logger type - Updated logger base class to handle redaction logic based on logger type and consumer preferences
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/common/logger.ts | Core logger refactoring with new LogPayload interface and redaction control logic |
| src/transports/streamableHttp.ts | Updated logger calls to object format, added noRedaction: true for server startup message |
| src/tools/tool.ts | Migrated logger calls to object format with selective noRedaction flags |
| src/telemetry/telemetry.ts | Updated telemetry logging to use new object-based logger interface |
| src/server.ts | Converted server initialization logging to new format |
| src/index.ts | Updated main function logging to use object-based payloads |
| src/common/sessionStore.ts | Migrated session management logging to new format |
| src/common/session.ts | Updated session lifecycle logging to object format |
| src/resources/resource.ts | Converted resource update logging to new interface |
| src/tools/mongodb/mongodbTool.ts | Updated MongoDB tool logging to object format |
| src/tools/atlas/connect/connectCluster.ts | Migrated Atlas cluster connection logging with selective redaction control |
| src/tools/atlas/atlasTool.ts | Updated Atlas tool argument parsing logging |
| src/common/atlas/cluster.ts | Converted cluster inspection logging to new format |
| src/common/atlas/apiClient.ts | Updated API client logging to object-based interface |
| src/common/atlas/accessListUtils.ts | Migrated access list utility logging to new format |
| src/transports/stdio.ts | Updated stdio transport logging to object format |
nirinchev
changed the title
Pull Request Test Coverage Report for Build 16779406063Details
💛 - Coveralls |
|
The coverage reduction is just a result of reformatting of the logger calls (multiline vs single line). |
| id: MongoLogId; | ||
| context: string; | ||
| message: string; | ||
| noRedaction?: boolean | LoggerType | LoggerType[]; |
There was a problem hiding this comment.
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?
| noRedaction?: boolean | LoggerType | LoggerType[]; | |
| redaction?: { | |
| enabled: boolean; | |
| exceptFor?: LoggerType[]; | |
| }; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Proposed changes
This is a refactoring of the logger interface to allow consumers to opt out of the automatic redacting logic. It enables a blanket opt out or opting out for specific logger type(s).
This should only be used where the messages are safe to log or where not logging the unredacted messages makes the logs meaningless. The impetus for this change is logging by the HTTP transport, which would otherwise log things like
Server started on http://<ip-address>:3000.This is related to MCP-103, but not addressing the core of the issue.
Checklist