diff --git a/src/fastmcp/client/mixins/tools.py b/src/fastmcp/client/mixins/tools.py index 1c21f3138..e92e0729c 100644 --- a/src/fastmcp/client/mixins/tools.py +++ b/src/fastmcp/client/mixins/tools.py @@ -7,6 +7,7 @@ import weakref from typing import TYPE_CHECKING, Any, Literal, overload import mcp.types +from opentelemetry.trace import Status, StatusCode from pydantic import RootModel if TYPE_CHECKING: @@ -151,7 +152,7 @@ class ClientToolsMixin: name, session_id=self.transport.get_session_id(), tool_name=name, - ): + ) as span: logger.debug(f"[{self.name}] called call_tool: {name}") # Inject trace context into meta for propagation to server @@ -166,6 +167,18 @@ class ClientToolsMixin: meta=propagated_meta if propagated_meta else None, ) ) + + # Reflect tool-level errors on the span so callers see ERROR + # status even though the MCP protocol call itself succeeded. + if result.isError: + span.set_attribute("error.type", "tool_error") + description = "" + if result.content and isinstance( + result.content[0], mcp.types.TextContent + ): + description = result.content[0].text + span.set_status(Status(StatusCode.ERROR, description)) + return result async def _parse_call_tool_result( @@ -284,23 +297,16 @@ class ClientToolsMixin: name, arguments, task_id, ttl, meta=request_meta or None ) - with client_span( - f"tools/call {name}", - "tools/call", - name, - session_id=self.transport.get_session_id(), - tool_name=name, - ): - result = await self.call_tool_mcp( - name=name, - arguments=arguments or {}, - timeout=timeout, - progress_handler=progress_handler, - meta=request_meta or None, - ) - return await self._parse_call_tool_result( - name, result, raise_on_error=raise_on_error - ) + result = await self.call_tool_mcp( + name=name, + arguments=arguments or {}, + timeout=timeout, + progress_handler=progress_handler, + meta=request_meta or None, + ) + return await self._parse_call_tool_result( + name, result, raise_on_error=raise_on_error + ) async def _call_tool_as_task( self: Client, diff --git a/src/fastmcp/client/telemetry.py b/src/fastmcp/client/telemetry.py index b2f4cf781..bcbcf7aee 100644 --- a/src/fastmcp/client/telemetry.py +++ b/src/fastmcp/client/telemetry.py @@ -42,7 +42,12 @@ def client_span( try: yield span except Exception as e: - span.set_attribute("error.type", type(e).__name__) + span.set_attribute( + "error.type", + "tool_error" + if type(e).__name__ == "ToolError" + else type(e).__name__, + ) span.record_exception(e) span.set_status(Status(StatusCode.ERROR, str(e))) raise diff --git a/src/fastmcp/server/server.py b/src/fastmcp/server/server.py index 9101dcf26..2d095eb76 100644 --- a/src/fastmcp/server/server.py +++ b/src/fastmcp/server/server.py @@ -606,10 +606,7 @@ class FastMCP( and middleware execution. Returns all versions (no deduplication). Protocol handlers deduplicate for MCP wire format. """ - with server_span( - "tools/list", "tools/list", self.name, "tool", "" - ): - async with fastmcp.server.context.Context(fastmcp=self) as ctx: + async with fastmcp.server.context.Context(fastmcp=self) as ctx: if run_middleware: mw_context = MiddlewareContext( message=mcp.types.ListToolsRequest(method="tools/list"), @@ -623,31 +620,35 @@ class FastMCP( call_next=lambda context: self.list_tools(run_middleware=False), ) - # Get all tools, apply session transforms, then filter enabled - # and model-visible (app-only tools are hidden from the model). - tools = list(await super().list_tools()) - tools = await apply_session_transforms(tools) - tools = [t for t in tools if is_enabled(t) and _is_model_visible(t)] + # Core logic: list tools + with server_span( + "tools/list", "tools/list", self.name, "tool", "" + ): + # Get all tools, apply session transforms, then filter enabled + # and model-visible (app-only tools are hidden from the model). + tools = list(await super().list_tools()) + tools = await apply_session_transforms(tools) + tools = [t for t in tools if is_enabled(t) and _is_model_visible(t)] - # Rewrite per-tool Prefab renderer URIs based on the tool's - # mount-point address. The walk pairs each tool with the - # provider that yielded it, computes the hashed URI, and - # produces a model_copy with the URI in place. Original - # Tool objects are not mutated. - tools = self._rewrite_prefab_uris(tools) + # Rewrite per-tool Prefab renderer URIs based on the tool's + # mount-point address. The walk pairs each tool with the + # provider that yielded it, computes the hashed URI, and + # produces a model_copy with the URI in place. Original + # Tool objects are not mutated. + tools = self._rewrite_prefab_uris(tools) - skip_auth, token = _get_auth_context() - authorized: list[Tool] = [] - for tool in tools: - if not skip_auth and tool.auth is not None: - ctx = AuthContext(token=token, component=tool) - try: - if not await run_auth_checks(tool.auth, ctx): + skip_auth, token = _get_auth_context() + authorized: list[Tool] = [] + for tool in tools: + if not skip_auth and tool.auth is not None: + ctx = AuthContext(token=token, component=tool) + try: + if not await run_auth_checks(tool.auth, ctx): + continue + except AuthorizationError: continue - except AuthorizationError: - continue - authorized.append(tool) - return authorized + authorized.append(tool) + return authorized async def _get_tool( self, name: str, version: VersionSpec | None = None @@ -743,10 +744,7 @@ class FastMCP( and middleware execution. Returns all versions (no deduplication). Protocol handlers deduplicate for MCP wire format. """ - with server_span( - "resources/list", "resources/list", self.name, "resource", "" - ): - async with fastmcp.server.context.Context(fastmcp=self) as ctx: + async with fastmcp.server.context.Context(fastmcp=self) as ctx: if run_middleware: mw_context = MiddlewareContext( message={}, @@ -760,32 +758,36 @@ class FastMCP( call_next=lambda context: self.list_resources(run_middleware=False), ) - # Get all resources, apply session transforms, then filter enabled - resources = list(await super().list_resources()) - resources = await apply_session_transforms(resources) - resources = [r for r in resources if is_enabled(r)] + # Core logic: list resources + with server_span( + "resources/list", "resources/list", self.name, "resource", "" + ): + # Get all resources, apply session transforms, then filter enabled + resources = list(await super().list_resources()) + resources = await apply_session_transforms(resources) + resources = [r for r in resources if is_enabled(r)] - # Append synthetic Prefab renderer resources — one per - # prefab tool, hashed by mount address. These don't live on - # any provider's storage; they're computed on demand. - from fastmcp.server.providers.prefab_synthesis import ( - synthesize_prefab_resources, - ) + # Append synthetic Prefab renderer resources — one per + # prefab tool, hashed by mount address. These don't live on + # any provider's storage; they're computed on demand. + from fastmcp.server.providers.prefab_synthesis import ( + synthesize_prefab_resources, + ) - resources.extend(await synthesize_prefab_resources(self)) + resources.extend(await synthesize_prefab_resources(self)) - skip_auth, token = _get_auth_context() - authorized: list[Resource] = [] - for resource in resources: - if not skip_auth and resource.auth is not None: - ctx = AuthContext(token=token, component=resource) - try: - if not await run_auth_checks(resource.auth, ctx): + skip_auth, token = _get_auth_context() + authorized: list[Resource] = [] + for resource in resources: + if not skip_auth and resource.auth is not None: + ctx = AuthContext(token=token, component=resource) + try: + if not await run_auth_checks(resource.auth, ctx): + continue + except AuthorizationError: continue - except AuthorizationError: - continue - authorized.append(resource) - return authorized + authorized.append(resource) + return authorized async def _get_resource( self, uri: str, version: VersionSpec | None = None @@ -877,14 +879,7 @@ class FastMCP( auth filtering, and middleware execution. Returns all versions (no deduplication). Protocol handlers deduplicate for MCP wire format. """ - with server_span( - "resources/templates/list", - "resources/templates/list", - self.name, - "resource_template", - "", - ): - async with fastmcp.server.context.Context(fastmcp=self) as ctx: + async with fastmcp.server.context.Context(fastmcp=self) as ctx: if run_middleware: mw_context = MiddlewareContext( message={}, @@ -900,23 +895,31 @@ class FastMCP( ), ) - # Get all templates, apply session transforms, then filter enabled - templates = list(await super().list_resource_templates()) - templates = await apply_session_transforms(templates) - templates = [t for t in templates if is_enabled(t)] + # Core logic: list resource templates + with server_span( + "resources/templates/list", + "resources/templates/list", + self.name, + "resource_template", + "", + ): + # Get all templates, apply session transforms, then filter enabled + templates = list(await super().list_resource_templates()) + templates = await apply_session_transforms(templates) + templates = [t for t in templates if is_enabled(t)] - skip_auth, token = _get_auth_context() - authorized: list[ResourceTemplate] = [] - for template in templates: - if not skip_auth and template.auth is not None: - ctx = AuthContext(token=token, component=template) - try: - if not await run_auth_checks(template.auth, ctx): + skip_auth, token = _get_auth_context() + authorized: list[ResourceTemplate] = [] + for template in templates: + if not skip_auth and template.auth is not None: + ctx = AuthContext(token=token, component=template) + try: + if not await run_auth_checks(template.auth, ctx): + continue + except AuthorizationError: continue - except AuthorizationError: - continue - authorized.append(template) - return authorized + authorized.append(template) + return authorized async def _get_resource_template( self, uri: str, version: VersionSpec | None = None @@ -1010,10 +1013,7 @@ class FastMCP( and middleware execution. Returns all versions (no deduplication). Protocol handlers deduplicate for MCP wire format. """ - with server_span( - "prompts/list", "prompts/list", self.name, "prompt", "" - ): - async with fastmcp.server.context.Context(fastmcp=self) as ctx: + async with fastmcp.server.context.Context(fastmcp=self) as ctx: if run_middleware: mw_context = MiddlewareContext( message={}, @@ -1027,23 +1027,27 @@ class FastMCP( call_next=lambda context: self.list_prompts(run_middleware=False), ) - # Get all prompts, apply session transforms, then filter enabled - prompts = list(await super().list_prompts()) - prompts = await apply_session_transforms(prompts) - prompts = [p for p in prompts if is_enabled(p)] + # Core logic: list prompts + with server_span( + "prompts/list", "prompts/list", self.name, "prompt", "" + ): + # Get all prompts, apply session transforms, then filter enabled + prompts = list(await super().list_prompts()) + prompts = await apply_session_transforms(prompts) + prompts = [p for p in prompts if is_enabled(p)] - skip_auth, token = _get_auth_context() - authorized: list[Prompt] = [] - for prompt in prompts: - if not skip_auth and prompt.auth is not None: - ctx = AuthContext(token=token, component=prompt) - try: - if not await run_auth_checks(prompt.auth, ctx): + skip_auth, token = _get_auth_context() + authorized: list[Prompt] = [] + for prompt in prompts: + if not skip_auth and prompt.auth is not None: + ctx = AuthContext(token=token, component=prompt) + try: + if not await run_auth_checks(prompt.auth, ctx): + continue + except AuthorizationError: continue - except AuthorizationError: - continue - authorized.append(prompt) - return authorized + authorized.append(prompt) + return authorized async def _get_prompt( self, name: str, version: VersionSpec | None = None diff --git a/src/fastmcp/server/telemetry.py b/src/fastmcp/server/telemetry.py index 218b308f0..fa687553c 100644 --- a/src/fastmcp/server/telemetry.py +++ b/src/fastmcp/server/telemetry.py @@ -93,7 +93,12 @@ def server_span( try: yield span except Exception as e: - span.set_attribute("error.type", type(e).__name__) + span.set_attribute( + "error.type", + "tool_error" + if type(e).__name__ == "ToolError" + else type(e).__name__, + ) span.record_exception(e) span.set_status(Status(StatusCode.ERROR, str(e))) raise @@ -123,8 +128,14 @@ def delegate_span( try: yield span except Exception as e: + span.set_attribute( + "error.type", + "tool_error" + if type(e).__name__ == "ToolError" + else type(e).__name__, + ) span.record_exception(e) - span.set_status(Status(StatusCode.ERROR)) + span.set_status(Status(StatusCode.ERROR, str(e))) raise diff --git a/tests/client/telemetry/test_client_tracing.py b/tests/client/telemetry/test_client_tracing.py index fcadab636..8f5ae5316 100644 --- a/tests/client/telemetry/test_client_tracing.py +++ b/tests/client/telemetry/test_client_tracing.py @@ -74,7 +74,7 @@ class TestClientToolTracing: async def test_call_tool_error_caught_by_client_span( self, trace_exporter: InMemorySpanExporter ): - """ToolError from _parse_call_tool_result should be caught by the client span.""" + """Tool error should be reflected on the client span via isError check.""" server = FastMCP("test-server") @server.tool() @@ -88,7 +88,7 @@ class TestClientToolTracing: spans = trace_exporter.get_finished_spans() - # Find the outer client span (from call_tool wrapping _parse_call_tool_result) + # Find the client span (from call_tool_mcp) client_spans = [ s for s in spans @@ -97,17 +97,15 @@ class TestClientToolTracing: and "fastmcp.server.name" not in s.attributes ] - # At least one client span should have ERROR status (the one catching ToolError) - error_client_spans = [ - s for s in client_spans if s.status.status_code == StatusCode.ERROR - ] - assert len(error_client_spans) >= 1, ( - "At least one client span should capture the ToolError" + # Exactly one client span should exist (no duplicate from call_tool) + assert len(client_spans) == 1, ( + "There should be exactly one client span for call_tool" ) - error_span = error_client_spans[0] + error_span = client_spans[0] + assert error_span.status.status_code == StatusCode.ERROR assert error_span.attributes is not None - assert error_span.attributes["error.type"] == "ToolError" + assert error_span.attributes["error.type"] == "tool_error" class TestClientResourceTracing: diff --git a/tests/server/telemetry/test_server_tracing.py b/tests/server/telemetry/test_server_tracing.py index 3cb2c33f8..a6e405bd1 100644 --- a/tests/server/telemetry/test_server_tracing.py +++ b/tests/server/telemetry/test_server_tracing.py @@ -64,7 +64,7 @@ class TestToolTracing: assert span.status.status_code == StatusCode.ERROR assert "Something went wrong" in span.status.description assert span.attributes is not None - assert span.attributes["error.type"] == "ToolError" + assert span.attributes["error.type"] == "tool_error" assert len(span.events) > 0 # Exception recorded async def test_call_nonexistent_tool_sets_error( @@ -82,6 +82,7 @@ class TestToolTracing: assert span.name == "tools/call nonexistent" assert span.status.status_code == StatusCode.ERROR assert span.attributes is not None + # NotFoundError is not a ToolError, so uses class name as fallback assert span.attributes["error.type"] == "NotFoundError"