fix(logging): emit log without stack trace when exception is in disable_stack_trace#4588
Open
skylarkoo7 wants to merge 4 commits intolitestar-org:mainfrom
Open
fix(logging): emit log without stack trace when exception is in disable_stack_trace#4588skylarkoo7 wants to merge 4 commits intolitestar-org:mainfrom
skylarkoo7 wants to merge 4 commits intolitestar-org:mainfrom
Conversation
…le_stack_trace When an exception's status code or class matched an entry in `disable_stack_trace`, the entire `handle_exception_logging` call was short-circuited — neither the stack trace nor a one-line error log was emitted. The option's documented intent is to suppress the stack trace while still logging that the exception occurred. Separate the "should log" decision from the "include stack trace" decision: - When the exception matches `disable_stack_trace`, the handler is still invoked but receives an empty traceback list (`[]`). - The default exception logging handlers now check whether `tb` is non-empty: `logger.exception()` (with traceback) when it is, `logger.error()` (without traceback) when it is not. Updated both standard-lib and structlog handler factories, plus their corresponding tests. Fixes litestar-org#4510
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4588 +/- ##
=======================================
Coverage 97.85% 97.85%
=======================================
Files 297 297
Lines 15339 15347 +8
Branches 1721 1723 +2
=======================================
+ Hits 15010 15018 +8
Misses 188 188
Partials 141 141 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Extract exception_logging_handler into a local variable before the compound should_log condition so mypy/pyright can narrow the type from Optional to non-None inside the if-block. - Add test_default_handler_uses_error_when_stack_trace_suppressed for both standard logging and structlog to exercise the logger.error branch in _default_exception_logging_handler_factory.
- Restructure handle_exception_logging so `handler is not None` is an inline condition in the if-statement, allowing mypy/pyright to narrow the type correctly (stored bool variables don't propagate narrowing). - Replace integration-style default handler tests with direct unit tests of _default_exception_logging_handler_factory using a MagicMock logger. This avoids the AttributeError from picologging.Logger whose attributes are read-only and cannot be patched. - Fix import ordering in test_structlog_config.py (ruff I001).
The handler factory returns a callable expecting Scope (Union[HTTPScope, WebSocketScope]). Passing a plain dict literal triggers arg-type errors. Using Any silences the checkers while keeping the test readable.
Member
|
#4593 is probably a better place for this update. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
disable_stack_tracesuppresses exception logging entirely instead of only suppressing the stack trace. When an exception's status code or class matches an entry indisable_stack_trace, thehandle_exception_loggingmethod short-circuits completely — no log line is emitted at all, not even a one-line error message.The fix separates the "should we log?" decision from the "should we include the stack trace?" decision:
disable_stack_tracedisable_stack_trace, the handler receives an empty traceback list ([]) instead of the formatted exceptionChanges
litestar/middleware/_internal/exceptions/middleware.py— split thehandle_exception_loggingconditional into a "should log" check and a separate "include stack trace" check; pass[]when stack trace is suppressedlitestar/logging/config.py— default exception logging handlers (both standard-lib and structlog) now checktb: uselogger.exception()when traceback is present,logger.error()when it is emptytests/unit/test_logging/test_logging_config.py— updatedtest_disable_stack_traceto assert the handler IS always called, and thattbis empty when stack trace is suppressedtests/unit/test_logging/test_structlog_config.py— same update fortest_structlog_disable_stack_traceTest plan
tbis non-empty when stack trace is expected,tbis[]when stack trace is suppressedtbargument will see[]for suppressed exceptions and can decide their own behaviordisable_stack_traceis empty (the default)Fixes #4510