fix(mcp): better instrumentation for FastMCP #3372
Conversation
|
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 FastMCP-specific instrumentation and a shared dont_throw utility, refactors MCP client patching to create Traceloop-style tool and MCP-method spans with sanitized I/O and traceparent propagation, generalizes streaming instrumentation for multiple item shapes, and adds FastMCP-focused tests and sample app/server scripts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant MCPClient
participant Patch as PatchedSend
participant Tracer
participant Server
Client->>MCPClient: request(method, params, meta?)
MCPClient->>Patch: patched send
Patch->>Tracer: startSpan (name from method/tool, traceloop.kind)
Patch->>Patch: extract_clean_input (serialize/truncate)
Patch->>Patch: inject traceparent into meta (if meta exists)
MCPClient->>Server: send request
Server-->>MCPClient: response or error
alt success
Patch->>Tracer: setStatus OK, add sanitized output
else error
Patch->>Tracer: recordException, setStatus ERROR
end
Patch-->>Client: response
sequenceDiagram
autonumber
participant ToolManager
participant FastWrapper as FastMCP wrapper
participant Tracer
participant Tool
ToolManager->>FastWrapper: call_tool(name, args/kwargs)
FastWrapper->>Tracer: startSpan "<tool>.tool" (TOOL)
FastWrapper->>FastWrapper: extract_clean_input, truncate if needed
FastWrapper->>Tool: await execution(...)
alt success
FastWrapper->>Tracer: setStatus OK, set TRACELOOP_ENTITY_OUTPUT
else error
FastWrapper->>Tracer: recordException, setStatus ERROR
end
FastWrapper-->>ToolManager: result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![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 feb6129 in 1 minute and 47 seconds. Click for details.
- Reviewed
388lines of code in2files - Skipped
1files when reviewing. - Skipped posting
6draft 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:17
- Draft comment:
Verify 'TraceloopSpanKindValues' is available and compatible with semantic conventions. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify compatibility, which is against the rules. It doesn't provide a specific suggestion or point out a specific issue with the code.
2. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:244
- Draft comment:
Ensure result is a list in _fastmcp_resource_wrapper before using len() and indexing. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The code already has some error handling - it's in a try/except block and checks if result exists. The comment suggests being more defensive by checking the type. However, this is instrumentation code that's just trying to extract metadata - if result isn't a list, it will be caught by the exception handler and gracefully fall back to str(result). Being overly defensive here doesn't add much value. The comment does identify a potential runtime error if result is not a list. Type checking could prevent confusing error messages. The existing error handling is sufficient for instrumentation code. A type error would just result in falling back to the string representation, which is an acceptable outcome. The comment should be deleted. The code already has appropriate error handling for instrumentation purposes.
3. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:297
- Draft comment:
Review span naming in patch_mcp_client to ensure consistent handling across method types. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:422
- Draft comment:
For 'tools/call', consider handling multiple content items rather than only the first one. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
5. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:395
- Draft comment:
Removal of the RequestStreamWriter span seems intentional; confirm that dropping low-level protocol spans is desired. - Reason this comment was not posted:
Comment looked like it was already resolved.
6. packages/opentelemetry-instrumentation-mcp/pyproject.toml:37
- Draft comment:
Added 'fastmcp' dependency; ensure version '^2.0.1' is compatible with the new instrumentation. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is related to a dependency change and is asking the author to ensure compatibility, which violates the rules against commenting on dependencies and asking for confirmation or verification.
Workflow ID: wflow_vfQZRq4sLA9qdhkS
You can customize 
| if arguments: | ||
| try: | ||
| span.set_attribute("fastmcp.tool.input", json.dumps(arguments)) | ||
| except (TypeError, ValueError): |
There was a problem hiding this comment.
Consider validating that the result is a list in _fastmcp_tool_wrapper before iteration.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (1)
332-337: Make traceparent injection robust for both dict- and object-shaped meta.Current code assumes attribute access; handle dict too and reuse global textmap injector.
- if meta and len(args) > 0: - carrier = {} - TraceContextTextMapPropagator().inject(carrier) - meta.traceparent = carrier["traceparent"] - args[0].root.params.meta = meta + if meta and len(args) > 0: + carrier = {} + propagate.get_global_textmap().inject(carrier) + traceparent = carrier.get("traceparent") + if isinstance(meta, dict): + meta["traceparent"] = traceparent + else: + setattr(meta, "traceparent", traceparent) + args[0].root.params.meta = meta
🧹 Nitpick comments (5)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (5)
296-331: Safer defaults for span/entity names when method is None.Avoid "None.tool" spans; provide a stable fallback.
- entity_name = method - span_name = f"{method}.tool" + entity_name = method or "mcp.request" + span_name = f"{entity_name}.tool"
340-354: Output sanitization looks good; consider size limits.To avoid high-cardinality attributes, consider truncating long outputs (e.g., first item/1KB cap).
374-415: Prefer model_dump()/asdict for cleaner, safer param extraction.If params is a Pydantic model or dataclass, use model_dump(exclude_none=True) or asdict to avoid leaking private fields.
416-461: Mirror input approach for outputs; avoid non-serializable fields.For dict paths, consider filtering to JSON-serializable primitives or using model_dump if available.
117-130: Confirm FastMCP hook targets; add resource fallbacks.
- Tool hook confirmed: fastmcp.tools.tool_manager.ToolManager.call_tool — existing registration is correct.
- Resource APIs: fastmcp.resources.resource_manager.ResourceManager.get_resource exists, but resource access is also exposed via fastmcp.server.FastMCP.get_resource / .read_resource and via ProxyResourceManager.read_resource implementations. Register fallback post-import hooks for fastmcp.server.FastMCP.get_resource and .read_resource (and for proxy managers' read_resource implementations — module path varies) to cover minor-version differences.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- 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 (2)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py(9 hunks)packages/opentelemetry-instrumentation-mcp/pyproject.toml(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/opentelemetry/instrumentation/mcp/instrumentation.py
🧬 Code graph analysis (1)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (2)
packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py (2)
SpanAttributes(64-261)TraceloopSpanKindValues(301-306)packages/opentelemetry-instrumentation-anthropic/opentelemetry/instrumentation/anthropic/utils.py (1)
dont_throw(33-61)
🪛 Ruff (0.12.2)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py
191-191: Unused function argument: instance
(ARG001)
229-229: Do not catch blind exception: Exception
(BLE001)
233-233: Consider moving this statement to an else block
(TRY300)
245-245: Unused function argument: instance
(ARG001)
249-249: f-string without any placeholders
Remove extraneous f prefix
(F541)
268-268: Do not catch blind exception: Exception
(BLE001)
272-272: Consider moving this statement to an else block
(TRY300)
309-310: try-except-pass detected, consider logging the exception
(S110)
309-309: Do not catch blind exception: Exception
(BLE001)
413-413: Do not catch blind exception: Exception
(BLE001)
459-459: Do not catch blind exception: Exception
(BLE001)
🪛 Flake8 (7.2.0)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py
[error] 249-249: f-string is missing placeholders
(F541)
⏰ 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.12)
- GitHub Check: Test Packages (3.10)
- GitHub Check: Test Packages (3.11)
- GitHub Check: Build Packages (3.11)
- GitHub Check: Lint
🔇 Additional comments (5)
packages/opentelemetry-instrumentation-mcp/pyproject.toml (1)
37-37: Add fastmcp test dep looks fine; check compat and CI cache.
- Confirm fastmcp ^2.0.1 matches the hooked module paths added in instrumentation.py (see comments there).
- Note: instrumentation_dependencies requires mcp >= 1.6.0, but tests pin mcp ^1.3.0 (Line 36). If intentional, fine; otherwise consider bumping to avoid skewed test coverage.
Would you like me to open a follow-up to bump test mcp to >=1.6 and run a matrix against fastmcp 2.0.x?
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (4)
17-17: Import of TraceloopSpanKindValues is correct.Good for setting TOOL span-kind later.
536-547: Broader request-shape handling in reader LGTM.Covers item.message.root and item.root gracefully with fallbacks.
581-591: Writer shape handling LGTM.Early return fallback maintains original behavior.
644-647: Context-only writer change is sensible.Dropping low-level RequestStreamWriter spans reduces noise while preserving context propagation.
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py
Outdated
Show resolved
Hide resolved
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py
Outdated
Show resolved
Hide resolved
- Separate FastMCP instrumentation logic into dedicated fastmcp_instrumentation.py module - Extract shared utilities into utils.py file - Fix semantic issues: only tools/call operations get tool span semantics - Add comprehensive test coverage for FastMCP instrumentation and attributes - Improve code organization with helper methods for cleaner separation of concerns - Fix all lint issues and improve code quality - Update test configuration to use simplified in-memory span exporter 🤖 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 02ec866 in 1 minute and 42 seconds. Click for details.
- Reviewed
1775lines of code in13files - Skipped
1files when reviewing. - Skipped posting
6draft 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/fastmcp_instrumentation.py:33
- Draft comment:
uninstrument() is left as a stub due to wrapt limitations; consider adding explicit logging or documentation. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The code already has a clear comment explaining why uninstrument() is a stub. The comment mentions "need to document" which implies documentation will be handled elsewhere. Adding logging wouldn't change the fundamental limitation. The comment seems redundant given the existing explanatory comment. Perhaps explicit logging could help with debugging in production environments. Maybe there should be more specific documentation guidance. The existing comment is sufficient for developers to understand the limitation. Adding logging to a no-op function wouldn't provide meaningful value. Delete the comment as it suggests additions that don't add significant value beyond the existing explanatory comment in the code.
2. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/fastmcp_instrumentation.py:117
- Draft comment:
The _get_json_encoder() method returns None, implying the default encoder is used. Consider documenting the rationale. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment suggests documenting the rationale for a method returning None. This is asking for documentation, which is not allowed by the rules. The comment does not provide a specific code suggestion or identify a potential issue with the code itself.
3. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/fastmcp_instrumentation.py:111
- Draft comment:
The _should_send_prompts() method defaults to 'true' if TRACELOOP_TRACE_CONTENT is unset. Verify this default is intended. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to verify if the default behavior of a method is intended. This falls under the rule of not asking the author to confirm their intention or ensure behavior is intended. Therefore, this comment should be removed.
4. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:156
- Draft comment:
In patch_mcp_client(), accessing args[0].root assumes a specific structure. Consider adding defensive checks. - 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.
5. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:238
- Draft comment:
get_error_type() is used to convert error messages; review its logic to ensure non-numeric messages are handled as expected. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to review the logic of a function to ensure it handles non-numeric messages as expected. This is a request for confirmation of intended behavior, which violates the rules.
6. packages/sample-app/sample_app/mcp_dev_assistant_demo.py:166
- Draft comment:
Typo: 'OpenLLMetry' is used in the print statement. Please confirm whether this should be 'OpenTelemetry'. - 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% The consistent use of "OpenLLMetry" multiple times in the file suggests it might be an intentional variation or branding choice, especially since it's used in both comments and strings. The fact that it imports the actual 'opentelemetry' package but consistently uses "OpenLLMetry" in text makes me think this could be a deliberate choice. Without more context about the project's conventions, I can't be certain this is actually a typo. I could be wrong about this being intentional - it could just be a consistent typo throughout the file. Also, the presence of the official package name in imports suggests the official spelling should be used. While those are valid points, making assumptions about naming without understanding project conventions could cause more harm than good. The consistency of usage suggests deliberateness. Given the uncertainty and the consistent usage throughout the file, we should not suggest changing just one instance of "OpenLLMetry" without understanding if it's actually a mistake.
Workflow ID: wflow_jyfXizfrMjB4eDV7
You can customize
| 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])}") |
There was a problem hiding this comment.
The _handle_mcp_method() directly serializes args[0] with serialize(); consider guarding against absent or malformed args.
| span.set_attribute(SpanAttributes.TRACELOOP_ENTITY_INPUT, f"{serialize(args[0])}") | |
| span.set_attribute(SpanAttributes.TRACELOOP_ENTITY_INPUT, f"{serialize(args[0])}" if args and len(args) > 0 and args[0] is not None else "{}") |
| return func(*args, **kwargs) | ||
| except Exception as e: | ||
| logger.debug( | ||
| "OpenLLMetry failed to trace in %s, error: %s", |
There was a problem hiding this comment.
Typographical error: "OpenLLMetry" appears to be a misspelling. Consider changing it to "OpenTelemetry" to match the module's naming.
| "OpenLLMetry failed to trace in %s, error: %s", | |
| "OpenTelemetry failed to trace in %s, error: %s", |
| #!/usr/bin/env python3 | ||
| """ | ||
| MCP Development Assistant Demo Client | ||
| Demonstrates the enhanced OpenLLMetry instrumentation with tool-like spans. |
There was a problem hiding this comment.
Typo: The term 'OpenLLMetry' appears here. If the intention was to refer to 'OpenTelemetry', please correct it.
| Demonstrates the enhanced OpenLLMetry instrumentation with tool-like spans. | |
| Demonstrates the enhanced OpenTelemetry instrumentation with tool-like spans. |
| self.session: Optional[ClientSession] = None | ||
| self.exit_stack = AsyncExitStack() | ||
|
|
||
| # Initialize OpenLLMetry with console exporter to show the enhanced spans |
There was a problem hiding this comment.
Typo: 'OpenLLMetry' is used in this comment. Consider verifying if it should be 'OpenTelemetry' to match the standard spelling.
| # Initialize OpenLLMetry with console exporter to show the enhanced spans | |
| # Initialize OpenTelemetry with console exporter to show the enhanced spans |
| #!/usr/bin/env python3 | ||
| """ | ||
| MCP Development Assistant Server | ||
| A sample MCP server with useful development tools to demonstrate OpenLLMetry instrumentation. |
There was a problem hiding this comment.
Typo: In the file docstring, "OpenLLMetry" appears on line 4. It should likely be "OpenTelemetry".
| A sample MCP server with useful development tools to demonstrate OpenLLMetry instrumentation. | |
| A sample MCP server with useful development tools to demonstrate OpenTelemetry instrumentation. |
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (2)
408-440: Avoid swallowing stream reader errors; remove @dont_throw.
Instrumentation should not hide real transport or parsing issues.- @dont_throw async def __aiter__(self) -> AsyncGenerator[Any, None]:
454-499: Avoid swallowing stream writer errors; remove @dont_throw.
Preserve error propagation from send().- @dont_throw async def send(self, item: Any) -> Any:
♻️ Duplicate comments (2)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (2)
156-183: Do not swallow MCP client errors: remove @dont_throw.
This wrapper must re-raise to preserve behavior; prior feedback already requested this.- def patch_mcp_client(self, tracer: Tracer): - @dont_throw - async def traced_method(wrapped, instance, args, kwargs): + def patch_mcp_client(self, tracer: Tracer): + async def traced_method(wrapped, instance, args, kwargs): meta = None method = None params = None @@ return traced_method
519-524: Same here for ContextSavingStreamWriter.send.- @dont_throw async def send(self, item: Any) -> Any:
🧹 Nitpick comments (11)
packages/opentelemetry-instrumentation-mcp/tests/test_fastmcp.py (1)
45-49: Drop noisy debug prints from testsPrinting all spans on every run clutters CI logs. Consider removing or guarding with an env flag.
- # Debug: Print span details - print(f"\nTotal spans: {len(spans)}") - for i, span in enumerate(spans): - print(f"Span {i}: name='{span.name}', trace_id={span.get_span_context().trace_id}") + # Debug logging intentionally omitted to keep test output cleanpackages/opentelemetry-instrumentation-mcp/tests/test_fastmcp_attributes.py (1)
151-163: Make error assertions resilient to different error pathsIf underlying exceptions propagate instead of being swallowed, keep both status and message checks but don’t rely solely on attribute presence.
- assert error_span.status.status_code.name == "ERROR" - assert "Intentional test error" in error_span.status.description + assert error_span.status.status_code.name == "ERROR" + if error_span.status.description: + assert "Intentional test error" in error_span.status.description @@ - assert ( - error_span.attributes.get("error.type") == "ToolError" or - "error" in error_span.attributes.get("traceloop.entity.output", "").lower() - ) + assert ( + error_span.attributes.get("error.type") in {"ToolError", "ValueError"} + or "error" in (error_span.attributes.get("traceloop.entity.output", "") or "").lower() + )packages/sample-app/sample_app/mcp_dev_assistant_demo.py (1)
155-159: Narrow or annotate broad exception at top-level demoFor a demo, printing is fine; annotate to silence linters.
- except Exception as e: + except Exception as e: # noqa: BLE001 print(f"\n❌ Demo failed: {e}")packages/sample-app/sample_app/mcp_dev_assistant_server.py (3)
7-21: Remove unused imports to satisfy Flake8 and reduce overhead.
- asyncio, json, TextContent, Tool are unused.
Apply:
-import asyncio -import json ... -from mcp.types import TextContent, Tool
203-281: Git helpers: avoid bareexcept, use from e, and check return codes.
- Replace bare
except: passwith narrow handling and log or comment.- When subprocess.run returns non-zero, treat as info instead of silent pass.
- try: + try: ahead_behind_result = subprocess.run( ["git", "rev-list", "--left-right", "--count", f"{branch}...origin/{branch}"], cwd=repo_path, capture_output=True, text=True ) if ahead_behind_result.returncode == 0: parts = ahead_behind_result.stdout.strip().split('\t') ahead = int(parts[0]) if len(parts) > 0 else 0 behind = int(parts[1]) if len(parts) > 1 else 0 - except: - pass + except Exception as e: + # Non-fatal: remote may be missing; keep ahead/behind at 0 + ahead, behind = 0, 0Also consider using shutil.which("git") to fail fast if git is unavailable.
366-384: Prefer introspecting tools instead of hardcoding names.
Keeps list in sync with actual registered tools.For FastMCP, if available:
- tool_names = [ - "list_files", "read_file", "write_file", "run_command", - "git_status", "search_code", "get_system_info" - ] + tool_names = sorted(getattr(server, "tools", {}).keys()) or []Confirm FastMCP exposes a public registry; if not, keep the current approach.
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/fastmcp_instrumentation.py (1)
111-116: Parse boolean env robustly.
Empty string or mixed-case values should be handled; use distutils.util.strtobool-like logic.- return ( - os.getenv("TRACELOOP_TRACE_CONTENT") or "true" - ).lower() == "true" + val = os.getenv("TRACELOOP_TRACE_CONTENT", "true").strip().lower() + return val in ("1", "true", "t", "yes", "y", "on")packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (4)
20-22: Drop dont_throw import if no longer used.
If you accept the changes removing @dont_throw below, this import becomes unused.-from opentelemetry.instrumentation.mcp.utils import dont_throw
184-214: Span naming default for tools without params.
If params is None, span_name becomes "tools/call.tool"; consider defaulting entity_name to "unknown_tool" for clarity.- entity_name = method - span_name = f"{method}.tool" + entity_name = "unknown_tool" + span_name = "unknown_tool.tool"Guarded by the existing branches when params.name is available.
296-336: Graceful handling of result/content shapes.
current approach is fine; ensure result.content access is guarded (it is). Consider capturing multiple content items, not just [0], if FastMCP returns more than one.
469-499: Set span kind attributes for ResponseStreamWriter (parity).
Optional: add MCP attrs (request id, method) if present to aid correlation.
📜 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 (13)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/fastmcp_instrumentation.py(1 hunks)packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py(6 hunks)packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/utils.py(1 hunks)packages/opentelemetry-instrumentation-mcp/pyproject.toml(1 hunks)packages/opentelemetry-instrumentation-mcp/tests/conftest.py(1 hunks)packages/opentelemetry-instrumentation-mcp/tests/mcp_server.py(0 hunks)packages/opentelemetry-instrumentation-mcp/tests/test_fastmcp.py(1 hunks)packages/opentelemetry-instrumentation-mcp/tests/test_fastmcp_attributes.py(1 hunks)packages/opentelemetry-instrumentation-mcp/tests/test_mcp_server.py(0 hunks)packages/opentelemetry-instrumentation-mcp/tests/trace_collector.py(0 hunks)packages/opentelemetry-instrumentation-mcp/tests/whoami.py(0 hunks)packages/sample-app/sample_app/mcp_dev_assistant_demo.py(1 hunks)packages/sample-app/sample_app/mcp_dev_assistant_server.py(1 hunks)
💤 Files with no reviewable changes (4)
- packages/opentelemetry-instrumentation-mcp/tests/whoami.py
- packages/opentelemetry-instrumentation-mcp/tests/trace_collector.py
- packages/opentelemetry-instrumentation-mcp/tests/mcp_server.py
- packages/opentelemetry-instrumentation-mcp/tests/test_mcp_server.py
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opentelemetry-instrumentation-mcp/pyproject.toml
🧰 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.pypackages/opentelemetry-instrumentation-mcp/tests/test_fastmcp_attributes.pypackages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/fastmcp_instrumentation.pypackages/sample-app/sample_app/mcp_dev_assistant_server.pypackages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/utils.pypackages/sample-app/sample_app/mcp_dev_assistant_demo.pypackages/opentelemetry-instrumentation-mcp/tests/conftest.pypackages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py
🧬 Code graph analysis (7)
packages/opentelemetry-instrumentation-mcp/tests/test_fastmcp.py (1)
packages/traceloop-sdk/traceloop/sdk/utils/in_memory_span_exporter.py (1)
get_finished_spans(40-43)
packages/opentelemetry-instrumentation-mcp/tests/test_fastmcp_attributes.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 (3)
packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py (2)
SpanAttributes(64-261)TraceloopSpanKindValues(301-306)packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/utils.py (1)
dont_throw(11-33)packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (3)
traced_method(104-130)traced_method(135-151)traced_method(157-180)
packages/sample-app/sample_app/mcp_dev_assistant_server.py (3)
packages/traceloop-sdk/traceloop/sdk/prompts/model.py (2)
TextContent(17-19)Tool(47-49)packages/traceloop-sdk/traceloop/sdk/__init__.py (2)
Traceloop(37-275)init(49-206)packages/traceloop-sdk/traceloop/sdk/decorators/__init__.py (1)
tool(63-73)
packages/sample-app/sample_app/mcp_dev_assistant_demo.py (2)
packages/traceloop-sdk/traceloop/sdk/__init__.py (2)
Traceloop(37-275)init(49-206)packages/sample-app/sample_app/mcp_dev_assistant_server.py (2)
git_status(204-280)main(366-383)
packages/opentelemetry-instrumentation-mcp/tests/conftest.py (3)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (1)
McpInstrumentor(26-337)packages/traceloop-sdk/traceloop/sdk/utils/in_memory_span_exporter.py (3)
export(45-51)InMemorySpanExporter(22-61)clear(35-38)packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/fastmcp_instrumentation.py (2)
instrument(21-31)uninstrument(33-37)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (3)
packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py (2)
SpanAttributes(64-261)TraceloopSpanKindValues(301-306)packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/utils.py (1)
dont_throw(11-33)packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/fastmcp_instrumentation.py (4)
FastMCPInstrumentor(15-131)instrument(21-31)uninstrument(33-37)traced_method(42-107)
🪛 Ruff (0.12.2)
packages/opentelemetry-instrumentation-mcp/tests/test_fastmcp.py
1-1: Unused function argument: tracer_provider
(ARG001)
packages/opentelemetry-instrumentation-mcp/tests/test_fastmcp_attributes.py
6-6: Unused function argument: tracer_provider
(ARG001)
13-13: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
125-125: Unused function argument: tracer_provider
(ARG001)
135-135: Avoid specifying long messages outside the exception class
(TRY003)
142-143: try-except-pass detected, consider logging the exception
(S110)
142-142: Do not catch blind exception: Exception
(BLE001)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/fastmcp_instrumentation.py
42-42: Unused function argument: instance
(ARG001)
101-101: Consider moving this statement to an else block
(TRY300)
packages/sample-app/sample_app/mcp_dev_assistant_server.py
1-1: Shebang is present but file is not executable
(EXE001)
76-76: Abstract raise to an inner function
(TRY301)
76-76: Avoid specifying long messages outside the exception class
(TRY003)
90-90: Consider moving this statement to an else block
(TRY300)
92-92: Do not catch blind exception: Exception
(BLE001)
93-93: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
93-93: Avoid specifying long messages outside the exception class
(TRY003)
93-93: Use explicit conversion flag
Replace with conversion flag
(RUF010)
111-111: Abstract raise to an inner function
(TRY301)
111-111: Avoid specifying long messages outside the exception class
(TRY003)
114-114: Abstract raise to an inner function
(TRY301)
114-114: Avoid specifying long messages outside the exception class
(TRY003)
127-127: Do not catch blind exception: Exception
(BLE001)
128-128: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
128-128: Avoid specifying long messages outside the exception class
(TRY003)
128-128: Use explicit conversion flag
Replace with conversion flag
(RUF010)
155-155: Consider moving this statement to an else block
(TRY300)
157-157: Do not catch blind exception: Exception
(BLE001)
158-158: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
158-158: Avoid specifying long messages outside the exception class
(TRY003)
158-158: Use explicit conversion flag
Replace with conversion flag
(RUF010)
177-177: subprocess call with shell=True identified, security issue
(S602)
198-198: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
198-198: Avoid specifying long messages outside the exception class
(TRY003)
199-199: Do not catch blind exception: Exception
(BLE001)
200-200: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
200-200: Avoid specifying long messages outside the exception class
(TRY003)
200-200: Use explicit conversion flag
Replace with conversion flag
(RUF010)
217-217: Abstract raise to an inner function
(TRY301)
217-217: Avoid specifying long messages outside the exception class
(TRY003)
221-221: Starting a process with a partial executable path
(S607)
230-230: Starting a process with a partial executable path
(S607)
257-257: subprocess call: check for execution of untrusted input
(S603)
258-258: Starting a process with a partial executable path
(S607)
267-267: Do not use bare except
(E722)
267-268: try-except-pass detected, consider logging the exception
(S110)
279-279: Do not catch blind exception: Exception
(BLE001)
280-280: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
280-280: Avoid specifying long messages outside the exception class
(TRY003)
280-280: Use explicit conversion flag
Replace with conversion flag
(RUF010)
284-284: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
303-303: Abstract raise to an inner function
(TRY301)
303-303: Avoid specifying long messages outside the exception class
(TRY003)
329-330: try-except-continue detected, consider logging the exception
(S112)
329-329: Do not catch blind exception: Exception
(BLE001)
332-332: Consider moving this statement to an else block
(TRY300)
334-334: Do not catch blind exception: Exception
(BLE001)
335-335: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
335-335: Avoid specifying long messages outside the exception class
(TRY003)
335-335: Use explicit conversion flag
Replace with conversion flag
(RUF010)
362-362: Do not catch blind exception: Exception
(BLE001)
363-363: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
363-363: Avoid specifying long messages outside the exception class
(TRY003)
363-363: Use explicit conversion flag
Replace with conversion flag
(RUF010)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/utils.py
24-24: Do not catch blind exception: Exception
(BLE001)
packages/sample-app/sample_app/mcp_dev_assistant_demo.py
1-1: Shebang is present but file is not executable
(EXE001)
38-38: Avoid specifying long messages outside the exception class
(TRY003)
73-73: Local variable system_info is assigned to but never used
Remove assignment to unused variable system_info
(F841)
74-74: f-string without any placeholders
Remove extraneous f prefix
(F541)
75-75: Do not catch blind exception: Exception
(BLE001)
81-81: Local variable files_result is assigned to but never used
Remove assignment to unused variable files_result
(F841)
82-82: f-string without any placeholders
Remove extraneous f prefix
(F541)
83-83: Do not catch blind exception: Exception
(BLE001)
89-89: Local variable search_result is assigned to but never used
Remove assignment to unused variable search_result
(F841)
95-95: f-string without any placeholders
Remove extraneous f prefix
(F541)
96-96: Do not catch blind exception: Exception
(BLE001)
102-102: Local variable git_status is assigned to but never used
Remove assignment to unused variable git_status
(F841)
103-103: f-string without any placeholders
Remove extraneous f prefix
(F541)
104-104: Do not catch blind exception: Exception
(BLE001)
110-110: Local variable cmd_result is assigned to but never used
Remove assignment to unused variable cmd_result
(F841)
114-114: f-string without any placeholders
Remove extraneous f prefix
(F541)
115-115: Do not catch blind exception: Exception
(BLE001)
122-122: Local variable write_result is assigned to but never used
Remove assignment to unused variable write_result
(F841)
123-123: Probable insecure usage of temporary file or directory: "/tmp/mcp_test.txt"
(S108)
126-126: f-string without any placeholders
Remove extraneous f prefix
(F541)
129-129: Local variable read_result is assigned to but never used
Remove assignment to unused variable read_result
(F841)
130-130: Probable insecure usage of temporary file or directory: "/tmp/mcp_test.txt"
(S108)
133-133: f-string without any placeholders
Remove extraneous f prefix
(F541)
135-135: Do not catch blind exception: Exception
(BLE001)
157-157: Do not catch blind exception: Exception
(BLE001)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py
197-198: try-except-pass detected, consider logging the exception
(S110)
197-197: Do not catch blind exception: Exception
(BLE001)
244-244: Consider moving this statement to an else block
(TRY300)
290-290: Do not catch blind exception: Exception
(BLE001)
336-336: Do not catch blind exception: Exception
(BLE001)
🪛 Flake8 (7.2.0)
packages/sample-app/sample_app/mcp_dev_assistant_server.py
[error] 7-7: 'asyncio' imported but unused
(F401)
[error] 8-8: 'json' imported but unused
(F401)
[error] 18-18: 'mcp.types.TextContent' imported but unused
(F401)
[error] 18-18: 'mcp.types.Tool' imported but unused
(F401)
[error] 267-267: do not use bare 'except'
(E722)
packages/sample-app/sample_app/mcp_dev_assistant_demo.py
[error] 73-73: local variable 'system_info' is assigned to but never used
(F841)
[error] 74-74: f-string is missing placeholders
(F541)
[error] 81-81: local variable 'files_result' is assigned to but never used
(F841)
[error] 82-82: f-string is missing placeholders
(F541)
[error] 89-89: local variable 'search_result' is assigned to but never used
(F841)
[error] 95-95: f-string is missing placeholders
(F541)
[error] 102-102: local variable 'git_status' is assigned to but never used
(F841)
[error] 103-103: f-string is missing placeholders
(F541)
[error] 110-110: local variable 'cmd_result' is assigned to but never used
(F841)
[error] 114-114: f-string is missing placeholders
(F541)
[error] 122-122: local variable 'write_result' is assigned to but never used
(F841)
[error] 126-126: f-string is missing placeholders
(F541)
[error] 129-129: local variable 'read_result' is assigned to but never used
(F841)
[error] 133-133: f-string is missing placeholders
(F541)
⏰ 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: Lint
- GitHub Check: Test Packages (3.11)
- GitHub Check: Test Packages (3.12)
- GitHub Check: Test Packages (3.10)
- GitHub Check: Build Packages (3.11)
🔇 Additional comments (2)
packages/opentelemetry-instrumentation-mcp/tests/conftest.py (1)
24-33: LGTM — clean, minimal test instrumentation wiringSession-scoped provider + in-memory exporter with autouse instrumentor and teardown looks good.
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (1)
221-250: Minor: avoid JSON-double-encoding.
serialize() already returns JSON; attributes expect strings, so you’re OK, but ensure consumers don’t decode twice. No change required; just a heads-up.
| @dont_throw | ||
| async def traced_method(wrapped, instance, args, kwargs): | ||
| if not self._tracer: | ||
| return await wrapped(*args, **kwargs) | ||
|
|
There was a problem hiding this comment.
Do not swallow tool errors: remove @dont_throw and mark unused param.
@dont_throw here will swallow exceptions from tool execution and change behavior. Let exceptions propagate after recording them.
- @dont_throw
- async def traced_method(wrapped, instance, args, kwargs):
+ async def traced_method(wrapped, _instance, args, kwargs):
if not self._tracer:
return await wrapped(*args, **kwargs)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @dont_throw | |
| async def traced_method(wrapped, instance, args, kwargs): | |
| if not self._tracer: | |
| return await wrapped(*args, **kwargs) | |
| async def traced_method(wrapped, _instance, args, kwargs): | |
| if not self._tracer: | |
| return await wrapped(*args, **kwargs) |
🧰 Tools
🪛 Ruff (0.12.2)
42-42: Unused function argument: instance
(ARG001)
🤖 Prompt for AI Agents
In
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/fastmcp_instrumentation.py
around lines 41-45, the traced_method is decorated with @dont_throw which
swallows exceptions from tool execution; remove the @dont_throw decorator so
exceptions propagate after being recorded, and mark the unused parameter (likely
"instance") with a leading underscore (e.g., _instance) or use an explicit
unused-name comment to indicate it's intentionally unused; ensure any exception
recording logic remains but allow the original exceptions to bubble up.
| # Add output in traceloop format | ||
| 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) | ||
| span.set_attribute(SpanAttributes.TRACELOOP_ENTITY_OUTPUT, truncated_output) | ||
| except (TypeError, ValueError): | ||
| pass # Skip output logging if serialization fails | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard output serialization against non-iterables.
Result may not be iterable (e.g., dict/obj). Handle list/tuple specifically and fall back to str().
- # Add output in traceloop format
- if self._should_send_prompts() and result:
+ # Add output in traceloop format
+ if self._should_send_prompts() and result is not None:
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))
+ def to_serializable(item):
+ if hasattr(item, "text"):
+ return {"type": "text", "content": item.text}
+ if hasattr(item, "__dict__"):
+ return {k: v for k, v in item.__dict__.items() if not k.startswith("_")}
+ return str(item)
+
+ if isinstance(result, (list, tuple)):
+ output_data = [to_serializable(i) for i in result]
+ else:
+ output_data = to_serializable(result)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Add output in traceloop format | |
| 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) | |
| span.set_attribute(SpanAttributes.TRACELOOP_ENTITY_OUTPUT, truncated_output) | |
| except (TypeError, ValueError): | |
| pass # Skip output logging if serialization fails | |
| # Add output in traceloop format | |
| if self._should_send_prompts() and result is not None: | |
| try: | |
| def to_serializable(item): | |
| if hasattr(item, "text"): | |
| return {"type": "text", "content": item.text} | |
| if hasattr(item, "__dict__"): | |
| return {k: v for k, v in item.__dict__.items() if not k.startswith("_")} | |
| return str(item) | |
| if isinstance(result, (list, tuple)): | |
| output_data = [to_serializable(i) for i in result] | |
| else: | |
| output_data = to_serializable(result) | |
| json_output = json.dumps(output_data, cls=self._get_json_encoder()) | |
| truncated_output = self._truncate_json_if_needed(json_output) | |
| span.set_attribute(SpanAttributes.TRACELOOP_ENTITY_OUTPUT, truncated_output) | |
| except (TypeError, ValueError): | |
| pass # Skip output logging if serialization fails |
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/utils.py
Outdated
Show resolved
Hide resolved
| """Comprehensive test for FastMCP instrumentation attributes.""" | ||
|
|
||
| import json | ||
|
|
||
|
|
||
| async def test_fastmcp_comprehensive_attributes(span_exporter, tracer_provider) -> None: | ||
| """Test all FastMCP span attributes comprehensively.""" | ||
| from fastmcp import FastMCP, Client | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Import pytest and Optional; mark async tests explicitly
Ensure async tests run consistently and fix implicit Optional typing.
-"""Comprehensive test for FastMCP instrumentation attributes."""
+"""Comprehensive test for FastMCP instrumentation attributes."""
-import json
+import json
+from typing import Optional
+import pytestAdd marker and silence unused fixture warnings:
-async def test_fastmcp_comprehensive_attributes(span_exporter, tracer_provider) -> None:
+@pytest.mark.anyio
+async def test_fastmcp_comprehensive_attributes(span_exporter, tracer_provider) -> None: # noqa: ARG001📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| """Comprehensive test for FastMCP instrumentation attributes.""" | |
| import json | |
| async def test_fastmcp_comprehensive_attributes(span_exporter, tracer_provider) -> None: | |
| """Test all FastMCP span attributes comprehensively.""" | |
| from fastmcp import FastMCP, Client | |
| """Comprehensive test for FastMCP instrumentation attributes.""" | |
| import json | |
| from typing import Optional | |
| import pytest | |
| @pytest.mark.anyio | |
| async def test_fastmcp_comprehensive_attributes(span_exporter, tracer_provider) -> None: # noqa: ARG001 | |
| """Test all FastMCP span attributes comprehensively.""" | |
| from fastmcp import FastMCP, Client |
🧰 Tools
🪛 Ruff (0.12.2)
6-6: Unused function argument: tracer_provider
(ARG001)
🤖 Prompt for AI Agents
In packages/opentelemetry-instrumentation-mcp/tests/test_fastmcp_attributes.py
around lines 1 to 9, the test is missing pytest and typing imports, lacks an
explicit async test marker, and may cause unused-fixture warnings; import pytest
and typing.Optional at the top, add @pytest.mark.asyncio above the async test
function, annotate the tracer_provider parameter with Optional[...] to make its
typing explicit, and silence unused fixture warnings by either prefixing unused
fixture parameter names with an underscore (e.g., _span_exporter) or using
@pytest.mark.usefixtures(...) if fixtures are only needed for setup.
| @server.tool() | ||
| async def process_data(items: list, operation: str, metadata: dict = None) -> dict: | ||
| """Process data with operation and metadata.""" | ||
| if operation == "sum": | ||
| result = sum(items) | ||
| elif operation == "count": | ||
| result = len(items) | ||
| else: | ||
| result = f"Unknown operation: {operation}" | ||
|
|
||
| return { | ||
| "result": result, | ||
| "operation": operation, | ||
| "metadata": metadata or {}, | ||
| "processed_items": len(items) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
PEP 484: avoid implicit Optional; make type explicit
- async def process_data(items: list, operation: str, metadata: dict = None) -> dict:
+ async def process_data(items: list, operation: str, metadata: Optional[dict] = None) -> dict:Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.12.2)
13-13: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
🤖 Prompt for AI Agents
In packages/opentelemetry-instrumentation-mcp/tests/test_fastmcp_attributes.py
around lines 12 to 27, the function signature uses a bare default of None for
metadata which creates an implicit Optional; update the type annotation to make
Optional explicit by importing typing (from typing import Optional, Dict, Any)
and change the parameter to metadata: Optional[Dict[str, Any]] = None (or
metadata: dict | None = None if using Python 3.10+), ensuring imports are added
and the rest of the function remains unchanged.
| @server.tool() | ||
| def read_file(file_path: str, max_lines: Optional[int] = None) -> str: | ||
| """ | ||
| Read the contents of a text file. | ||
| Args: | ||
| file_path: Path to the file to read | ||
| max_lines: Maximum number of lines to read (optional) | ||
| Returns: | ||
| File contents as a string | ||
| """ | ||
| try: | ||
| path = Path(file_path) | ||
| if not path.exists(): | ||
| raise FileNotFoundError(f"File {file_path} does not exist") | ||
|
|
||
| if not path.is_file(): | ||
| raise ValueError(f"{file_path} is not a file") | ||
|
|
||
| with open(path, 'r', encoding='utf-8') as f: | ||
| if max_lines: | ||
| lines = [] | ||
| for i, line in enumerate(f): | ||
| if i >= max_lines: | ||
| break | ||
| lines.append(line.rstrip('\n')) | ||
| return '\n'.join(lines) | ||
| else: | ||
| return f.read() | ||
|
|
||
| except Exception as e: | ||
| raise RuntimeError(f"Failed to read file: {str(e)}") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same here: avoid blind except and use from e.
- except Exception as e:
- raise RuntimeError(f"Failed to read file: {str(e)}")
+ except Exception as e:
+ raise RuntimeError(f"Failed to read file: {e}") from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @server.tool() | |
| def read_file(file_path: str, max_lines: Optional[int] = None) -> str: | |
| """ | |
| Read the contents of a text file. | |
| Args: | |
| file_path: Path to the file to read | |
| max_lines: Maximum number of lines to read (optional) | |
| Returns: | |
| File contents as a string | |
| """ | |
| try: | |
| path = Path(file_path) | |
| if not path.exists(): | |
| raise FileNotFoundError(f"File {file_path} does not exist") | |
| if not path.is_file(): | |
| raise ValueError(f"{file_path} is not a file") | |
| with open(path, 'r', encoding='utf-8') as f: | |
| if max_lines: | |
| lines = [] | |
| for i, line in enumerate(f): | |
| if i >= max_lines: | |
| break | |
| lines.append(line.rstrip('\n')) | |
| return '\n'.join(lines) | |
| else: | |
| return f.read() | |
| except Exception as e: | |
| raise RuntimeError(f"Failed to read file: {str(e)}") | |
| @server.tool() | |
| def read_file(file_path: str, max_lines: Optional[int] = None) -> str: | |
| """ | |
| Read the contents of a text file. | |
| Args: | |
| file_path: Path to the file to read | |
| max_lines: Maximum number of lines to read (optional) | |
| Returns: | |
| File contents as a string | |
| """ | |
| try: | |
| path = Path(file_path) | |
| if not path.exists(): | |
| raise FileNotFoundError(f"File {file_path} does not exist") | |
| if not path.is_file(): | |
| raise ValueError(f"{file_path} is not a file") | |
| with open(path, 'r', encoding='utf-8') as f: | |
| if max_lines: | |
| lines = [] | |
| for i, line in enumerate(f): | |
| if i >= max_lines: | |
| break | |
| lines.append(line.rstrip('\n')) | |
| return '\n'.join(lines) | |
| else: | |
| return f.read() | |
| except Exception as e: | |
| raise RuntimeError(f"Failed to read file: {e}") from e |
🧰 Tools
🪛 Ruff (0.12.2)
111-111: Abstract raise to an inner function
(TRY301)
111-111: Avoid specifying long messages outside the exception class
(TRY003)
114-114: Abstract raise to an inner function
(TRY301)
114-114: Avoid specifying long messages outside the exception class
(TRY003)
127-127: Do not catch blind exception: Exception
(BLE001)
128-128: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
128-128: Avoid specifying long messages outside the exception class
(TRY003)
128-128: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In packages/sample-app/sample_app/mcp_dev_assistant_server.py around lines 96 to
129, the helper catches Exception and re-raises a RuntimeError without
preserving the original traceback; change the error handling to either catch
specific exceptions (FileNotFoundError, ValueError, OSError) or keep the broad
except but re-raise using "from e" so the original exception is chained (e.g.,
raise RuntimeError(f'Failed to read file: {e}') from e); ensure the original
exception message is included and do not swallow the traceback.
| @server.tool() | ||
| def write_file(file_path: str, content: str, append: bool = False) -> str: | ||
| """ | ||
| Write content to a file. | ||
| Args: | ||
| file_path: Path where to write the file | ||
| content: Content to write | ||
| append: Whether to append to existing file (default: False) | ||
| Returns: | ||
| Success message with file info | ||
| """ | ||
| try: | ||
| path = Path(file_path) | ||
|
|
||
| # Create parent directories if they don't exist | ||
| path.parent.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| mode = 'a' if append else 'w' | ||
| with open(path, mode, encoding='utf-8') as f: | ||
| f.write(content) | ||
|
|
||
| stat = path.stat() | ||
| return f"Successfully {'appended to' if append else 'wrote'} {file_path} ({stat.st_size} bytes)" | ||
|
|
||
| except Exception as e: | ||
| raise RuntimeError(f"Failed to write file: {str(e)}") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same here: preserve traceback and avoid blind except.
- except Exception as e:
- raise RuntimeError(f"Failed to write file: {str(e)}")
+ except Exception as e:
+ raise RuntimeError(f"Failed to write file: {e}") from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @server.tool() | |
| def write_file(file_path: str, content: str, append: bool = False) -> str: | |
| """ | |
| Write content to a file. | |
| Args: | |
| file_path: Path where to write the file | |
| content: Content to write | |
| append: Whether to append to existing file (default: False) | |
| Returns: | |
| Success message with file info | |
| """ | |
| try: | |
| path = Path(file_path) | |
| # Create parent directories if they don't exist | |
| path.parent.mkdir(parents=True, exist_ok=True) | |
| mode = 'a' if append else 'w' | |
| with open(path, mode, encoding='utf-8') as f: | |
| f.write(content) | |
| stat = path.stat() | |
| return f"Successfully {'appended to' if append else 'wrote'} {file_path} ({stat.st_size} bytes)" | |
| except Exception as e: | |
| raise RuntimeError(f"Failed to write file: {str(e)}") | |
| @server.tool() | |
| def write_file(file_path: str, content: str, append: bool = False) -> str: | |
| """ | |
| Write content to a file. | |
| Args: | |
| file_path: Path where to write the file | |
| content: Content to write | |
| append: Whether to append to existing file (default: False) | |
| Returns: | |
| Success message with file info | |
| """ | |
| try: | |
| path = Path(file_path) | |
| # Create parent directories if they don't exist | |
| path.parent.mkdir(parents=True, exist_ok=True) | |
| mode = 'a' if append else 'w' | |
| with open(path, mode, encoding='utf-8') as f: | |
| f.write(content) | |
| stat = path.stat() | |
| return f"Successfully {'appended to' if append else 'wrote'} {file_path} ({stat.st_size} bytes)" | |
| except Exception as e: | |
| raise RuntimeError(f"Failed to write file: {e}") from e |
🧰 Tools
🪛 Ruff (0.12.2)
155-155: Consider moving this statement to an else block
(TRY300)
157-157: Do not catch blind exception: Exception
(BLE001)
158-158: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
158-158: Avoid specifying long messages outside the exception class
(TRY003)
158-158: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In packages/sample-app/sample_app/mcp_dev_assistant_server.py around lines 131
to 159, the try/except is catching Exception and re-raising a new RuntimeError
without preserving the original traceback; change this to either catch specific
I/O-related exceptions (e.g., OSError, IOError) or remove the broad try/except
so the original exception propagates, and if you must wrap the error keep the
original traceback by using exception chaining (raise RuntimeError("Failed to
write file") from e) so callers see the root cause and stack trace.
| @server.tool() | ||
| def run_command(command: str, working_directory: str = ".", timeout: int = 30) -> ProcessResult: | ||
| """ | ||
| Execute a shell command and return the result. | ||
| Args: | ||
| command: Shell command to execute | ||
| working_directory: Directory to run the command in (default: current) | ||
| timeout: Command timeout in seconds (default: 30) | ||
| Returns: | ||
| Process execution result with stdout, stderr, and exit code | ||
| """ | ||
| try: | ||
| start_time = datetime.now() | ||
|
|
||
| result = subprocess.run( | ||
| command, | ||
| shell=True, | ||
| cwd=working_directory, | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=timeout | ||
| ) | ||
|
|
||
| end_time = datetime.now() | ||
| execution_time = (end_time - start_time).total_seconds() | ||
|
|
||
| return ProcessResult( | ||
| command=command, | ||
| exit_code=result.returncode, | ||
| stdout=result.stdout, | ||
| stderr=result.stderr, | ||
| execution_time=execution_time | ||
| ) | ||
|
|
||
| except subprocess.TimeoutExpired: | ||
| raise RuntimeError(f"Command timed out after {timeout} seconds") | ||
| except Exception as e: | ||
| raise RuntimeError(f"Failed to execute command: {str(e)}") | ||
|
|
There was a problem hiding this comment.
Command injection risk: avoid shell=True and sanitize execution.
User-controlled command with shell=True is dangerous. Use shell=False with shlex.split(), and optionally enforce an allowlist and cwd sandbox.
+import shlex
@@
-def run_command(command: str, working_directory: str = ".", timeout: int = 30) -> ProcessResult:
+def run_command(command: str, working_directory: str = ".", timeout: int = 30) -> ProcessResult:
@@
- result = subprocess.run(
- command,
- shell=True,
+ args = shlex.split(command)
+ result = subprocess.run(
+ args,
+ shell=False,
cwd=working_directory,
capture_output=True,
text=True,
timeout=timeout
)
@@
- except subprocess.TimeoutExpired:
- raise RuntimeError(f"Command timed out after {timeout} seconds")
- except Exception as e:
- raise RuntimeError(f"Failed to execute command: {str(e)}")
+ except subprocess.TimeoutExpired as e:
+ raise RuntimeError(f"Command timed out after {timeout} seconds") from e
+ except Exception as e:
+ raise RuntimeError(f"Failed to execute command: {e}") from eOptional: restrict working_directory to a base dir and cap stdout/stderr size.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @server.tool() | |
| def run_command(command: str, working_directory: str = ".", timeout: int = 30) -> ProcessResult: | |
| """ | |
| Execute a shell command and return the result. | |
| Args: | |
| command: Shell command to execute | |
| working_directory: Directory to run the command in (default: current) | |
| timeout: Command timeout in seconds (default: 30) | |
| Returns: | |
| Process execution result with stdout, stderr, and exit code | |
| """ | |
| try: | |
| start_time = datetime.now() | |
| result = subprocess.run( | |
| command, | |
| shell=True, | |
| cwd=working_directory, | |
| capture_output=True, | |
| text=True, | |
| timeout=timeout | |
| ) | |
| end_time = datetime.now() | |
| execution_time = (end_time - start_time).total_seconds() | |
| return ProcessResult( | |
| command=command, | |
| exit_code=result.returncode, | |
| stdout=result.stdout, | |
| stderr=result.stderr, | |
| execution_time=execution_time | |
| ) | |
| except subprocess.TimeoutExpired: | |
| raise RuntimeError(f"Command timed out after {timeout} seconds") | |
| except Exception as e: | |
| raise RuntimeError(f"Failed to execute command: {str(e)}") | |
| import shlex | |
| @server.tool() | |
| def run_command(command: str, working_directory: str = ".", timeout: int = 30) -> ProcessResult: | |
| """ | |
| Execute a shell command and return the result. | |
| Args: | |
| command: Shell command to execute | |
| working_directory: Directory to run the command in (default: current) | |
| timeout: Command timeout in seconds (default: 30) | |
| Returns: | |
| Process execution result with stdout, stderr, and exit code | |
| """ | |
| try: | |
| start_time = datetime.now() | |
| args = shlex.split(command) | |
| result = subprocess.run( | |
| args, | |
| shell=False, | |
| cwd=working_directory, | |
| capture_output=True, | |
| text=True, | |
| timeout=timeout | |
| ) | |
| end_time = datetime.now() | |
| execution_time = (end_time - start_time).total_seconds() | |
| return ProcessResult( | |
| command=command, | |
| exit_code=result.returncode, | |
| stdout=result.stdout, | |
| stderr=result.stderr, | |
| execution_time=execution_time | |
| ) | |
| except subprocess.TimeoutExpired as e: | |
| raise RuntimeError(f"Command timed out after {timeout} seconds") from e | |
| except Exception as e: | |
| raise RuntimeError(f"Failed to execute command: {e}") from e |
🧰 Tools
🪛 Ruff (0.12.2)
177-177: subprocess call with shell=True identified, security issue
(S602)
198-198: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
198-198: Avoid specifying long messages outside the exception class
(TRY003)
199-199: Do not catch blind exception: Exception
(BLE001)
200-200: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
200-200: Avoid specifying long messages outside the exception class
(TRY003)
200-200: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In packages/sample-app/sample_app/mcp_dev_assistant_server.py around lines
161-201, the run_command tool currently calls subprocess.run with shell=True on
user-controlled input which risks command injection; update it to use
shell=False and pass an argv list (e.g., shlex.split(command)) instead of a
shell string, validate the parsed command against an allowlist (or explicit
permitted executables) before running, restrict working_directory to a confined
base directory (reject or normalize paths outside that base), enforce a maximum
stdout/stderr length by truncating results before returning, and keep proper
timeout handling (include command context in TimeoutExpired and other error
messages). Ensure subprocess.run uses capture_output=True, text=True, timeout,
cwd only after validation, and raise a clear error if validation fails.
| @server.tool() | ||
| def search_code(pattern: str, directory: str = ".", file_extensions: List[str] = None, max_results: int = 50) -> Dict[str, List[str]]: | ||
| """ | ||
| Search for a pattern in code files. | ||
| Args: | ||
| pattern: Text pattern to search for | ||
| directory: Directory to search in (default: current) | ||
| file_extensions: List of file extensions to include (e.g., ['.py', '.js']) | ||
| max_results: Maximum number of matches to return (default: 50) | ||
| Returns: | ||
| Dictionary mapping file paths to lists of matching lines | ||
| """ | ||
| try: | ||
| if file_extensions is None: | ||
| file_extensions = ['.py', '.js', '.ts', '.java', '.cpp', '.c', '.h', '.rb', '.go', '.rs'] | ||
|
|
||
| search_path = Path(directory) | ||
| if not search_path.exists(): | ||
| raise FileNotFoundError(f"Directory {directory} does not exist") | ||
|
|
||
| results = {} | ||
| total_matches = 0 | ||
|
|
||
| for ext in file_extensions: | ||
| if total_matches >= max_results: | ||
| break | ||
|
|
||
| for file_path in search_path.rglob(f"*{ext}"): | ||
| if total_matches >= max_results: | ||
| break | ||
|
|
||
| try: | ||
| with open(file_path, 'r', encoding='utf-8', errors='ignore') as f: | ||
| matches = [] | ||
| for line_num, line in enumerate(f, 1): | ||
| if pattern.lower() in line.lower(): | ||
| matches.append(f"{line_num}: {line.strip()}") | ||
| total_matches += 1 | ||
| if total_matches >= max_results: | ||
| break | ||
|
|
||
| if matches: | ||
| results[str(file_path.relative_to(search_path))] = matches | ||
|
|
||
| except Exception: | ||
| continue # Skip files that can't be read | ||
|
|
||
| return results | ||
|
|
||
| except Exception as e: | ||
| raise RuntimeError(f"Failed to search code: {str(e)}") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type hint fix and exception handling in search_code.
- file_extensions should be Optional[List[str]] per PEP 484.
- Avoid blind except and preserve traceback.
-def search_code(pattern: str, directory: str = ".", file_extensions: List[str] = None, max_results: int = 50) -> Dict[str, List[str]]:
+from typing import Optional
+def search_code(pattern: str, directory: str = ".", file_extensions: Optional[List[str]] = None, max_results: int = 50) -> Dict[str, List[str]]:
@@
- except Exception:
- continue # Skip files that can't be read
+ except Exception:
+ continue # Skip unreadable files
@@
- except Exception as e:
- raise RuntimeError(f"Failed to search code: {str(e)}")
+ except Exception as e:
+ raise RuntimeError(f"Failed to search code: {e}") from eCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.12.2)
284-284: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
303-303: Abstract raise to an inner function
(TRY301)
303-303: Avoid specifying long messages outside the exception class
(TRY003)
329-330: try-except-continue detected, consider logging the exception
(S112)
329-329: Do not catch blind exception: Exception
(BLE001)
332-332: Consider moving this statement to an else block
(TRY300)
334-334: Do not catch blind exception: Exception
(BLE001)
335-335: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
335-335: Avoid specifying long messages outside the exception class
(TRY003)
335-335: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In packages/sample-app/sample_app/mcp_dev_assistant_server.py around lines
283-336, update the search_code signature and exception handling: change
file_extensions type to Optional[List[str]] (import Optional from typing) so it
follows PEP 484, replace the inner bare "except Exception:" that skips
unreadable files with a narrower exception catch (e.g., except (OSError,
UnicodeDecodeError) as e:) and handle or log the error instead of swallowing it,
and change the outer "except Exception as e: raise RuntimeError(...)" to
re-raise with preserved traceback using "raise RuntimeError(... ) from e" so the
original exception is chained.
| @server.tool() | ||
| def get_system_info() -> Dict[str, Any]: | ||
| """ | ||
| Get system information including Python version, OS, and environment. | ||
| Returns: | ||
| Dictionary containing system information | ||
| """ | ||
| try: | ||
| import platform | ||
|
|
||
| return { | ||
| "python_version": sys.version, | ||
| "platform": platform.platform(), | ||
| "architecture": platform.architecture(), | ||
| "processor": platform.processor(), | ||
| "python_executable": sys.executable, | ||
| "current_directory": str(Path.cwd()), | ||
| "environment_variables": { | ||
| k: v for k, v in os.environ.items() | ||
| if not any(secret in k.lower() for secret in ['key', 'token', 'password', 'secret']) | ||
| } | ||
| } | ||
|
|
||
| except Exception as e: | ||
| raise RuntimeError(f"Failed to get system info: {str(e)}") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid exposing environment variables by default.
Even with filtering, this can leak secrets (tokens often have nonstandard names). Remove envs or gate behind an explicit opt-in.
- "environment_variables": {
- k: v for k, v in os.environ.items()
- if not any(secret in k.lower() for secret in ['key', 'token', 'password', 'secret'])
- }
+ # Intentionally exclude environment variables to avoid secret leakage
+ "environment_variables": {}If you need this, add a parameter like include_env: bool = False and redact values.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @server.tool() | |
| def get_system_info() -> Dict[str, Any]: | |
| """ | |
| Get system information including Python version, OS, and environment. | |
| Returns: | |
| Dictionary containing system information | |
| """ | |
| try: | |
| import platform | |
| return { | |
| "python_version": sys.version, | |
| "platform": platform.platform(), | |
| "architecture": platform.architecture(), | |
| "processor": platform.processor(), | |
| "python_executable": sys.executable, | |
| "current_directory": str(Path.cwd()), | |
| "environment_variables": { | |
| k: v for k, v in os.environ.items() | |
| if not any(secret in k.lower() for secret in ['key', 'token', 'password', 'secret']) | |
| } | |
| } | |
| except Exception as e: | |
| raise RuntimeError(f"Failed to get system info: {str(e)}") | |
| @server.tool() | |
| def get_system_info() -> Dict[str, Any]: | |
| """ | |
| Get system information including Python version, OS, and environment. | |
| Returns: | |
| Dictionary containing system information | |
| """ | |
| try: | |
| import platform | |
| return { | |
| "python_version": sys.version, | |
| "platform": platform.platform(), | |
| "architecture": platform.architecture(), | |
| "processor": platform.processor(), | |
| "python_executable": sys.executable, | |
| "current_directory": str(Path.cwd()), | |
| # Intentionally exclude environment variables to avoid secret leakage | |
| "environment_variables": {} | |
| } | |
| except Exception as e: | |
| raise RuntimeError(f"Failed to get system info: {str(e)}") |
🧰 Tools
🪛 Ruff (0.12.2)
362-362: Do not catch blind exception: Exception
(BLE001)
363-363: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
363-363: Avoid specifying long messages outside the exception class
(TRY003)
363-363: Use explicit conversion flag
Replace with conversion flag
(RUF010)
- Replace sync-only dont_throw decorator with async-aware version from OpenAI - Properly handles both synchronous and asynchronous functions - Uses asyncio.iscoroutinefunction() to detect function type - Maintains consistent exception handling across all instrumentation packages - Preserves existing behavior while adding async support 🤖 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 e64ebe1 in 1 minute and 16 seconds. Click for details.
- Reviewed
55lines 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/utils.py:31
- Draft comment:
Consider moving the _handle_exception definition above its usage in async_wrapper and sync_wrapper for improved readability. - 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 define functions before using them, Python's function scope rules mean this code works fine as is. The suggestion is purely stylistic. The code is contained within a small decorator function (dont_throw) and the organization is clear enough. This seems like a nitpicky comment that doesn't meaningfully improve the code. The current organization could slightly impact readability for developers scanning the code from top to bottom. Moving the function up would follow a more natural reading order. The code is contained within a small, focused decorator function where all pieces are easily visible. The minor readability benefit doesn't justify a code change. This comment should be deleted as it suggests a minor stylistic change that doesn't meaningfully improve the code.
2. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/utils.py:32
- Draft comment:
Verify if 'OpenLLMetry' in the log message is intentional or a typo (possibly meant 'OpenTelemetry'). - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_yY9G7y28O9hHZlUb
You can customize
|
|
||
| def _handle_exception(e, func, logger): | ||
| logger.debug( | ||
| "OpenLLMetry failed to trace in %s, error: %s", |
There was a problem hiding this comment.
Typographical error: It appears that the string 'OpenLLMetry' is misspelled. It should likely be 'OpenTelemetry' to match the intended instrumentation framework.
| "OpenLLMetry failed to trace in %s, error: %s", | |
| "OpenTelemetry failed to trace in %s, error: %s", |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/utils.py (1)
12-16: Decorator swallows cancellations and all errors; make swallowing configurable and preserve metadata.Swallowing all exceptions breaks cancellation semantics and can mask real client/server errors when used around production calls. Also missing functools.wraps.
Apply this diff to add a configurable swallow flag, re-raise cancellations, and preserve metadata:
@@ -import asyncio +import asyncio import logging import traceback +from functools import wraps +from typing import Any, Awaitable, Callable, Optional, TypeVar, cast @@ -class Config: - exception_logger = None +class Config: + exception_logger: Optional[Callable[[BaseException], None]] = None @@ -def dont_throw(func): - """ - A decorator that wraps the passed in function and logs exceptions instead of throwing them. - Works for both synchronous and asynchronous functions. - """ - logger = logging.getLogger(func.__module__) +F = TypeVar("F", bound=Callable[..., Any]) + +def dont_throw(func: Optional[F] = None, *, swallow: bool = True) -> F: + """ + Decorator (or decorator factory) that logs exceptions; optionally swallows them. + Works for both sync and async functions. If swallow=False, the exception is re-raised. + """ + def _decorate(f: F) -> F: + logger = logging.getLogger(f.__module__) @@ - async def async_wrapper(*args, **kwargs): - try: - return await func(*args, **kwargs) - except Exception as e: - _handle_exception(e, func, logger) + if asyncio.iscoroutinefunction(f): + @wraps(f) + async def async_wrapper(*args, **kwargs): + try: + return await cast(Callable[..., Awaitable[Any]], f)(*args, **kwargs) + except asyncio.CancelledError: + # Preserve cooperative cancellation semantics + raise + except Exception as e: # noqa: BLE001 + logger.debug("OpenLLMetry failed to trace in %s", f.__name__, exc_info=e) + _safe_call_exception_logger(e, logger) + if not swallow: + raise + return None + return cast(F, async_wrapper) + else: + @wraps(f) + def wrapper(*args, **kwargs): + try: + return f(*args, **kwargs) + except Exception as e: # noqa: BLE001 + logger.debug("OpenLLMetry failed to trace in %s", f.__name__, exc_info=e) + _safe_call_exception_logger(e, logger) + if not swallow: + raise + return None + return cast(F, wrapper) - def sync_wrapper(*args, **kwargs): - try: - return func(*args, **kwargs) - except Exception as e: - _handle_exception(e, func, logger) + return _decorate(func) if func is not None else cast(F, _decorate) - def _handle_exception(e, func, logger): - logger.debug( - "OpenLLMetry failed to trace in %s, error: %s", - func.__name__, - traceback.format_exc(), - ) - if Config.exception_logger: - Config.exception_logger(e) - - return async_wrapper if asyncio.iscoroutinefunction(func) else sync_wrapper +def _safe_call_exception_logger(e: BaseException, logger: logging.Logger) -> None: + if Config.exception_logger: + try: + Config.exception_logger(e) + except Exception: # noqa: BLE001 + logger.debug("Exception in Config.exception_logger", exc_info=True)
🧹 Nitpick comments (3)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/utils.py (3)
19-30: Silence BLE001 intentionally or narrow scope.Catching broad Exception is acceptable here to avoid instrumentation breaking apps, but ruff flags BLE001. Add the inline noqa with rationale (as in the diff) or further narrow exceptions if you can.
31-39: Avoid eager traceback formatting and hardening the callback.traceback.format_exc() is computed even when DEBUG is off. Use exc_info to defer formatting and guard exception_logger to avoid secondary failures. Covered in the diff above.
9-9: Type the exception logger hook.Annotate Config.exception_logger to Optional[Callable[[BaseException], None]] for clarity and linting. Included in the diff.
📜 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/utils.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/opentelemetry/instrumentation/mcp/utils.py
🪛 Ruff (0.12.2)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/utils.py
22-22: Do not catch blind exception: Exception
(BLE001)
28-28: Do not catch blind exception: Exception
(BLE001)
⏰ 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: Test Packages (3.11)
- GitHub Check: Build Packages (3.11)
- GitHub Check: Lint
| def dont_throw(func): | ||
| """ | ||
| A decorator that wraps the passed in function and logs exceptions instead of throwing them. | ||
| Works for both synchronous and asynchronous functions. | ||
| """ |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Follow-ups: verify callers that must not swallow real errors pass swallow=False.
Scan instrumentation wrappers (e.g., MCP client/server patch points) and set swallow=False where appropriate.
🏁 Script executed:
#!/bin/bash
# Find decorator usages and call sites
rg -nP '@dont_throw\b|dont_throw\s*\(' -C2
# Grep likely patch points to ensure swallow=False where needed
rg -nP 'patch_.*mcp|fastmcp|instrumentor|Instrumentor' -C3Length of output: 316107
MCP: Do not swallow protocol errors — make these wrappers propagate exceptions
Remove or change @dont_throw on protocol/transport patch points so errors can propagate (add swallow=False to the decorator API or remove the decorator). Key locations to update:
- packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py — traced_method (around line 156), aiter (around 408–410), send (around 454–456 and 518–521).
- packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/fastmcp_instrumentation.py — traced_method (around line 41).
🤖 Prompt for AI Agents
In
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/utils.py
around lines 12–16, the dont_throw decorator currently swallows all exceptions;
update its API and usage so protocol/transport errors are not suppressed. Change
the decorator to accept an optional swallow=False parameter (default preserving
current behavior) and, when swallow is False, re-raise exceptions after logging;
when True, log and suppress as now. Then update the listed patch points to
remove the decorator or call it with swallow=False: instrumentation.py
traced_method (~156), __aiter__ (~408–410), send (~454–456 and ~518–521), and
fastmcp_instrumentation.py traced_method (~41). Ensure async and sync paths both
re-raise when swallow is False and adjust tests/usages accordingly.
- Remove unused imports (asyncio, json, TextContent, Tool) from server - Fix unused variables in demo by removing assignment when result not used - Replace f-strings without placeholders with regular strings - Fix bare except clause to catch specific Exception - Break long function signature across multiple lines - Remove trailing whitespace and fix blank line formatting - Add missing newlines at end of files All 32 packages now pass lint checks with zero issues. 🤖 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 79297fd in 39 seconds. Click for details.
- Reviewed
528lines of code in2files - Skipped
0files when reviewing. - Skipped posting
5draft 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/sample-app/sample_app/mcp_dev_assistant_demo.py:73
- Draft comment:
Removed the unused assignment in tool calls (e.g. get_system_info) to simplify the code. Using 'await self.session.call_tool(...)' directly improves clarity since the result isn’t used. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. packages/sample-app/sample_app/mcp_dev_assistant_demo.py:159
- Draft comment:
The addition of a 'finally' clause calling cleanup() ensures that resources are properly released after the demo. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. packages/sample-app/sample_app/mcp_dev_assistant_server.py:7
- Draft comment:
Unused imports (e.g. asyncio, json) have been removed, which cleans up the code and reduces unnecessary dependencies. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. packages/sample-app/sample_app/mcp_dev_assistant_server.py:263
- Draft comment:
Switching from a bare 'except:' to 'except Exception:' in the git_status ahead/behind handling improves error clarity and avoids masking unexpected errors. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. packages/sample-app/sample_app/mcp_dev_assistant_server.py:280
- Draft comment:
Consistent whitespace and docstring formatting improvements in functions like search_code enhance readability and maintainability. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_1DSxQCLjLhKHKxYj
You can customize
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
packages/sample-app/sample_app/mcp_dev_assistant_server.py (4)
2-5: Fix typo: "OpenLLMetry" → "OpenTelemetry".Keep terminology consistent with the rest of the repo.
- A sample MCP server with useful development tools to demonstrate OpenLLMetry instrumentation. + A sample MCP server with useful development tools to demonstrate OpenTelemetry instrumentation.
351-366: Avoid leaking env vars in get_system_info; chain exceptions.Even with key-name filtering, secrets can leak. Remove by default.
return { "python_version": sys.version, "platform": platform.platform(), "architecture": platform.architecture(), "processor": platform.processor(), "python_executable": sys.executable, "current_directory": str(Path.cwd()), - "environment_variables": { - k: v for k, v in os.environ.items() - if not any(secret in k.lower() for secret in ['key', 'token', 'password', 'secret']) - } + # Intentionally exclude environment variables to avoid secret leakage + "environment_variables": {} } - except Exception as e: - raise RuntimeError(f"Failed to get system info: {str(e)}") + except Exception as e: + raise RuntimeError(f"Failed to get system info: {e}") from e
281-338: search_code typing and exception handling.Use Optional[List[str]], narrow inner except, and chain outer.
- file_extensions: List[str] = None, + file_extensions: Optional[List[str]] = None, @@ - except Exception: + except (OSError, UnicodeDecodeError): continue # Skip files that can't be read @@ - except Exception as e: - raise RuntimeError(f"Failed to search code: {str(e)}") + except Exception as e: + raise RuntimeError(f"Failed to search code: {e}") from e
7-13: Fix command-injection: remove shell=True in run_commandUse shlex.split and pass an argv list to subprocess.run with shell=False; validate/confine working_directory to the repo (or allowed base) and chain exceptions.
Location: packages/sample-app/sample_app/mcp_dev_assistant_server.py — run_command (lines 159–197; subprocess.run call at 174–181; exception handlers 194–197)
+import shlex @@ - result = subprocess.run( - command, - shell=True, - cwd=working_directory, + # Parse command safely and avoid shell injection + args = shlex.split(command) + # Confine working directory to repo root/current dir + base = Path.cwd().resolve() + wd = (Path(working_directory).resolve() if working_directory else base) + if base not in wd.parents and wd != base: + raise ValueError("working_directory is outside the allowed base path") + result = subprocess.run( + args, + shell=False, + cwd=str(wd), capture_output=True, text=True, timeout=timeout ) @@ - except subprocess.TimeoutExpired: - raise RuntimeError(f"Command timed out after {timeout} seconds") - except Exception as e: - raise RuntimeError(f"Failed to execute command: {str(e)}") + except subprocess.TimeoutExpired as e: + raise RuntimeError(f"Command timed out after {timeout} seconds") from e + except Exception as e: + raise RuntimeError(f"Failed to execute command: {e}") from epackages/sample-app/sample_app/mcp_dev_assistant_demo.py (3)
3-5: Fix typo: "OpenLLMetry" → "OpenTelemetry".-Demonstrates the enhanced OpenLLMetry instrumentation with tool-like spans. +Demonstrates the enhanced OpenTelemetry instrumentation with tool-like spans.
166-168: Fix remaining "OpenLLMetry" occurrences in user-facing prints.- print("This demo showcases the enhanced OpenLLMetry MCP instrumentation") + print("This demo showcases the enhanced OpenTelemetry MCP instrumentation")
118-136: Use a secure temp file instead of hardcoded /tmp; clean up afterwards.+import tempfile @@ - try: + try: - # Write a test file - await self.session.call_tool("write_file", { - "file_path": "/tmp/mcp_test.txt", + fd, tmp_path = tempfile.mkstemp(prefix="mcp_demo_", suffix=".txt") + os.close(fd) + # Write a test file + await self.session.call_tool("write_file", { + "file_path": tmp_path, "content": "Hello from MCP Development Assistant!\nThis is a test file created during the demo." }) print("Test file created successfully") # Read it back await self.session.call_tool("read_file", { - "file_path": "/tmp/mcp_test.txt", + "file_path": tmp_path, "max_lines": 5 }) print("Test file read successfully") except Exception as e: print(f"File operations failed: {e}") + finally: + try: + Path(tmp_path).unlink(missing_ok=True) # type: ignore[name-defined] + except Exception: + pass
🧹 Nitpick comments (8)
packages/sample-app/sample_app/mcp_dev_assistant_server.py (6)
59-91: Tighten exceptions and preserve traceback in list_files; move return to else.Avoid blind except, chain errors, and use else after successful try.
try: path = Path(directory) if not path.exists(): raise FileNotFoundError(f"Directory {directory} does not exist") files = [] for item in path.iterdir(): stat = item.stat() files.append(FileInfo( name=item.name, size=stat.st_size, modified=datetime.fromtimestamp(stat.st_mtime).isoformat(), is_directory=item.is_dir() )) # Sort directories first, then files alphabetically - files.sort(key=lambda x: (not x.is_directory, x.name.lower())) - return files - - except Exception as e: - raise RuntimeError(f"Failed to list files: {str(e)}") + files.sort(key=lambda x: (not x.is_directory, x.name.lower())) + except FileNotFoundError: + raise + except (PermissionError, OSError) as e: + raise RuntimeError(f"Failed to list files: {e}") from e + except Exception as e: + raise RuntimeError("Failed to list files") from e + else: + return files
105-126: Handle max_lines=0 correctly and chain exceptions in read_file.Truthiness check misbehaves for 0; also avoid blind except.
- with open(path, 'r', encoding='utf-8') as f: - if max_lines: + if max_lines is not None and max_lines < 0: + raise ValueError("max_lines must be non-negative") + with open(path, 'r', encoding='utf-8') as f: + if max_lines is not None: lines = [] for i, line in enumerate(f): if i >= max_lines: break lines.append(line.rstrip('\n')) return '\n'.join(lines) else: return f.read() - except Exception as e: - raise RuntimeError(f"Failed to read file: {str(e)}") + except FileNotFoundError: + raise + except (UnicodeDecodeError, OSError) as e: + raise RuntimeError(f"Failed to read file: {e}") from e + except Exception as e: + raise RuntimeError("Failed to read file") from e
141-155: Preserve traceback and use else for return in write_file.Avoid blind except and return from else.
try: path = Path(file_path) # Create parent directories if they don't exist path.parent.mkdir(parents=True, exist_ok=True) mode = 'a' if append else 'w' with open(path, mode, encoding='utf-8') as f: f.write(content) - stat = path.stat() - return f"Successfully {'appended to' if append else 'wrote'} {file_path} ({stat.st_size} bytes)" - - except Exception as e: - raise RuntimeError(f"Failed to write file: {str(e)}") + stat = path.stat() + except (OSError, IOError) as e: + raise RuntimeError(f"Failed to write file: {e}") from e + except Exception as e: + raise RuntimeError("Failed to write file") from e + else: + return f"Successfully {'appended to' if append else 'wrote'} {file_path} ({stat.st_size} bytes)"
254-266: Narrow the inner except around ahead/behind or log at debug level.Silently swallowing all exceptions hides real issues (e.g., missing upstream). Consider catching CalledProcessError or logging.
375-383: Avoid manual tool list; derive from server if API allows.Manual lists drift. If FastMCP exposes a registry, enumerate it; otherwise keep this section minimal or gate behind debug prints.
1-1: Shebang present but file may not be executable.Either make the file executable (chmod +x) or drop the shebang to satisfy linters.
packages/sample-app/sample_app/mcp_dev_assistant_demo.py (2)
70-77: Narrow overly broad except clauses or annotate intentionally.Catching Exception everywhere hides real errors. Prefer specific exceptions or add a noqa for BLE001 where broad catch is intentional in the demo.
Also applies to: 80-85, 88-98, 101-106, 109-117, 155-159
1-1: Shebang present but file may not be executable.Either chmod +x or remove the shebang.
📜 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 (2)
packages/sample-app/sample_app/mcp_dev_assistant_demo.py(1 hunks)packages/sample-app/sample_app/mcp_dev_assistant_server.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/sample-app/sample_app/mcp_dev_assistant_demo.pypackages/sample-app/sample_app/mcp_dev_assistant_server.py
🧠 Learnings (1)
📚 Learning: 2025-08-17T15:06:48.109Z
Learnt from: CR
PR: traceloop/openllmetry#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-17T15:06:48.109Z
Learning: For debugging OpenTelemetry spans, use ConsoleSpanExporter with Traceloop to print spans to console
Applied to files:
packages/sample-app/sample_app/mcp_dev_assistant_demo.py
🧬 Code graph analysis (2)
packages/sample-app/sample_app/mcp_dev_assistant_demo.py (3)
packages/traceloop-sdk/traceloop/sdk/__init__.py (2)
Traceloop(37-275)init(49-206)packages/traceloop-sdk/traceloop/sdk/decorators/__init__.py (1)
tool(63-73)packages/sample-app/sample_app/mcp_dev_assistant_server.py (1)
main(368-385)
packages/sample-app/sample_app/mcp_dev_assistant_server.py (2)
packages/traceloop-sdk/traceloop/sdk/__init__.py (2)
Traceloop(37-275)init(49-206)packages/traceloop-sdk/traceloop/sdk/decorators/__init__.py (1)
tool(63-73)
🪛 Ruff (0.12.2)
packages/sample-app/sample_app/mcp_dev_assistant_demo.py
1-1: Shebang is present but file is not executable
(EXE001)
38-38: Avoid specifying long messages outside the exception class
(TRY003)
75-75: Do not catch blind exception: Exception
(BLE001)
83-83: Do not catch blind exception: Exception
(BLE001)
96-96: Do not catch blind exception: Exception
(BLE001)
104-104: Do not catch blind exception: Exception
(BLE001)
115-115: Do not catch blind exception: Exception
(BLE001)
123-123: Probable insecure usage of temporary file or directory: "/tmp/mcp_test.txt"
(S108)
130-130: Probable insecure usage of temporary file or directory: "/tmp/mcp_test.txt"
(S108)
135-135: Do not catch blind exception: Exception
(BLE001)
157-157: Do not catch blind exception: Exception
(BLE001)
packages/sample-app/sample_app/mcp_dev_assistant_server.py
1-1: Shebang is present but file is not executable
(EXE001)
73-73: Abstract raise to an inner function
(TRY301)
73-73: Avoid specifying long messages outside the exception class
(TRY003)
87-87: Consider moving this statement to an else block
(TRY300)
89-89: Do not catch blind exception: Exception
(BLE001)
90-90: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
90-90: Avoid specifying long messages outside the exception class
(TRY003)
90-90: Use explicit conversion flag
Replace with conversion flag
(RUF010)
108-108: Abstract raise to an inner function
(TRY301)
108-108: Avoid specifying long messages outside the exception class
(TRY003)
111-111: Abstract raise to an inner function
(TRY301)
111-111: Avoid specifying long messages outside the exception class
(TRY003)
124-124: Do not catch blind exception: Exception
(BLE001)
125-125: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
125-125: Avoid specifying long messages outside the exception class
(TRY003)
125-125: Use explicit conversion flag
Replace with conversion flag
(RUF010)
152-152: Consider moving this statement to an else block
(TRY300)
154-154: Do not catch blind exception: Exception
(BLE001)
155-155: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
155-155: Avoid specifying long messages outside the exception class
(TRY003)
155-155: Use explicit conversion flag
Replace with conversion flag
(RUF010)
174-174: subprocess call with shell=True identified, security issue
(S602)
195-195: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
195-195: Avoid specifying long messages outside the exception class
(TRY003)
196-196: Do not catch blind exception: Exception
(BLE001)
197-197: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
197-197: Avoid specifying long messages outside the exception class
(TRY003)
197-197: Use explicit conversion flag
Replace with conversion flag
(RUF010)
214-214: Abstract raise to an inner function
(TRY301)
214-214: Avoid specifying long messages outside the exception class
(TRY003)
218-218: Starting a process with a partial executable path
(S607)
227-227: Starting a process with a partial executable path
(S607)
254-254: subprocess call: check for execution of untrusted input
(S603)
255-255: Starting a process with a partial executable path
(S607)
264-265: try-except-pass detected, consider logging the exception
(S110)
264-264: Do not catch blind exception: Exception
(BLE001)
276-276: Do not catch blind exception: Exception
(BLE001)
277-277: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
277-277: Avoid specifying long messages outside the exception class
(TRY003)
277-277: Use explicit conversion flag
Replace with conversion flag
(RUF010)
284-284: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
305-305: Abstract raise to an inner function
(TRY301)
305-305: Avoid specifying long messages outside the exception class
(TRY003)
331-332: try-except-continue detected, consider logging the exception
(S112)
331-331: Do not catch blind exception: Exception
(BLE001)
334-334: Consider moving this statement to an else block
(TRY300)
336-336: Do not catch blind exception: Exception
(BLE001)
337-337: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
337-337: Avoid specifying long messages outside the exception class
(TRY003)
337-337: Use explicit conversion flag
Replace with conversion flag
(RUF010)
364-364: Do not catch blind exception: Exception
(BLE001)
365-365: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
365-365: Avoid specifying long messages outside the exception class
(TRY003)
365-365: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ 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: Test Packages (3.11)
- GitHub Check: Lint
- GitHub Check: Build Packages (3.11)
🔇 Additional comments (2)
packages/sample-app/sample_app/mcp_dev_assistant_server.py (1)
276-278: Preserve traceback in git_status.
[ suggest_nitpick_refactor ]- except Exception as e: - raise RuntimeError(f"Failed to get Git status: {str(e)}") + except Exception as e: + raise RuntimeError(f"Failed to get Git status: {e}") from epackages/sample-app/sample_app/mcp_dev_assistant_demo.py (1)
25-30: ConsoleSpanExporter usage looks good for debugging spans.Nice alignment with our prior learning to use ConsoleSpanExporter with Traceloop for local debugging.
3 participants
feat(instrumentation): ...orfix(instrumentation): ....Important
Enhances FastMCP instrumentation with improved trace-context propagation, error handling, and comprehensive testing, including a demo application.
FastMCPInstrumentorinfastmcp_instrumentation.pyfor tool/resource spans.instrumentation.py.test_fastmcp.pyandtest_fastmcp_attributes.pyfor integration and attribute tests.InMemorySpanExporterinconftest.pyfor testing.mcp_dev_assistant_demo.pyandmcp_dev_assistant_server.pyfor demoing enhanced instrumentation.pyproject.tomlto includefastmcp.This description was created by
for 79297fd. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Chores