Skip to content

fix(mcp): honor TRACELOOP_TRACE_CONTENT on the client path #4466

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
IdoGol24 wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix(mcp): honor TRACELOOP_TRACE_CONTENT on the client path #4466

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c93b63a
fix(mcp): honor TRACELOOP_TRACE_CONTENT on the client path
IdoGol24 Sep 9, 2026
4ff0dd4
docs(mcp): docstring the functions this branch touches
IdoGol24 Sep 9, 2026
4bdbabd
fix(mcp): gate error text on TRACELOOP_TRACE_CONTENT too
IdoGol24 Sep 17, 2026
fd35087
fix(mcp): keep the stack frames, drop only the text that is content
IdoGol24 Sep 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 13 additions & 14 deletions ...elemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/fastmcp_instrumentation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,9 @@
from opentelemetry.trace import Tracer
from opentelemetry.trace.status import Status, StatusCode
from opentelemetry.semconv_ai import SpanAttributes, TraceloopSpanKindValues
from opentelemetry.semconv.attributes.error_attributes import ERROR_TYPE
from wrapt import register_post_import_hook, wrap_function_wrapper

from .utils import dont_throw
from .utils import dont_throw, record_error, should_send_prompts


class FastMCPInstrumentor:
Expand Down Expand Up @@ -82,15 +81,22 @@ async def traced_method(wrapped, instance, args, kwargs):
entity_name = tool_key if tool_key else "unknown_tool"

# Create parent server.mcp span
with self._tracer.start_as_current_span("mcp.server") as mcp_span:
# The error paths below record the failure themselves, with the
# message gated on content capture. OTel's own exception recording
# would re-add that text ungated on the way out.
with self._tracer.start_as_current_span(
"mcp.server", record_exception=False, set_status_on_exception=False
) as mcp_span:
mcp_span.set_attribute(SpanAttributes.TRACELOOP_SPAN_KIND, "server")
mcp_span.set_attribute(SpanAttributes.TRACELOOP_ENTITY_NAME, "mcp.server")
if self._server_name:
mcp_span.set_attribute(SpanAttributes.TRACELOOP_WORKFLOW_NAME, self._server_name)

# Create nested tool span
span_name = f"{entity_name}.tool"
with self._tracer.start_as_current_span(span_name) as tool_span:
with self._tracer.start_as_current_span(
span_name, record_exception=False, set_status_on_exception=False
) as tool_span:
tool_span.set_attribute(SpanAttributes.TRACELOOP_SPAN_KIND, TraceloopSpanKindValues.TOOL.value)
tool_span.set_attribute(SpanAttributes.TRACELOOP_ENTITY_NAME, entity_name)
if self._server_name:
Expand All @@ -111,13 +117,8 @@ async def traced_method(wrapped, instance, args, kwargs):
try:
result = await wrapped(*args, **kwargs)
except Exception as e:
tool_span.set_attribute(ERROR_TYPE, type(e).__name__)
tool_span.record_exception(e)
tool_span.set_status(Status(StatusCode.ERROR, str(e)))

mcp_span.set_attribute(ERROR_TYPE, type(e).__name__)
mcp_span.record_exception(e)
mcp_span.set_status(Status(StatusCode.ERROR, str(e)))
record_error(tool_span, e)
record_error(mcp_span, e)
raise

try:
Expand Down Expand Up @@ -155,9 +156,7 @@ async def traced_method(wrapped, instance, args, kwargs):

def _should_send_prompts(self):
"""Check if content tracing is enabled (matches traceloop SDK)"""
return (
os.getenv("TRACELOOP_TRACE_CONTENT") or "true"
).lower() == "true"
return should_send_prompts()

def _get_json_encoder(self):
"""Get JSON encoder class (simplified - traceloop SDK uses custom JSONEncoder)"""
Expand Down
129 changes: 86 additions & 43 deletions ...es/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py
View file
Open in desktop

@doronkopit5 doronkopit5 Sep 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(File-level since the line is outside the diff.) One more place the gate doesn't reach: span.record_exception(e) in _execute_and_handle_result writes exception.message and the full exception.stacktrace as event attributes. On the client path the McpError message comes from the server, so it is content. fastmcp_instrumentation.py around line 114 does the same.

