fix: Make context logging functions spec-compliant - Fixes #397#1907
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to make context logging spec-compliant by allowing structured (JSON-serializable) log payloads (issue #397) and updates tests to validate logging of non-string values.
Changes:
- Adds a new test covering structured log data types (dict, list, number, boolean, string).
- Updates the client logging-callback test to pass structured fields via
data=instead ofextra=. - Adds a new
Context.log(..., data: Any)+ convenience logging methods inmcpserver/server.py.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
tests/server/mcpserver/test_server.py |
Adds a structured-data logging test intended to validate spec compliance. |
tests/client/test_logging_callback.py |
Updates one logging call to use the new data= shape rather than extra=. |
src/mcp/server/mcpserver/server.py |
Introduces new Context/logging implementation and a StreamableHTTPASGIApp class definition. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
36b4521 to
ff86c75
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| level: Literal["debug", "info", "warning", "error"], | ||
| message: str, | ||
| message: Any, | ||
| *, | ||
| logger_name: str | None = None, | ||
| extra: dict[str, Any] | None = None, |
There was a problem hiding this comment.
PR description says the extra parameter was removed from all logging methods (and marks this as a breaking change), but Context.log() and the convenience methods still accept extra. Either remove extra to match the stated API change or update the PR description/breaking-change note to reflect the actual behavior.
| async def structured_logging_tool(msg: str, ctx: Context) -> str: | ||
| # Test with dictionary | ||
| await ctx.info({"status": "success", "message": msg, "count": 42}) | ||
| # Test with list | ||
| await ctx.debug([1, 2, 3, "item"]) | ||
| # Test with number | ||
| await ctx.warning(404) | ||
| # Test with boolean | ||
| await ctx.error(True) |
There was a problem hiding this comment.
This test covers object/array/number/bool/string, but not JSON null (None). Since the PR description claims coverage of “all JSON types” and the spec allows any JSON-serializable type, add a None case and assert it is passed through to send_log_message unchanged.
| @pytest.mark.anyio | ||
| async def test_context_logging_with_structured_data(self): |
There was a problem hiding this comment.
This module is already marked with pytestmark = pytest.mark.anyio, so this decorator is redundant here. Consider removing it for consistency with the surrounding async tests in this file.
…ance with MCP spec
Fixes #397 - FastMCP Context logging methods now accept
Anytype instead of restricting tostr, aligning with MCP specification requirements.Motivation and Context
The MCP specification requires logging functions to accept any JSON-serializable data (
Anytype), but FastMCP'sContext.log()only accepted strings. This prevented users from logging structured data like dictionaries, lists, numbers, and booleans.How Has This Been Tested?
test_context_logging_with_structured_datacovering all JSON types (dict, list, number, boolean, string)Breaking Changes
Yes - Removed the
extraparameter from all logging methods:ctx.info("message", extra={"key": "value"})ctx.info({"message": "message", "key": "value"})Types of changes
Checklist
Additional context
Changed
Context.log()signature frommessage: str, extra: dicttodata: Any. Theextraparameter was not part of the MCP spec and created confusion. Updated all convenience methods (debug(),info(),warning(),error()) for consistency.