aminghadersohi opened a new pull request, #41921:
URL: https://github.com/apache/superset/pull/41921
## SUMMARY
The MCP service emitted zero StatsD/Prometheus metrics, and its middleware
swallowed every exception before any error tracker could see them. This adds:
- **Per-tool metrics**: success/error counters and timing in
`LoggingMiddleware.on_call_tool`, plus a user-vs-system error counter split in
`GlobalErrorHandlerMiddleware._handle_error` — mirrors the `@statsd_metrics`
pattern already used in `views/base_api.py`.
- **Fixed silently-dropped audit rows**: `LoggingMiddleware` skipped its
`event_logger.log(...)` call entirely when `has_app_context()` was `False` at
the time the middleware `finally` block ran (the per-tool app context had
already exited). Both call sites now wrap the log call in the existing
`_get_app_context_manager()` helper instead of skipping it.
- **Pluggable `MCP_ERROR_HOOK` config**: a vendor-neutral
`Callable[[Exception, dict], None] | None` hook invoked for system-class errors
in `GlobalErrorHandlerMiddleware._handle_error` (primary capture point) and in
`StructuredContentStripperMiddleware.on_call_tool`'s last-resort except block
(for exceptions that bypass the primary handler entirely). This lets operators
wire an external error tracker (e.g. Sentry) without the OSS repo taking a hard
dependency on any vendor SDK. `PRODUCTION.md` is updated to note that
`FlaskIntegration` does not cover the FastMCP tool-execution path and shows
wiring Sentry via the hook instead.
- **`error_type` extraction**: `LoggingMiddleware` previously
substring-sniffed `"error_type"` in serialized tool responses only to decide
success/failure, discarding the actual value. It's now parsed and included in
the log line, curated payload, and metric tag.
- **Small fixes**: the generic "Internal error" branch used a
second-granularity `f"err_{int(time.time())}"` ID that collides under
concurrent failures — replaced with the existing per-call `mcp_call_id`. Added
`event_logger.log_context` instrumentation to `get_chart_type_schema`
(previously the only uninstrumented tool). Added server-side
`logger.warning`/`logger.exception` calls next to several `return XError(...)`
sites in `get_chart_data`, `get_tag_info`, and `query_dataset` that only logged
to the MCP client (`ctx.error`/`ctx.warning`, which never reaches server logs)
or didn't log at all.
## BEFORE/AFTER
**Before:** `grep -r "stats_logger\|statsd\|incr(\|timing("
superset/mcp_service/` → no hits. No error tracker could ever see an MCP tool
failure, and DB audit rows for `mcp_tool_call`/`mcp_message` were silently
dropped whenever the ambient Flask app context had already exited.
**After:** Per-tool `mcp.tool.{name}.{success|error}` counters and
`mcp.tool.{name}.time` timing are emitted on every tool call. Audit rows are
always written. Operators can wire `MCP_ERROR_HOOK` to forward system-class
errors to an external tracker.
## TESTING INSTRUCTIONS
- Extended `tests/unit_tests/mcp_service/test_middleware.py` and
`test_middleware_logging.py` with unit tests covering: metrics emission
(success/error counters + timing, including the `call_tool` proxy tool-name
resolution case), `error_type` extraction from structured responses, the
`MCP_ERROR_HOOK` invocation/no-op/exception-swallowing paths in both
middlewares, the `mcp_call_id`-as-`error_id` fix, and regression tests
confirming both `LoggingMiddleware.on_call_tool` and `on_message` route their
audit-row log call through `_get_app_context_manager()` rather than
conditionally skipping it.
- `pytest tests/unit_tests/mcp_service/` — 2691 passed.
- `ruff check` / `ruff format --check` / `mypy` / `pylint` all pass on the
changed files.
## ADDITIONAL INFORMATION
- [x] Has associated tests
- [ ] Has screenshots (N/A — backend-only observability change)
- [ ] Changes UI
- [ ] Introduces new feature or API
- [ ] Removes existing feature
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
