util/log: Combine the warning/error log levels into a single alert level#34730
util/log: Combine the warning/error log levels into a single alert level#34730ajtowns wants to merge 2 commits intobitcoin:masterfrom
Conversation
-BEGIN VERIFY SCRIPT-
sed -i 's/LogWarning(/LogAlert(/g; s/LogError(/LogAlert(/g; s/\(LogAlert("[^\"]*\)\\n\("[,)]\)/\1\2/g' src/node/blockstorage.cpp
-END VERIFY SCRIPT-
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. |
ryanofsky
left a comment
There was a problem hiding this comment.
Code review ACK 4ce720b. Concept -0.
This PR is definitely not the change I would make, but it seems fine and helps decrease the gap between way the macros are actually used and the way they were originally intended to be used.
IMO, a more ideal solution would:
- Keep the error and warning levels instead of removing them, because
[error][warning]tags shown in log output are helpful for distinguishing actual problems from potential problems from informational messages. - Make error and warning log levels conditional (users can configure whether messages are logged) instead of unconditional.
- Introduce a new "critical" level and
LogCriticalmacro which is unconditional and always logs even when debug logging is turned off. To me "critical" sounds more negative and "alert" sounds more fake like "Alert! Walmart's prices are lower than competitors". But obviously the word choice is not important.
There's little benefit in having two error levels that the node operator should pay attention to when the only difference between them is that one means the node is about to shut down -- the admin can easily see that just by looking at whether the node did shutdown. So this PR replaces LogWarning and LogError with LogAlert, which hopefully emphasises that the point of using that level is to alert the node operator.
Leaves LogWarning and LogError available as deprecated aliases to reduce code churn and avoid invalidating PRs.
Left as draft pending #34038 as the "warning" and "error" levels are currently exposed to users via the
-loglevelconfig option.