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: [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]