With the switch off I still got exception.message='failure involving <marker>' and the marker inside the stacktrace, on both boom.tool and mcp.server.

Fine by me if exception text is deliberately out of scope, but then let's note it in the PR description — as written the claim is that content is withheld.

@IdoGol24 IdoGol24 Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It ran deeper than the two explicit record_exception calls. start_as_current_span defaults to record_exception=True, set_status_on_exception=True, so as the exception left each with block OTel re-recorded the message and stacktrace ungated and overwrote the gated status, which is also why every failure was carrying two identical exception events.

Both flags are now off at the five spans whose code reports failures itself.

Your follow-up on this thread is in too: dropping the event entirely also cost exception.type and exception.stacktrace, so record_error now keeps both and withholds only the message. Your exact case is what test_error_text_suppressed_when_content_capture_off asserts against, and it fails without the fix.

Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,13 @@
from opentelemetry.semconv.attributes.error_attributes import ERROR_TYPE

from opentelemetry.instrumentation.mcp.version import __version__
from opentelemetry.instrumentation.mcp.utils import dont_throw, Config
from opentelemetry.instrumentation.mcp.utils import (
Config,
dont_throw,
error_status,
record_error,
should_send_prompts,
)
from opentelemetry.instrumentation.mcp.fastmcp_instrumentation import (
FastMCPInstrumentor,
)
Expand Down Expand Up @@ -216,24 +222,29 @@ def _fastmcp_client_enter_wrapper(self, tracer):
@dont_throw
async def traced_method(wrapped, instance, args, kwargs):
# Start a root span for the MCP client session and make it current
span_context_manager = tracer.start_as_current_span("mcp.client.session")
span_context_manager = tracer.start_as_current_span(
"mcp.client.session",
record_exception=False,

@doronkopit5 doronkopit5 Sep 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasoning in the comment at 295-297 holds for the other spans, but not for this one: nothing reports a failure on mcp.client.session. The exit wrapper at 268-274 only forwards to context_manager.__exit__(type(e), e, e.__traceback__) and re-raises into @dont_throw, which swallows it and logs at DEBUG. With these two flags off that __exit__ is a no-op, so a teardown failure now leaves nothing behind.

Base vs. head, with a Client.__aexit__ that raises:

base: mcp.client.session status=ERROR desc='RuntimeError: teardown exploded' events=[exception]
head: mcp.client.session status=UNSET desc=None      events=[]

This happens with TRACELOOP_TRACE_CONTENT at its default, so it isn't a content-gating tradeoff. Stdio/SSE transports raising on teardown (Attempted to exit cancel scope in a different task, BrokenResourceError, ClosedResourceError) is the case you'd most want visible on the session span.

Either keep the defaults on this span, or give the exit wrapper's except the same three lines the enter wrapper got at 243-246. Nothing in the suite covers this path either way.

@IdoGol24 IdoGol24 Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed, I took the second option rather than restoring the defaults, so this span follows the same rule as the other five instead of being the one exception.

The exit wrapper couldn't call record_error as-is: it only had the context manager, not the span. The enter wrapper now also stores the span on the instance (_tracing_session_span), and the exit wrapper's except records on it before __exit__ ends it.

You were right that nothing covered this path. test_session_teardown_failure_is_recorded drives the enter and exit wrappers directly - Client.__aexit__ is already wrapped, so monkeypatching it would replace the wrapper under test. Parameterized off/on: status is ERROR either way, the message only when content capture is on. It fails against the previous head.

Side effect worth noting: error.type now appears on this span and on ResponseStreamWriter, neither of which carried it on main.

set_status_on_exception=False,
)
span = span_context_manager.__enter__()
span.set_attribute(SpanAttributes.TRACELOOP_SPAN_KIND, "session")
span.set_attribute(
SpanAttributes.TRACELOOP_ENTITY_NAME, "mcp.client.session"
)

# Store the span context manager on the instance to properly exit it later
# Store the span context manager on the instance to properly exit it
# later, and the span itself so the exit wrapper can report a
# teardown failure on it.
setattr(instance, "_tracing_session_context_manager", span_context_manager)
setattr(instance, "_tracing_session_span", span)

try:
# Call the original method
result = await wrapped(*args, **kwargs)
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)))
record_error(span, e)
raise

