-
-
Notifications
You must be signed in to change notification settings - Fork 339
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(logging): do not log exceptions twice, deprecate traceback_line_limit
and fix pretty_print_tty
#3507
fix(logging): do not log exceptions twice, deprecate traceback_line_limit
and fix pretty_print_tty
#3507
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3507 +/- ##
=======================================
Coverage ? 98.28%
=======================================
Files ? 328
Lines ? 14848
Branches ? 2358
=======================================
Hits ? 14593
Misses ? 116
Partials ? 139 ☔ View full report in Codecov by Sentry. |
af04969
to
a2b292a
Compare
Can you ELI5 these please? |
@peterschutt Basically, that's what I tried to do with the To sum up:
|
Thanks for that. I think this means you'd need to point this at the v3 branch, unless you think there's a way you can do this in a non-breaking way. However, on the topic of pointing this as v3, you should also read #2858 |
@peterschutt Thank you for pointing me to this issue. Is there a timeline for a v3 realase? In fact, I have this PR but I also started working on other improvements on another branch a while ago. So I'm wondering if it's worth to continue the work. If v2 is still around for some months (like 4+ months), I think it might be worth.
I can probably rollback the change on My take: When you discover a new framework, you generally start a new project and one of the first tasks is to setup the logging. The current state of litestar logging doesn't inspire trust in the framework. So it might be interesting to solve what can be easily solved, maybe at the cost of minor breaking changes. And if v3 can get rid of the logging complexity in the future, it would be even better. |
No timeline at this point.
V2 will still be around for a long time, we'll continue to support it just as we currently support V1 (aka starlite). V2 will go into maintenance mode once V3 is out.
Perhaps we should bite the bullet and deprecate the use of LoggingConfig in the next minor release? And at the same time, replace the usage documentation for logging with examples of how to configure popular logging libraries alongside litestar. It would be a step in the direction of #2858 (comment), without yet making changes that would break existing code. |
Yep, I read #2858 (comment) and that's what I was refering to in my previous message. I already had the same feeling. But I feel like deprecating I think that what I call "small breaking changes" here are easy to adapt for the minority of users impacted (who should be familiar with Despite of the complexity behind When I discovered Litestar and started a project a few weeks ago, with Python 3.12, the According to #3482 I'm not the only one to struggle. So my main goal is just to get decent logging defaults out of the box for new users. I fixed the Depending on your final decision, I could continue this work and we could make a single release with all these logging breaking changes (but trying to limit them). Or I could stop the work around this subject. Just let me know :) |
If we are to make the proposed changes in v3, the current behavior must be deprecated in v2 - the functionality won't go away, we just wouldn't be suggesting its use.
To me, this is another indicator that this facet of our code base should be deprecated, and removed. If so many surface-level issues are present, how much could the feature possibly be in use in the wild? I for one don't use it.
@litestar-org/maintainers - any input here? |
I agree with you on this. Our current logging config tries to do way to much, i.e. being a one-stop shop to configure various different logging libraries for different scenarios, with no real benefit. We don't need to be able to configure all of this just to get access to a logger anywhere in Litestar, and it would probably best if we leave this all up to the user. |
@provinzkraut While we agree on this, the question now is about this PR and the future one I might open. To sum up, Here are the breaking changes I have mentioned for this PR in a previous comment, with more context (in bold):
|
Hey @jderrien, sorry for the delay here.
As @peterschutt pointed out, if there are breaking changes, even if they are minor, we can't have this go into 2.x. Maybe there is a way to make these things non-breaking somehow.
This seems to have been reverted already.
I think this is not that problematic and might go in. I'm not sure whether we consider the exact wording of log messages part of the public API and therefore changes to them breaking, but I'm inclined to say this is not the case.
Is this necessary? From what I understand, this just tidies things up a bit. So while I'd prefer not introducing any more configuration, maybe this is the way to go? Add a flag to exclude it, and have it off by default. Removing the traceback could be a major breaking change IMO if people rely on it.
Maybe this one could be fixed by using e.g. With all these alterations in place, would you say this PR still accomplished its goal? If so, I'd be okay with having it go in like that. |
c830a4e
to
ecd80be
Compare
Hey @provinzkraut, I've update the PR. No more breaking changes in the API. About removing
I'd say this is necessary since #3228 is "exceptions are logged twice". So we have to remove this field for structlog to fix the issue. The full stacktrace was and is still logged in the AFAIK, it might break some external tool setup (i.e. log parsing). But this is almost the same risk as updating the wording of log messages. So not really a breaking change (?). In the end, this is easy to update for users. I think the release note should just make it clear. The outcome is:
I don't think we should wait v3 release to fix that. About adding I've reverted it. And I wasn't comfortable with keeping the feature in the standard logging config while removing it in the structlog one. I'm wondering if a deprecation warning is enough (since they're masked by default)? For a possible release, if I don't miss anything, the outline might be: Exception logging:
|
Thanks for the effort @jderrien! This is good to go in from my side. The coverage decrease is expected since you're deleting tests, so that can be ignored! |
8dcabb8
to
b85fc38
Compare
b85fc38
to
4b1c1fd
Compare
pretty_print_tty
traceback_line_limit
and fix pretty_print_tty
4b1c1fd
to
a6338ba
Compare
Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/3507 |
Description
Currently we're logging the exceptions twice: the full one and a truncated one.
This PR tries to improve this,
which implies small breaking changes.Almost finished but still a WIP, I need some feedback please!Outlines:
traceback
field in the log message (which contained a truncated stacktrace) has been removed. Theexception
field is still around and contains the full stacktrace.traceback_line_limit
has been deprecated. The value is now ignored, the full stacktrace will be logged.Summary
❓ TODO Anyway, to solve the original issue (#1294), truncating the request's body would have been a better solution. I'd be happy to dig a little bit more here. Any idea about how to reproduce the issue? (I already tried to write a test with a file upload, but the request's body doesn't get printed)
TypeExceptionLoggingHandler
and signature ofhandle_exception_logging()
get updated by this PR, seems better and more flexible this way.Logging
✅ Behavior is still the same, just slightly changed how we count the lines to display (see the code and the testtest_traceback_truncate_default_logging
).✅ Ability to settraceback_line_limit
toNone
.⚠️ Default value for , I don't think we should truncate stacktraces by default.traceback_line_limit
isNone
❓ → ✅ Maybe we could deprecate
traceback_line_limit
to keep things in sync with Structlog. And if it's really something wanted by users, they can bring it back easily via a customexception_logging_handler
.ℹ️ Updated log messages to get something similar to structlog.
StructLog
✅ Fixed a bug with
pretty_print_tty
(as a separated commit).✅ When
pretty_print_tty=True
, we don't log the stacktrace twice and the remaining one gets truncated automatically by structlog (see below).✅ When
pretty_print_tty=False
, exception gets logged as json. Previously it was logged in 2 separate fields (exception
contains the full stacktrace &traceback
a truncated version). This PR removestraceback
. It will reduce the log size. It's still possible to parse the JSON after, with some tools, to limit the lines displayed.❓ → ✅ If it's fine on your side, can I mark
traceback_line_limit
as deprecated and document it?Extras
structlog.testing.capture_logs
. When using it to capture thelogging.exception()
output, the traceback is not yet rendered. That's something to keep in mind.❓ What about turning
log_exceptions
toalways
by default? (Hiding errors by default seems a bad choice according to my sysadmin background).Structlog: before & after
With
pretty_print_tty=False
Before, both
traceback
andexception
were logged:After, we only log
exception
and the global size is reduced:With
pretty_print_tty=True
:Before, the truncated
traceback
value doesn't really help.After (more concise):
Closes
Closes #3228