chore: adjust logging and telemetry setup MCP-410#993
Merged
Conversation
gagik
changed the title
Collaborator
Pull Request Test Coverage Report for Build 23495353136Details
💛 - Coveralls |
LoggerBase.log() previously only redacted the message field, leaving attributes (key=value pairs) unredacted in console and disk loggers. This could expose sensitive data passed as log attributes.
Keychain now supports a parent-child hierarchy. Each session creates a child keychain via Keychain.root.createChild(), ensuring session-specific secrets are scoped while remaining visible to shared loggers via upward propagation. Config override secrets are now registered on the session keychain in createServer(), fixing the gap where overrides bypassed keychain registration entirely.
Telemetry send-failure log messages interpolate error.message which can contain sensitive fragments like URLs or tokens from network/API errors. Removing noRedaction: true ensures these messages go through the standard redaction pipeline.
ToolBase.handleError() now passes error messages through mongodb-redact with the session keychain before including them in CallToolResult. This prevents driver/API error messages from leaking sensitive fragments like hostnames, auth details, or connection URIs to MCP clients.
The monitoring server's /metrics endpoint was returning the full exception message in 500 responses, potentially leaking internal details about the Prometheus registry or server state.
…secrets" This reverts commit de25a4b.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens logging/telemetry safety by expanding redaction coverage (including attributes and tool error messages) and by reducing error-detail exposure over HTTP and telemetry.
Changes:
- Introduces parent/child
Keychainbehavior and wires session-level keychains into transport/session creation. - Expands redaction to logger attributes and to
ToolBaseerror responses; reduces telemetry/log payload verbosity. - Makes HTTP error responses less verbose and introduces a
ConfigOverrideErrortype to selectively expose safe override errors.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/tools/atlas/streams/streamsToolBase.test.ts | Updates session mock to include a Keychain. |
| tests/unit/toolBase.test.ts | Updates session mock to include a Keychain. |
| tests/unit/logger.test.ts | Adds coverage for redaction inside logger attributes. |
| tests/unit/common/keychain.test.ts | Adds tests for parent/child keychain behavior. |
| src/transports/streamableHttp.ts | Reduces error detail in HTTP responses; adjusts metrics endpoint error handling. |
| src/transports/base.ts | Creates a session keychain and registers config secrets at session creation time; passes keychain to Session/McpLogger. |
| src/tools/tool.ts | Redacts tool error messages before returning them to callers. |
| src/telemetry/telemetry.ts | Avoids logging full event payloads and removes unredacted failure logs. |
| src/server.ts | Removes error message propagation into telemetry properties. |
| src/lib.ts | Re-exports registerConfigSecrets and ConfigOverrideError. |
| src/index.ts | Removes noRedaction from shutdown error logging. |
| src/common/logging/loggerBase.ts | Adds redaction for log attributes values. |
| src/common/keychain.ts | Implements keychain parent/child hierarchy with upward propagation and chain aggregation. |
| src/common/config/parseUserConfig.ts | Exports registerConfigSecrets; moves matchingConfigKey logic here. |
| src/common/config/configUtils.ts | Removes matchingConfigKey implementation (moved elsewhere). |
| src/common/config/configOverrides.ts | Introduces ConfigOverrideError and uses it for override-related failures. |
gagik
commented
Mar 24, 2026
| orgId, | ||
| }, | ||
| query: { | ||
| itemsPerPage: 500, |
Collaborator
Author
There was a problem hiding this comment.
not sure why this became necessary...?
Collaborator
Author
There was a problem hiding this comment.
apparently the limit we had was just falling back to default, we should fix it generally but this is bumping it to a higher number so is a temporary bandaid
cveticm
approved these changes
Mar 26, 2026
Collaborator
cveticm
left a comment
cveticm
left a comment
There was a problem hiding this comment.
LGTM % one suggestion
Thanks for breaking PR description down by commits, very handy for reviewing
| event.properties.runtime_duration_ms = Date.now() - this.startTime; | ||
| if (error) { | ||
| event.properties.result = "failure"; | ||
| event.properties.reason = error.message; |
Collaborator
There was a problem hiding this comment.
Could we sanitise this error instead of removing it? assume seeing the reason for failure could be useful but perhaps not?
Collaborator
Author
There was a problem hiding this comment.
we could change this to be error.name instead, it'll be a lot more vague but yeah better than nothing
Collaborator
Author
There was a problem hiding this comment.
going to add it as error_type because we might want to block reason being sent in the future
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes logging more strict.
commit 0c87e61
Remove noRedaction from telemetry error logs
commit d267c70
commit 24221ce
commit 6581369