Severity-based logging -- parent PR #25203
Conversation
|
This partially depends on #25202 and needs to be updated after the merge of that PR for the change in refactor: Change LogPrintLevel order to category, severity, so leaving in draft for now. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
c1b9fe7 to
bbf52db
Compare
ea89754 to
3168b62
Compare
| LogPrint(BCLog::NET, "Large reorg, won't direct fetch to %s (%d)\n", | ||
| pindexLast->GetBlockHash().ToString(), | ||
| pindexLast->nHeight); | ||
| LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "Large reorg, won't direct fetch to %s (%d)\n", |
There was a problem hiding this comment.
Is the switch to unconditional logging intended here?
If yes, then it's probably better done in a separate commit because it is not immediately clear that this is safe and it's quite easy to miss mixed in with all the other changes in this commit.
There was a problem hiding this comment.
Yes, good point that any changes from conditional to unconditional logging should be separated out.
| if (level >= BCLog::Level::Warning) return true; | ||
| // Log the Info, Warning, and Error message levels unconditionally, so that | ||
| // important troubleshooting information isn't lost. | ||
| if (level >= BCLog::Level::Info) return true; |
There was a problem hiding this comment.
I feel like this almost deserves its own PR (maybe you are already planning on doing so). afaict we don't log anything right now using Info which is the only reason this change is safe but that might not be clear to reviewers of this rather large PR. (If for example we had a lot of log locations using Info then they would all need to be revisited w.r.t. to disk filling attacks)
There was a problem hiding this comment.
Good point. My original thinking was that info would be conditional like debug and trace and that only warning and error would be unconditional, which would also reduce the volume of default logging. However, that changed during the discussions in #25306. Will keep your suggestion in mind while restructuring the update here.
There was a problem hiding this comment.
Noting here that this change was done in #28318, resolving.
|
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
Updating and rebasing. |
|
I guess I've got conceptual questions about this.
if (level == TRACE) return (m_trace_categories.load(relaxed) & category) != 0;
else if (level == DEBUG) return (m_debug_categories.load(relaxed) & category) != 0;
else return true;
|
|
Thanks for the detailed feedback, @ajtowns. Will update with it in mind; looks like it will be helpful in simplifying things. |
8c47d59 doc: improve -debuglogfile help to be a bit clearer (jonatack) 20d89d6 bench: document expected results in logging benchmarks (jonatack) d8deba8 bench: add LogPrintfCategory and LogPrintLevel benchmarks (Jon Atack) 102b203 bench: order the logging benchmark code by output (Jon Atack) 4b3fdbf bench: update logging benchmark naming for clarity (Jon Atack) 4684aa8 bench: allow logging benchmarks to be order-independent (Larry Ruane) Pull request description: Update our logging benchmarks for evaluating ongoing work like #25203 and refactoring proposals like #26619 and #26697. - make the logging benchmarks order-independent (Larry Ruane) - add missing benchmarks for the `LogPrintLevel` and `LogPrintfCategory` macros that our logging is migrating to; at some later point it should be feasible to drop some of the previous logging benchmarks - update the logging benchmark naming to be clear which benchmark corresponds to which log macro, and update the ordering to be the same as the output - add clarifying documentation to the logging benchmarks - improve the `-debuglogfile` config option help to be clearer; can be tested by running `./src/bitcoind -help | grep -A4 '\-debuglogfile'` Reviewers can run the logging benchmarks with: ```bash ./src/bench/bench_bitcoin -filter='LogP*.*' ``` ACKs for top commit: LarryRuane: ACK 8c47d59 martinus: code review & tested ACK 8c47d59, here are my benchmark results: achow101: ACK 8c47d59 Tree-SHA512: 705f8720c9ceaf14a1945039c7578a0c17a12215cbc44908099af4ac444561c3f95d833c5a91b325cdd4470737d8a01e2da64db2d542dd7c9a3747fbfdbf213e
8c47d59 doc: improve -debuglogfile help to be a bit clearer (jonatack) 20d89d6 bench: document expected results in logging benchmarks (jonatack) d8deba8 bench: add LogPrintfCategory and LogPrintLevel benchmarks (Jon Atack) 102b203 bench: order the logging benchmark code by output (Jon Atack) 4b3fdbf bench: update logging benchmark naming for clarity (Jon Atack) 4684aa8 bench: allow logging benchmarks to be order-independent (Larry Ruane) Pull request description: Update our logging benchmarks for evaluating ongoing work like bitcoin#25203 and refactoring proposals like bitcoin#26619 and bitcoin#26697. - make the logging benchmarks order-independent (Larry Ruane) - add missing benchmarks for the `LogPrintLevel` and `LogPrintfCategory` macros that our logging is migrating to; at some later point it should be feasible to drop some of the previous logging benchmarks - update the logging benchmark naming to be clear which benchmark corresponds to which log macro, and update the ordering to be the same as the output - add clarifying documentation to the logging benchmarks - improve the `-debuglogfile` config option help to be clearer; can be tested by running `./src/bitcoind -help | grep -A4 '\-debuglogfile'` Reviewers can run the logging benchmarks with: ```bash ./src/bench/bench_bitcoin -filter='LogP*.*' ``` ACKs for top commit: LarryRuane: ACK 8c47d59 martinus: code review & tested ACK 8c47d59, here are my benchmark results: achow101: ACK 8c47d59 Tree-SHA512: 705f8720c9ceaf14a1945039c7578a0c17a12215cbc44908099af4ac444561c3f95d833c5a91b325cdd4470737d8a01e2da64db2d542dd7c9a3747fbfdbf213e
|
Closing temporarily so that I can re-open it -- am still interested to finish this. |
|
Have been waiting for bugfix #27231 to be merged to update here, but it seems useful to re-open it before so that people are aware of it. Given how long the bugfix has been open, I'll propose further non-conflicting steps from this parent without waiting further. |
Much of the added value from moving to severity-level logging IMO comes from splitting LogPrint to trace or debug (so, non-automatically), e.g. for
Consistency and least astonishment, which is clearer for regular users. Logging the category for error vs warning vs info seems useful to me. Perhaps I misunderstand your comment.
The idea is for |
|
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
…logged levels ec779a2 doc: add unconditional info loglevel following merge of PR 28318 (Jon Atack) Pull request description: Commit ab34dc6 of #28318 was an incomplete version of [`118c756` (#25203)](118c756) from the `Severity-based logging` parent PR. Add the missing text to update the `-loglevel` help doc. While here, make the help text a little easier to understand. Can be tested by running: ``` ./src/bitcoind -regtest -help-debug | grep -A12 loglevel= ``` before ``` -loglevel=<level>|<category>:<level> Set the global or per-category severity level for logging categories enabled with the -debug configuration option or the logging RPC: info, debug, trace (default=debug); warning and error levels are always logged. ``` after ``` -loglevel=<level>|<category>:<level> Set the global or per-category severity level for logging categories enabled with the -debug configuration option or the logging RPC. Possible values are info, debug, trace (default=debug). The following levels are always logged: error, warning, info. ``` ACKs for top commit: stickies-v: ACK ec779a2 Tree-SHA512: 0c375e30a5a4c168ca7d97720e8c287f598216767afedae329824e09a480830faf8537b792c5c4bb647c68681c287fe3005c62093708ce85624e9a71c8245e42
|
Closing to be able to re-open it. |
…always logged levels ec779a2 doc: add unconditional info loglevel following merge of PR 28318 (Jon Atack) Pull request description: Commit ab34dc6 of bitcoin#28318 was an incomplete version of [`118c756` (bitcoin#25203)](bitcoin@118c756) from the `Severity-based logging` parent PR. Add the missing text to update the `-loglevel` help doc. While here, make the help text a little easier to understand. Can be tested by running: ``` ./src/bitcoind -regtest -help-debug | grep -A12 loglevel= ``` before ``` -loglevel=<level>|<category>:<level> Set the global or per-category severity level for logging categories enabled with the -debug configuration option or the logging RPC: info, debug, trace (default=debug); warning and error levels are always logged. ``` after ``` -loglevel=<level>|<category>:<level> Set the global or per-category severity level for logging categories enabled with the -debug configuration option or the logging RPC. Possible values are info, debug, trace (default=debug). The following levels are always logged: error, warning, info. ``` ACKs for top commit: stickies-v: ACK ec779a2 Tree-SHA512: 0c375e30a5a4c168ca7d97720e8c287f598216767afedae329824e09a480830faf8537b792c5c4bb647c68681c287fe3005c62093708ce85624e9a71c8245e42
This is a parent PR for continuing updates to severity-based logging. See #20576 for motivation and #25306 for discussion.
Infoseverity level messagesLogLevelsList()vector with a programmatic one derived from theLevelenum classGetLogCategory()tostd::optionalLogCategorycodeLogPrintfandLogPrint: addrdb, addrman, banman, i2p, mempool, netbase, net, net_processing, timedata, torcontrolLogPrintLevelto simplyLog, which will be the only log macro needed after migration from the other macros is complete