logging: Update to new logging API

ajtowns commented at 4:01 pm on January 11, 2024: contributor

Updates various logging calls to the new API from #28318. All commits are scripted diffs, so should be easy to update if needed, and also easy to reuse the scripts to update other code if needed when rebasing it after this is merged. Changes all uses of LogPrintLevel() where the level is hardcoded, and changes all LogPrintf and LogPrint uses in init.cpp.

DrahtBot commented at 4:01 pm on January 11, 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.

Type	Reviewers
Concept ACK	fanquake
Stale ACK	stickies-v, davidgumberg

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29975 (blockstorage: Separate reindexing from saving new blocks by mzumsande)
#29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
#29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)
#28830 ([refactor] Check CTxMemPool options in ctor by TheCharlatan)
#28765 (p2p: Fill reconciliation sets (Erlay) by naumenkogs)
#28676 ([WIP] Cluster mempool implementation by sdaftuar)
#27826 (validation: log which peer sent us a header by Sjors)
#26022 (Add util::ResultPtr class by ryanofsky)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
#25665 (refactor: Add util::Result failure values, multiple error and warning messages 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.

ajtowns commented at 4:04 pm on January 11, 2024: contributor

Depending on how many conflicts drahtbot detects, maybe this should be targeting a merge around 27.0’s feature freeze?

Many of the log statements in init.cpp could be done at a better level, eg LogPrintf("Error: Out of memory. Terminating.\n"); should obviously be logging at the “error” level, not the “info” level. In order to keep this PR scripted-diff only, I won’t fix that here; but it can fixed in an independent PR either before or after this PR is merged.

jonatack commented at 4:12 pm on January 11, 2024: contributor

I don’t think updating all the macros is a good idea until the API is finished.

fanquake commented at 5:06 pm on January 11, 2024: member

Concept ACK - only 3 conflicts, and none look like priority projects.

in src/validation.cpp:2716 in 2632d03875 outdated

