Add logging infrastructure with MCPD_LOG_LEVEL support

[peteski22]

Implements SDK logging supporting MCPD_LOG_LEVEL environment variable to help users diagnose issues with server health and tool availability.

• Add LogLevel enum and Logger protocol in _logger.py
• Integrate logger into McpdClient with lazy evaluation pattern
• Log warnings for non-existent servers, unhealthy servers, and tool fetch failures
• Export Logger and LogLevel in public API
• Add comprehensive test coverage for logging functionality
• Document logging configuration and custom logger usage in README

Logging is disabled by default to avoid contaminating stdout/stderr, particularly important for MCP server contexts where stdout is used for JSON-RPC communication.

Closes: #35

Refs: mozilla-ai/mcpd-sdk-javascript#18

Summary by CodeRabbit

• New Features

  • Optional configurable logging added (TRACE, DEBUG, INFO, WARN, ERROR, OFF); clients can accept a supplied logger.

• Documentation

  • Expanded logging guide with environment-variable control, custom/partial logger examples and note that original exceptions are chained for full context.

• Tests

  • New tests validating logger initialization and logging behaviour across client flows.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Adds an internal logging subsystem (LogLevel, Logger protocol, create_logger), exposes Logger/LogLevel from the package, integrates an optional logger into McpdClient (constructor accepts logger and uses create_logger), replaces TODO warnings with real logging, updates README, and adds unit tests for logging behaviour. No public runtime API changes beyond new exports and optional logger parameter.

Changes

Cohort / File(s)	Change Summary
Documentation README.md	Adds a Logging section (MCPD_LOG_LEVEL, levels, examples), documents custom and partial logger usage, notes error chaining, and duplicates the logging docs under Core Methods.
Logging subsystem src/mcpd/_logger.py	New LogLevel enum (TRACE, DEBUG, INFO, WARN, ERROR, OFF), Logger protocol (trace/debug/info/warn/error) and create_logger factory that returns a full logger or wraps partial implementations; default logger reads MCPD_LOG_LEVEL lazily and implements TRACE level.
Public API export src/mcpd/__init__.py	Imports and adds Logger and LogLevel to __all__ to expose them from the package.
Client integration src/mcpd/mcpd_client.py	McpdClient.__init__ signature extended with `logger: Logger
Tests tests/unit/test_mcpd_client.py	Adds TestLogger class with tests for default/custom logger initialization and assertions that logging occurs for non-existent servers, unhealthy servers, and tool-fetch failures.

Possibly related PRs

• agent_tools: allow filtering and improved caching behaviour #34 — Modifies McpdClient health/tool discovery and _get_healthy_servers, overlapping with the health-check and tool-retrieval changes where logging was introduced.

Suggested reviewers

• dpoulopoulos
• aittalam

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.91% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarises the main change—adding logging infrastructure with MCPD_LOG_LEVEL support—which is the primary objective of the pull request.
Linked Issues check	✅ Passed	All coding requirements from issue #35 are met: logging infrastructure in _logger.py with LogLevel enum and Logger protocol [#35], optional logger parameter in McpdClient.init with create_logger integration [#35], logging integrated for skipped servers and tool fetch errors [#35], Logger and LogLevel exported from public API [#35], and comprehensive test coverage added.
Out of Scope Changes check	✅ Passed	All changes are directly aligned with issue #35 requirements: logging system implementation, client integration, public API exports, documentation updates, and test coverage—no unrelated changes detected.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch peteski22/logging-warnings-skipped-servers

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3afb591 and d382b98.

📒 Files selected for processing (2)

• README.md (2 hunks)
• src/mcpd/_logger.py (1 hunks)

🔇 Additional comments (11)

src/mcpd/_logger.py (8)

1-14: LGTM! Clear module documentation and appropriate imports.

The module docstring effectively communicates the purpose and critical warning about MCP server contexts. The imports are minimal and appropriate.

---

17-56: LGTM! Well-designed enum and protocol.

The LogLevel enum with str inheritance enables straightforward string comparisons, and the Logger Protocol provides a clear, consistent interface for all logger implementations.

---

59-71: LGTM! Correct TRACE level setup and ranking.

The custom TRACE level at 5 sits correctly below DEBUG (10), and the ranking dictionary properly orders all levels. The "warning" alias provides helpful backwards compatibility.

---

74-99: LGTM! Robust level resolution with lazy evaluation.

The whitespace stripping (line 84) correctly addresses the previous review feedback, and the fallback to LogLevel.OFF.value for invalid inputs provides safe default behaviour. The lazy evaluation pattern supports dynamic level changes during runtime.

---

102-157: Consider the performance implications of repeated setLevel calls.

The current implementation calls _logger.setLevel() on every log statement (lines 126, 133, 140, 147, 154) to support lazy evaluation of MCPD_LOG_LEVEL. Whilst this enables dynamic level changes without module reloading, it incurs a small overhead on each log call and could theoretically cause race conditions in multi-threaded scenarios.

However, given that:

• Logging is rarely on a hot path
• The lazy evaluation requirement is explicit
• Thread-safety concerns are minimal for typical SDK usage

This design is acceptable. If performance becomes a concern, consider caching the resolved level with a TTL or invalidation mechanism.

---

160-210: LGTM! Partial logger wrapper correctly implemented.

The wrapper properly delegates to custom methods when available and falls back to the default logger otherwise. The return type annotation (line 167) correctly addresses the previous review feedback.

---

213-264: LGTM! Well-designed factory function with excellent documentation.

The create_logger function correctly handles all three cases (None, full Logger, partial implementation), and the use of LogLevel enum values to build the required methods list (lines 253-259) ensures maintainability. The comprehensive docstring with examples is particularly helpful.

---

267-268: LGTM! Appropriate module-level default logger.

Creating the default logger at import time ensures a single instance is reused across all clients that don't provide a custom logger.

README.md (3)

160-161: LGTM! Clear documentation of the logger parameter.

The documentation accurately describes the optional logger parameter and provides a helpful reference to the detailed Logging section.

---

186-223: LGTM! Comprehensive logging documentation with appropriate warnings.

The Logging section clearly explains the environment variable control, provides a prominent warning about MCP server contexts, and includes practical usage examples. The log level descriptions are clear and helpful.

---

225-279: LGTM! Excellent custom logger examples using stderr.

The custom and partial logger examples are comprehensive and correctly demonstrate the API. All examples properly use file=sys.stderr (lines 237, 240, 243, 246, 249, 268, 272), addressing the previous review feedback and modelling the safe pattern for MCP server contexts.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

README.md (1)

155-162: Clarify logger parameter usage in initialisation snippet

You mention logger in the comment but the example call does not show it. Consider either adding logger= to the example (e.g. logger=CustomLogger()) or adding a second snippet demonstrating passing a logger so readers can immediately see how to wire it in.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b319c3c and 96df4c4.

📒 Files selected for processing (5)

• README.md (2 hunks)
• src/mcpd/__init__.py (2 hunks)
• src/mcpd/_logger.py (1 hunks)
• src/mcpd/mcpd_client.py (6 hunks)
• tests/unit/test_mcpd_client.py (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (4)

src/mcpd/_logger.py (1)

tests/unit/test_mcpd_client.py (20)

• trace (987-988)
• trace (1015-1016)
• trace (1056-1057)
• trace (1100-1101)
• debug (990-991)
• debug (1018-1019)
• debug (1059-1060)
• debug (1103-1104)
• info (993-994)
• info (1021-1022)
• info (1062-1063)
• info (1106-1107)
• warn (996-997)
• warn (1024-1025)
• warn (1065-1066)
• warn (1109-1110)
• error (999-1000)
• error (1027-1028)
• error (1068-1069)
• error (1112-1113)

src/mcpd/mcpd_client.py (2)

src/mcpd/_logger.py (5)

• Logger (31-56)
• create_logger (213-264)
• warn (50-52)
• warn (143-148)
• warn (198-203)

src/mcpd/exceptions.py (4)

• TimeoutError (275-305)
• AuthenticationError (83-102)
• ServerNotFoundError (105-132)
• McpdError (8-59)

tests/unit/test_mcpd_client.py (2)

tests/conftest.py (3)

• client (52-53)
• tools_side_effect (62-83)
• side_effect (78-79)

src/mcpd/_logger.py (9)

• trace (38-40)
• trace (122-127)
• trace (177-182)
• warn (50-52)
• warn (143-148)
• warn (198-203)
• error (54-56)
• error (150-155)
• error (205-210)

src/mcpd/__init__.py (1)

src/mcpd/_logger.py (2)

• Logger (31-56)
• LogLevel (17-28)

🪛 Ruff (0.14.5)

src/mcpd/_logger.py

167-167: Missing return type annotation for special method __init__

Add return type annotation: None

(ANN204)

src/mcpd/mcpd_client.py

113-113: Missing return type annotation for special method __init__

Add return type annotation: None

(ANN204)

---

539-539: Logging statement uses warn instead of warning

Convert to warning

(G010)

---

570-570: Logging statement uses warn instead of warning

Convert to warning

(G010)

---

575-575: Logging statement uses warn instead of warning

Convert to warning

(G010)

tests/unit/test_mcpd_client.py

983-983: Missing return type annotation for special method __init__

Add return type annotation: None

(ANN204)

---

1012-1012: Missing return type annotation for special method __init__

Add return type annotation: None

(ANN204)

---

1053-1053: Missing return type annotation for special method __init__

Add return type annotation: None

(ANN204)

---

1097-1097: Missing return type annotation for special method __init__

Add return type annotation: None

(ANN204)

---

1125-1125: Missing return type annotation for private function tools_side_effect

(ANN202)

---

1129-1129: Avoid specifying long messages outside the exception class

(TRY003)

🔇 Additional comments (6)

README.md (1)

186-272: Logging documentation matches implementation and is clear

The description of MCPD_LOG_LEVEL, available levels, default “off” behaviour, stderr-only output, and the custom/partial logger examples all line up with the new _logger module and McpdClient wiring. The warning about not enabling logging in MCP server contexts is especially important and well‑placed.

src/mcpd/__init__.py (1)

15-33: Public export of Logger and LogLevel looks appropriate

Exposing Logger and LogLevel via __all__ matches the README examples (from mcpd import Logger) and keeps the logging API discoverable from the package root. No behavioural risks here.

src/mcpd/mcpd_client.py (2)

527-546: Warning log on tool fetch failures is appropriate; silence Ruff’s false positive

Catching ConnectionError, TimeoutError, AuthenticationError, ServerNotFoundError, and McpdError here and logging a warning while continuing ensures one misbehaving server does not prevent tools from healthy servers being exposed – that matches the docstring comment about ignoring servers whose schemas cannot be retrieved.

Ruff’s G010 warning about .warn is a false positive in this context because self._logger is your own Logger protocol, not logging.Logger. If Ruff is enforced as an error, consider explicitly suppressing it:

-                self._logger.warn("Server '%s' became unavailable or unhealthy during tool fetch: %s", server_name, e)
+                self._logger.warn(  # noqa: G010 - custom Logger.warn, not logging.Logger.warn
+                    "Server '%s' became unavailable or unhealthy during tool fetch: %s",
+                    server_name,
+                    e,
+                )

---

564-580: Health filtering logs are correct; consider linter suppression for .warn

The new warnings for non‑existent servers and unhealthy servers in _get_healthy_servers are consistent with the “silent ignore” contract when logging is disabled (default), while providing useful diagnostics when MCPD_LOG_LEVEL enables logging.

As with the other call site, Ruff’s G010 about .warn is not applicable to your custom Logger protocol. If needed, you can silence it similarly:

-            if not health:
-                self._logger.warn("Skipping non-existent server '%s'", name)
+            if not health:
+                self._logger.warn(  # noqa: G010 - custom Logger.warn, not logging.Logger.warn
+                    "Skipping non-existent server '%s'",
+                    name,
+                )
                 return False

             status = health.get("status")
             if not HealthStatus.is_healthy(status):
-                self._logger.warn("Skipping unhealthy server '%s' with status '%s'", name, status)
+                self._logger.warn(  # noqa: G010 - custom Logger.warn, not logging.Logger.warn
+                    "Skipping unhealthy server '%s' with status '%s'",
+                    name,
+                    status,
+                )
                 return False

src/mcpd/_logger.py (2)

17-57: LogLevel and Logger protocol are well‑designed and match docs

The LogLevel enum values (trace, debug, info, warn, error, off) and the Logger protocol’s methods align with the README and the way McpdClient calls into the logger. Using str, Enum makes env/value comparisons straightforward and avoids accidental mismatches.

---

59-157: Default logger correctly honours MCPD_LOG_LEVEL and is safe by default

The combination of _RANKS, _resolve_log_level, _get_level, and _DefaultLogger gives you:

• Logging fully disabled when MCPD_LOG_LEVEL is unset/invalid (off).
• Lazy evaluation of the env var on every call, so tests or callers can change the level at runtime.
• Proper mapping to Python’s logging levels (including a custom TRACE level and alias support for warning).

The use of a single module‑level logger with a stderr StreamHandler and propagate = False ensures you don’t accidentally inherit application‑level logging configuration, matching the “don’t contaminate stdout/stderr” requirement.

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 96df4c4 and eda8234.

📒 Files selected for processing (5)

• README.md (2 hunks)
• src/mcpd/__init__.py (2 hunks)
• src/mcpd/_logger.py (1 hunks)
• src/mcpd/mcpd_client.py (6 hunks)
• tests/unit/test_mcpd_client.py (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (3)

src/mcpd/__init__.py (1)

src/mcpd/_logger.py (2)

• Logger (31-56)
• LogLevel (17-28)

tests/unit/test_mcpd_client.py (2)

src/mcpd/mcpd_client.py (1)

• McpdClient (71-1021)

src/mcpd/_logger.py (15)

• trace (38-40)
• trace (122-127)
• trace (177-182)
• debug (42-44)
• debug (129-134)
• debug (184-189)
• info (46-48)
• info (136-141)
• info (191-196)
• warn (50-52)
• warn (143-148)
• warn (198-203)
• error (54-56)
• error (150-155)
• error (205-210)

src/mcpd/mcpd_client.py (1)

src/mcpd/_logger.py (5)

• Logger (31-56)
• create_logger (213-264)
• warn (50-52)
• warn (143-148)
• warn (198-203)

🪛 Ruff (0.14.5)

tests/unit/test_mcpd_client.py

983-983: Missing return type annotation for special method __init__

Add return type annotation: None

(ANN204)

---

1012-1012: Missing return type annotation for special method __init__

Add return type annotation: None

(ANN204)

---

1053-1053: Missing return type annotation for special method __init__

Add return type annotation: None

(ANN204)

---

1097-1097: Missing return type annotation for special method __init__

Add return type annotation: None

(ANN204)

---

1125-1125: Missing return type annotation for private function tools_side_effect

(ANN202)

---

1129-1129: Avoid specifying long messages outside the exception class

(TRY003)

src/mcpd/mcpd_client.py

539-539: Logging statement uses warn instead of warning

Convert to warning

(G010)

---

570-570: Logging statement uses warn instead of warning

Convert to warning

(G010)

---

575-575: Logging statement uses warn instead of warning

Convert to warning

(G010)

🔇 Additional comments (10)

src/mcpd/__init__.py (1)

15-15: LGTM! Logger and LogLevel properly exported.

The imports and public API exports align correctly with the new logging subsystem.

Also applies to: 32-33

README.md (1)

186-272: Excellent documentation of logging features.

The Logging section is comprehensive and well-structured:

• Clear warning about stdout contamination in MCP server contexts
• Complete list of available log levels
• Practical examples for default, custom, and partial loggers
• Aligns well with the implementation

src/mcpd/mcpd_client.py (4)

539-539: Static analysis hint about warn vs warning is a false positive.

Ruff suggests using warning instead of warn for consistency with Python's standard logging module. However, the Logger protocol (defined in src/mcpd/_logger.py) intentionally uses warn to align with the mcpd server binary's log levels for consistency across the mcpd ecosystem, as documented in the LogLevel enum.

This design choice is correct given the project's requirements.

Based on relevant code snippets showing the Logger protocol defines warn, not warning.

Also applies to: 570-570, 575-575

---

113-119: Logger integration is well-implemented.

The optional logger parameter with lazy evaluation via create_logger(logger) cleanly supports default, full custom, and partial custom loggers without changing existing behaviour when logger is omitted. The return type annotation -> None is already present, satisfying type checking requirements.

Also applies to: 152-152

---

537-540: Logging for tool fetch failures is appropriate.

The warning log when a previously healthy server becomes unavailable during tool fetch provides valuable diagnostic information without breaking the agent tools flow.

---

566-578: Health filtering with logging is well-designed.

The _get_healthy_servers method correctly logs warnings for non-existent and unhealthy servers whilst filtering them out. The early return pattern is clean and the logging provides useful diagnostic information.

src/mcpd/_logger.py (4)

17-56: LogLevel enum and Logger protocol are well-defined.

The LogLevel enum values align with the mcpd server binary log levels for ecosystem consistency. The Logger protocol provides a clear interface that supports both custom implementations and standard logging semantics.

---

59-71: Custom TRACE level correctly implemented.

The custom TRACE level positioned at 5 (below DEBUG=10) and registered via logging.addLevelName() is correct. The _RANKS dictionary provides a clean mapping for level comparisons.

---

102-157: Default logger implementation with lazy evaluation is sound.

The lazy evaluation pattern (_get_level() called on each log statement) enables dynamic level changes without module reloading, which is useful for testing. The handler is created once with proper configuration (stderr output, no propagation), and each log method correctly checks the level before logging.

---

160-264: Partial logger wrapper and factory function are well-designed.

The _PartialLoggerWrapper correctly routes each log method to the custom implementation when present, otherwise falling back to the default logger. The create_logger factory's detection of full implementations via required_methods using LogLevel enum values keeps method names synchronised and the logic is robust. The __init__ return type annotation is already present.

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (1)

tests/unit/test_mcpd_client.py (1)

968-1149: Logging tests are thorough; type‑hint nits are optional

The TestLogger suite gives good coverage of default/custom logger initialisation and the three key warning paths (_get_healthy_servers non‑existent/unhealthy servers and _agent_tools tool‑fetch failures). This should catch regressions in the new logging behaviour.

If you want to satisfy Ruff’s ANN20x hints, you can optionally add explicit -> None to the inner CustomLogger.__init__ methods and a return type on tools_side_effect, e.g.:

-        class CustomLogger:
-            def __init__(self):
+        class CustomLogger:
+            def __init__(self) -> None:
                 self.warnings = []
                 self.errors = []
...
-        def tools_side_effect(server_name=None):
+        def tools_side_effect(server_name: str | None = None) -> list[dict]:
             ...

Purely cosmetic; behaviour stays the same.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between eda8234 and 3afb591.

📒 Files selected for processing (5)

• README.md (2 hunks)
• src/mcpd/__init__.py (2 hunks)
• src/mcpd/_logger.py (1 hunks)
• src/mcpd/mcpd_client.py (6 hunks)
• tests/unit/test_mcpd_client.py (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

src/mcpd/__init__.py (1)

src/mcpd/_logger.py (2)

• Logger (31-56)
• LogLevel (17-28)

src/mcpd/mcpd_client.py (2)

src/mcpd/_logger.py (5)

• Logger (31-56)
• create_logger (213-264)
• warn (50-52)
• warn (143-148)
• warn (198-203)

src/mcpd/exceptions.py (4)

• TimeoutError (275-305)
• AuthenticationError (83-102)
• ServerNotFoundError (105-132)
• McpdError (8-59)

🪛 Ruff (0.14.5)

src/mcpd/mcpd_client.py

539-539: Logging statement uses warn instead of warning

Convert to warning

(G010)

---

570-570: Logging statement uses warn instead of warning

Convert to warning

(G010)

---

575-575: Logging statement uses warn instead of warning

Convert to warning

(G010)

tests/unit/test_mcpd_client.py

983-983: Missing return type annotation for special method __init__

Add return type annotation: None

(ANN204)

---

1012-1012: Missing return type annotation for special method __init__

Add return type annotation: None

(ANN204)

---

1053-1053: Missing return type annotation for special method __init__

Add return type annotation: None

(ANN204)

---

1097-1097: Missing return type annotation for special method __init__

Add return type annotation: None

(ANN204)

---

1125-1125: Missing return type annotation for private function tools_side_effect

(ANN202)

---

1129-1129: Avoid specifying long messages outside the exception class

(TRY003)

🔇 Additional comments (5)

src/mcpd/__init__.py (1)

15-15: Public Logger/LogLevel exports look correct

Importing Logger and LogLevel from ._logger and adding them to __all__ keeps the public API aligned with the new logging subsystem and README usage examples.

Also applies to: 29-43

src/mcpd/mcpd_client.py (3)

21-21: Logger wiring in McpdClient is sound and non‑intrusive

Importing Logger/create_logger, adding the logger: Logger | None = None parameter, and initialising self._logger = create_logger(logger) cleanly support default, full custom, and partial custom loggers without changing behaviour when the parameter is omitted. Keeping logging behind this indirection and defaulting MCPD_LOG_LEVEL to off preserves current clients by default.

Also applies to: 113-120, 151-166

---

506-547: Warning on tool‑fetch failure for previously healthy servers is well‑scoped

Catching ConnectionError, TimeoutError, AuthenticationError, ServerNotFoundError, and McpdError around self.tools(server_name=...) and emitting self._logger.warn(...) before skipping that server matches the requirement to warn when assumed‑healthy servers fail during schema retrieval, while still allowing other healthy servers’ tools to be built. This is a sensible trade‑off between robustness and observability.

---

548-581: Health‑filter warnings behave as described in the docstring

_get_healthy_servers now logs a warning when:

• A requested server name is absent from health_map (treated as non‑existent); and
• A server’s status is not healthy, using the detailed status string.

Using HealthStatus.is_healthy here keeps the logic in sync with the enum, and centralising the health map fetch in server_health() avoids redundant calls.

src/mcpd/_logger.py (1)

1-71: Logging shim design matches requirements and is safely default‑off

LogLevel, the Logger protocol, the default _DefaultLogger, and _PartialLoggerWrapper together provide:

• A consistent level vocabulary (trace, debug, info, warn, error, off) matching the README.
• A shared module‑level default logger that is disabled unless MCPD_LOG_LEVEL is set.
• Lazy level evaluation on each log call via _get_level, so config changes and tests can adjust behaviour at runtime.
• Partial custom logger support, with only overridden methods bypassing the default logger.

Using a dedicated logger with a single StreamHandler to stderr and propagate = False is a good fit for avoiding accidental contamination of application‑level logging.

Also applies to: 102-211, 213-268