Skip to content

feat: centralized error management with Data360MCPError hierarchy#38

Open
rafmacalaba wants to merge 5 commits intodevfrom
feat/error-management
Open

feat: centralized error management with Data360MCPError hierarchy#38
rafmacalaba wants to merge 5 commits intodevfrom
feat/error-management

Conversation

@rafmacalaba
Copy link
Collaborator

@rafmacalaba rafmacalaba commented Mar 6, 2026

Summary

Addresses #19

Note

This branch is based on feat/improve-tool-descriptions (not yet merged to dev). Merge that branch first before merging this PR.

Centralizes error management by introducing a Data360MCPError class hierarchy, replacing scattered try/except blocks across the codebase with structured, LLM-actionable error responses.

Changes

src/data360/errors.py [NEW]

  • Data360MCPError base exception with error_code, detail, and to_dict() serialization
  • Subclasses: APIError, Data360TimeoutError, RequestError, ParseError, ValidationError, NotFoundError
  • classify_error() converter for clean exception handling
  • Message registry mapping error codes to LLM-actionable messages

src/data360/api.py

  • Refactored _search_raw(), get_metadata(), get_disaggregation(), get_data() to raise/catch Data360MCPError subclasses instead of raw exceptions

src/data360/visualization.py

  • get_viz_spec now returns a "warning" key when Draco fails to find an optimal encoding and a fallback line chart is generated

tests/test_errors.py [NEW]

  • 18 tests covering all error classes and classify_error mapping logic

tests/test_visualization.py [NEW]

  • 3 tests: fallback returns warning, Draco success has no warning, both-fail returns error

tests/test_api.py

  • Updated assertions to match new error message format

Test Results

All 41 tests pass with no regressions.

@rafmacalaba rafmacalaba requested a review from avsolatorio March 8, 2026 03:50
Copy link
Owner

@avsolatorio avsolatorio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

Not a blocker, but wondering what your view is about adding the logging to the Data360MCPError? Essentially, creating an instance of the Data360MCPError with an optional logger argument automatically logs self.detail after super().__init__(self.detail).

@rafmacalaba
Copy link
Collaborator Author

rafmacalaba commented Mar 18, 2026

My view is that it's worth doing, the current code has inconsistent coverage, where directly constructed errors like NotFoundError and ValidationError have no logging at all while some call sites log manually. The suggestion closes that gap cleanly.

The one thing I'd adjust is the mechanism: rather than an injected logger argument (which still requires callers to opt in), I'd prefer a module-level _logger in errors.py that logs unconditionally in __init__. Same outcome, no risk of a call site forgetting to pass the logger. I'd also add a log_level class attribute so subclasses like NotFoundError can log at WARNING instead of ERROR — a 'no results found' condition shouldn't show up in error dashboards.

Implemented this.

@avsolatorio
Copy link
Owner

Hello @rafmacalaba , thank you for considering adding the centralized logging for the errors! Wanted to merge this, but I see there's a conflict. Could you please take a look at it? Thanks!

Addresses #19: Implement centralized error management.

- Add src/data360/errors.py with Data360MCPError base class and subclasses:
  APIError, TimeoutError, RequestError, ParseError, ValidationError, NotFoundError
- Add from_httpx_error() converter for clean httpx exception handling
- Add message registry mapping error codes to LLM-actionable messages
- Refactor api.py: replace scattered try/except blocks in _search_raw(),
  get_metadata(), get_disaggregation(), get_data() with centralized errors
- Update test assertions to match new error message format
- All 20 existing tests pass
When Draco cannot determine an optimal encoding (StopIteration), the
function now returns a 'warning' key in the response dict alongside the
URL. Previously the fallback was silent and indistinguishable from a
Draco success.

Added tests/test_visualization.py with 3 test cases:
- Fallback returns warning key
- Draco success has no warning key
- Both Draco and fallback fail returns error
- Move _FALLBACK_WARNING to module-level constant
- Update docstring to document the optional 'warning' key
- Mock get_codelist_mapping in tests to avoid real network calls
- Rename TimeoutError to Data360TimeoutError to avoid shadowing builtin
- Rename from_httpx_error to classify_error (handles non-httpx exceptions)
- Add tests/test_errors.py with 18 tests covering all error classes and
  classify_error mapping logic
- All 41 tests pass
…log levels

- Add module-level _logger to errors.py; Data360MCPError.__init__ now
  logs automatically on construction via _logger.log(self.log_level, ...)
- Fix exc_info: use (type, value, traceback) tuple so tracebacks are
  captured even when original_error was never re-raised
- Add log_level class attribute (default ERROR); NotFoundError overrides
  to WARNING so 'no results' queries don't pollute error dashboards
- Remove 12 redundant manual _logger.* calls from api.py that duplicated
  the now-automatic logging
- Add autouse suppress_error_logs fixture in test_errors.py to prevent
  noisy log output during existing tests
- Add TestLogging with 4 tests: auto-log fires, NotFoundError at WARNING,
  traceback captured for raised exceptions, no exc_info without original_error

45 tests pass, 100% coverage on errors.py
@rafmacalaba rafmacalaba force-pushed the feat/error-management branch from a4f5544 to 8db5a1e Compare March 18, 2026 11:13
@rafmacalaba
Copy link
Collaborator Author

Hello @rafmacalaba , thank you for considering adding the centralized logging for the errors! Wanted to merge this, but I see there's a conflict. Could you please take a look at it? Thanks!

Fixed the conflicts! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants