Emit one SERVER span per request and adopt spec-correct error codes - #4445
Conversation
marvin-context-protocol
Bot
added
breaking change
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a131ace697
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.middleware = [ | ||
| mw for mw in self.middleware if not isinstance(mw, OpenTelemetryMiddleware) | ||
| ] |
There was a problem hiding this comment.
Dropping every SDK OpenTelemetryMiddleware removes tracing for request methods that FastMCP does not wrap in its own server_span. The FastMCP spans are only created inside the high-level tool/resource/prompt paths in server.py; registered handlers such as initialize, tasks/get|result|list|cancel in server/mixins/lifespan.py, and the proxy ping handler only had the SDK middleware's generic SERVER span. In deployments with an OTel exporter, those requests now emit zero SERVER spans rather than the intended single span, so this filter needs either replacement coverage for all handlers or a narrower way to suppress only duplicate spans.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f33fd0a69e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ( | ||
| fastmcp is None | ||
| or ctx.request_id is None | ||
| or ctx.method in _HIGH_LEVEL_SPANNED_METHODS |
There was a problem hiding this comment.
Skipping every method in _HIGH_LEVEL_SPANNED_METHODS here means the only remaining SERVER span is opened later inside call_tool/list_*/read_resource after the FastMCP middleware chain reaches call_next. Built-in middleware can legally return or raise before that point — for example CachingMiddleware.on_call_tool returns cached results at fastmcp_slim/fastmcp/server/middleware/caching.py:438-439, and auth/rate-limit middleware can reject before call_next — so those high-level requests now export no SERVER span after the SDK middleware is removed. The seam needs to cover the outer request and suppress duplicates only when the high-level span actually opens.
Useful? React with 👍 / 👎.
jlowin
added
the
bug
1 participant
Two correctness fixes surfaced by the post-migration modernization audit.
One SERVER span per request. SDK v2 seeds its own
OpenTelemetryMiddlewareinto every low-level server, and FastMCP emits its own richer SERVER span — so anyone with an OTel exporter configured got two SERVER spans per request (plus extras for initialize) with different attribute conventions.LowLevelServernow removes the SDK's seeded middleware (matched by type, other middleware preserved) and keeps FastMCP's spans. Distributed trace propagation is unaffected — FastMCP extractstraceparentfrom_metaitself, and the new regression tests assert exactly one SERVER span that shares the client's trace id, going through the real dispatcher (the pre-existing telemetry tests calledmcp.call_toolin-process, which is why this was never caught). The SDK's client-sideMCP send …span was checked too: that one is a legitimate parent/child pair with ours, not a duplicate, and is left alone.Spec-correct wire error codes. FastMCP returned
-32002for resource-not-found where SEP-2164 (and the SDK's own mapping) specify-32602INVALID_PARAMS. The hand-rolled per-adapter error literals are replaced by a centralto_mcp_error()translator infastmcp.exceptionsbuilt on the SDK's code constants; resource- and prompt-not-found now return-32602over the wire with messages unchanged. This is a deliberate wire-visible correction for 4.0, recorded in the v4 change register. The opt-inErrorHandlingMiddlewarekeeps its separately-documented per-method code mapping.Labels: bugs, breaking change.