2625@@ -2626,7 +2626,7 @@ bool Chainstate::FlushStateToDisk(
2626                 // TODO: Handle return error, or add detailed comment why it is
2627                 // safe to not return an error upon failure.
2628                 if (!m_blockman.FlushChainstateBlockFile(m_chain.Height())) {
2629-                    LogPrintLevel(BCLog::VALIDATION, BCLog::Level::Warning, "%s: Failed to flush block file.\n", __func__);
2630+                    LogWarning("%s: Failed to flush block file.\n", __func__);

maflcko commented at 7:06 pm on January 11, 2024:

Can remove the useless __func__ when changing the log output anyway?

stickies-v approved

stickies-v commented at 8:16 pm on January 11, 2024: contributor

ACK 2632d038754d975e76926b7c068309c7aadc82f5

Since we don’t have significant merge conflicts I would this support this PR having a non-scripted diff commit to first fix incorrect levels (instead of having to fix and then re-fix) but am okay with this approach too.

Suggested fixes in https://github.com/bitcoin/bitcoin/compare/master...stickies-v:bitcoin:2024-01/fix-log-level-29231 - happy to open a separate pull for it too?

DrahtBot requested review from fanquake on Jan 11, 2024

fanquake renamed this:
~~Update to new logging API~~
logging: Update to new logging API
on Jan 12, 2024

fanquake added the label Utils/log/libs on Jan 12, 2024

DrahtBot added the label CI failed on Jan 15, 2024

maflcko commented at 10:34 am on January 15, 2024: member

happy to open a separate pull for it too?

Yeah, I think it is better to change a line of code only once, instead of twice, unless there is a reason to change it twice.

ajtowns force-pushed on Jan 16, 2024

ajtowns commented at 1:06 am on January 16, 2024: contributor

Suggested fixes in master…stickies-v:bitcoin:2024-01/fix-log-level-29231 - happy to open a separate pull for it too?

Added these commits

DrahtBot removed the label CI failed on Jan 16, 2024

stickies-v approved

stickies-v commented at 11:58 am on January 16, 2024: contributor

ACK 1441ab74d6cd40e75f1dba80b68bc4b1791bf667

DrahtBot added the label Needs rebase on Jan 31, 2024

ajtowns force-pushed on Feb 1, 2024

ajtowns commented at 8:26 pm on February 1, 2024: contributor

Rebased

DrahtBot removed the label Needs rebase on Feb 1, 2024

epiccurious commented at 3:23 am on February 11, 2024: none

Changes all uses of LogPrintLevel() where the level is hardcoded, and changes all LogPrintf and LogPrint uses in init.cpp.

Trying to understand the concept here. What problem does this solve?

DrahtBot added the label Needs rebase on Mar 20, 2024

ajtowns force-pushed on Mar 21, 2024

DrahtBot removed the label Needs rebase on Mar 21, 2024

stickies-v commented at 11:48 am on March 21, 2024: contributor

re-ACK 8d46bd6750b47caaf2556f915800c6a22c997bc3 - no change since 1441ab74d6cd40e75f1dba80b68bc4b1791bf667 except for addressing merge conflicts

in src/init.cpp:1145 in 8d46bd6750 outdated

1142+    LogInfo("Using at most %i automatic connections (%i file descriptors available)\n", nMaxConnections, nFD);
1143 
1144     // Warn about relative -datadir path.
1145     if (args.IsArgSet("-datadir") && !args.GetPathArg("-datadir").is_absolute()) {
1146-        LogPrintf("Warning: relative datadir option '%s' specified, which will be interpreted relative to the "
1147+        LogWarning("Warning: relative datadir option '%s' specified, which will be interpreted relative to the "

davidgumberg commented at 2:18 am on April 12, 2024:

nit:

0        LogWarning("Relative datadir option '%s' specified, which will be interpreted relative to the "

davidgumberg commented at 2:23 am on April 12, 2024: contributor

utACK https://github.com/bitcoin/bitcoin/commit/8d46bd6750b47caaf2556f915800c6a22c997bc3

Tangential to this PR: Is LogPrintLevel in void libevent_log_cb(...) an example of:

“when […] a category needs to be added for an info/warning/error log message”

 0 static void libevent_log_cb(int severity, const char *msg)
 1 {
 2-    BCLog::Level level;
 3     switch (severity) {
 4     case EVENT_LOG_DEBUG:
 5-        level = BCLog::Level::Debug;
 6+        LogDebug(BCLog::LIBEVENT, "%s\n", msg);
 7         break;
 8     case EVENT_LOG_MSG:
 9-        level = BCLog::Level::Info;
10+        LogInfo("%s\n", msg);
11         break;
12     case EVENT_LOG_WARN:
13-        level = BCLog::Level::Warning;
14+        LogWarning("%s\n", msg);
15         break;
16     default: // EVENT_LOG_ERR and others are mapped to error
17-        level = BCLog::Level::Error;
18+        LogError("%s\n", msg);
19         break;
20     }
21-    LogPrintLevel(BCLog::LIBEVENT, level, "%s\n", msg);
22 }

DrahtBot added the label Needs rebase on Apr 25, 2024

log: use error level for critical log messages

As per doc/developer-notes#logging, LogError should be used for
severe problems that require the node to shut down

3ebadba802

log: use warning level for non-critical log messages

As per doc/developer-notes#logging, LogWarning should be used for
severe problems that do not warrant shutting down the node

9f69289512

scripted-diff: Switch various LogPrintLevel(*,Debug,*) to LogDebug()

-BEGIN VERIFY SCRIPT-
sed -i 's/LogPrintLevel(\(BCLog::[^,]*\), BCLog::Level::Debug,/LogDebug(\1,/' src/net_processing.cpp src/dbwrapper.cpp src/net.cpp src/node/txreconciliation.cpp src/validation.cpp
-END VERIFY SCRIPT-

e5a1c6af8c

scripted-diff: Switch various LogPrintLevel(*,Error,*) to LogError()

Changes log output from:

  [net:error] Something bad happened

to either

  [error] Something bad happened

or, when -loglevelalways=1 is enabled:

  [all:error] Something bad happened

-BEGIN VERIFY SCRIPT-
sed -i 's/LogPrintLevel(\(BCLog::[^,]*\), BCLog::Level::Error, */LogError(/' src/net.cpp src/txdb.cpp src/txmempool.cpp
-END VERIFY SCRIPT-

7e3fc9f9fe

scripted-diff: Switch various LogPrintLevel(*,Warning,*) to LogWarning()

Changes log output from:

  [net:warning] Something problematic happened

to either

  [warning] Something problematic happened

or, when -loglevelalways=1 is enabled:

  [all:warning] Something problematic happened

-BEGIN VERIFY SCRIPT-
sed -i 's/LogPrintLevel(\(BCLog::[^,]*\), BCLog::Level::Warning, */LogWarning(/' src/net.cpp src/node/blockstorage.cpp src/validation.cpp
-END VERIFY SCRIPT-

49b8a0091d

scripted-diff: Update init.cpp to use LogInfo and LogDebug

-BEGIN VERIFY SCRIPT-
sed -i 's/LogPrint(\(BCLog::[^,]*\),/LogDebug(\1,/' src/init.cpp
sed -i 's/LogPrintf(/LogInfo(/' src/init.cpp
-END VERIFY SCRIPT-

fd97be4aec

ajtowns force-pushed on Apr 28, 2024

ajtowns commented at 0:42 am on April 28, 2024: contributor

Rebased over #28834, removed redundancy from LogWarning("Warning: ..")

DrahtBot removed the label Needs rebase on Apr 28, 2024

ajtowns added the label Up for grabs on Apr 30, 2024

ajtowns closed this on Apr 30, 2024

laanwj commented at 12:25 pm on April 30, 2024: member

@ajtowns Sorry, i didn’t mean to derail this, i shouldn’t have commented.

logging: Update to new logging API #29231

Code Coverage

Reviews

Conflicts