You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by mi...@apache.org on 2026/10/10 04:33:56 UTC
(superset) branch master updated: fix(mcp): keep isError through call_tool and test native tool surface parity (#45081)
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 8aeb2850914 fix(mcp): keep isError through call_tool and test native tool surface parity (#45081)
8aeb2850914 is described below
commit 8aeb28509148cd8329322135e77732cf3d1eb46a
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Sat Oct 10 14:33:40 2026 +1000
fix(mcp): keep isError through call_tool and test native tool surface parity (#45081)
---
docs/admin_docs/configuration/mcp-server.mdx | 10 +
superset/mcp_service/middleware.py | 45 ++++-
.../mcp_service/test_middleware_logging.py | 108 ++++++++++-
.../mcp_service/test_native_tool_surface.py | 210 ++++++++++++++++++++-
4 files changed, 359 insertions(+), 14 deletions(-)
diff --git a/docs/admin_docs/configuration/mcp-server.mdx b/docs/admin_docs/configuration/mcp-server.mdx
index 922ebec8ccb..0ad3774e7f9 100644
--- a/docs/admin_docs/configuration/mcp-server.mdx
+++ b/docs/admin_docs/configuration/mcp-server.mdx
@@ -1361,6 +1361,16 @@ All MCP tool calls are logged to Superset's event logger, the same system used b
This means MCP activity is auditable alongside normal user activity. No additional configuration is required — logging is on by default whenever the event logger is enabled in your Superset deployment.
+### Tool Metrics
+
+The configured stats logger records one outcome counter
+(`mcp.tool.<tool_name>.success`, `.warning`, or `.error`) and one timing
+(`mcp.tool.<tool_name>.time`) per tool execution. Calls forwarded through the
+tool-search proxy, including a custom `call_tool_name`, use the target tool's
+metrics without a second counter or timing from the proxy. This applies to both
+successful and failed calls; exceptions raised by the proxy itself are still
+counted.
+
## Tool Pagination
MCP list tools (`list_datasets`, `list_charts`, `list_dashboards`, `list_databases`) use **offset pagination** via `page` (1-based) and `page_size` parameters. Responses include `page`, `page_size`, `total_count`, `total_pages`, `has_previous`, and `has_next`. To iterate through all results:
diff --git a/superset/mcp_service/middleware.py b/superset/mcp_service/middleware.py
index 83ef9fa4d97..e17ba265c7f 100644
--- a/superset/mcp_service/middleware.py
+++ b/superset/mcp_service/middleware.py
@@ -549,8 +549,29 @@ class LoggingMiddleware(Middleware):
payload).
"""
- #: Proxy name used by FastMCP tool-search transforms.
- _CALL_TOOL_PROXY = "call_tool"
+ @staticmethod
+ def _call_tool_proxy_name() -> str:
+ """Return the configured name of the tool-search call proxy.
+
+ Read from the same ``MCP_TOOL_SEARCH_CONFIG["call_tool_name"]`` setting
+ the proxy is registered under, so a renamed proxy is still recognised.
+ Falls back to the packaged default when no Flask app is reachable.
+ """
+ from superset.mcp_service.mcp_config import MCP_TOOL_SEARCH_CONFIG
+
+ config: Any = MCP_TOOL_SEARCH_CONFIG
+ try:
+ from superset.mcp_service.flask_singleton import get_flask_app
+
+ config = get_flask_app().config.get(
+ "MCP_TOOL_SEARCH_CONFIG", MCP_TOOL_SEARCH_CONFIG
+ )
+ except Exception: # noqa: BLE001
+ logger.debug("Flask app unavailable; using default call proxy name")
+ name = config.get("call_tool_name") if isinstance(config, dict) else None
+ if isinstance(name, str) and name:
+ return name
+ return MCP_TOOL_SEARCH_CONFIG["call_tool_name"]
def _is_error_response(self, result: ToolResult) -> bool:
"""Check if a tool result contains an error schema response.
@@ -661,17 +682,17 @@ class LoggingMiddleware(Middleware):
def _resolve_tool_name(tool_name: str | None, params: Any) -> str | None:
"""Resolve the underlying tool name from call_tool proxy arguments.
- When tool search is enabled, the MCP client uses the ``call_tool``
- proxy and passes the real tool name as the ``name`` argument. This
- helper extracts that value so we can log which tool was actually
- executed rather than just ``"call_tool"``.
+ When tool search is enabled, the MCP client uses the configured call
+ proxy (``call_tool`` by default) and passes the real tool name as the
+ ``name`` argument. This helper extracts that value so we can log which
+ tool was actually executed rather than just ``"call_tool"``.
Returns:
The resolved tool name if *tool_name* is the call_tool proxy and
``params["name"]`` is a non-empty string, otherwise ``None``.
"""
if (
- tool_name == LoggingMiddleware._CALL_TOOL_PROXY
+ tool_name == LoggingMiddleware._call_tool_proxy_name()
and isinstance(params, dict)
and isinstance(params.get("name"), str)
and params["name"]
@@ -842,7 +863,7 @@ class LoggingMiddleware(Middleware):
registered = None
if registered is not None:
return candidate
- return "call_tool" if mcp_tool else "unknown"
+ return LoggingMiddleware._call_tool_proxy_name() if mcp_tool else "unknown"
async def _emit_call_metrics(
self,
@@ -856,6 +877,12 @@ class LoggingMiddleware(Middleware):
) -> None:
"""Emit the per-tool outcome counter and timing for one call.
+ An outcome the ``call_tool`` proxy merely forwards (``mcp_tool`` set,
+ nothing raised) was already counted and timed, with its own
+ user/system classification, when the proxied tool ran through this
+ middleware. Skip the redundant proxy emission for both successes
+ and failures, but still count exceptions raised by the proxy itself.
+
Single emission point for the per-tool outcome counters —
GlobalErrorHandlerMiddleware (inner) re-raises every failure as
ToolError, so counting there as well would double-count raised
@@ -865,6 +892,8 @@ class LoggingMiddleware(Middleware):
free-form error_type that cannot be reliably classified, so they
count as error (the parsed error_type is in the curated payload).
"""
+ if mcp_tool is not None and raised_is_user_error is None:
+ return
metric_tool = await self._resolve_metric_tool_name(context, tool_name, mcp_tool)
if success:
outcome = "success"
diff --git a/tests/unit_tests/mcp_service/test_middleware_logging.py b/tests/unit_tests/mcp_service/test_middleware_logging.py
index 74f921885f4..df81d7afd7c 100644
--- a/tests/unit_tests/mcp_service/test_middleware_logging.py
+++ b/tests/unit_tests/mcp_service/test_middleware_logging.py
@@ -1094,11 +1094,13 @@ class TestOnCallToolStatsMetrics:
@patch("superset.mcp_service.middleware.event_logger")
@patch("superset.mcp_service.middleware.get_user_id", return_value=42)
@pytest.mark.asyncio
- async def test_metric_uses_resolved_tool_name_for_call_tool_proxy(
- self, mock_get_user_id, mock_event_logger, mock_stats
+ async def test_forwarded_success_is_not_recounted_by_call_tool_proxy(
+ self,
+ mock_get_user_id: MagicMock,
+ mock_event_logger: MagicMock,
+ mock_stats: MagicMock,
) -> None:
- """When invoked via the call_tool search proxy, the metric key
- must use the real tool name, not the literal 'call_tool'."""
+ """The outer proxy must not repeat the inner tool's success metrics."""
middleware = LoggingMiddleware()
ctx = _make_context(
name="call_tool",
@@ -1108,8 +1110,43 @@ class TestOnCallToolStatsMetrics:
await middleware.on_call_tool(ctx, call_next)
+ mock_stats.instance.incr.assert_not_called()
+ mock_stats.instance.timing.assert_not_called()
+
+ @patch("superset.mcp_service.middleware.stats_logger_manager")
+ @patch("superset.mcp_service.middleware.event_logger")
+ @patch("superset.mcp_service.middleware.get_user_id", return_value=42)
+ @pytest.mark.asyncio
+ @pytest.mark.parametrize(
+ ("error", "outcome"),
+ [(ValueError("bad arguments"), "warning"), (RuntimeError("boom"), "error")],
+ )
+ async def test_proxy_level_exception_is_still_counted(
+ self,
+ mock_get_user_id: MagicMock,
+ mock_event_logger: MagicMock,
+ mock_stats: MagicMock,
+ error: Exception,
+ outcome: str,
+ ) -> None:
+ """De-duplication must not suppress an exception raised by the proxy."""
+ middleware = LoggingMiddleware()
+ ctx = _make_context(
+ name="call_tool",
+ params={"name": "list_datasets", "arguments": {}},
+ )
+ call_next = AsyncMock(side_effect=error)
+
+ with pytest.raises(type(error), match=str(error)):
+ await middleware.on_call_tool(ctx, call_next)
+
mock_stats.instance.incr.assert_called_once_with(
- "mcp.tool.list_datasets.success"
+ f"mcp.tool.list_datasets.{outcome}"
+ )
+ mock_stats.instance.timing.assert_called_once()
+ assert (
+ mock_stats.instance.timing.call_args.args[0]
+ == "mcp.tool.list_datasets.time"
)
@patch("superset.mcp_service.middleware.logger")
@@ -1156,6 +1193,67 @@ class TestOnCallToolStatsMetrics:
assert any("Failed to emit MCP tool metrics" in m for m in warning_messages)
+class TestConfiguredCallToolProxyName:
+ """The call proxy name comes from MCP_TOOL_SEARCH_CONFIG, not a literal."""
+
+ @staticmethod
+ def _app_with_proxy(name: str) -> MagicMock:
+ app = MagicMock()
+ app.config = {"MCP_TOOL_SEARCH_CONFIG": {"call_tool_name": name}}
+ return app
+
+ def test_resolves_configured_proxy_name(self) -> None:
+ with patch(
+ "superset.mcp_service.flask_singleton.get_flask_app",
+ return_value=self._app_with_proxy("invoke_tool"),
+ ):
+ assert (
+ LoggingMiddleware._resolve_tool_name(
+ "invoke_tool", {"name": "list_datasets"}
+ )
+ == "list_datasets"
+ )
+ # The default literal is no longer the proxy once renamed.
+ assert (
+ LoggingMiddleware._resolve_tool_name(
+ "call_tool", {"name": "list_datasets"}
+ )
+ is None
+ )
+
+ def test_falls_back_to_default_without_a_flask_app(self) -> None:
+ with patch(
+ "superset.mcp_service.flask_singleton.get_flask_app",
+ side_effect=RuntimeError("no app"),
+ ):
+ assert LoggingMiddleware._call_tool_proxy_name() == "call_tool"
+
+ @patch("superset.mcp_service.middleware.stats_logger_manager")
+ @patch("superset.mcp_service.middleware.event_logger")
+ @patch("superset.mcp_service.middleware.get_user_id", return_value=42)
+ @pytest.mark.asyncio
+ async def test_forwarded_failure_through_renamed_proxy_is_not_recounted(
+ self, mock_get_user_id, mock_event_logger, mock_stats
+ ) -> None:
+ middleware = LoggingMiddleware()
+ ctx = _make_context(name="invoke_tool", params={"name": "list_datasets"})
+ call_next = AsyncMock(
+ return_value=ToolResult(
+ content=[mt.TextContent(type="text", text="bad arguments")],
+ is_error=True,
+ )
+ )
+
+ with patch(
+ "superset.mcp_service.flask_singleton.get_flask_app",
+ return_value=self._app_with_proxy("invoke_tool"),
+ ):
+ await middleware.on_call_tool(ctx, call_next)
+
+ mock_stats.instance.incr.assert_not_called()
+ mock_stats.instance.timing.assert_not_called()
+
+
class TestResolveMetricToolName:
"""Tests for the StatsD metric-key validation in
_resolve_metric_tool_name — client-controlled tool names must never
diff --git a/tests/unit_tests/mcp_service/test_native_tool_surface.py b/tests/unit_tests/mcp_service/test_native_tool_surface.py
index 8029e7b3e45..e06dba6ad0c 100644
--- a/tests/unit_tests/mcp_service/test_native_tool_surface.py
+++ b/tests/unit_tests/mcp_service/test_native_tool_surface.py
@@ -50,6 +50,7 @@ import math
import os
from collections import Counter
from collections.abc import Iterator
+from contextlib import contextmanager
from pathlib import Path
from typing import Any
from unittest.mock import MagicMock, patch
@@ -131,6 +132,7 @@ async def build_server(
compatibility: bool,
compact: bool = False,
max_description_length: int = COMPACT_MAX_DESCRIPTION,
+ search_config: dict[str, Any] | None = None,
) -> FastMCP:
"""Assemble a server from the registered tools as ``run_server`` does."""
server = FastMCP(
@@ -142,7 +144,9 @@ async def build_server(
for tool in await canonical_tools():
server.add_tool(tool)
if compatibility:
- _apply_tool_search_transform(server, dict(MCP_TOOL_SEARCH_CONFIG))
+ _apply_tool_search_transform(
+ server, dict(search_config or MCP_TOOL_SEARCH_CONFIG)
+ )
else:
native_config = COMPACT_CONFIG if compact else MCP_NATIVE_TOOL_LIST_CONFIG
_apply_compact_tool_list_transform(
@@ -291,6 +295,20 @@ async def test_largest_native_entry_fits_one_gateway_page(mode: str) -> None:
assert sizes[largest] <= GATEWAY_PAGE_BYTE_LIMIT, (largest, sizes[largest])
+@pytest.mark.asyncio
+@pytest.mark.parametrize("mode", OUTPUT_MODES)
+async def test_compatibility_listing_fits_one_gateway_page(mode: str) -> None:
+ """The default compatibility ``tools/list`` is one page under the gateway cap."""
+ server = await build_server(
+ structured_output_enabled=OUTPUT_MODES[mode], compatibility=True
+ )
+ async with Client(server) as client:
+ listing = await client.list_tools_mcp()
+ assert listing.nextCursor is None
+ page_bytes = compact_bytes([wire_entry(tool) for tool in listing.tools])
+ assert page_bytes <= GATEWAY_PAGE_BYTE_LIMIT, page_bytes
+
+
@pytest.mark.asyncio
@pytest.mark.parametrize("mode", OUTPUT_MODES)
async def test_native_list_exposes_every_registered_tool_by_name(mode: str) -> None:
@@ -934,3 +952,193 @@ async def test_compact_listing_keeps_chart_guidance_reachable() -> None:
assert all(
example["distribute_across"] for example in payload["examples"]
)
+
+
+@contextmanager
+def recorded_outcome_metrics() -> Iterator[MagicMock]:
+ """Capture the per-tool stats emitted by the logging middleware."""
+ with patch("superset.mcp_service.middleware.stats_logger_manager") as stats:
+ yield stats.instance
+
+
+def outcome_keys(stats: MagicMock) -> list[str]:
+ return [call.args[0] for call in stats.incr.call_args_list]
+
+
+def timing_keys(stats: MagicMock) -> list[str]:
+ return [call.args[0] for call in stats.timing.call_args_list]
+
+
+async def call_and_count(
+ server: FastMCP,
+ name: str,
+ arguments: dict[str, Any],
+ *,
+ proxied: bool,
+ proxy_name: str = CALL_TOOL,
+) -> tuple[CallToolResult, list[str], list[str]]:
+ """Call a tool and return its result with the outcome and timing metrics."""
+ with recorded_outcome_metrics() as stats:
+ async with Client(server) as client:
+ if proxied:
+ result = await client.call_tool(
+ proxy_name,
+ {"name": name, "arguments": arguments},
+ raise_on_error=False,
+ )
+ else:
+ result = await client.call_tool(name, arguments, raise_on_error=False)
+ return result, outcome_keys(stats), timing_keys(stats)
+
+
+@pytest.mark.asyncio
+@pytest.mark.usefixtures("unprivileged_user")
+async def test_proxied_validation_failure_is_one_failure_and_an_error_result() -> None:
+ """A validation failure through call_tool is reported as an error result
+ and counted once, with the same outcome as the direct call."""
+ _, compatibility = await build_both()
+ arguments = {"include_examples": "x"}
+
+ direct, direct_keys, direct_timing = await call_and_count(
+ compatibility, "get_chart_type_schema", arguments, proxied=False
+ )
+ proxied, proxied_keys, proxied_timing = await call_and_count(
+ compatibility, "get_chart_type_schema", arguments, proxied=True
+ )
+
+ assert direct.is_error is True
+ assert proxied.is_error is True
+ assert direct_keys == ["mcp.tool.get_chart_type_schema.warning"]
+ assert proxied_keys == direct_keys
+ assert direct_timing == ["mcp.tool.get_chart_type_schema.time"]
+ assert proxied_timing == direct_timing
+
+
+@pytest.mark.asyncio
+@pytest.mark.usefixtures("unprivileged_user")
+@pytest.mark.parametrize("proxy_name", [CALL_TOOL, "invoke_tool"])
+async def test_proxied_failure_is_counted_once_under_any_configured_proxy_name(
+ proxy_name: str,
+) -> None:
+ """The proxy name is configurable; a failure forwarded through it must
+ still be counted once, as the inner tool's warning, never also as an
+ error on the proxy itself."""
+ search_config = {**MCP_TOOL_SEARCH_CONFIG, "call_tool_name": proxy_name}
+ flask_app = MagicMock()
+ flask_app.config = {"MCP_TOOL_SEARCH_CONFIG": search_config}
+ with patch("superset.mcp_service.flask_singleton.get_flask_app") as get_app:
+ get_app.return_value = flask_app
+ compatibility = await build_server(
+ structured_output_enabled=False,
+ compatibility=True,
+ search_config=search_config,
+ )
+ proxied, keys, timings = await call_and_count(
+ compatibility,
+ "get_chart_type_schema",
+ {"include_examples": "x"},
+ proxied=True,
+ proxy_name=proxy_name,
+ )
+
+ assert proxied.is_error is True
+ assert keys == ["mcp.tool.get_chart_type_schema.warning"]
+ assert timings == ["mcp.tool.get_chart_type_schema.time"]
+
+
+@pytest.mark.asyncio
+@pytest.mark.usefixtures("unprivileged_user", "rbac_enabled")
+async def test_proxied_permission_failure_is_one_failure_and_an_error_result() -> None:
+ """A permission denial through call_tool is reported as an error result
+ and counted once, with the same outcome as the direct call."""
+ security_manager = MagicMock()
+ security_manager.can_access.return_value = True
+ with (
+ patch("superset.mcp_service.auth.security_manager", security_manager),
+ patch(
+ "superset.mcp_service.auth._get_token_scopes",
+ return_value={"superset:read"},
+ ),
+ ):
+ _, compatibility = await build_both()
+ direct, direct_keys, direct_timing = await call_and_count(
+ compatibility, "create_theme", THEME_REQUEST, proxied=False
+ )
+ proxied, proxied_keys, proxied_timing = await call_and_count(
+ compatibility, "create_theme", THEME_REQUEST, proxied=True
+ )
+
+ assert direct.is_error is True
+ assert proxied.is_error is True
+ assert len(direct_keys) == 1
+ assert direct_keys[0].startswith("mcp.tool.create_theme.")
+ assert not direct_keys[0].endswith(".success")
+ assert proxied_keys == direct_keys
+ assert proxied_timing == direct_timing
+
+
+@pytest.mark.asyncio
+@pytest.mark.usefixtures("unprivileged_user")
+async def test_direct_failure_metrics_are_unchanged_in_both_modes() -> None:
+ """Direct calls count one failure whether or not the proxy is installed."""
+ native, compatibility = await build_both()
+ arguments = {"include_examples": "x"}
+ for server in (native, compatibility):
+ result, keys, timings = await call_and_count(
+ server, "get_chart_type_schema", arguments, proxied=False
+ )
+ assert result.is_error is True
+ assert keys == ["mcp.tool.get_chart_type_schema.warning"]
+ assert timings == ["mcp.tool.get_chart_type_schema.time"]
+
+
+@pytest.mark.asyncio
+@pytest.mark.usefixtures("unprivileged_user")
+async def test_proxied_success_metrics_are_unchanged() -> None:
+ """A successful proxied call is still an ordinary success."""
+ _, compatibility = await build_both()
+ arguments = {"chart_type": "table", "include_examples": False}
+
+ direct, direct_keys, direct_timing = await call_and_count(
+ compatibility, "get_chart_type_schema", arguments, proxied=False
+ )
+ proxied, proxied_keys, proxied_timing = await call_and_count(
+ compatibility, "get_chart_type_schema", arguments, proxied=True
+ )
+
+ assert direct.is_error is False
+ assert proxied.is_error is False
+ assert direct_keys == ["mcp.tool.get_chart_type_schema.success"]
+ assert proxied_keys == direct_keys
+ assert direct_timing == ["mcp.tool.get_chart_type_schema.time"]
+ assert proxied_timing == direct_timing
+
+
+@pytest.mark.asyncio
+@pytest.mark.usefixtures("unprivileged_user")
+@pytest.mark.parametrize("proxy_name", [CALL_TOOL, "invoke_tool"])
+async def test_proxied_success_is_counted_once_under_any_configured_proxy_name(
+ proxy_name: str,
+) -> None:
+ """A forwarded success emits one counter and timing, even with a renamed proxy."""
+ search_config = {**MCP_TOOL_SEARCH_CONFIG, "call_tool_name": proxy_name}
+ flask_app = MagicMock()
+ flask_app.config = {"MCP_TOOL_SEARCH_CONFIG": search_config}
+ with patch("superset.mcp_service.flask_singleton.get_flask_app") as get_app:
+ get_app.return_value = flask_app
+ compatibility = await build_server(
+ structured_output_enabled=False,
+ compatibility=True,
+ search_config=search_config,
+ )
+ proxied, keys, timings = await call_and_count(
+ compatibility,
+ "get_chart_type_schema",
+ {"chart_type": "table", "include_examples": False},
+ proxied=True,
+ proxy_name=proxy_name,
+ )
+
+ assert proxied.is_error is False
+ assert keys == ["mcp.tool.get_chart_type_schema.success"]
+ assert timings == ["mcp.tool.get_chart_type_schema.time"]