Skip to content

fix: remove global logger MCP-103 #425

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.

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 3 commits into from
Aug 6, 2025
Merged

fix: remove global logger MCP-103 #425

merged 3 commits into from
Aug 6, 2025

Conversation

nirinchev
Copy link
Collaborator

Proposed changes

This refactoring removes the global logger and instead creates a session-scoped logger for each session, which is then used by the whole server machinery. Additionally, we add a session id attribute once the session is bound to ensure that is included in the log messages.

The way this works is we create a composite logger that includes all configured loggers for the transport manager. Then as we create sessions, for each session we create a new composite logger that includes the transport logger and the MCP logger (if configured). Then all tools and api clients will use the session logger.

Checklist

@nirinchev nirinchev requested a review from a team as a code owner August 5, 2025 15:44
Copy link
Collaborator

@kmruiz kmruiz left a comment

Choose a reason for hiding this comment

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

[nit] Not blocking at all, but maybe it would be worth to have the logger at the session level because we have access to the session from almost everywhere?

Base automatically changed from ni/logger-changes to main August 6, 2025 17:18
@Copilot Copilot AI review requested due to automatic review settings August 6, 2025 17:24
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the logging system to eliminate the global logger pattern in favor of session-scoped loggers. The changes create a composite logger system where each session has its own logger instance that includes transport-specific and MCP-specific loggers as needed.

Key changes:

  • Replaced global logger singleton with session-scoped loggers that are passed down to all components
  • Added session ID attributes to loggers for better traceability
  • Enhanced logger system with attributes support and buffering for disk logger initialization

Reviewed Changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/common/logger.ts Enhanced logger classes with attributes support, added DiskLogger buffering, and NullLogger for testing
src/transports/base.ts Added logger creation logic to transport runners and session setup
src/transports/streamableHttp.ts Updated to use instance logger instead of global logger
src/transports/stdio.ts Updated to use instance logger instead of global logger
src/common/session.ts Modified to accept and use a logger instance
src/common/sessionStore.ts Updated to store logger per session instead of creating MCP loggers
src/server.ts Removed global logger setup, now uses session logger
src/index.ts Updated to use transport runner logger with fallback console logger
tests/unit/telemetry.test.ts Updated tests to use session logger and NullLogger
tests/unit/logger.test.ts Added comprehensive tests for new logger functionality
tests/integration/helpers.ts Added logger to session setup in tests
tests/unit/accessListUtils.test.ts Added logger to mock API client

@coveralls
Copy link
Collaborator

coveralls commented Aug 6, 2025

Pull Request Test Coverage Report for Build 16790782045

Details

  • 158 of 207 (76.33%) changed or added relevant lines in 17 files are covered.
  • 6 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.07%) to 81.233%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/common/atlas/accessListUtils.ts 3 4 75.0%
src/common/atlas/cluster.ts 1 2 50.0%
src/telemetry/telemetry.ts 7 8 87.5%
src/tools/atlas/atlasTool.ts 1 2 50.0%
src/tools/mongodb/mongodbTool.ts 1 2 50.0%
src/tools/tool.ts 4 5 80.0%
src/common/session.ts 4 6 66.67%
src/common/sessionStore.ts 10 12 83.33%
src/tools/atlas/connect/connectCluster.ts 5 7 71.43%
src/transports/stdio.ts 3 5 60.0%
Files with Coverage Reduction New Missed Lines %
src/transports/stdio.ts 1 64.41%
src/common/logger.ts 5 88.8%
Totals Coverage Status
Change from base Build 16783784184: -0.07%
Covered Lines: 3522
Relevant Lines: 4293

💛 - Coveralls

@nirinchev nirinchev merged commit 42837a4 into main Aug 6, 2025
17 of 18 checks passed
@nirinchev nirinchev deleted the ni/child-loggers branch August 6, 2025 23:17
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.

3 participants