logging: Update to new logging API #29231

ajtowns · 2024-01-11T16:01:53Z

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 · 2024-01-11T16:01:57Z

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 · 2024-01-11T16:04:42Z

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 · 2024-01-11T16:12:51Z

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

fanquake · 2024-01-11T17:06:21Z

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

maflcko · 2024-01-11T19:06:20Z

src/validation.cpp

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


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

stickies-v

ACK 2632d03

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 master...stickies-v:bitcoin:2024-01/fix-log-level-29231 - happy to open a separate pull for it too?

maflcko · 2024-01-15T10:34:53Z

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 · 2024-01-16T01:06:04Z

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

stickies-v

ACK 1441ab7

ajtowns · 2024-02-01T20:26:05Z

Rebased

epiccurious · 2024-02-11T03:23:13Z

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?

stickies-v · 2024-03-21T11:48:53Z

re-ACK 8d46bd6 - no change since 1441ab7 except for addressing merge conflicts

src/init.cpp

davidgumberg · 2024-04-12T02:23:35Z

utACK 8d46bd6

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"

Otherwise:

 static void libevent_log_cb(int severity, const char *msg)
 {
-    BCLog::Level level;
     switch (severity) {
     case EVENT_LOG_DEBUG:
-        level = BCLog::Level::Debug;
+        LogDebug(BCLog::LIBEVENT, "%s\n", msg);
         break;
     case EVENT_LOG_MSG:
-        level = BCLog::Level::Info;
+        LogInfo("%s\n", msg);
         break;
     case EVENT_LOG_WARN:
-        level = BCLog::Level::Warning;
+        LogWarning("%s\n", msg);
         break;
     default: // EVENT_LOG_ERR and others are mapped to error
-        level = BCLog::Level::Error;
+        LogError("%s\n", msg);
         break;
     }
-    LogPrintLevel(BCLog::LIBEVENT, level, "%s\n", msg);
 }

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

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

-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-

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-

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-

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

ajtowns · 2024-04-28T00:42:28Z

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

laanwj · 2024-04-30T12:25:22Z

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

maflcko reviewed Jan 11, 2024

View reviewed changes

stickies-v approved these changes Jan 11, 2024

View reviewed changes

DrahtBot requested a review from fanquake January 11, 2024 20:16

This was referenced Jan 11, 2024

[refactor] Check CTxMemPool options in ctor #28830

Open

p2p: adaptive connections services flags #28170

Merged

blockstorage: do not flush block to disk if it is already there #27039

Merged

fanquake changed the title ~~Update to new logging API~~ logging: Update to new logging API Jan 12, 2024

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

DrahtBot added the CI failed label Jan 15, 2024

ajtowns force-pushed the 202401-logscripted branch from 2632d03 to 1441ab7 Compare January 16, 2024 01:05

DrahtBot removed the CI failed label Jan 16, 2024

stickies-v approved these changes Jan 16, 2024

View reviewed changes

This was referenced Jan 17, 2024

Improve new LogDebug/Trace/Info/Warning/Error Macros #29256

Open

validation: log which peer sent us a header #27826

Open

p2p: Fill reconciliation sets (Erlay) #28765

Open

DrahtBot mentioned this pull request Jan 25, 2024

refactor: Add util::Result failure values, multiple error and warning messages #25665

Open

DrahtBot added the Needs rebase label Jan 31, 2024

ajtowns force-pushed the 202401-logscripted branch from 1441ab7 to 6248ea9 Compare February 1, 2024 19:20

DrahtBot removed the Needs rebase label Feb 1, 2024

This was referenced Feb 22, 2024

Add util::ResultPtr class #26022

Draft

refactor: Use util::Result class for wallet loading #25722

Draft

DrahtBot mentioned this pull request Mar 8, 2024

[WIP] Cluster mempool implementation #28676

Draft

8 tasks

This was referenced Mar 13, 2024

kernel: Handle fatal errors through return values #29642

Draft

scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint #29641

Draft

DrahtBot added the Needs rebase label Mar 20, 2024

ajtowns force-pushed the 202401-logscripted branch from 6248ea9 to 8d46bd6 Compare March 21, 2024 07:58

DrahtBot removed the Needs rebase label Mar 21, 2024

This was referenced Mar 21, 2024

net: attempts to connect to all resolved addresses when connecting to a node #28834

Merged

kernel, refactor: return error status on all fatal errors #29700

Draft

davidgumberg reviewed Apr 12, 2024

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Apr 25, 2024

stickies-v and others added 6 commits April 28, 2024 10:23

log: use error level for critical log messages

3ebadba

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

log: use warning level for non-critical log messages

9f69289

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

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

fd97be4

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

ajtowns force-pushed the 202401-logscripted branch from 8d46bd6 to fd97be4 Compare April 28, 2024 00:40

DrahtBot removed the Needs rebase label Apr 28, 2024

DrahtBot mentioned this pull request Apr 28, 2024

blockstorage: Separate reindexing from saving new blocks #29975

Open

ajtowns added the Up for grabs label Apr 30, 2024

ajtowns closed this Apr 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

logging: Update to new logging API #29231

logging: Update to new logging API #29231

ajtowns commented Jan 11, 2024

DrahtBot commented Jan 11, 2024 •

edited

ajtowns commented Jan 11, 2024 •

edited

jonatack commented Jan 11, 2024

fanquake commented Jan 11, 2024 •

edited

maflcko Jan 11, 2024

stickies-v left a comment

maflcko commented Jan 15, 2024

ajtowns commented Jan 16, 2024

stickies-v left a comment

ajtowns commented Feb 1, 2024

epiccurious commented Feb 11, 2024

stickies-v commented Mar 21, 2024

davidgumberg commented Apr 12, 2024 •

edited

ajtowns commented Apr 28, 2024

laanwj commented Apr 30, 2024

logging: Update to new logging API #29231

logging: Update to new logging API #29231

Conversation

ajtowns commented Jan 11, 2024

DrahtBot commented Jan 11, 2024 • edited

Code Coverage

Reviews

Conflicts

ajtowns commented Jan 11, 2024 • edited

jonatack commented Jan 11, 2024

fanquake commented Jan 11, 2024 • edited

maflcko Jan 11, 2024

Choose a reason for hiding this comment

stickies-v left a comment

Choose a reason for hiding this comment

maflcko commented Jan 15, 2024

ajtowns commented Jan 16, 2024

stickies-v left a comment

Choose a reason for hiding this comment

ajtowns commented Feb 1, 2024

epiccurious commented Feb 11, 2024

stickies-v commented Mar 21, 2024

davidgumberg commented Apr 12, 2024 • edited

ajtowns commented Apr 28, 2024

laanwj commented Apr 30, 2024

DrahtBot commented Jan 11, 2024 •

edited

ajtowns commented Jan 11, 2024 •

edited

fanquake commented Jan 11, 2024 •

edited

davidgumberg commented Apr 12, 2024 •

edited