-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Reduce log pollution with unnecessary stack traces #4966
Comments
My main suggestion on this is: Use the logging framework's configuration as much as possible. Then you have fewer decisions to make each time you add a log call, and one place where you can make updates on policy or add switches for different use cases. |
Investing more in the logger's configuration may also involve a side discussion: Do we want to stick with slf4j? I think that part of the java ecosystem has shifted since we adopted it. |
I'm a bit concerned about configuring stack traces to not be present by default (please correct me if I read that wrong), because until we are at the point at which we improved the logging and error msg throughout the code base, we'll be likely hit with a lot of bug reports that are very hard to debug because we have neither the stack traces nor other relevant info. Edit: what we could do is keep the stack traces configuration-wise at least when we start games from launcher. |
I think for this discussion we'll want to grab a few log files as case studies to center the conversation, and take notes on them as we go. I am imagining this with a bunch of printouts and markers or crayons on a table, but I guess we will have to figure out some remote-friendly way to do it. 😆 |
To start with some examples: Example 1This log was noticed when testing #4962
Issues with this log:
Example 2This log can be reproduced currently (might be fixed in the future) by entering the "Join Game" menu.
Issues with this log:
|
Example 1 (StateMainMenu.java:86)Puzzling that it is printed twice. I wonder, does it appear in the log file twice? Or is one of those the log, and the other one is what is written to stderr on the terminal when the process exits due to exception? If that is what's happening, I'm not sure which of them to suppress. 🤔 It does seem like the stack trace is less than useful, as it came though #processPendingState and it would be more useful to know what caused the state change, but you can't, in general, know what is a few levels up the stack when deciding to add a log message. We do agree (as recently mentioned in #4960) that we want better patterns for managing these state transitions and their error handling in general. In the short term, perhaps we should move that exception (because it's conditional based on the |
Example 2 (ServerListDownloader)I think I do not share your expectations about what an error being "non-fatal" has to do with the presence or absence of stack traces in the log. That is, this didn't result in a crash to desktop, but it clearly was fatal to ServerListDownloader. Something exceptional happened and it couldn't complete its job and now the system is in a state where it can't let the user do what they'd expected to do. Logging something about how it got in to that state sounds like an appropriate use of Though in this case we can see that the author had some awareness that this might not be the best way to handle this exception: Lines 87 to 92 in 43e1b90
Using Reactor's schedulers for asynchronous operations gives us an alternative to the suggested
Hmm. It takes a lot of special-case error handling logic to recognize that, as If cost were no issue, I agree we'd want error handling here that would give relevant guidance on the error. "Plug in the network cable" or "re-check the server hostname in the configuration" or "see status.github.com for updates, go for a walk until it comes back online"—depending on the type of exception. Is this an error we expect to happen often enough to deserve that level of attention? |
Motivation
It's hard to find relevant information in the log output if it's polluted with stack traces of non-fatal errors or errors that are gracefully handled and mask the actual issue.
Proposal
Investigate options to reduce non-relevant information in the log, including
--verbose
flag enabling stack traces (and disabling them by default)Consideration that may help with the decision-making for these options:
Additional notes
There's no general "this is the right way to do this".
But we should investigate, brainstorm and discuss how we want to handle it.
Also, once we got to an agreement, we might want to document this in the contributor guides.
The text was updated successfully, but these errors were encountered: