fix(mcp): never let instrumentation errors break the traced MCP call

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.debug(
+                    "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.debug(
+                "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"""

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"