diff --git a/packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py b/packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py index 688cdb2c7f..23aa85eaac 100644 --- a/packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py +++ b/packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py @@ -178,25 +178,41 @@ def traced_method( return traced_method def patch_mcp_client(self, tracer: Tracer): - @dont_throw async def traced_method(wrapped, instance, args, kwargs): meta = None method = None params = None - if len(args) > 0 and hasattr(args[0].root, "method"): - method = args[0].root.method - if len(args) > 0 and hasattr(args[0].root, "params"): - params = args[0].root.params - if params: - if hasattr(args[0].root.params, "meta"): - meta = args[0].root.params.meta - - # Handle trace context propagation - if meta and len(args) > 0: - carrier = {} - TraceContextTextMapPropagator().inject(carrier) - meta.traceparent = carrier["traceparent"] - args[0].root.params.meta = meta + # Extract request metadata and propagate the trace context. A + # failure here is an instrumentation problem only: log it and let + # the request go through uninstrumented. BaseSession.send_request + # returns the RPC result, so swallowing an error and returning + # None (as @dont_throw did) breaks the traced call itself (#4463). + try: + if len(args) > 0 and hasattr(args[0].root, "method"): + method = args[0].root.method + if len(args) > 0 and hasattr(args[0].root, "params"): + params = args[0].root.params + if params: + if hasattr(args[0].root.params, "meta"): + meta = args[0].root.params.meta + + # Handle trace context propagation. Nothing is injected when + # the request happens outside any span (e.g. the first call + # after instrumenting) or the span context is invalid, so the + # traceparent key may be absent. + if meta and len(args) > 0: + carrier = {} + TraceContextTextMapPropagator().inject(carrier) + traceparent = carrier.get("traceparent") + if traceparent is not None: + meta.traceparent = traceparent + args[0].root.params.meta = meta + except Exception as e: + logging.warning( + "OpenLLMetry failed to extract MCP request metadata or " + "propagate trace context, error: %s", + e, + ) # Create different span types based on method if method == "tools/call": @@ -318,9 +334,23 @@ async def _handle_mcp_method(self, tracer, method, args, kwargs, wrapped): async def _execute_and_handle_result( self, span, method, args, kwargs, wrapped, clean_output=False ): - """Execute the wrapped function and handle the result""" + """Execute the wrapped function and handle the result. + + Errors raised by the wrapped call are recorded on the span and + re-raised to the caller. Errors raised by the instrumentation around + the call (attribute reads, serialization) must never change the + outcome of the traced call: they are logged and the real result is + returned instead of being replaced with None (#4463). + """ try: result = await wrapped(*args, **kwargs) + except Exception as e: + span.set_attribute(ERROR_TYPE, type(e).__name__) + span.record_exception(e) + span.set_status(Status(StatusCode.ERROR, str(e))) + raise + + try: # Add output if clean_output: clean_output_data = self._extract_clean_output(method, result) @@ -343,17 +373,24 @@ async def _execute_and_handle_result( if hasattr(result, "isError") and result.isError: span.set_attribute(ERROR_TYPE, "tool_error") if len(result.content) > 0: - span.set_status( - Status(StatusCode.ERROR, f"{result.content[0].text}") - ) + error_content = result.content[0] + # Content blocks can be text, image, audio or embedded + # resources; only TextContent exposes .text. + error_message = getattr(error_content, "text", None) + if error_message is None: + error_message = ( + f"{type(error_content).__name__} error response" + ) + span.set_status(Status(StatusCode.ERROR, error_message)) else: span.set_status(Status(StatusCode.OK)) - return result except Exception as e: - span.set_attribute(ERROR_TYPE, type(e).__name__) - span.record_exception(e) - span.set_status(Status(StatusCode.ERROR, str(e))) - raise + logging.warning( + "OpenLLMetry failed to record span output for %s, error: %s", + method, + e, + ) + return result def _extract_clean_input(self, method: str, params: Any) -> dict: """Extract clean input parameters for different MCP method types""" diff --git a/packages/opentelemetry-instrumentation-mcp/tests/test_send_request_safety.py b/packages/opentelemetry-instrumentation-mcp/tests/test_send_request_safety.py new file mode 100644 index 0000000000..8ac403f041 --- /dev/null +++ b/packages/opentelemetry-instrumentation-mcp/tests/test_send_request_safety.py @@ -0,0 +1,99 @@ +"""Tests for #4463: instrumentation failures must never change the outcome of +the traced MCP call. BaseSession.send_request returns the RPC result, so a +wrapper that swallows an instrumentation error and returns None replaces a +real result (e.g. CallToolResult) with None and crashes the caller.""" + +import pytest +from opentelemetry.trace.status import StatusCode + + +@pytest.mark.asyncio +async def test_tool_call_outside_active_span_returns_real_result( + span_exporter, tracer_provider +): + """ + #4463: when send_request runs outside any span (first call after + instrumenting, a detached task, a non-recording context), the + TraceContextTextMapPropagator writes no traceparent header. The old + unconditional `carrier["traceparent"]` read then raised KeyError, dont_throw + swallowed it and returned None, and ClientSession.call_tool crashed with + AttributeError: 'NoneType' object has no attribute 'isError'. + """ + from fastmcp import Client, FastMCP + from opentelemetry import context + + server = FastMCP("no-active-span") + + @server.tool() + async def ping() -> str: + return "pong" + + async with Client(server) as client: + # Client.__aenter__ makes the session span current. Push an empty + # context so the tool call happens outside any span, as reported in + # production for #4463. The meta dict makes the request carry an MCP + # Meta object, which is what forces the trace-context injection branch + # in patch_mcp_client to run. + token = context.attach(context.Context()) + try: + result = await client.call_tool("ping", {}, meta={"clientID": "x"}) + finally: + context.detach(token) + + assert result.content[0].text == "pong" + + # The call was still traced as a root span, but with no errors. + ping_spans = [ + s for s in span_exporter.get_finished_spans() if s.name == "ping.tool" + ] + assert ping_spans, "expected a ping.tool span for the traced call" + assert all( + s.status.status_code != StatusCode.ERROR for s in ping_spans + ), "tracing failure was recorded as a span error" + + +@pytest.mark.asyncio +async def test_error_result_with_non_text_content_block_is_returned( + span_exporter, tracer_provider +): + """ + #4463: post-call span decoration read result.content[0].text unguarded. + Error results can carry non-text content blocks (image, audio, embedded + resource), which have no .text attribute. The AttributeError was raised + after the RPC had already succeeded, swallowed by dont_throw, and the real + result was thrown away in favour of None. + """ + from mcp.types import CallToolResult, ImageContent + + from opentelemetry.instrumentation.mcp.instrumentation import McpInstrumentor + + instrumentor = McpInstrumentor() + tracer = tracer_provider.get_tracer("test-send-request-safety") + span = tracer.start_span("image-error.tool") + + image_error = CallToolResult( + content=[ + ImageContent( + type="image", data="iVBORw0KGgo=", mimeType="image/png" + ) + ], + isError=True, + ) + + async def fake_wrapped(*args, **kwargs): + return image_error + + result = await instrumentor._execute_and_handle_result( + span, "tools/call", [], {}, fake_wrapped, clean_output=True + ) + span.end() + + assert result is image_error, "the real RPC result must be returned" + + # The error is still recorded on the span (error.type + ERROR status). + error_spans = [ + s for s in span_exporter.get_finished_spans() if s.name == "image-error.tool" + ] + assert len(error_spans) == 1 + assert error_spans[0].status.status_code == StatusCode.ERROR + assert error_spans[0].attributes.get("error.type") == "tool_error"