return traced_method
Expand All @@ -256,7 +267,12 @@ async def traced_method(wrapped, instance, args, kwargs):

return result
except Exception as e:
# End the session span context manager with exception info
# Record the teardown failure before __exit__ ends the span --
# the span's own exception recording is off, so nothing else
# would report it.
span = getattr(instance, "_tracing_session_span", None)
if span is not None:
record_error(span, e)
context_manager = getattr(
instance, "_tracing_session_context_manager", None
)
Expand All @@ -282,15 +298,25 @@ async def _handle_tool_call(self, tracer, method, params, args, kwargs, wrapped)
except Exception:
pass

with tracer.start_as_current_span(span_name) as span:
# _execute_and_handle_result records the failure itself, with the
# message gated on content capture; OTel's own exception recording
# would re-add that text ungated on the way out.
with tracer.start_as_current_span(
span_name, record_exception=False, set_status_on_exception=False
) as span:
# Set tool-specific attributes
span.set_attribute(
SpanAttributes.TRACELOOP_SPAN_KIND, TraceloopSpanKindValues.TOOL.value
)
span.set_attribute(SpanAttributes.TRACELOOP_ENTITY_NAME, entity_name)

# Add input
clean_input = self._extract_clean_input(method, params)
# Add input. Tool arguments are request content, so they are
# recorded only when content capture is enabled.
clean_input = (
self._extract_clean_input(method, params)
if should_send_prompts()
else None
)
if clean_input:
try:
span.set_attribute(
Expand All @@ -307,10 +333,15 @@ async def _handle_tool_call(self, tracer, method, params, args, kwargs, wrapped)

async def _handle_mcp_method(self, tracer, method, args, kwargs, wrapped):
"""Handle non-tool MCP methods with simple serialization"""
with tracer.start_as_current_span(f"{method}.mcp") as span:
span.set_attribute(
SpanAttributes.TRACELOOP_ENTITY_INPUT, f"{serialize(args[0])}"
)
with tracer.start_as_current_span(
f"{method}.mcp", record_exception=False, set_status_on_exception=False
) as span:
# The serialized request is content: it carries caller-supplied
# params, so it is recorded only when content capture is enabled.
if should_send_prompts():
span.set_attribute(
SpanAttributes.TRACELOOP_ENTITY_INPUT, f"{serialize(args[0])}"
)
return await self._execute_and_handle_result(
span, method, args, kwargs, wrapped, clean_output=False
)
Expand All @@ -321,8 +352,11 @@ async def _execute_and_handle_result(
"""Execute the wrapped function and handle the result"""
try:
result = await wrapped(*args, **kwargs)
# Add output
if clean_output:
# Add output. The response body is content, so it is recorded only
# when content capture is enabled.
if not should_send_prompts():

@doronkopit5 doronkopit5 Sep 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The gate stops here, but ~20 lines down we still do span.set_status(Status(StatusCode.ERROR, f"{result.content[0].text}")) for isError results, and that text is the tool's response body.

I tried it: with TRACELOOP_TRACE_CONTENT=false, a tool raising ToolError(f"failure involving {token}") gave a boom.tool span whose status description was failure involving <my marker> — the caller's own argument round-tripping back out. Status descriptions are exported like attributes, so every failed tool call still carries content with the switch off.

Either gate that one too, or set a fixed description ("tool error") when capture is off and keep the error code.

@IdoGol24 IdoGol24 Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Took the "gate it" option over a fixed "tool error" string:
error.type already names the failure class, so a constant description gives a consumer nothing extra to key on while looking like real text. StatusCode.ERROR is preserved.

Also worth flagging since it came out of this: the sweep in the test helper only walked span.attributes, so none of these leaks could have been caught. It now folds in status descriptions and event attributes, which is what turned your three reports into failing tests

pass
elif clean_output:
clean_output_data = self._extract_clean_output(method, result)
if clean_output_data:
try:
Expand All @@ -343,16 +377,12 @@ 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}")
)
span.set_status(error_status(f"{result.content[0].text}"))
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)))
record_error(span, e)
raise

def _extract_clean_input(self, method: str, params: Any) -> dict:
Expand Down Expand Up @@ -563,31 +593,44 @@ async def send(self, item: Any) -> Any:
else:
return await self.__wrapped__.send(item)

