fix(mcp): remove faulty logic of trying to deduce HTTP errors #3477

nirga · 2025-11-27T11:59:56Z

I have added tests that cover my changes.
If adding a new instrumentation or changing an existing one, I've added screenshots from some observability platform showing the change.
PR name follows conventional commits format: feat(instrumentation): ... or fix(instrumentation): ....
(If applicable) I have updated the documentation accordingly.

Important

Remove faulty logic for deducing HTTP errors in McpInstrumentor by eliminating get_error_type function and its usage.

Behavior:
- Remove get_error_type function and its usage in instrumentation.py, which attempted to deduce HTTP errors from error messages.
- Affects error handling in McpInstrumentor class, specifically in _execute_and_handle_result() and InstrumentedStreamWriter.send().
Misc:
- Minor formatting changes for consistency in instrumentation.py.

^{This description was created by}^{for 388f133. You can customize this summary. It will automatically update as commits are pushed.}

Summary by CodeRabbit

Refactor
- Internal improvements to telemetry instrumentation processing, enhancing code efficiency and maintainability without affecting external functionality.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-11-27T12:00:08Z

Walkthrough

The PR refactors MCP instrumentation error handling by removing the unreliable get_error_type function that parsed arbitrary numbers from error messages as HTTP status codes, and introduces a new internal _execute_and_handle_result method to handle result processing and error attribution more robustly.

Changes

Cohort / File(s)	Summary
MCP Instrumentation Error Handling Refactor `packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py`	Removed module-level `get_error_type(error_message)` function that attempted to extract HTTP status codes from error messages. Added new private `_execute_and_handle_result()` method to consolidate span execution and result handling logic, fixing flaky error parsing that would fail on non-HTTP-status numbers (e.g., port numbers).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Single file modified with straightforward internal refactoring
Removal of one problematic utility function and introduction of one replacement private method
Changes directly address a known bug with clear intent (replacing flaky HTTP status parsing with more robust error handling)

Suggested reviewers

galkleinman

Poem

🐰 Hop hop, the numbers won't mislead,
Port 443 no longer fills us with dread!
A helper method now takes the lead,
Error parsing fixed—exactly what we need! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 47.06% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately describes the main change: removing flawed logic for deducing HTTP errors from error messages.
Linked Issues check	✅ Passed	The PR directly addresses issue #3474 by removing the faulty HTTP status code parsing logic from error messages, which is the core requirement.
Out of Scope Changes check	✅ Passed	All changes are scoped to fixing the identified issue; removing get_error_type and adding _execute_and_handle_result are directly related to the bug fix.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch mcp-bug

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

Provide your own instructions using the high_level_summary_instructions setting.
Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

📝 Description — Summarize the main change in 50–60 words, explaining what was done.

📓 References — List relevant issues, discussions, documentation, or related PRs.

📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.

📊 Contributor Summary — Include a Markdown table showing contributions:
| Contributor | Lines Added | Lines Removed | Files Changed |

✔️ Additional Notes — Add any extra reviewer context.
Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

ellipsis-dev

Important

Looks good to me! 👍

Reviewed everything up to 388f133 in 1 minute and 48 seconds. Click for details.

Reviewed 320 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 4 draft 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:293

Draft comment:
Good fix: Removed the faulty HTTP error deduction (via get_error_type) and regex parsing. This avoids exceptions when non-HTTP numbers appear in error messages.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is purely informative, as it praises the fix and explains what was done without providing any actionable feedback or suggestions. It does not align with the rules for useful comments in a pull request review.

2. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:398

Draft comment:
Removal of the get_error_type function (and its import dependency ‘re’) is appropriate and addresses issue #3474 by eliminating faulty HTTP status deduction.
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:343

Draft comment:
Note: In _execute_and_handle_result, if result.isError is true but result.content is empty, no error status is set. Consider if a default error status should be applied in such cases.
Reason this comment was not posted:
Comment was on unchanged code.

4. packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py:231

Draft comment:
Using type(e).name for the ERROR_TYPE attribute in exception handling of _fastmcp_client_enter_wrapper improves robustness by avoiding faulty HTTP status mapping.
Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_Mmi6TQXw0lTcBr9n

