fix(mcp): wrap streamable_http_client under mcp>=2.0 (fixes #4413) - #4416
fix(mcp): wrap streamable_http_client under mcp>=2.0 (fixes #4413)#4416Anai-Guo wants to merge 2 commits into
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughThe MCP instrumentation now checks for supported streamable HTTP client symbols and wraps the first available symbol. This prevents initialization failure when the expected symbol is absent in MCP 2.x. ChangesMCP streamable HTTP instrumentation
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: 🟡 Moderate · up to The client instrumentation now supports MCP 2.x naming, but versions that expose both names may wrap the legacy alias first, allowing direct calls to the canonical function to bypass transport tracing and omit spans. Merge should wait for the selection order to be corrected. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
|
![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
🧹 Nitpick comments (1)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py (1)
94-106: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftAdd compatibility tests for both MCP client names.
Add tests for modules with only
streamablehttp_client, onlystreamable_http_client, both names, and neither name. MCP 1.24–1.28 expose both names, while MCP 2.x removesstreamablehttp_client. The test dependency excludes MCP 2.x. Add an MCP 2.x CI job if real-package validation is required.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py` around lines 94 - 106, Add compatibility tests covering _wrap_streamable_http_client with modules exposing only streamablehttp_client, only streamable_http_client, both names, and neither name. Verify the available function is wrapped exactly once, the legacy name takes precedence when both exist, and no wrapper is registered when neither exists; add an MCP 2.x CI job only if validation against the real package is required.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py`:
- Around line 98-103: Update the name-priority loop in the instrumentation setup
to check streamable_http_client before streamablehttp_client, then return after
wrapping the first available function. Update the adjacent comment to document
the canonical-first selection while retaining the legacy-name fallback for older
MCP versions.
---
Nitpick comments:
In
`@packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py`:
- Around line 94-106: Add compatibility tests covering
_wrap_streamable_http_client with modules exposing only streamablehttp_client,
only streamable_http_client, both names, and neither name. Verify the available
function is wrapped exactly once, the legacy name takes precedence when both
exist, and no wrapper is registered when neither exists; add an MCP 2.x CI job
only if validation against the real package is required.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 864c1035-35d9-479a-b2ab-5c5268812084
📒 Files selected for processing (1)
packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py
| for name in ("streamablehttp_client", "streamable_http_client"): | ||
| if hasattr(module, name): | ||
| wrap_function_wrapper( | ||
| module, name, self._transport_wrapper(tracer) | ||
| ) | ||
| return |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
for version in 1.23.3 1.24.0 1.28.0 2.0.0; do
uv run --isolated --with "mcp==${version}" python - <<'PY'
import importlib
import importlib.metadata
module = importlib.import_module("mcp.client.streamable_http")
names = [
name
for name in ("streamable_http_client", "streamablehttp_client")
if hasattr(module, name)
]
print(importlib.metadata.version("mcp"), names)
PY
doneRepository: traceloop/openllmetry
Length of output: 199
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- changed file ---'
sed -n '70,120p' packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py
printf '%s\n' '--- MCP package metadata and tests ---'
rg -n -C 3 'mcp[<=>]|streamable.?http_client|transport_wrapper|wrap_function_wrapper' \
packages/opentelemetry-instrumentation-mcp pyproject.toml uv.lock 2>/dev/null || true
printf '%s\n' '--- repository files related to MCP ---'
git ls-files packages/opentelemetry-instrumentation-mcp | sed -n '1,120p'Repository: traceloop/openllmetry
Length of output: 21552
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import json
import urllib.request
versions = ["v1.23.3", "v1.24.0", "v1.26.0", "v1.28.0", "v2.0.0"]
base = "https://raw.githubusercontent.com/modelcontextprotocol/python-sdk/{}/src/mcp/client/streamable_http.py"
for version in versions:
url = base.format(version)
print(f"--- {version} ---")
try:
text = urllib.request.urlopen(url, timeout=15).read().decode()
except Exception as exc:
print(type(exc).__name__, exc)
continue
lines = text.splitlines()
for i, line in enumerate(lines, 1):
if "streamablehttp_client" in line or "streamable_http_client" in line:
start = max(1, i - 3)
end = min(len(lines), i + 12)
print("\n".join(f"{n}: {lines[n-1]}" for n in range(start, end + 1)))
print()
PYRepository: traceloop/openllmetry
Length of output: 958
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
for version in v1.23.3 v1.24.0 v1.26.0 v1.28.0 v2.0.0; do
printf '%s\n' "--- ${version} ---"
url="https://raw.githubusercontent.com/modelcontextprotocol/python-sdk/${version}/src/mcp/client/streamable_http.py"
if ! text="$(curl -ksSf --max-time 20 "$url")"; then
echo "source unavailable"
continue
fi
printf '%s\n' "$text" | nl -ba | grep -E -B 3 -A 12 'streamablehttp_client|streamable_http_client' || true
doneRepository: traceloop/openllmetry
Length of output: 423
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
for version in v1.23.3 v1.24.0 v1.26.0 v1.28.0 v2.0.0; do
printf '%s\n' "--- ${version} ---"
url="https://raw.githubusercontent.com/modelcontextprotocol/python-sdk/${version}/src/mcp/client/streamable_http.py"
curl -ksSf --max-time 20 "$url" |
grep -E -B 4 -A 12 'streamablehttp_client|streamable_http_client' || true
doneRepository: traceloop/openllmetry
Length of output: 5266
Select streamable_http_client before streamablehttp_client.
MCP 1.24.0–1.28.0 export both names, and streamablehttp_client delegates to streamable_http_client. Because the loop returns after wrapping the legacy name, direct calls to the canonical function bypass _transport_wrapper and produce no transport spans. Reverse the order and update the comment. MCP 1.23.3 exposes only the legacy name, so the fallback remains compatible.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/opentelemetry-instrumentation-mcp/opentelemetry/instrumentation/mcp/instrumentation.py`
around lines 98 - 103, Update the name-priority loop in the instrumentation
setup to check streamable_http_client before streamablehttp_client, then return
after wrapping the first available function. Update the adjacent comment to
document the canonical-first selection while retaining the legacy-name fallback
for older MCP versions.
Source: MCP tools
There was a problem hiding this comment.
@Anai-Guo , Shall I update this PR with the recommended backward compatible changes? Or let me know if you are on it.
for name in ("streamable_http_client", "streamablehttp_client"):mcp 1.24 added streamable_http_client and kept streamablehttp_client as a deprecated shim that delegates to it. Wrapping the legacy name first left direct calls to the canonical function untraced on 1.24-1.28; wrapping the canonical name first covers both call paths there, and the legacy name is still used as a fallback on mcp<1.24. Signed-off-by: Tai An <antai12232931@outlook.com>
|
Thanks @sathishsw — I'm on it, just pushed it in 517be99. I checked the upstream sources before changing the order, and the delegation direction is what makes canonical-first the right call rather than wrapping both names:
So the loop keeps the 🤖 Generated with Claude Code |
3 participants
Problem
McpInstrumentor._instrumentregisters a post-import hook that wrapsmcp.client.streamable_http.streamablehttp_client. In mcp 2.0.0 thatcallable was renamed to
streamable_http_client, so bootstrapping theinstrumentor against
mcp>=2.0fails with:Fixes #4413.
Fix
The post-import hook now wraps whichever name the imported module exposes:
mcp1.9–1.27 → onlystreamablehttp_client(old name)mcp1.28 → both names (the new alias was introduced here)mcp2.0.0 → onlystreamable_http_clientmcp1.6.x → nomcp.client.streamable_httpmodule at all, so the hook never fires_instrumentsstays atmcp >= 1.6.0; the change keeps that whole range working.Only the client hook is affected — the server-side
StreamableHTTPServerTransport.connecthook still resolves under mcp 2.0.0.
Verification
Reproduced the original failure and confirmed the fix by registering the real
wraptpost-import hook against actual mcp installs (old hardcoded name vs. thenew attribute-detecting hook):
streamablehttp_client)has no attribute 'streamablehttp_client'🤖 Generated with Claude Code
Summary by CodeRabbit