You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "aminghadersohi (via GitHub)" <gi...@apache.org> on 2026/07/10 04:08:27 UTC
[PR] feat(mcp): add observability to MCP service [superset]
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add observability to MCP service [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #41921:
URL: https://github.com/apache/superset/pull/41921#issuecomment-5049092971
Addressed the two remaining automated-review items in 664b5c7d8b573f7dc6a456123771c03deab0304d:
**Unprotected metrics emission in the `finally` block** (CodeAnt) — valid: `_emit_call_metrics` was awaited unguarded, so a raising stats client could mask the tool's real result or exception. It's now wrapped in its own try/except (warning-logged, swallowed), matching the guard on the `event_logger.log` call. Two regression tests pin the behavior: with `incr` raising `RuntimeError`, a successful call still returns its result, and a failing call still propagates its original `ValueError` rather than the metrics error.
**Missing OAuth test coverage for the `get_chart_data` re-raise fix** (Bito) — added `TestOAuthErrorRouting` driving the full tool through the FastMCP client with `ChartDataCommand.run` raising: query-time `OAuth2RedirectError` now provably returns `error_type="OAUTH2_REDIRECT"` (not a generic `DataError`), and `OAuth2Error` returns `error_type="OAUTH2_REDIRECT_ERROR"`.
Also merged the latest master into the branch (includes #41923's dead-middleware removal — no conflicts with this PR's changes). Full MCP unit suite: 3125 passed locally.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add observability to MCP service [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41921:
URL: https://github.com/apache/superset/pull/41921#issuecomment-5187084524
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add observability to MCP service [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41921:
URL: https://github.com/apache/superset/pull/41921#issuecomment-5013550015
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add observability to MCP service [superset]
Posted by "rebenitez1802 (via GitHub)" <gi...@apache.org>.
rebenitez1802 commented on code in PR #41921:
URL: https://github.com/apache/superset/pull/41921#discussion_r3616776744
##########
superset/mcp_service/PRODUCTION.md:
##########
@@ -217,19 +217,49 @@ All MCP tools return consistent error schemas:
**Error Tracking (Sentry)**:
+MCP tool execution runs on the FastMCP/Starlette asyncio stack, not Flask
+request handling, so `FlaskIntegration` alone does **not** capture MCP tool
+errors — it only sees the regular Superset web app's Flask requests. Use the
+vendor-neutral `MCP_ERROR_HOOK` config to forward system-class MCP errors
+(unexpected exceptions — not user errors like bad params or permission
+denials) to Sentry instead:
+
```python
# superset_config.py
import sentry_sdk
from sentry_sdk.integrations.flask import FlaskIntegration
sentry_sdk.init(
dsn="https://your-dsn@sentry.io/project-id",
- integrations=[FlaskIntegration()],
+ integrations=[FlaskIntegration()], # covers the Flask web app only
environment="production",
traces_sample_rate=0.1, # 10% of transactions
)
+
+
+def _mcp_error_hook(error: Exception, context: dict) -> None:
+ """Forward system-class MCP tool errors to Sentry.
+
+ ``context`` includes tool_name, mcp_call_id, user_id, error_type,
+ sanitized_message, and duration_ms — already sanitized by the MCP
+ service before this hook runs.
+ """
+ with sentry_sdk.push_scope() as scope:
Review Comment:
Deprecated in sentry-sdk 2.x — `push_scope()` is on the removal path. Use `new_scope()` (same body):
```suggestion
with sentry_sdk.new_scope() as scope:
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add observability to MCP service [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #41921:
URL: https://github.com/apache/superset/pull/41921
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add observability to MCP service [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41921:
URL: https://github.com/apache/superset/pull/41921#issuecomment-5050800215
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add observability to MCP service [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #41921:
URL: https://github.com/apache/superset/pull/41921#discussion_r3632216036
##########
superset/mcp_service/middleware.py:
##########
@@ -363,6 +506,14 @@ async def on_call_tool(
mcp_call_id,
extra,
)
+ await self._emit_call_metrics(
+ context,
+ tool_name,
+ mcp_tool,
+ success=success,
+ raised_is_user_error=raised_is_user_error,
+ duration_ms=duration_ms,
+ )
Review Comment:
Valid catch — fixed in 664b5c7d8b573f7dc6a456123771c03deab0304d. The `await self._emit_call_metrics(...)` in the `finally` is now wrapped in its own try/except that logs a warning and swallows the failure, matching the guard already applied to the `event_logger.log` call above it. Added two regression tests: a stats client raising `RuntimeError` neither turns a successful call into an error (result still returned) nor replaces the tool's original exception (the original `ValueError` still propagates, not the metrics `RuntimeError`).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add observability to MCP service [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #41921:
URL: https://github.com/apache/superset/pull/41921#discussion_r3649029864
##########
superset/mcp_service/middleware.py:
##########
@@ -132,6 +138,29 @@ def _sanitize_error_for_logging(error: Exception) -> str:
return error_str
+def _invoke_error_hook(error: Exception, hook_context: dict[str, Any]) -> None:
+ """Invoke the operator-configured ``MCP_ERROR_HOOK``, if any.
+
+ Kept vendor-neutral (no ``sentry_sdk`` import here) so the OSS repo has
+ no hard dependency on any particular error tracker — operators wire
+ their own hook (e.g. calling ``sentry_sdk.capture_exception``) via
+ ``MCP_ERROR_HOOK`` in ``superset_config.py``. Hook failures are logged
+ and swallowed; they must never affect the MCP response.
+ """
+ try:
+ from superset.mcp_service.flask_singleton import get_flask_app
+
+ hook = get_flask_app().config.get("MCP_ERROR_HOOK")
+ except Exception: # noqa: BLE001
+ return
+ if hook is None:
+ return
+ try:
+ hook(error, hook_context)
Review Comment:
Declining this one as a documented, intentional contract rather than a bug. The synchronous execution is deliberate and already called out explicitly in both the config docstring (`mcp_config.py`) and `PRODUCTION.md`: "The hook runs synchronously on the asyncio event loop, so a blocking hook stalls all in-flight tool handling — hand events to a background transport rather than doing network I/O inline." The reference Sentry wiring uses `capture_exception`, which already enqueues to a background worker.
The hook is operator-provided config (the operator owns deployment-time choices per the threat model), and this behavior was reviewed and accepted by @rebenitez1802 in the round-2 pass. Offloading it ourselves — e.g. `run_in_executor` — would be a net negative here: it fires-and-forgets, so hook exceptions could no longer be caught by our surrounding try/except and logged, we'd lose the ContextVar/Flask-context the hook may read, and it changes ordering guarantees. The clean contract is "keep the hook fast; if it must do I/O, hand off to your own background transport," which is exactly what's documented. No code change.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add observability to MCP service [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #41921:
URL: https://github.com/apache/superset/pull/41921#issuecomment-4931965456
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/41921?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `3.07692%` with `63 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 63.92%. Comparing base ([`0ff52d4`](https://app.codecov.io/gh/apache/superset/commit/0ff52d46e076cd02e45bd776e0387c138055343a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`9ab2905`](https://app.codecov.io/gh/apache/superset/commit/9ab2905abba7435200524e6a1a3a751ad0c91b06?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/41921?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/mcp\_service/middleware.py](https://app.codecov.io/gh/apache/superset/pull/41921?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fmiddleware.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvbWlkZGxld2FyZS5weQ==) | 0.00% | [44 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/41921?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/chart/tool/get\_chart\_data.py](https://app.codecov.io/gh/apache/superset/pull/41921?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Ftool%2Fget_chart_data.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvdG9vbC9nZXRfY2hhcnRfZGF0YS5weQ==) | 0.00% | [15 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/41921?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...et/mcp\_service/chart/tool/get\_chart\_type\_schema.py](https://app.codecov.io/gh/apache/superset/pull/41921?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fchart%2Ftool%2Fget_chart_type_schema.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvY2hhcnQvdG9vbC9nZXRfY2hhcnRfdHlwZV9zY2hlbWEucHk=) | 33.33% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/41921?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/dataset/tool/query\_dataset.py](https://app.codecov.io/gh/apache/superset/pull/41921?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fdataset%2Ftool%2Fquery_dataset.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvZGF0YXNldC90b29sL3F1ZXJ5X2RhdGFzZXQucHk=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/41921?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/tag/tool/get\_tag\_info.py](https://app.codecov.io/gh/apache/superset/pull/41921?src=pr&el=tree&filepath=superset%2Fmcp_service%2Ftag%2Ftool%2Fget_tag_info.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvdGFnL3Rvb2wvZ2V0X3RhZ19pbmZvLnB5) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/41921?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #41921 +/- ##
==========================================
- Coverage 64.74% 63.92% -0.83%
==========================================
Files 2738 2738
Lines 152950 153009 +59
Branches 35055 35059 +4
==========================================
- Hits 99026 97809 -1217
- Misses 52024 53310 +1286
+ Partials 1900 1890 -10
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/41921/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/41921/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.96% <3.07%> (-0.03%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/41921/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/41921/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/41921/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.92% <3.07%> (-0.03%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/41921/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.69% <3.07%> (-1.68%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/41921/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.55% <3.07%> (-0.05%)` | :arrow_down: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/41921/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/41921?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add observability to MCP service [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41921:
URL: https://github.com/apache/superset/pull/41921#issuecomment-5108875147
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add observability to MCP service [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41921:
URL: https://github.com/apache/superset/pull/41921#discussion_r3649030594
##########
superset/mcp_service/middleware.py:
##########
@@ -132,6 +138,29 @@ def _sanitize_error_for_logging(error: Exception) -> str:
return error_str
+def _invoke_error_hook(error: Exception, hook_context: dict[str, Any]) -> None:
+ """Invoke the operator-configured ``MCP_ERROR_HOOK``, if any.
+
+ Kept vendor-neutral (no ``sentry_sdk`` import here) so the OSS repo has
+ no hard dependency on any particular error tracker — operators wire
+ their own hook (e.g. calling ``sentry_sdk.capture_exception``) via
+ ``MCP_ERROR_HOOK`` in ``superset_config.py``. Hook failures are logged
+ and swallowed; they must never affect the MCP response.
+ """
+ try:
+ from superset.mcp_service.flask_singleton import get_flask_app
+
+ hook = get_flask_app().config.get("MCP_ERROR_HOOK")
+ except Exception: # noqa: BLE001
+ return
+ if hook is None:
+ return
+ try:
+ hook(error, hook_context)
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag synchronous MCP error hook execution on the asyncio event loop as a bug; this is an intentional contract, and hooks are expected to remain fast or hand off I/O to their own background transport.
**Applied to:**
- `superset/mcp_service/middleware.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add observability to MCP service [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41921:
URL: https://github.com/apache/superset/pull/41921#issuecomment-5076967072
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add observability to MCP service [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #41921:
URL: https://github.com/apache/superset/pull/41921#issuecomment-5041665161
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 46c2332a6aca40a05c29223c187bfc891f9335cb |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a603e3d2a6d88000703dca4 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-41921--superset-docs-preview.netlify.app](https://deploy-preview-41921--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#q1YqKMrPLShRslJS0lFKKkrMS84AsnOTC3Tzk4pTi8oSkzJzMksqgZIpqQU5-ZWeKUBps0QzA-NU4xSjRLMUCwsDAwNzA-OU5EQTpVoA) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-41921--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add observability to MCP service [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #41921:
URL: https://github.com/apache/superset/pull/41921#issuecomment-5041657300
Thanks for the thorough review — all findings addressed in 46c2332a6aca40a05c29223c187bfc891f9335cb.
**Double-counted / misclassified error counters** — took the suggested fix: the per-tool outcome counter is now emitted from exactly one place, `LoggingMiddleware` (new `_emit_call_metrics`), and the `incr` in `GlobalErrorHandlerMiddleware._handle_error` is removed (replaced with a comment explaining why it must not emit). The user/system classification is recovered in `LoggingMiddleware` by unwrapping the `ToolError.__cause__` that `_handle_error` attaches via `raise ... from error`, then running `_is_user_error` on the original exception — so raised user errors count `.warning`, raised system errors count `.error`, each exactly once. This also fixes the keying inconsistency in the proxy case (the single emission point uses the resolved tool name) and makes the logged `error_type` the original exception class rather than `ToolError`. Structured error responses (no raise) still count `.error` once — their free-form `error_type` can't be reliably mapped to user/system, so the
parsed value goes to the curated payload instead; noted in the docstring.
**Unbounded / injectable metric key** — added `_resolve_metric_tool_name`: the candidate name (proxied or direct — the raw message name is client-controlled on the error path too) is validated against the FastMCP tool registry via `context.fastmcp_context.fastmcp.get_tool(...)`; unregistered names fall back to `"call_tool"` (proxied) or `"unknown"` (direct). When no registry is reachable from the context (mocked contexts in tests), a conservative charset/length regex applies instead, so metadata characters and unbounded lengths never reach the key in any mode. The raw name still reaches the curated payload and log line, which are not StatsD keys.
**Chain-level test gap** — added `TestChainLevelStatsMetrics`: drives a raising tool (user-error and system-error variants, plus a success case) through the real `build_middleware_list()` chain with `stats_logger_manager` patched and asserts the **exact list** of `incr`/`timing` calls — one outcome counter per call, correctly classified. The old per-middleware GEH stats tests are repurposed to assert GEH emits nothing (regression guard against reintroducing the double emission).
**Unguarded `event_logger.log` in the finally** — both `on_call_tool` and `on_message` now wrap the app-context entry + log call in try/except (matching `_handle_error`), so a failing custom `EVENT_LOGGER` can't mask the tool's real exception or skip the metrics/log line that follow.
**Hook-context contract** — the last-resort site in `StructuredContentStripperMiddleware` now populates all six contract keys (`user_id`/`duration_ms` as `None`, `sanitized_message` via the sanitizer), and the stripper test pins the exact key set. Docs in `mcp_config.py` and `PRODUCTION.md` now state the guarantee explicitly and warn that the first argument is the raw exception (only `sanitized_message` is scrubbed).
**OAuth branch reachability in `get_chart_data`** — agreed it was pre-existing, but since the new log line sits on that branch I fixed the routing: the inner generic handler is now preceded by `except (OAuth2RedirectError, OAuth2Error): raise`, so query-time OAuth errors reach the dedicated outer handlers (and return the redirect message instead of a generic `DataError`). No existing tests pinned the old behavior.
**Doc nits** — `push_scope()` → `new_scope()` (your inline suggestion), and both `PRODUCTION.md` and the `mcp_config.py` comment now note the hook runs synchronously on the event loop and that network I/O should go through a background transport (which Sentry's SDK already does).
Full MCP unit suite passes locally (3103 tests; the one failure, `test_mcp_e2e_smoke`, fails identically on the merge base without these changes — local-env artifact, green in CI).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add observability to MCP service [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41921:
URL: https://github.com/apache/superset/pull/41921#discussion_r3641285286
##########
superset/mcp_service/middleware.py:
##########
@@ -132,6 +138,29 @@ def _sanitize_error_for_logging(error: Exception) -> str:
return error_str
+def _invoke_error_hook(error: Exception, hook_context: dict[str, Any]) -> None:
+ """Invoke the operator-configured ``MCP_ERROR_HOOK``, if any.
+
+ Kept vendor-neutral (no ``sentry_sdk`` import here) so the OSS repo has
+ no hard dependency on any particular error tracker — operators wire
+ their own hook (e.g. calling ``sentry_sdk.capture_exception``) via
+ ``MCP_ERROR_HOOK`` in ``superset_config.py``. Hook failures are logged
+ and swallowed; they must never affect the MCP response.
+ """
+ try:
+ from superset.mcp_service.flask_singleton import get_flask_app
+
+ hook = get_flask_app().config.get("MCP_ERROR_HOOK")
+ except Exception: # noqa: BLE001
+ return
+ if hook is None:
+ return
+ try:
+ hook(error, hook_context)
Review Comment:
**Suggestion:** The configured error hook is invoked synchronously from the async request path, so any slow or blocking hook implementation will block the event loop and stall concurrent tool calls. Execute hook calls off the event loop (or support async hooks with timeout/isolation) so observability side effects cannot degrade request throughput. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Slow MCP_ERROR_HOOK blocks FastMCP async event loop.
- ⚠️ Concurrent MCP tool calls experience increased latency under errors.
- ⚠️ Error bursts can degrade MCP service throughput.
- ⚠️ Affects GlobalErrorHandler and fallback StructuredContentStripper paths.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure an MCP error hook via Flask config as used in `_invoke_error_hook` (lines
141–162 in `superset/mcp_service/middleware.py`): in `superset_config.py` or tests, set
`get_flask_app().config["MCP_ERROR_HOOK"]` to a function that performs blocking work, e.g.
`def slow_hook(error, ctx): import time; time.sleep(5)`.
2. Trigger the hook from the primary error handler by causing a system-class error in a
tool: make a tool implementation raise a database `OperationalError` so
`GlobalErrorHandlerMiddleware._handle_error` (starting around line 773) classifies it as
non-user and calls `_invoke_error_hook(error, {...})` at lines 792–806.
3. Run this flow under an async event loop (the MCP server path uses `async def`
middleware): while invoking the failing tool call from step 2, concurrently schedule
another quick MCP tool call or async task in the same loop.
4. Observe that when `_invoke_error_hook` executes `hook(error, hook_context)`
synchronously at line 159, the event loop is blocked for the duration of `slow_hook` (e.g.
~5 seconds), delaying the concurrent tool call and demonstrating that a slow or blocking
MCP_ERROR_HOOK can stall other in-flight MCP requests.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=36c9affab9f147478df61b42242acc1b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=36c9affab9f147478df61b42242acc1b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/middleware.py
**Line:** 159:159
**Comment:**
*Possible Bug: The configured error hook is invoked synchronously from the async request path, so any slow or blocking hook implementation will block the event loop and stall concurrent tool calls. Execute hook calls off the event loop (or support async hooks with timeout/isolation) so observability side effects cannot degrade request throughput.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41921&comment_hash=c1892d01da304f0ecc93b5dda9c2a707b6997443acd128c95315cafdd26fd53f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41921&comment_hash=c1892d01da304f0ecc93b5dda9c2a707b6997443acd128c95315cafdd26fd53f&reaction=dislike'>👎</a>
##########
superset/mcp_service/middleware.py:
##########
@@ -456,8 +615,38 @@ async def on_call_tool(
# GlobalErrorHandlerMiddleware, ValueError, TypeError, etc. —
# will cause encoding failures on the wire.
mcp_call_id = _mcp_call_id_var.get(None)
+ # This is the documented "must never propagate" point, but
+ # formatting/sanitizing both call str(e) — a pathological
+ # __str__ would make this handler itself raise past the
+ # middleware chain. Fall back to the exception class name.
+ try:
+ error_text = f"Error: {e}"
+ sanitized_message = _sanitize_error_for_logging(e)
Review Comment:
**Suggestion:** This returns raw exception text to the client in the fallback error path, which can leak sensitive internal details (SQL fragments, connection info, tokens) when unexpected exceptions bypass the primary handler. Use the sanitized message for client-facing content and keep raw exception details only for internal logging/hook capture. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ MCP tool clients see raw internal exception messages.
- ⚠️ Sensitive SQL or connection details leaked in responses.
- ⚠️ Bypasses existing _sanitize_error_for_logging protections for fallback.
- ⚠️ Affects StructuredContentStripperMiddleware fallback error responses.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In a test module, import `StructuredContentStripperMiddleware` and
`_sanitize_error_for_logging` from `superset/mcp_service/middleware.py` (see `async def
on_call_tool` starting at line 601 and the `except Exception as e:` block around lines
617–627).
2. Define an async `call_next` stub that always raises a non-ToolError exception with
sensitive text, for example:
`async def call_next(context): raise ValueError("SELECT secret_token FROM users WHERE
password='raw'")`.
3. Construct a minimal `MiddlewareContext` (from
`fastmcp.server.middleware.MiddlewareContext` imported at line 27) and invoke `await
StructuredContentStripperMiddleware().on_call_tool(context, call_next)` so the exception
from step 2 is caught by the `except Exception as e:` block at lines 617–627.
4. Observe that the returned `ToolResult` (constructed at lines 648–651) has
`result.content[0].text` equal to `f"Error: {e}"`, which includes the full unsanitized
exception message (including the SQL and raw secret text), while
`_sanitize_error_for_logging(e)` (line 624) is only used for the hook context and not for
the client-facing error string.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9169da6ab9644ecba8cca82703c9e1d5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9169da6ab9644ecba8cca82703c9e1d5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/middleware.py
**Line:** 623:624
**Comment:**
*Security: This returns raw exception text to the client in the fallback error path, which can leak sensitive internal details (SQL fragments, connection info, tokens) when unexpected exceptions bypass the primary handler. Use the sanitized message for client-facing content and keep raw exception details only for internal logging/hook capture.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41921&comment_hash=b5f5600d2cffaee3415f5b1f4bf5fa3a4c8cf67d8f3daac5525c6eead056c19f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41921&comment_hash=b5f5600d2cffaee3415f5b1f4bf5fa3a4c8cf67d8f3daac5525c6eead056c19f&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add observability to MCP service [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on PR #41921:
URL: https://github.com/apache/superset/pull/41921#issuecomment-5063205976
Thanks for the round-2 pass and the approval — folded the follow-ups into 11fc221ce4cfa180f22d6c7c1b95e460ba933826 so they land here rather than as a separate PR.
**Medium — OAuth swallowed on the `form_data_key` path**: applied exactly your sketch — `_query_from_form_data` now re-raises `(OAuth2RedirectError, OAuth2Error)` ahead of its generic handler, so the caller's outer OAuth handlers produce the redirect. Added `test_oauth2_redirect_on_form_data_key_path` driving the full tool through the client with no identifier and a raising command: asserts `error_type == "OAUTH2_REDIRECT"`, not `DataError`.
**Low — last-resort handler can raise via `str(exc)`**: the stripper's final except now guards its own formatting/sanitizing and falls back to `type(e).__name__` for both the response text and the hook's `sanitized_message`. Regression test with a hostile `__str__` asserts the handler returns `Error: HostileStrError` instead of propagating.
**Low — test gaps**: the two metrics-guard tests now assert the swallowed error is logged (`"Failed to emit MCP tool metrics"`), added a chain-level test pinning the structured-error `.error` bucket through `build_middleware_list()`, and the hostile-name rejection is now also exercised through a reachable registry returning `None` (the production path), not just the regex branch.
**Low — doc nits**: the Sentry example now practices its own caveat (`EventScrubber(recursive=True)` + `send_default_pii=False`, with a pointer to `before_send` for deployment-specific keys), and both the `mcp_config.py` contract comment and the example docstring now spell out per-path availability: `user_id`/`duration_ms` None on the last-resort path, `mcp_call_id` None outside a tool call, `tool_name` → `"unknown"` for non-tool messages.
**Low — structured errors bucketing into `.error`** (design note): leaving as-is in this PR, deliberately. Agreed the tradeoff is real — mapping a curated set of user-class `error_type` strings to `.warning` (or a separate `.structured_error` key) would make `.error` mean "system failure", but with ~100 free-form `error_type` literals across the tools today any curated list would be immediately stale. The right sequencing is the planned `error_type` StrEnum consolidation from the audit (P1); once the vocabulary is closed, classifying structured responses becomes a one-line mapping. Happy to pick that up as the follow-up.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add observability to MCP service [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41921:
URL: https://github.com/apache/superset/pull/41921#issuecomment-5064633542
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add observability to MCP service [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41921:
URL: https://github.com/apache/superset/pull/41921#issuecomment-4931894541
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add observability to MCP service [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41921:
URL: https://github.com/apache/superset/pull/41921#issuecomment-5042837740
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add observability to MCP service [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41921:
URL: https://github.com/apache/superset/pull/41921#discussion_r3627368746
##########
superset/mcp_service/middleware.py:
##########
@@ -363,6 +506,14 @@ async def on_call_tool(
mcp_call_id,
extra,
)
+ await self._emit_call_metrics(
+ context,
+ tool_name,
+ mcp_tool,
+ success=success,
+ raised_is_user_error=raised_is_user_error,
+ duration_ms=duration_ms,
+ )
Review Comment:
**Suggestion:** The metrics emission is awaited directly inside the `finally` block without protection, so any runtime failure in stats logging (for example, backend outage or client error) will raise a new exception and can mask the original tool result/exception. Wrap this call in its own `try/except` and swallow/log metric failures so observability side effects never alter MCP tool behavior. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ MCP tool calls can fail when stats logger misbehaves.
- ⚠️ Original ToolError replaced by metrics RuntimeError in finally.
- ⚠️ Downstream MCP clients see spurious failures unrelated to tools.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `superset/mcp_service/middleware.py:409-517`, inspect
`LoggingMiddleware.on_call_tool`; its `finally` block ends with an awaited call to
`_emit_call_metrics` at lines 509-516.
2. In a test, import `LoggingMiddleware` from `superset/mcp_service/middleware.py` and
construct a middleware instance plus a minimal `MiddlewareContext` and `call_next` that
returns a successful `ToolResult`.
3. Monkeypatch `stats_logger_manager.instance.incr` or `timing` (called inside
`_emit_call_metrics` at lines 398-405) to raise `RuntimeError("metrics backend down")`,
simulating a failing metrics backend or misconfigured client.
4. Await `LoggingMiddleware.on_call_tool(context, call_next)`; `call_next` returns
successfully, but when the `finally` block executes, the awaited `_emit_call_metrics` call
raises `RuntimeError`, causing `on_call_tool` to propagate this new exception instead of
returning the `ToolResult` (or instead of preserving any original exception),
demonstrating that metrics failures can alter or mask the real tool outcome.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d3fdc50dc261493fb50121ee028a8f48&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d3fdc50dc261493fb50121ee028a8f48&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/middleware.py
**Line:** 509:516
**Comment:**
*Possible Bug: The metrics emission is awaited directly inside the `finally` block without protection, so any runtime failure in stats logging (for example, backend outage or client error) will raise a new exception and can mask the original tool result/exception. Wrap this call in its own `try/except` and swallow/log metric failures so observability side effects never alter MCP tool behavior.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41921&comment_hash=572fd6e20321135030b6088448966238c2308d3ad5a30ec6f5789fa6aa786d27&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41921&comment_hash=572fd6e20321135030b6088448966238c2308d3ad5a30ec6f5789fa6aa786d27&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add observability to MCP service [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #41921:
URL: https://github.com/apache/superset/pull/41921#discussion_r3627338000
##########
superset/mcp_service/PRODUCTION.md:
##########
@@ -217,19 +217,49 @@ All MCP tools return consistent error schemas:
**Error Tracking (Sentry)**:
+MCP tool execution runs on the FastMCP/Starlette asyncio stack, not Flask
+request handling, so `FlaskIntegration` alone does **not** capture MCP tool
+errors — it only sees the regular Superset web app's Flask requests. Use the
+vendor-neutral `MCP_ERROR_HOOK` config to forward system-class MCP errors
+(unexpected exceptions — not user errors like bad params or permission
+denials) to Sentry instead:
+
```python
# superset_config.py
import sentry_sdk
from sentry_sdk.integrations.flask import FlaskIntegration
sentry_sdk.init(
dsn="https://your-dsn@sentry.io/project-id",
- integrations=[FlaskIntegration()],
+ integrations=[FlaskIntegration()], # covers the Flask web app only
environment="production",
traces_sample_rate=0.1, # 10% of transactions
)
+
+
+def _mcp_error_hook(error: Exception, context: dict) -> None:
+ """Forward system-class MCP tool errors to Sentry.
+
+ ``context`` includes tool_name, mcp_call_id, user_id, error_type,
+ sanitized_message, and duration_ms — already sanitized by the MCP
+ service before this hook runs.
+ """
+ with sentry_sdk.push_scope() as scope:
Review Comment:
Applied in 46c2332a6aca40a05c29223c187bfc891f9335cb — switched to `sentry_sdk.new_scope()`.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add observability to MCP service [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #41921:
URL: https://github.com/apache/superset/pull/41921#discussion_r3649029429
##########
superset/mcp_service/middleware.py:
##########
@@ -456,8 +615,38 @@ async def on_call_tool(
# GlobalErrorHandlerMiddleware, ValueError, TypeError, etc. —
# will cause encoding failures on the wire.
mcp_call_id = _mcp_call_id_var.get(None)
+ # This is the documented "must never propagate" point, but
+ # formatting/sanitizing both call str(e) — a pathological
+ # __str__ would make this handler itself raise past the
+ # middleware chain. Fall back to the exception class name.
+ try:
+ error_text = f"Error: {e}"
+ sanitized_message = _sanitize_error_for_logging(e)
Review Comment:
Good catch — fixed in 38dbdbd80b8e029e6eb27d88a4d26d68df8d19cd. The last-resort client-facing text now runs through `_sanitize_error_for_logging` (the same sanitizer `GlobalErrorHandlerMiddleware` and every other error path already use), so an exception that bypasses the primary handler can't leak connection strings/tokens/SQL to the caller. Kept the guarded fallback to `type(e).__name__` for a pathological `__str__`. Added a regression test asserting an embedded `postgresql://user:pass@host/db` is redacted from the response.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org