fix(mcp): add mcp.server parent span wrapper for FastMCP tool calls #3382
Conversation
- Wrap FastMCP tool calls with mcp.server parent span - Tool execution spans are now nested under mcp.server spans - Add server span with traceloop.span.kind="server" attribute - Maintain existing tool span attributes and functionality - Add comprehensive test for parent-child span relationship - Fix linting issues in test files 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a parent Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant FastMCP as FastMCP Server
participant Instr as Instrumentation
participant Tool as Tool
Client->>FastMCP: invoke tool(name,args)
activate FastMCP
FastMCP->>Instr: handle request
activate Instr
note over Instr: start parent span "mcp.server"\n(kind=server, entity=mcp.server)
Instr->>Instr: start nested span "entity_name.tool"\n(kind=tool, entity=tool)
note right of Instr: if prompts logging enabled:\nrecord input on tool span
Instr->>Tool: execute(args)
activate Tool
Tool-->>Instr: result / exception
deactivate Tool
alt success
Instr->>Instr: serialize output\nrecord on tool span\nset MCP_RESPONSE_VALUE on parent
Instr->>Instr: set status OK on tool and parent
else error
Instr->>Instr: record exception on tool and parent\nset status ERROR on both
end
Instr-->>FastMCP: return result / raise
deactivate Instr
FastMCP-->>Client: deliver result / error
deactivate FastMCP
sequenceDiagram
autonumber
participant C as Client (context manager)
participant I as Instrumentation
C->>I: __aenter__()
activate I
note over I: start session span "mcp.client.session"\n(kind=session)
I-->>C: return client
deactivate I
C->>I: __aexit__()
activate I
note over I: call wrapped __aexit__, then\nend stored session context manager
I-->>C: exit completed
deactivate I
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (1)packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (2)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 847d784 in 1 minute and 28 seconds. Click for details.
- Reviewed
207lines of code in3files - Skipped
1files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/opentelemetry-instrumentation-mcp/tests/test_fastmcp_server_span.py:23
- Draft comment:
Remove or replace debug print statements before finalizing the tests to keep test output clean. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While it's generally good practice to keep test output clean, these print statements serve a valuable debugging purpose for a complex test involving span relationships. They are clearly marked as debug statements and help developers understand span relationships when tests fail. The prints don't interfere with test assertions or functionality. The comment has merit since clean test output is desirable. However, in complex tests involving distributed tracing, debug output can be very valuable for troubleshooting. The debug prints provide significant value for understanding and debugging span relationships, which outweighs the minor downside of verbose test output. The comment should be deleted as these debug prints serve a valuable purpose in this specific test case and don't require immediate action.
2. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/fastmcp_instrumentation.py:125
- Draft comment:
Review the default logic in _should_send_prompts(). The use of (os.getenv(...) or 'true') defaults to true when unset—confirm this is the intended behavior. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_wYGGCJYXDr4UYNze
You can customize 
...entelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/fastmcp_instrumentation.py
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/fastmcp_instrumentation.py (5)
61-71: Parent mcp.server + child tool span: solid; add MCP method attribute for clarity.Set MCP method for easier querying (MCP spec commonly uses "tools/call").
Apply:
with self._tracer.start_as_current_span("mcp.server") as mcp_span: mcp_span.set_attribute(SpanAttributes.TRACELOOP_SPAN_KIND, "server") mcp_span.set_attribute(SpanAttributes.TRACELOOP_ENTITY_NAME, "mcp.server") + mcp_span.set_attribute(SpanAttributes.MCP_METHOD_NAME, "tools/call")
72-83: Redact sensitive inputs before logging.Arguments may contain secrets/PII. Redact common keys before serialization.
Apply:
- input_data = { - "tool_name": entity_name, - "arguments": tool_arguments - } + input_data = { + "tool_name": entity_name, + "arguments": self._redact_sensitive(tool_arguments), + }Add helper (outside this hunk):
def _redact_sensitive(self, obj): SENSITIVE_KEYS = {"api_key", "apikey", "authorization", "auth", "password", "token", "secret", "access_token", "refresh_token"} try: if isinstance(obj, dict): return {k: ("***" if k.lower() in SENSITIVE_KEYS else self._redact_sensitive(v)) for k, v in obj.items()} if isinstance(obj, list): return [self._redact_sensitive(v) for v in obj] return obj except Exception: return obj
84-112: Narrow try/except; use try/except/else to satisfy Ruff TRY300 and improve clarity.Keeps exception path isolated and avoids returning inside try.
Apply:
- try: - result = await wrapped(*args, **kwargs) - - # Add output in traceloop format to tool span - if self._should_send_prompts() and result: - try: - # Convert FastMCP Content objects to serializable format - output_data = [] - for item in result: - if hasattr(item, 'text'): - output_data.append({"type": "text", "content": item.text}) - elif hasattr(item, '__dict__'): - output_data.append(item.__dict__) - else: - output_data.append(str(item)) - - json_output = json.dumps(output_data, cls=self._get_json_encoder()) - truncated_output = self._truncate_json_if_needed(json_output) - tool_span.set_attribute(SpanAttributes.TRACELOOP_ENTITY_OUTPUT, truncated_output) - - # Also add response to MCP span - mcp_span.set_attribute(SpanAttributes.MCP_RESPONSE_VALUE, truncated_output) - except (TypeError, ValueError): - pass # Skip output logging if serialization fails - - tool_span.set_status(Status(StatusCode.OK)) - mcp_span.set_status(Status(StatusCode.OK)) - return result - - except Exception as e: + 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))) raise + else: + # Add output in traceloop format to tool span + if self._should_send_prompts() and result: + try: + # Convert FastMCP Content objects to serializable format + output_data = [] + for item in result: + if hasattr(item, 'text'): + output_data.append({"type": "text", "content": item.text}) + elif hasattr(item, '__dict__'): + output_data.append(item.__dict__) + else: + output_data.append(str(item)) + + json_output = json.dumps(output_data, cls=self._get_json_encoder()) + truncated_output = self._truncate_json_if_needed(json_output) + tool_span.set_attribute(SpanAttributes.TRACELOOP_ENTITY_OUTPUT, truncated_output) + + # Also add response to MCP span + mcp_span.set_attribute(SpanAttributes.MCP_RESPONSE_VALUE, truncated_output) + except (TypeError, ValueError): + pass # Skip output logging if serialization fails + + tool_span.set_status(Status(StatusCode.OK)) + mcp_span.set_status(Status(StatusCode.OK)) + return result
90-107: Be defensive about non-sequence results.If a tool returns a scalar or dict, iterating it yields keys. Guard before iterating.
Apply:
- output_data = [] - for item in result: + seq = result if isinstance(result, (list, tuple)) else [result] + output_data = [] + for item in seq:
125-130: Broaden env parsing for TRACELOOP_TRACE_CONTENT.Accept common truthy/falsey values.
Apply:
- return ( - os.getenv("TRACELOOP_TRACE_CONTENT") or "true" - ).lower() == "true" + val = os.getenv("TRACELOOP_TRACE_CONTENT", "true").strip().lower() + return val in {"1", "true", "yes", "y", "on"}packages/opentelemetry-instrumentation-mcp/tests/test_fastmcp_server_span.py (3)
1-1: Silence unused fixture param warning.Keep the fixture for setup but mark as intentionally unused.
Apply:
-async def test_fastmcp_server_mcp_parent_span(span_exporter, tracer_provider) -> None: +async def test_fastmcp_server_mcp_parent_span(span_exporter, tracer_provider) -> None: # noqa: ARG001
23-36: Remove noisy debug prints from tests.These clutter CI output. Prefer logging only on failure.
Apply:
- # Debug: Print span details with parent info - print(f"\nTotal spans: {len(spans)}") - for i, span in enumerate(spans): - parent_id = span.parent.span_id if span.parent else "None" - print(f"Span {i}: name='{span.name}', span_id={span.get_span_context().span_id}, " - f"parent_id={parent_id}, trace_id={span.get_span_context().trace_id}") - - # Look specifically for mcp.server and tool spans + # Look specifically for mcp.server and tool spans
31-33: Tighten tool span selection to reduce false positives.Also filter by entity name.
Apply:
- tool_spans = [span for span in spans if span.name.endswith('.tool')] + tool_spans = [ + span for span in spans + if span.name.endswith('.tool') and span.attributes.get('traceloop.entity.name') == 'test_tool' + ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
packages/opentelemetry-instrumentation-mcp/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/fastmcp_instrumentation.py(1 hunks)packages/opentelemetry-instrumentation-mcp/pyproject.toml(1 hunks)packages/opentelemetry-instrumentation-mcp/tests/test_fastmcp_server_span.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Store API keys only in environment variables/secure vaults; never hardcode secrets in code
Use Flake8 for code linting and adhere to its rules
Files:
packages/opentelemetry-instrumentation-mcp/tests/test_fastmcp_server_span.pypackages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/fastmcp_instrumentation.py
🧬 Code graph analysis (2)
packages/opentelemetry-instrumentation-mcp/tests/test_fastmcp_server_span.py (2)
packages/traceloop-sdk/traceloop/sdk/decorators/__init__.py (1)
tool(63-73)packages/traceloop-sdk/traceloop/sdk/utils/in_memory_span_exporter.py (1)
get_finished_spans(40-43)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/fastmcp_instrumentation.py (1)
packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py (2)
SpanAttributes(64-261)TraceloopSpanKindValues(301-306)
🪛 Ruff (0.12.2)
packages/opentelemetry-instrumentation-mcp/tests/test_fastmcp_server_span.py
1-1: Unused function argument: tracer_provider
(ARG001)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/fastmcp_instrumentation.py
111-111: Consider moving this statement to an else block
(TRY300)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test Packages (3.10)
- GitHub Check: Test Packages (3.12)
- GitHub Check: Test Packages (3.11)
- GitHub Check: Build Packages (3.11)
- GitHub Check: Lint
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
packages/opentelemetry-instrumentation-mcp/pyproject.toml (1)
42-42: Approve: pytest-asyncio ^1.2.0 is compatible with pytest ^8.2.x and Python 3.10–3.13. pyproject pins pytest ^8.2.2 and pytest-asyncio ^1.2.0; pytest-asyncio 1.2.0 (released Sep 12, 2025) lists Python 3.10–3.13 support and the project changelog documents pytest‑8 compatibility. (pypi.org)
Instrument FastMCP Client context manager to create a session-level span that ensures all MCP operations (initialize, list_tools, call_tool, list_resources, read_resource) within a client session belong to the same trace. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 3126550 in 1 minute and 55 seconds. Click for details.
- Reviewed
78lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:203
- Draft comment:
Using a synchronous context manager (via enter/exit) in an async wrapper may be non-idiomatic. Ensure that tracer.start_as_current_span is safe in async contexts or consider an async-compatible alternative. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% This is a tricky case. The comment raises a valid concern about mixing sync and async context managers. However, OpenTelemetry's tracer.start_as_current_span() is designed to work this way - it returns a synchronous context manager by design. The code is following the standard OpenTelemetry pattern. Making it async would actually be wrong since the underlying API is synchronous. I could be wrong about OpenTelemetry's design intentions. Maybe there is an async-specific tracing API I'm not aware of. Looking at OpenTelemetry's documentation and common usage patterns, this is definitely the standard way to use the tracing API, even in async code. The sync/async mixing is intentional in the OpenTelemetry design. Delete this comment. While it raises an interesting point about sync/async mixing, the code is correctly following OpenTelemetry's intended usage patterns.
Workflow ID: wflow_A15c6yGJjGB2jNGe
You can customize
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (1)
41-54: Optional: also wrap if module already imported.If
fastmcp.clientis imported before_instrument()runs, the post-import hook won’t fire. Consider best-effort immediate wrapping when already present insys.modules(keep the post-import hook).I can provide a minimal guarded snippet using
sys.modulesif you want it added.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Store API keys only in environment variables/secure vaults; never hardcode secrets in code
Use Flake8 for code linting and adhere to its rules
Files:
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py
🧬 Code graph analysis (1)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (1)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/utils.py (1)
dont_throw(12-40)
🪛 Ruff (0.12.2)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py
209-209: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
214-214: Consider moving this statement to an else block
(TRY300)
222-222: Unused method argument: tracer
(ARG002)
235-235: Consider moving this statement to an else block
(TRY300)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test Packages (3.10)
- GitHub Check: Test Packages (3.12)
- GitHub Check: Build Packages (3.11)
- GitHub Check: Test Packages (3.11)
- GitHub Check: Lint
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py
Show resolved
Hide resolved
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py
Show resolved
Hide resolved
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py
Show resolved
Hide resolved
Add exception_logger parameter to McpInstrumentor constructor to match the pattern used by other instrumentation packages. This allows users to provide a custom exception logger callback that will be called when exceptions are caught by the dont_throw decorator. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 3f0c4eb in 1 minute and 25 seconds. Click for details.
- Reviewed
23lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:27
- Draft comment:
Consider adding a type annotation for the 'exception_logger' parameter. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:29
- Draft comment:
Assigning 'exception_logger' to a global Config variable may introduce side effects; ensure this global state usage is thread-safe and well-documented. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a legitimate concern about thread-safety when modifying global state. However, the comment is somewhat speculative ("may introduce side effects") and asks for documentation rather than suggesting a specific fix. The comment also asks to "ensure" something rather than making a concrete suggestion. Per the rules, we should not keep comments that ask the author to verify or ensure things. The underlying thread-safety concern is valid and important. Perhaps I'm being too strict about the wording of the comment. While the concern is valid, the rules are clear that we should not keep comments that ask authors to "ensure" things or that are speculative. A better comment would propose a specific solution like using thread-local storage or a lock. Delete this comment since it asks the author to "ensure" something rather than making a specific suggestion, and is somewhat speculative about potential issues.
Workflow ID: wflow_O0z1jnOEg2K3XqEV
You can customize
…traceloop/openllmetry into feature/mcp-server-span-wrapper
|
Hi @nirga , 2nd issue - CC: @gyliu513 |
nirga
changed the title
Summary
Changes
mcp.serverspan withtraceloop.span.kind="server"{tool_name}.toolspan with existing tool attributesTest Plan
test_fastmcp_server_mcp_parent_spanverifies proper nesting🤖 Generated with Claude Code
Important
Adds
mcp.serverparent span wrapper for FastMCP tool calls, ensuring proper span nesting and session-level tracing.mcp.serverparent span wrapper for FastMCP tool calls infastmcp_instrumentation.py.mcp.serverspans, maintaining existing attributes.FastMCPInstrumentorto create nested span structure withmcp.serveras parent and{tool_name}.toolas child.instrumentation.py.test_fastmcp_server_mcp_parent_spanintest_fastmcp_server_span.pyto verify span nesting.pytest-asynciotopyproject.tomlfor async test support.This description was created by
for 3f0c4eb. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Tests
Chores