eschutho opened a new pull request, #45849:
URL: https://github.com/apache/superset/pull/45849
### SUMMARY
`get_chart_data` (MCP) logged every data-retrieval failure with
`logger.error`. When a chart's query fails in the underlying database (e.g.
`SQL compilation error ... invalid identifier`, or a missing table),
`ChartDataCommand.run()` raises `ChartDataQueryFailedError` (a
`CommandException`). That failure reaches the generic `except
(CommandException, SupersetException, ValueError)` handler (it is not caught by
the earlier `_VALIDATION_ERRORS` / `SupersetSecurityException` handlers). It is
already reported to the caller via `ctx.error(...)` and returned as
`ChartError(error_type="DataError")`, yet the ERROR-level log makes each such
user/data-side failure look like a service fault in error tracking.
**Fix:** in that handler, log at `WARNING` (with the bounded error text, not
raw SQL, like the sibling warnings) when the exception is a
`ChartDataQueryFailedError`; every other exception in the tuple still logs at
`ERROR`. The `ctx.error` call and returned `ChartError` are unchanged, so
behavior differs only in log level.
**Tradeoffs:** `ChartDataQueryFailedError` is raised for any query execution
failure, so it also covers database-infrastructure failures (connectivity,
timeouts, warehouse outages), not only mistakes in the customer's query. Those
are now also downgraded from ERROR to WARNING. They are still logged and still
returned to the caller via `ctx.error` and `ChartError`; only the severity
changes. Distinguishing the two would need a richer error classification from
the command layer and is out of scope here.
**Follow-ups:** the unsaved-chart (`form_data_key`) path in the same module
has a similarly shaped `logger.error("Domain error querying unsaved chart
data")` handler, and the outer `Domain error in get_chart_data` handler also
logs at ERROR. They are left untouched to keep this change surgical.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
### TESTING INSTRUCTIONS
`pytest tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py`
Added `test_saved_chart_data_error_log_level`, which drives the real MCP
tool with a command stand-in that raises and asserts the returned `DataError`
and the log level: `ChartDataQueryFailedError` logs only at WARNING;
`SupersetException` and `ValueError` still log at ERROR. The WARNING case fails
without the source change (it logs at ERROR). Full file: 314 passed. `ruff
check` and `ruff format` are clean on the touched files.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
