doc: Drop description of LogError messages as fatal #30361

1. 
   ryanofsky commented at 10:18 pm on June 28, 2024: contributor

   Remove documentation that says LogError should be used for “severe problems that require the node (or a subsystem) to shut down entirely” because:

   • It is not how LogError and Level::Error are used currently. Of 129 current uses only 58 cases are fatal according to #30348

   • “[T]here’s not much benefit in a log that says “hey this error is about to cause the node to stop” – you already get that information by seeing “Shutdown: in progress…” immediately following.” according to #30347 (comment)

   Fixes #30348

2. 
   DrahtBot commented at 10:18 pm on June 28, 2024: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #30364 (logging: Replace LogError and LogWarning with LogAlert by ryanofsky)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

3. DrahtBot added the label Docs on Jun 28, 2024
4. 
   ajtowns commented at 11:58 pm on June 28, 2024: contributor

   I think both entries should continue to include examples.

   I think removing “severe” from the LogWarning description doesn’t make sense – anything the admin might have to address is a severe (potential) problem, isn’t it?

   Having LogError mean “the node admin will need to address” vs LogWarning mean “the node admin may need to address” seems to argue towards retaining the “LogError is fatal” interpretation to me – fatal errors are ones the admin does need address; anything else they can ignore until it becomes fatal.

   A potential advantage of having LogError (or LogFatal) reserved for fatal errors is that you could grep through the codebase for LogError to see all the potentially fatal errors, and perhaps work on finding ways to make them less fatal. Not convinced that’s very beneficial though.

   I don’t think there’s much other benefit to splitting potential problems up from actual problems otherwise if the admin still needs to attend to them either way, though; so if we’re already making LogError just mean actual errors; is there any reason not to merge them both into a single LogAlert() or similar?

5. doc: Drop description of LogError messages as fatal

   Remove documentation that says LogError should be used for "severe problems
   that require the node (or a subsystem) to shut down entirely" because:
   
   - This is not how `LogError` and `Level::Error` are used currently. Of 129
     current uses only 58 cases are fatal according to
     https://github.com/bitcoin/bitcoin/issues/30348
   
   - "[T]here's not much benefit in a log that says "hey this error is about to
     cause the node to stop" -- you already get that information by seeing
     "Shutdown: in progress..." immediately following." according to
     https://github.com/bitcoin/bitcoin/pull/30347#discussion_r1659096893

   b7aae361b2
6. ryanofsky force-pushed on Jun 30, 2024
7. 
   ryanofsky commented at 3:55 pm on June 30, 2024: contributor

   re: #30361 (comment)

   Thanks, added back examples and “severe”.

   I think probably just looking at AbortNode callers would be as effective as as searching for fatal error messages, if looking for places to make error handling more robust.

   I do like the alert level idea and posted an implementation in #30364.

   Updated 5175cbcbb00d6212cf548de428f43cb0c2a1be4b -> b7aae361b273f2f439d3b278214b7e37908c8cb0 (pr/nofat.1 -> pr/nofat.2, compare) with clarifications and examples.

8. ryanofsky marked this as ready for review on Jun 30, 2024

Contributors
ryanofsky DrahtBot ajtowns

Labels
Docs