with self._tracer.start_as_current_span("ResponseStreamWriter") as span:
if hasattr(request, "result"):
span.set_attribute(
SpanAttributes.MCP_RESPONSE_VALUE, f"{serialize(request.result)}"
)
if "isError" in request.result:
if request.result["isError"] is True:
span.set_status(
Status(
StatusCode.ERROR,
f"{request.result['content'][0]['text']}",
)
# The wrapped send runs inside this span, so a transport failure would
# be recorded by OTel with its message and stacktrace ungated. The
# except below reports it instead, with the message gated.
with self._tracer.start_as_current_span(
"ResponseStreamWriter",
record_exception=False,
set_status_on_exception=False,
) as span:
try:
if hasattr(request, "result"):
# The response body is content, and so is the error text
# below -- it is the same payload.
if should_send_prompts():
span.set_attribute(
SpanAttributes.MCP_RESPONSE_VALUE,
f"{serialize(request.result)}",
)
if hasattr(request, "id"):
span.set_attribute(SpanAttributes.MCP_REQUEST_ID, f"{request.id}")
if "isError" in request.result:
if request.result["isError"] is True:
span.set_status(
error_status(f"{request.result['content'][0]['text']}")
)
if hasattr(request, "id"):
span.set_attribute(SpanAttributes.MCP_REQUEST_ID, f"{request.id}")

if not isinstance(request, JSONRPCRequest):
if not isinstance(request, JSONRPCRequest):
return await self.__wrapped__.send(item)
meta = None
if not request.params:
request.params = {}
meta = request.params.setdefault("_meta", {})

propagate.get_global_textmap().inject(meta)
return await self.__wrapped__.send(item)
meta = None
if not request.params:
request.params = {}
meta = request.params.setdefault("_meta", {})

propagate.get_global_textmap().inject(meta)
return await self.__wrapped__.send(item)
except Exception as e:
record_error(span, e)
raise


@dataclass(slots=True, frozen=True)
Expand Down
58 changes: 58 additions & 0 deletions packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,71 @@

import asyncio
import logging
import os
import traceback

from opentelemetry.semconv.attributes.error_attributes import ERROR_TYPE
from opentelemetry.trace import Status, StatusCode


class Config:
exception_logger = None


def should_send_prompts() -> bool:
"""Whether request/response content may be recorded on spans.

Mirrors the traceloop SDK's ``TRACELOOP_TRACE_CONTENT`` switch: content
capture is on unless an operator explicitly turns it off. Shared by the
FastMCP server wrapper and the MCP client path so a single environment
variable governs both, which is what the package README documents.
"""
return (os.getenv("TRACELOOP_TRACE_CONTENT") or "true").lower() == "true"


def error_status(description: str) -> Status:
"""An ERROR status, carrying `description` only when content capture is on.

The status code is not content, but its description is: on the client path
it is the server's own error text. Callers record the exception type
separately, so the failure stays visible either way.
"""
if should_send_prompts():
return Status(StatusCode.ERROR, description)
return Status(StatusCode.ERROR)


def record_error(span, exc) -> None:
"""Mark `span` failed, withholding only the parts of `exc` that are content.

The exception type and the stack frames are not content and are recorded
either way -- the stacktrace is the part you debug from. The message is,
and it reaches a traceback twice over: as the last line of a formatted
one, and inside the source line of the raise site's own frame. Hence the
frames are rendered by hand, without either.
"""
span.set_attribute(ERROR_TYPE, type(exc).__name__)
if should_send_prompts():
span.record_exception(exc)
else:
span.add_event(
"exception",
{
"exception.type": f"{type(exc).__module__}.{type(exc).__qualname__}",
# File, line and function, but not the frame's source line: a
# raise site like ToolError("...") reproduces its own message
# there, and that message is the thing being withheld.
"exception.stacktrace": "\n".join(
f' File "{frame.filename}", line {frame.lineno},'
f" in {frame.name}"
for frame in traceback.extract_tb(exc.__traceback__)
),
"exception.escaped": False,
},
)
span.set_status(error_status(str(exc)))


def dont_throw(func):
"""
A decorator that wraps the passed in function and logs exceptions instead of throwing them.
Expand Down
Loading
Loading