^{You can customize}^{by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (1)
241-267: Unused parameter can be prefixed with underscore.

The tracer parameter is unused in this wrapper method. Consider renaming it to _tracer to indicate it's intentionally unused while maintaining the signature pattern.

Apply this diff:
-    def _fastmcp_client_exit_wrapper(self, tracer):
+    def _fastmcp_client_exit_wrapper(self, _tracer):
         """Wrapper for FastMCP Client.__aexit__ to end the session trace"""

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between a47c4e5 and 388f133.

📒 Files selected for processing (1)

packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (13 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

🧠 Learnings (1)

📚 Learning: 2025-09-18T14:36:24.693Z

Learnt from: nina-kollman
Repo: traceloop/openllmetry PR: 3388
File: packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/fastmcp_instrumentation.py:20-20
Timestamp: 2025-09-18T14:36:24.693Z
Learning: In packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/fastmcp_instrumentation.py, the team is aware that the global self._server_name field is unsafe for multi-server or concurrent usage but has consciously decided not to support multiple FastMCP servers currently.

Applied to 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/fastmcp_instrumentation.py (2)

traced_method (51-60)

traced_method (65-152)

packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/utils.py (1)

dont_throw (12-40)

🪛 Ruff (0.14.6)

packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py

146-146: Do not catch blind exception: Exception

(BLE001)

151-151: Do not catch blind exception: Exception

(BLE001)

227-227: Do not call setattr with a constant attribute value. It is not any safer than normal property access.

Replace setattr with assignment

(B010)

241-241: Unused method argument: tracer

(ARG002)

257-257: Consider moving this statement to an else block

(TRY300)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

GitHub Check: Build Packages (3.11)
GitHub Check: Test Packages (3.10)
GitHub Check: Test Packages (3.12)
GitHub Check: Test Packages (3.11)
GitHub Check: Lint

🔇 Additional comments (5)

packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (5)

146-155: LGTM: Exception logging improves debuggability.

The addition of warning logs for exceptions in the transport wrapper is helpful for diagnosing instrumentation issues. The broad exception catches are appropriate here since instrumentation should not break the application.

203-209: LGTM: Refactoring improves code organization.

Extracting tool call and MCP method handling into separate methods enhances readability and maintainability.

304-306: LGTM: Proper delegation to error handling helper.

The call to _execute_and_handle_result with clean_output=True correctly delegates result processing and error handling to the centralized helper method, which resolves the faulty HTTP status code parsing issue.

308-316: LGTM: Consistent error handling for MCP methods.

The refactored method correctly delegates to _execute_and_handle_result with clean_output=False, ensuring consistent error handling without attempting to parse HTTP status codes from error messages.

318-355: LGTM: Robust error handling fixes the HTTP status parsing issue.

This new helper method properly addresses the bug from issue #3474:

Uses type(e).__name__ for ERROR_TYPE instead of attempting to parse HTTP status codes from error messages (line 352)

Properly records exceptions and sets error status without brittle number parsing

Correctly handles both error results (via isError flag) and exceptions

Centralizes result processing logic for maintainability

The fix eliminates the failure mode where numbers like port 443 in error messages would cause ValueError: 443 is not a valid HTTPStatus.

fix(mcp): remove faulty logic of trying to deduce HTTP errors

388f133

ellipsis-dev bot reviewed Nov 27, 2025

View reviewed changes

coderabbitai bot reviewed Nov 27, 2025

View reviewed changes

galkleinman approved these changes Nov 27, 2025

View reviewed changes

nirga merged commit 083c327 into main Nov 27, 2025
12 checks passed

nirga deleted the mcp-bug branch November 27, 2025 12:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(mcp): remove faulty logic of trying to deduce HTTP errors #3477

fix(mcp): remove faulty logic of trying to deduce HTTP errors #3477

Uh oh!

nirga commented Nov 27, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Nov 27, 2025 •

edited

Loading

Uh oh!

ellipsis-dev bot left a comment

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fix(mcp): remove faulty logic of trying to deduce HTTP errors #3477

fix(mcp): remove faulty logic of trying to deduce HTTP errors #3477

Uh oh!

Conversation

nirga commented Nov 27, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nirga commented Nov 27, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 27, 2025 •

edited

Loading