feat(mcp): MCP response Span Capture for Stdio Mode #3383
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 instrumentation for Changes
Sequence Diagram(s)sequenceDiagram
participant Server
participant JSONResp as JSONRPCResponse.__init__ (wrapped)
participant Tracer
Server->>JSONResp: instantiate(content, isError, id)
activate JSONResp
JSONResp->>Tracer: start span "MCP_Tool_Response"
JSONResp->>JSONResp: determine result_value (kwargs or args), serialize content
JSONResp->>Tracer: set attributes (MCP_RESPONSE_VALUE, MCP_REQUEST_ID)
alt isError true
JSONResp->>Tracer: set span status ERROR
else
JSONResp->>Tracer: set span status OK
end
JSONResp->>Server: return original __init__ result
deactivate JSONResp
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 97fe5df in 2 minutes and 16 seconds. Click for details.
- Reviewed
194lines of code in1files - 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/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:4
- Draft comment:
Removal of the logging import and logging in exception handlers reduces available debug info. Consider adding minimal debug-level logging to help troubleshoot instrumentation issues. - 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 removed logging was in exception handlers that are part of instrumentation code. The exceptions are caught and handled gracefully - the code continues working by yielding the original result. The logging wasn't providing critical information, just noting that an exception occurred. Since this is instrumentation code, silent failure is acceptable - we don't want instrumentation issues to impact the main application flow. The logging could help identify instrumentation configuration issues or bugs. Without any logging, problems might be harder to track down. The exceptions occur in non-critical instrumentation paths and the code handles them gracefully. Adding logging here would add noise without providing much value, since the exceptions don't indicate actual problems. The logging removal is fine - it removes unnecessary noise from exception handlers that gracefully handle non-critical instrumentation issues. The comment should be deleted.
2. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:139
- Draft comment:
Swallowing exceptions in the _transport_wrapper without any logging may hinder diagnosing issues. Consider logging exceptions at a debug or trace level. - 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.
3. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:535
- Draft comment:
Changing ItemWithContext to a simple @DataClass (removing slots and frozen) might affect performance and immutability guarantees. Please document the rationale for this change. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to document the rationale for a change, which is not allowed according to the rules. It also implies a request for confirmation of intention, which is also not allowed. The comment does not provide a specific code suggestion or ask for a specific test to be written.
4. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:119
- Draft comment:
Typo: The client unwrap call uses "streamablehttp_client", which appears inconsistent with the module name 'streamable_http'. Consider renaming this to "streamable_http_client". - 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 comment suggests renaming "streamablehttp_client" to "streamable_http_client" to match the module name style. However, this appears to be a consistent naming pattern in the code. The function name is used correctly in both instrument and uninstrument calls. Making this change would be a purely stylistic change with no functional benefit, and could introduce bugs if not changed everywhere consistently. I could be wrong about this being intentional - maybe it really is an inconsistency that should be fixed. The module uses underscores while the function doesn't. Even if it is technically inconsistent, this kind of minor stylistic change has more risk than benefit. The code works as is, and changing function names can be risky. This isn't a clear enough issue to warrant a comment. Delete this comment. While there is a minor stylistic inconsistency, it's not worth the risk of changing working code just for naming style consistency.
5. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:123
- Draft comment:
Potential inconsistency: The unwrap argument is "JSONRPCResponse.init" but the commit message refers to "JSONRpcResponse" (with a lowercase 'p'). Please confirm the correct casing for consistency. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_3mL1DAJn47cGIvJx
You can customize 
| with tracer.start_as_current_span("MCP_Tool_Response") as span: | ||
| # Serialize the result data | ||
| result_serialized = serialize(result_value) | ||
| span.set_attribute(SpanAttributes.MCP_RESPONSE_VALUE, f"{result_serialized}") |
There was a problem hiding this comment.
In _jsonrpc_response_init_wrapper, wrapping the serialized result in an f-string is redundant, and using args[0] as a fallback for id_value assumes a specific init signature. Please document or validate the expected parameter order.
| span.set_attribute(SpanAttributes.MCP_RESPONSE_VALUE, f"{result_serialized}") | |
| span.set_attribute(SpanAttributes.MCP_RESPONSE_VALUE, result_serialized) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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)
276-304: Improve error handling in _execute_and_handle_result method.The method accesses
result.content[0].textwithout checking if the content list is non-empty or if the first item has atextattribute.Add proper validation before accessing nested attributes:
async def _execute_and_handle_result(self, span, method, args, kwargs, wrapped, clean_output=False): """Execute the wrapped function and handle the result""" try: result = await wrapped(*args, **kwargs) # Add output if clean_output: clean_output_data = self._extract_clean_output(method, result) if clean_output_data: try: span.set_attribute(SpanAttributes.TRACELOOP_ENTITY_OUTPUT, json.dumps(clean_output_data)) except (TypeError, ValueError): span.set_attribute(SpanAttributes.TRACELOOP_ENTITY_OUTPUT, str(clean_output_data)) else: span.set_attribute(SpanAttributes.TRACELOOP_ENTITY_OUTPUT, serialize(result)) # Handle errors if hasattr(result, "isError") and result.isError: - if len(result.content) > 0: - span.set_status(Status(StatusCode.ERROR, f"{result.content[0].text}")) - error_type = get_error_type(result.content[0].text) + if hasattr(result, "content") and result.content and len(result.content) > 0: + error_msg = "" + if hasattr(result.content[0], "text"): + error_msg = result.content[0].text + elif hasattr(result.content[0], "__str__"): + error_msg = str(result.content[0]) + + if error_msg: + span.set_status(Status(StatusCode.ERROR, error_msg)) + error_type = get_error_type(error_msg) if error_type is not None: span.set_attribute(ERROR_TYPE, error_type) + else: + span.set_status(Status(StatusCode.ERROR, "Unknown error")) else: span.set_status(Status(StatusCode.OK)) return result except Exception as e: span.set_attribute(ERROR_TYPE, type(e).__name__) span.record_exception(e) span.set_status(Status(StatusCode.ERROR, str(e))) raise
🧹 Nitpick comments (5)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (5)
91-107: Consider improving the fallback mechanism for JSONRPCResponse wrapping.The current implementation tries direct wrapping first and falls back to a post-import hook on exception. While this approach works, catching all exceptions might hide legitimate errors that should be surfaced.
Consider checking for the specific import/attribute error and only falling back in those cases:
- # Try multiple response creation points - # Try direct wrapping instead of post-import hook - try: - wrap_function_wrapper( - "mcp.types", - "JSONRPCResponse.__init__", - self._jsonrpc_response_init_wrapper(tracer), - ) - except Exception: - # Fallback to post-import hook - register_post_import_hook( - lambda _: wrap_function_wrapper( - "mcp.types", - "JSONRPCResponse.__init__", - self._jsonrpc_response_init_wrapper(tracer), - ), - "mcp.types", - ) + # Try wrapping JSONRPCResponse.__init__ with fallback to post-import hook + try: + wrap_function_wrapper( + "mcp.types", + "JSONRPCResponse.__init__", + self._jsonrpc_response_init_wrapper(tracer), + ) + except (ImportError, AttributeError): + # Module not yet imported or class not yet available, use post-import hook + register_post_import_hook( + lambda _: wrap_function_wrapper( + "mcp.types", + "JSONRPCResponse.__init__", + self._jsonrpc_response_init_wrapper(tracer), + ), + "mcp.types", + )
149-152: Avoid silently catching all exceptions in the transport wrapper.Catching all exceptions with bare
except Exception:can hide legitimate errors and make debugging difficult. The transport wrapper should either handle specific exceptions or at least log when unexpected exceptions occur.Consider handling specific exceptions or adding logging:
try: read_stream, write_stream, get_session_id_callback = result yield InstrumentedStreamReader( read_stream, tracer ), InstrumentedStreamWriter(write_stream, tracer), get_session_id_callback - except Exception: + except (ValueError, TypeError, AttributeError): + # Not a 3-tuple, yield as-is yield result - except Exception: + except (ValueError, TypeError): + # Not a 2-tuple, yield as-is yield result
252-253: Remove empty except block or add logging.The empty except block at Line 252-253 silently swallows exceptions, making debugging difficult.
Either remove the try-except if the code is expected to work, or add minimal logging:
elif hasattr(params, "__dict__") and "name" in params.__dict__: entity_name = params.__dict__["name"] span_name = f"{params.__dict__['name']}.tool" - except Exception: - pass + except AttributeError: + # Use default method name if params structure is unexpected + pass
345-346: Inconsistent empty except blocks in extraction methods.Both
_extract_clean_inputand_extract_clean_outputhave empty except blocks that return empty dicts. This could hide actual errors.Consider being more specific about which exceptions are expected:
- except Exception: + except (AttributeError, TypeError, ValueError): + # Unable to extract clean input/output, return empty dict return {}Also applies to: 391-392
554-584: Consider reducing span nesting complexity.The
ContextSavingStreamWriter.sendmethod creates two nested spans (main MCP span and RequestStreamWriter span). This might create overly complex traces.Consider whether both spans are necessary, or if the attributes could be combined into a single span:
@dont_throw async def send(self, item: Any) -> Any: # Extract method name for main span method_name = "unknown" if hasattr(item, "request") and hasattr(item.request, "root") and hasattr(item.request.root, "method"): method_name = item.request.root.method - # Create main MCP span first + # Create MCP span with all relevant attributes main_span_name = f"{method_name}.mcp" with self._tracer.start_as_current_span(main_span_name) as main_span: main_span.set_attribute(SpanAttributes.TRACELOOP_ENTITY_NAME, method_name) main_span.set_attribute(SpanAttributes.TRACELOOP_SPAN_KIND, TraceloopSpanKindValues.WORKFLOW.value) - - with self._tracer.start_as_current_span("RequestStreamWriter") as span: - if hasattr(item, "request_id"): - span.set_attribute(SpanAttributes.MCP_REQUEST_ID, f"{item.request_id}") - if hasattr(item, "request"): - if hasattr(item.request, "root"): - if hasattr(item.request.root, "method"): - span.set_attribute( - SpanAttributes.MCP_METHOD_NAME, - f"{item.request.root.method}", - ) - if hasattr(item.request.root, "params"): - span.set_attribute( - SpanAttributes.MCP_REQUEST_ARGUMENT, - f"{serialize(item.request.root.params)}", - ) - - ctx = context.get_current() - item_with_context = ItemWithContext(item=item, ctx=ctx) - return await self.__wrapped__.send(item_with_context) + + if hasattr(item, "request_id"): + main_span.set_attribute(SpanAttributes.MCP_REQUEST_ID, f"{item.request_id}") + if hasattr(item, "request") and hasattr(item.request, "root"): + if hasattr(item.request.root, "method"): + main_span.set_attribute( + SpanAttributes.MCP_METHOD_NAME, + f"{item.request.root.method}", + ) + if hasattr(item.request.root, "params"): + main_span.set_attribute( + SpanAttributes.MCP_REQUEST_ARGUMENT, + f"{serialize(item.request.root.params)}", + ) + + ctx = context.get_current() + item_with_context = ItemWithContext(item=item, ctx=ctx) + return await self.__wrapped__.send(item_with_context)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Store API keys only in environment variables/secure vaults; never hardcode secrets in code
Use Flake8 for code linting and adhere to its rules
Files:
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py
🧬 Code graph analysis (1)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (1)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/utils.py (1)
dont_throw(12-40)
🪛 Ruff (0.12.2)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py
98-98: Do not catch blind exception: Exception
(BLE001)
149-149: Do not catch blind exception: Exception
(BLE001)
151-151: Do not catch blind exception: Exception
(BLE001)
179-179: Unused function argument: instance
(ARG001)
⏰ 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.11)
- GitHub Check: Test Packages (3.12)
- GitHub Check: Test Packages (3.10)
- GitHub Check: Build Packages (3.11)
- GitHub Check: Lint
🔇 Additional comments (2)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (2)
535-538: Confirm the removal of frozen and slots from ItemWithContext dataclass.The dataclass decorator was changed from
@dataclass(slots=True, frozen=True)to just@dataclass. This makes theItemWithContextmutable and removes memory optimization.Was this change intentional? The frozen and slots attributes provide immutability guarantees and memory efficiency. If mutability is required, please confirm this is necessary for the new implementation.
524-532: Fix issue with modifying params on non-JSONRPCRequest items.The code modifies
request.paramswithout checking if it's aJSONRPCRequestfirst. This could cause issues if the request object doesn't support the expected operations.Apply this fix to ensure params are only modified for JSONRPCRequest objects:
- if not isinstance(request, JSONRPCRequest): - return await self.__wrapped__.send(item) - meta = None - if not request.params: - request.params = {} - meta = request.params.setdefault("_meta", {}) - - propagate.get_global_textmap().inject(meta) - return await self.__wrapped__.send(item) + if isinstance(request, JSONRPCRequest): + if not request.params: + request.params = {} + meta = request.params.setdefault("_meta", {}) + propagate.get_global_textmap().inject(meta) + + return await self.__wrapped__.send(item)Likely an incorrect or invalid review comment.
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (2)
143-146: Duplicate comment - Use more specific exception handling.These blind exception catches should also be replaced with more specific exception types for better error diagnosis. The logging was removed from the transport wrapper making it harder to debug issues.
171-202: Duplicate comment - Remove unused parameter and improve argument extraction logic.The
instanceparameter is unused and the argument extraction logic could be more robust as noted in the past review. Additionally, Line 182 has an unnecessary f-string wrapper around the serialized result.
🧹 Nitpick comments (1)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (1)
529-529: Consider using frozen=True for dataclass to maintain immutability.The dataclass decorator was changed from
@dataclass(slots=True, frozen=True)to@dataclass, removing the immutability constraint. For context objects that carry tracing information, immutability helps prevent accidental modification that could break trace propagation.Apply this diff to restore immutability:
-@dataclass +@dataclass(frozen=True) class ItemWithContext: item: Any ctx: context.Context
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py(6 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-instrumentation-mcp/opentelemetry/instrumentation/mcp/utils.py (1)
dont_throw(12-40)packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py (1)
SpanAttributes(64-261)
🪛 Ruff (0.12.2)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py
98-98: Do not catch blind exception: Exception
(BLE001)
143-143: Do not catch blind exception: Exception
(BLE001)
145-145: Do not catch blind exception: Exception
(BLE001)
173-173: Unused function argument: instance
(ARG001)
⏰ 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.11)
- GitHub Check: Test Packages (3.10)
- GitHub Check: Build Packages (3.11)
- GitHub Check: Test Packages (3.12)
- GitHub Check: Lint
🔇 Additional comments (2)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (2)
549-552: ItemWithContext wrapping verified — no compatibility issueItemWithContext is defined in the same module and the ResponseStreamWriter wraps items at packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:549–552 while ContextAttachingStreamReader casts/unpacks them at .../instrumentation.py:566–572; ripgrep found no other usages — change is internally consistent.
518-526: Verify JSONRPCRequest.params is writable and dict-like before using setdefault
- Scan: no JSONRPCRequest definition found in this repo (it's external), so .params mutability cannot be confirmed here.
- Location: packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (≈ lines 518–526).
- Action: confirm mcp.types.JSONRPCRequest.params is a writable dict for supported mcp versions; otherwise make the code defensive (example approach):
- params = getattr(request, "params", None)
- if not isinstance(params, dict): try assigning request.params = dict(params) or {} inside try/except (handle AttributeError/TypeError)
- then use meta = request.params.setdefault("_meta", {}) and inject; if assignment fails, skip propagation or use an alternative channel.
There was a problem hiding this comment.
Actionable comments posted: 1
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)
248-293: Remove @dont_throw from wrappers that invoke the wrapped callable — it swallows real exceptionsdont_throw is applied to enter/exit, stream reader/writer, and client wrappers that call wrapped(*args, **kwargs), which can absorb library exceptions; remove the decorator from those wrappers, ensure spans record exceptions, and re-raise so original error semantics are preserved.
Affected: packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py — lines 248–293, 518–550, 564–587, 608–614
I can open a focused PR to remove these decorators and keep span/error recording — proceed?
♻️ Duplicate comments (3)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (3)
318-345: Avoid redundant f-strings around serialized JSON.
serialize(...)already returns a string. Wrapping with f"" is unnecessary and can double-quote simple strings.- span.set_attribute(SpanAttributes.TRACELOOP_ENTITY_INPUT, f"{serialize(args[0])}") + span.set_attribute(SpanAttributes.TRACELOOP_ENTITY_INPUT, serialize(args[0]))(Already addressed similarly in the JSONRPCResponse wrapper diff.)
Also applies to: 479-504
186-218: Critical: dont_throw here will swallow exceptions from JSONRPCResponse.init.If the wrapped init fails, the decorator suppresses it, risking inconsistent state. Also remove redundant f-strings and unused parameter warning.
- def _jsonrpc_response_init_wrapper(self, tracer): - @dont_throw - def traced_method(wrapped, instance, args, kwargs): + def _jsonrpc_response_init_wrapper(self, tracer): + def traced_method(wrapped, _, args, kwargs): result_value = kwargs.get("result", None) if result_value is None and len(args) > 1: result_value = args[1] if result_value is not None and isinstance(result_value, dict) and "content" in result_value: with tracer.start_as_current_span("MCP_Tool_Response") as span: # Serialize the result data result_serialized = serialize(result_value) - span.set_attribute(SpanAttributes.MCP_RESPONSE_VALUE, f"{result_serialized}") + span.set_attribute(SpanAttributes.MCP_RESPONSE_VALUE, result_serialized) # Set span status if result_value.get("isError", False): span.set_status(Status(StatusCode.ERROR, "Tool execution error")) else: span.set_status(Status(StatusCode.OK)) # Add request ID if available - id_value = kwargs.get("id", None) - if id_value is None and len(args) > 0: + id_value = kwargs.get("id") + if id_value is None and args: id_value = args[0] if id_value is not None: - span.set_attribute(SpanAttributes.MCP_REQUEST_ID, f"{id_value}") + span.set_attribute(SpanAttributes.MCP_REQUEST_ID, str(id_value)) # Call the original method result = wrapped(*args, **kwargs) return result
186-218: Fix positional fallback for JSONRPCResponse.initmcp >= 1.6.0 JSONRPCResponse signature is (jsonrpc, id, result); current code reads result from args[1] and id from args[0], which misbinds positional calls. Update to bind parameters (inspect.signature) or at minimum use the correct positional index.
File: packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py Lines 186-218
+ import inspect + try: + bound = inspect.signature(wrapped).bind_partial(*args, **kwargs) + result_value = bound.arguments.get("result", result_value) + id_value = bound.arguments.get("id", None) + except Exception: + pass
🧹 Nitpick comments (7)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (7)
105-122: Narrow the blanket exception when wrapping JSONRPCResponse.init.Catching all exceptions hides real errors and triggers BLE001. Only fall back on import/attribute resolution failures.
- try: + try: wrap_function_wrapper( "mcp.types", "JSONRPCResponse.__init__", self._jsonrpc_response_init_wrapper(tracer), ) - except Exception: + except (ImportError, AttributeError): # Fallback to post-import hook register_post_import_hook( lambda _: wrap_function_wrapper( "mcp.types", "JSONRPCResponse.__init__", self._jsonrpc_response_init_wrapper(tracer), ), "mcp.types", )
146-161: Avoid blind catches in transport wrapper.These catches may mask programming errors and trigger BLE001. Limit to tuple-unpack/type issues.
- except Exception: + except (TypeError, AttributeError): yield result - except Exception: + except (TypeError, AttributeError): yield result
310-323: Set MCP attributes on tool spans per PR objective.Populate method and arguments to aid correlation.
with tracer.start_as_current_span(span_name) as span: # Set tool-specific attributes span.set_attribute(SpanAttributes.TRACELOOP_SPAN_KIND, TraceloopSpanKindValues.TOOL.value) span.set_attribute(SpanAttributes.TRACELOOP_ENTITY_NAME, entity_name) + span.set_attribute(SpanAttributes.MCP_METHOD_NAME, method) + try: + args_payload = None + if params: + if hasattr(params, "arguments"): + args_payload = params.arguments + elif hasattr(params, "__dict__") and "arguments" in params.__dict__: + args_payload = params.__dict__["arguments"] + if args_payload is not None: + span.set_attribute(SpanAttributes.MCP_REQUEST_ARGUMENT, serialize(args_payload)) + except Exception: + pass
325-330: Also set MCP attributes on non-tool method spans.Aligns with stated behavior changes.
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])}") + span.set_attribute(SpanAttributes.MCP_METHOD_NAME, method) + span.set_attribute(SpanAttributes.MCP_REQUEST_ARGUMENT, serialize(args[0])) + span.set_attribute(SpanAttributes.TRACELOOP_ENTITY_INPUT, serialize(args[0])) return await self._execute_and_handle_result(span, method, args, kwargs, wrapped, clean_output=False)
568-587: Safer typing checks, remove unused var, and avoid masking send errors.
- Prefer isinstance over type(...) is ...
- Drop unnecessary meta pre-init.
- Remove dont_throw on this wrapper (see separate comment), or at minimum ensure it doesn’t swallow transport errors.
- @dont_throw async def send(self, item: Any) -> Any: from mcp.types import JSONRPCMessage, JSONRPCRequest @@ - elif type(item) is JSONRPCMessage: + elif isinstance(item, JSONRPCMessage): request = cast(JSONRPCMessage, item).root @@ - if not isinstance(request, JSONRPCRequest): + if not isinstance(request, JSONRPCRequest): return await self.__wrapped__.send(item) - meta = None - if not request.params: + if not request.params: request.params = {} - meta = request.params.setdefault("_meta", {}) + meta = request.params.setdefault("_meta", {}) propagate.get_global_textmap().inject(meta) return await self.__wrapped__.send(item)
590-594: Restore immutability/slots on ItemWithContext.Previous frozen/slots likely reduced churn and improved safety when passing context through streams.
-@dataclass +@dataclass(slots=True, frozen=True) class ItemWithContext: item: Any ctx: context.Context
105-122: Tests missing for new JSONRPCResponse wrapping and context propagation.Please add unit/integration tests:
- Verifies span creation on JSONRPCResponse with content/isError and request id.
- Ensures request _meta gets traceparent injected and is restored on the server.
- Confirms exceptions from wrapped functions propagate (no swallowing).
I can draft pytest tests with minimal MCP stubs to validate these paths. Want me to scaffold them?
Also applies to: 186-218, 568-587, 608-614
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py(6 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 (3)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/utils.py (1)
dont_throw(12-40)packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/fastmcp_instrumentation.py (1)
traced_method(42-121)packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py (1)
SpanAttributes(64-261)
🪛 Ruff (0.12.2)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py
113-113: Do not catch blind exception: Exception
(BLE001)
158-158: Do not catch blind exception: Exception
(BLE001)
160-160: Do not catch blind exception: Exception
(BLE001)
188-188: Unused function argument: instance
(ARG001)
⏰ 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.11)
- GitHub Check: Test Packages (3.10)
- GitHub Check: Test Packages (3.12)
- GitHub Check: Build Packages (3.11)
- GitHub Check: Lint
🔇 Additional comments (2)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (2)
132-132: Good: unwrapping JSONRPCResponse.init during uninstrument.This closes the loop for the new wrapper and keeps uninstrumentation idempotent.
219-247: Remove dont_throw and add MCP span attributesdont_throw suppresses BaseSession.send_request exceptions — remove it from patch_mcp_client.traced_method and ensure spans set MCP_METHOD_NAME and MCP_REQUEST_ARGUMENT in _handle_tool_call and _handle_mcp_method.
File: packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py
- 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 @@ - if method == "tools/call": - return await self._handle_tool_call(tracer, method, params, args, kwargs, wrapped) - else: - return await self._handle_mcp_method(tracer, method, args, kwargs, wrapped) + if method == "tools/call": + return await self._handle_tool_call(tracer, method, params, args, kwargs, wrapped) + else: + return await self._handle_mcp_method(tracer, method, args, kwargs, wrapped)Confirm no residual @dont_throw wrappers around calls to wrapped( by running and pasting output of:
rg -nP -C2 '(?s)@dont_throw\s*\n\sasync?\s+def\s+\w+(.):.?await?\s+wrapped\s('
| @dont_throw | ||
| async def send(self, item: Any) -> Any: | ||
| # Removed RequestStreamWriter span creation - we don't need low-level protocol spans | ||
| ctx = context.get_current() | ||
| return await self.__wrapped__.send(ItemWithContext(item, ctx)) | ||
| # Create ResponseStreamWriter span for server-side responses | ||
| with self._tracer.start_as_current_span("ResponseStreamWriter") as _: | ||
| ctx = context.get_current() | ||
| return await self.__wrapped__.send(ItemWithContext(item, ctx)) | ||
|
|
There was a problem hiding this comment.
Don’t suppress transport errors in ContextSavingStreamWriter.send.
The decorator will swallow failures from the underlying writer. Remove it; keep simple span creation.
- @dont_throw
async def send(self, item: Any) -> Any:
- # Create ResponseStreamWriter span for server-side responses
- with self._tracer.start_as_current_span("ResponseStreamWriter") as _:
- ctx = context.get_current()
- return await self.__wrapped__.send(ItemWithContext(item, ctx))
+ # Create ResponseStreamWriter span for server-side responses
+ with self._tracer.start_as_current_span("ResponseStreamWriter"):
+ ctx = context.get_current()
+ return await self.__wrapped__.send(ItemWithContext(item, ctx))📝 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 send(self, item: Any) -> Any: | |
| # Removed RequestStreamWriter span creation - we don't need low-level protocol spans | |
| ctx = context.get_current() | |
| return await self.__wrapped__.send(ItemWithContext(item, ctx)) | |
| # Create ResponseStreamWriter span for server-side responses | |
| with self._tracer.start_as_current_span("ResponseStreamWriter") as _: | |
| ctx = context.get_current() | |
| return await self.__wrapped__.send(ItemWithContext(item, ctx)) | |
| async def send(self, item: Any) -> Any: | |
| # Create ResponseStreamWriter span for server-side responses | |
| with self._tracer.start_as_current_span("ResponseStreamWriter"): | |
| ctx = context.get_current() | |
| return await self.__wrapped__.send(ItemWithContext(item, ctx)) |
🤖 Prompt for AI Agents
In
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py
around lines 608 to 614, the send method is decorated with @dont_throw which
swallows transport errors; remove the @dont_throw decorator so exceptions from
the underlying writer propagate, keep the span creation exactly as-is
(start_as_current_span("ResponseStreamWriter") and capturing context via
context.get_current()), and ensure the method returns await
self.__wrapped__.send(ItemWithContext(item, ctx)) so any transport failures are
not suppressed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (2)
166-198: Tighten wrapper: remove unused param, drop redundant f-strings, prefer kwargs-first arg extraction.Also aligns with prior feedback.
- def _jsonrpc_response_init_wrapper(self, tracer): - @dont_throw - def traced_method(wrapped, instance, args, kwargs): - result_value = kwargs.get("result", None) - if result_value is None and len(args) > 1: - result_value = args[1] + def _jsonrpc_response_init_wrapper(self, tracer): + @dont_throw + def traced_method(wrapped, _, args, kwargs): + result_value = kwargs.get("result") + if result_value is None and len(args) > 1: + result_value = args[1] @@ - with tracer.start_as_current_span("MCP_Tool_Response") as span: + with tracer.start_as_current_span("MCP_Tool_Response") as span: # Serialize the result data result_serialized = serialize(result_value) - span.set_attribute(SpanAttributes.MCP_RESPONSE_VALUE, f"{result_serialized}") + span.set_attribute(SpanAttributes.MCP_RESPONSE_VALUE, result_serialized) @@ - id_value = kwargs.get("id", None) - if id_value is None and len(args) > 0: - id_value = args[0] + id_value = kwargs.get("id") + if id_value is None and args: + id_value = args[0] @@ - if id_value is not None: - span.set_attribute(SpanAttributes.MCP_REQUEST_ID, f"{id_value}") + if id_value is not None: + span.set_attribute(SpanAttributes.MCP_REQUEST_ID, str(id_value))Please confirm the
mcp.types.JSONRPCResponse.__init__parameter order so the positional fallbacks are correct:What are the parameters of mcp.types.JSONRPCResponse.__init__ in MCP v1.6+?
630-635: Don’t swallow transport errors; drop @dont_throw on ContextSavingStreamWriter.send.Decorator hides underlying I/O failures.
- @dont_throw async def send(self, item: Any) -> Any: # Removed RequestStreamWriter span creation - we don't need low-level protocol spans ctx = context.get_current() return await self.__wrapped__.send(ItemWithContext(item, ctx))PR description says this method “creates spans for MCP requests,” but the current code explicitly removes span creation. Confirm intended behavior.
🧹 Nitpick comments (1)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (1)
111-126: Avoid blind except; catch specific errors when wrapping JSONRPCResponse.init.Prevents swallowing unexpected issues and fixes Ruff BLE001.
- try: - wrap_function_wrapper( - "mcp.types", - "JSONRPCResponse.__init__", - self._jsonrpc_response_init_wrapper(tracer), - ) - except Exception: - # Fallback to post-import hook - register_post_import_hook( - lambda _: wrap_function_wrapper( - "mcp.types", - "JSONRPCResponse.__init__", - self._jsonrpc_response_init_wrapper(tracer), - ), - "mcp.types", - ) + try: + wrap_function_wrapper( + "mcp.types", + "JSONRPCResponse.__init__", + self._jsonrpc_response_init_wrapper(tracer), + ) + except (ModuleNotFoundError, ImportError, AttributeError): + # Fallback to post-import hook when module not yet imported or attribute missing + register_post_import_hook( + lambda _: wrap_function_wrapper( + "mcp.types", + "JSONRPCResponse.__init__", + self._jsonrpc_response_init_wrapper(tracer), + ), + "mcp.types", + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Store API keys only in environment variables/secure vaults; never hardcode secrets in code
Use Flake8 for code linting and adhere to its rules
Files:
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py
🪛 Ruff (0.12.2)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py
117-117: Do not catch blind exception: Exception
(BLE001)
128-128: Unused method argument: kwargs
(ARG002)
168-168: Unused function argument: instance
(ARG001)
⏰ 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: Lint
- GitHub Check: Test Packages (3.11)
- GitHub Check: Build Packages (3.11)
🔇 Additional comments (1)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (1)
131-131: Unwrapping on uninstrument — LGTM.
elinacse
changed the title
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py
Outdated
Show resolved
Hide resolved
feat(instrumentation): ...orfix(instrumentation): ....Important
Adds span creation for JSONRpcResponse formats in MCP instrumentation, including wrappers for JSONRPCResponse initialization and MCP request handling.
_jsonrpc_response_init_wrapper()ininstrumentation.pyto create spans forJSONRPCResponseinitialization, setting attributes likeMCP_RESPONSE_VALUEandMCP_REQUEST_ID.send()inContextSavingStreamWriterto create spans for MCP requests, setting attributes likeMCP_METHOD_NAMEandMCP_REQUEST_ARGUMENT.JSONRPCResponse.__init__inmcp.typeswith_jsonrpc_response_init_wrapper()._uninstrument()to unwrapJSONRPCResponse.__init__inmcp.types._transport_wrapper()ininstrumentation.py.This description was created by
for 97fe5df. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements