Severity-based logging -- parent PR

jonatack commented at 8:08 pm on May 24, 2022: contributor

This is a parent PR for continuing updates to severity-based logging. See #20576 for motivation and #25306 for discussion.

#24464
#25286
#25306
#25614
#26957
#27632
#27231
print the wallet name more clearly in wallet logging to distinguish it from category/level
unconditionally log Info severity level messages
replace the hardcoded LogLevelsList() vector with a programmatic one derived from the Level enum class
convert GetLogCategory() to std::optional
deduplicate the LogCategory code
update to severity-based logging in the following areas, dropping the use of LogPrintf and LogPrint: addrdb, addrman, banman, i2p, mempool, netbase, net, net_processing, timedata, torcontrol
when we’re ready to drop the other logging macros, rename LogPrintLevel to simply Log, which will be the only log macro needed after migration from the other macros is complete

in src/i2p.cpp:379 in 7408a876e4 outdated

375@@ -376,7 +376,7 @@ void Session::CreateIfNotCreatedAlready()
376     m_session_id = session_id;
377     m_control_sock = std::move(sock);
378 
379-    LogPrintf("I2P: SAM session created: session id=%s, my address=%s\n", m_session_id,
380+    LogPrintf("[i2p] SAM session created: session id=%s, my address=%s\n", m_session_id,

laanwj commented at 8:17 pm on May 24, 2022:

I understand the idea, but I don’t think we should do it this way by inserting [cat] manually. We need a way to log things with a category even without -debug=.

This is why I have a commit in #25202 to log everything >= Warning unconditionally. I’m not sure that is the best solution (as this is not a Warning level message, more like Info) but I like it more than this.

Or maybe allow,

0LogPrintLevel(BCLog::Level::None, BCLog::I2P, …);

Where the level None is logged unconditionally as well.

jonatack commented at 8:24 pm on May 24, 2022:

Thanks, SGTM, will just do the de-dupe then (also for ZMQ and PRUNE).

laanwj commented at 8:31 pm on May 24, 2022:

That’s fine but to be clear I mean I agree conceptually with doing it, just not in this way.

jonatack commented at 8:50 pm on May 24, 2022:

Agree, thanks for clarifying. I’ll start with reviewing your change and then go from there / update here.

jonatack commented at 4:07 pm on May 25, 2022:

How about LogPrintfCategory?

 0--- a/src/logging.h
 1+++ b/src/logging.h
 2@@ -198,6 +198,7 @@ static inline void LogPrintf_(const std::string& logging_function, const std::st
 3 
 4 #define LogPrintf(...) LogPrintLevel_(BCLog::LogFlags::NONE, BCLog::Level::None, __VA_ARGS__)
 5+#define LogPrintfCategory(category, ...) LogPrintLevel_(category, BCLog::Level::None, __VA_ARGS__)
 6
 7--- a/src/i2p.cpp
 8+++ b/src/i2p.cpp
 9@@ -376,7 +376,7 @@ void Session::CreateIfNotCreatedAlready()
10 
11-    LogPrintf("I2P: SAM session created: session id=%s, my address=%s\n", m_session_id,
12+    LogPrintfCategory(BCLog::I2P, "SAM session created: session id=%s, my address=%s\n", m_session_id,

debug log

02022-05-25T16:03:02Z [i2p] SAM session created

jonatack commented at 6:43 pm on May 25, 2022:

Done

laanwj commented at 5:26 pm on June 2, 2022:

Yes, lgtm!

jonatack renamed this:
~~log: de-dupe TOR/I2P/NET category prefixes and update their LogPrintf ones~~
log: de-duplicate logging category output
on May 24, 2022

jonatack force-pushed on May 24, 2022

DrahtBot added the label Refactoring on May 24, 2022

jonatack renamed this:
~~log: de-duplicate logging category output~~
log: de-duplicate categories, use severity-based logging for torcontrol/i2p/netbase
on May 25, 2022

jonatack force-pushed on May 25, 2022

jonatack commented at 11:13 pm on May 25, 2022: contributor

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.

DrahtBot commented at 9:23 am on May 26, 2022: 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	laanwj, maflcko
Approach ACK	klementtan, w0xlt, vasild

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:

#25683 (refactor: log nEvicted message in LimitOrphans then return void by chinggg)
#25678 (p2p: skip querying dns seeds if -onlynet disables IPv4 and IPv6 by mzumsande)
#25673 (refactor: make member functions const when applicable by aureleoules)
#25619 (net: avoid overriding non-virtual ToString() in CService and use better naming by vasild)
#25614 (logging: add -loglevel configuration option and a trace log level by jonatack)
#25555 (refactor: Move m_num_unconnecting_headers_msgs out of cs_main guard by MarcoFalke)
#25499 (Use steady clock for all millis bench logging by MarcoFalke)
#25421 (net: convert standalone IsSelectableSocket() and SetSocketNonBlocking() to Sock methods by vasild)
#25390 (sync: introduce a thread-safe smart container and use it to remove a bunch of “GlobalMutex"es by vasild)
#25355 (I2P: add support for transient addresses for outbound connections by vasild)
#25284 ([WIP] consensus: Remove dependency on net (BIP 155 / ADDRV2_FORMAT) by MarcoFalke)
#25172 (refactor: use std:: prefix for std lib funcs by fanquake)
#25136 (Torcontrol opt check by amadeuszpawlik)
#25077 (Fix chain tip data race and corrupt rest response by MarcoFalke)
#25038 (policy: nVersion=3 and Package RBF by glozow)
#24974 (refactor: Make FEELER_SLEEP_WINDOW type safe (std::chrono) by MarcoFalke)
#24950 (Add config option to set max debug log size by tehelsper)
#24697 (refactor address relay time by MarcoFalke)
#24606 (Change -maxtimeadjustment default from 70 minutes to 0 by jonatack)
#24571 (p2p: Prevent block index fingerprinting by sending additional getheaders messages by dergoegge)
#23605 (util: Add TorControlArgumentCheck function by shaavan)
#21878 (Make all networking code mockable by vasild)

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.

jonatack force-pushed on May 26, 2022

jonatack renamed this:
~~log: de-duplicate categories, use severity-based logging for torcontrol/i2p/netbase~~
log: dedup categories, add macro, use severity-based logging for tor/i2p/netbase
on May 26, 2022

DrahtBot added the label Needs rebase on May 27, 2022

jonatack force-pushed on May 27, 2022

DrahtBot removed the label Needs rebase on May 27, 2022

in src/logging.h:238 in d11db00023 outdated

202@@ -203,6 +203,7 @@ static inline void LogPrintf_(const std::string& logging_function, const std::st
203 #define LogPrintLevel_(category, level, ...) LogPrintf_(__func__, __FILE__, __LINE__, category, level, __VA_ARGS__)
204 
205 #define LogPrintf(...) LogPrintLevel_(BCLog::LogFlags::NONE, BCLog::Level::None, __VA_ARGS__)
206+#define LogPrintfCategory(category, ...) LogPrintLevel_(category, BCLog::Level::None, __VA_ARGS__)

maflcko commented at 5:30 pm on May 31, 2022:

Would be good to add a docstring for this. Is this meant to unconditionally log regardless of log level? If yes, it doesn’t seem it would work. BCLog::Level::None is 1 but the check for unconditional logging is >= Warning (3)?

jonatack commented at 7:39 pm on May 31, 2022:

Good catch; this is wrong for LogPrintf as well. Likely going to fix by reordering the BCLog::Level enums:

 0diff --git a/src/logging.cpp b/src/logging.cpp
 1index f1a86f0dce..3f316f0101 100644
 2--- a/src/logging.cpp
 3+++ b/src/logging.cpp
 4@@ -187,8 +187,6 @@ bool GetLogCategory(BCLog::LogFlags& flag, const std::string& str)
 5 std::string LogLevelToStr(BCLog::Level level)
 6 {
 7     switch (level) {
 8-    case BCLog::Level::None:
 9-        return "none";
10     case BCLog::Level::Debug:
11         return "debug";
12     case BCLog::Level::Info:
13@@ -197,6 +195,8 @@ std::string LogLevelToStr(BCLog::Level level)
14         return "warning";
15     case BCLog::Level::Error:
16         return "error";
17+    case BCLog::Level::None:
18+        return "";
19     }
20     assert(false);
21 }

 0diff --git a/src/logging.h b/src/logging.h
 1index 8a896b6b33..f8e55b88b2 100644
 2--- a/src/logging.h
 3+++ b/src/logging.h
 4@@ -69,10 +69,10 @@ namespace BCLog {
 5     };
 6     enum class Level {
 7         Debug = 0,
 8-        None = 1,
 9-        Info = 2,
10-        Warning = 3,
11-        Error = 4,
12+        Info,
13+        Warning,
14+        Error,
15+        None,
16     };

jonatack commented at 8:17 pm on May 31, 2022:

it doesn’t seem it would work. BCLog::Level::None is 1 but the check for unconditional logging is >= Warning (3)?

Ah, LogAcceptCategory() with the logic you mention is called by neither LogPrintf nor LogPrintfCategory, so it seems fine as-is. Will add a docstring.

jonatack commented at 5:01 pm on June 2, 2022:

Would be good to add a docstring for this.

Thanks for the suggestion; done.

jonatack force-pushed on Jun 2, 2022

jonatack renamed this:
~~log: dedup categories, add macro, use severity-based logging for tor/i2p/netbase~~
log: dedup categories, add macro, use severity-based logging
on Jun 2, 2022

jonatack renamed this:
~~log: dedup categories, add macro, use severity-based logging~~
logging: update to severity-based logging
on Jun 2, 2022

jonatack marked this as ready for review on Jun 2, 2022

jonatack force-pushed on Jun 2, 2022

in src/logging.h:238 in 3168b62549 outdated

205 
206+// Log unconditionally.
207 #define LogPrintf(...) LogPrintLevel_(BCLog::LogFlags::NONE, BCLog::Level::None, __VA_ARGS__)
208 
209+// Log unconditionally, prefixing the output with the passed category name.
210+#define LogPrintfCategory(category, ...) LogPrintLevel_(category, BCLog::Level::None, __VA_ARGS__)

laanwj commented at 6:06 pm on June 2, 2022:

It’s LogPrintLevel and LogPrintfCategory that’s a bit inconsistent. I think we can leave out the f here.

Edit: oh, no, it makes more sense like this (with regard to conditional/unconditional logging) Unfortunately.

jonatack force-pushed on Jun 2, 2022

jonatack force-pushed on Jun 3, 2022

klementtan commented at 11:26 am on June 3, 2022: contributor

Approach ACK

laanwj commented at 1:33 pm on June 3, 2022: member

Concept ACK

maflcko referenced this in commit 45d8b1e94a on Jun 7, 2022

DrahtBot added the label Needs rebase on Jun 7, 2022

jonatack force-pushed on Jun 7, 2022

jonatack commented at 2:05 pm on June 7, 2022: contributor

Rebased/updated following the merge of #25286 that contained the first commit of this PR.

DrahtBot removed the label Needs rebase on Jun 7, 2022

DrahtBot added the label Needs rebase on Jun 7, 2022

jonatack force-pushed on Jun 8, 2022

DrahtBot removed the label Needs rebase on Jun 8, 2022

in src/logging.h:73 in 76377df73f outdated

67@@ -68,11 +68,12 @@ namespace BCLog {
68         ALL         = ~(uint32_t)0,
69     };
70     enum class Level {
71-        Debug = 0,
72-        None = 1,
73-        Info = 2,
74-        Warning = 3,
75-        Error = 4,
76+        Trace = 0,

dergoegge commented at 3:21 pm on June 10, 2022:

I think some docs on the different levels wouldn’t hurt. For example, it’s not quite clear to me how you choose between Trace and Debug.

jonatack commented at 3:42 pm on June 10, 2022:

Thanks, I agree. There’s some discussion in the commit messages but it should be documented in the code too in that commit.

laanwj commented at 1:46 pm on June 14, 2022:

Generally, the distinction is:

debug is for reasonably noisy logging, but not high enough volume to make it unusable in a production scenario
tracing is super high detail, step by step, and will slow down the application and you’d definitely never want to enable it for a category unless you’re developing or trouble-shooting a specific issue

jonatack commented at 1:39 pm on July 1, 2022:

Thanks! Added documentation to the Level enum class.

laanwj referenced this in commit 9e4fbebcc8 on Jun 14, 2022

DrahtBot added the label Needs rebase on Jun 14, 2022

sidhujag referenced this in commit 78150ec7b6 on Jun 15, 2022

Onyeali approved

jonatack force-pushed on Jun 29, 2022

DrahtBot removed the label Needs rebase on Jun 29, 2022

jonatack force-pushed on Jul 1, 2022

jonatack commented at 1:38 pm on July 1, 2022: contributor

Rebased and updated significantly to pull in the changes in #25287 (with review feedback and some clean-up and moving the refactoring commits to after the functional ones) and the changes discussed in #25306 (unconditionally log Info level and up).

DrahtBot added the label Needs rebase on Jul 4, 2022

jonatack force-pushed on Jul 8, 2022

DrahtBot removed the label Needs rebase on Jul 8, 2022

DrahtBot added the label Needs rebase on Jul 18, 2022

logging: simplify BCLog::Level enum class and LogLevelToStr() function

- simplify the BCLog::Level enum class (and future changes to it) by
  only setting the value of the first enumerator

- move the BCLog::Level:None enumerator to the end of the BCLog::Level
  enum class and LogLevelToStr() member function, as the None enumerator
  is only used internally, and by being the highest BCLog::Level value it
  can be used to iterate over the enumerators

- replace the unused BCLog::Level:None string "none" with an empty string
  in LogLevelToStr(); the case will currently never be hit

- add documentation

f6dc368d78

logging: add -loglevel configuration option

- add a -loglevel=<level>|<category:level> config option, to allow users
  to set a global -loglevel and category-specific log levels

- do not print log messages having lower severity level than -loglevel

- add unit tests

a2e089ca32

doc: release note for -loglevel configuration option f036d993ec

logging, test: add BCLog::Level::Trace log severity level

for verbose log messages for development or debugging only, as bitcoind may run
more slowly, that are more granular/frequent than the Debug log level, i.e. for
very high-frequency, low-level messages to be logged distinctly from
higher-level, less-frequent debug logging that could still be usable in production.

An example would be to log higher-level peer events (connection, disconnection,
misbehavior, eviction) versus low-level, high-volume p2p messages in the
BCLog::NET category. This will enable the user to log only the former without
the latter, in order to focus on high-level peer management events.

With respect to the name, "trace" is suggested as the most granular level
in resources like the following:
- https://sematext.com/blog/logging-levels
- https://howtodoinjava.com/log4j2/logging-levels

Update the test framework and add test coverage.

4a2c0d01ff

logging: print wallet name more clearly to distinguish from category/level 361f186993

logging: unconditionally log Info severity level messages

in addition to Warning and Error ones.

118c7567f6

jonatack force-pushed on Jul 20, 2022

jonatack commented at 2:54 pm on July 20, 2022: contributor

Rebased and updated with the latest changes in subset PR #25614.

DrahtBot removed the label Needs rebase on Jul 20, 2022

refactor: replace hardcoded LogLevelsList vector with a programmatic one

built from the BCLog:Level enum class, as done in GetNetworkNames().

ce3e64c660

refactor: use std::optional for GetLogCategory() df84a4f1f6

refactor: deduplicate LogCategory code 74bacb381f

tor: log messages from torcontrol with category and debug level dd20c44af6

i2p: log messages from I2P with category and debug level fc0a50e92a

refactor: remove unused i2p::Session::Log() template method 43315a0f69

netbase: log netbase messages with category and debug level

and change a few of the NET category logging messages to PROXY when the message
relates to SOCKS5, per the following log category descriptions in
https://github.com/jonatack/bitcoin-development/blob/master/logging.md
which is an updated version of this Bitcoin Stack Exchange answer in 2017 by Andrew Chow:
https://bitcoin.stackexchange.com/questions/66892/what-are-the-debug-categories/66895

- net: All messages related to communicating with other nodes on the network,
  including what P2P messages were sent and received and to whom and other
  information about the network messages.

- proxy: Messages about using a SOCKS5 proxy and its authentication.

Before this commit, BCLog::PROXY was essentially unused (only one log message).

and reduce some unconditional logging that could be peer-provoked.

While here, improve the clarity of a few of the touched messages and use
consistent capitalization and naming, e.g. "Socks5()" vs "SOCKS5"

a0be62c904

net_processing: replace LogPrintf messages with category and debug level

using LogPrintLevel(), thereby removing all LogPrintf messages in this file.

Reduce some of the unconditional logging that could be peer-provoked
(disconnection for old chain, stalling, timeout, but not user-selected
cases like manual/noban/forcerelay peers), by moving it to the Debug level.

While here, improve the clarity of a few of the touched messages and use
consistent capitalization.

844290b399

net_processing: use severity-based logging for LogPrint messages

- Info for the minimum viable default logging for users

- Debug for high-level peer event messages: connection, disconnection,
  misbehavior, eviction

- Trace for logging of low-level, very high-frequency p2p messages.

While here, improve the clarity of the touched messages and use consistent
capitalization.

8248529713

net: replace LogPrintf messages with category and debug level

and reduce some of the unconditional logging that could be peer-provoked by
moving them to the Info and Debug levels:

- Info for the minimum viable default logging for users

- Debug for more detailed peer management messages: connection, disconnection,
  misbehavior, eviction, etc.

While here, improve the clarity of the touched messages and use consistent
capitalization.

a22b1a3391

net, timedata: use severity-based logging for LogPrint messages

- Info for the minimum viable default logging for users

- Debug for high-level peer events: connection, disconnection,
  misbehavior, eviction

- Trace for logging of low-level, very high-frequency p2p messages

While here, improve the clarity of the touched messages and use consistent
capitalization.

0d039f07df

logging: replace LogPrintf ADDRMAN messages with category and debug level 1116d90a46

logging: use severity-based logging for LogPrint ADDRMAN messages e97f1664aa

logging: replace mempool LogPrintf messages with category and debug level dc13d42146

logging: replace MEMPOOL LogPrint messages with category and debug level 5db4361e70

doc: update logging in fuzzing.md Honggfuzz code 10461c5e01

jonatack force-pushed on Jul 20, 2022

DrahtBot commented at 2:26 pm on July 26, 2022: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot added the label Needs rebase on Jul 26, 2022

w0xlt commented at 7:09 am on July 31, 2022: contributor

Approach ACK.

This PR improves significantly the quality of the log and the Log codebase. I wonder if the main Log classes could be a separate library in another repository. Awaiting rebase for ACK.

vasild commented at 12:29 pm on August 18, 2022: contributor

Approach ACK

jonatack closed this on Aug 22, 2022

maflcko commented at 10:43 am on August 22, 2022: member

Concept ACK

jonatack deleted the branch on Aug 22, 2022

achow101 referenced this in commit 7281fac2e0 on Sep 1, 2022

jonatack renamed this:
~~logging: update to severity-based logging~~
Severity-based logging -- parent PR
on Sep 2, 2022

jonatack restored the branch on Sep 2, 2022

jonatack commented at 10:33 am on September 2, 2022: contributor

A big thank you to @vasild who rebased this branch to current master. Re-opening and updating.

jonatack reopened this on Sep 2, 2022

in src/net_processing.cpp:2359 in 8248529713 outdated

2355@@ -2356,9 +2356,9 @@ void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, c
2356         // the main chain -- this shouldn't really happen.  Bail out on the
2357         // direct fetch and rely on parallel download instead.
2358         if (!m_chainman.ActiveChain().Contains(pindexWalk)) {
2359-            LogPrint(BCLog::NET, "Large reorg, won't direct fetch to %s (%d)\n",
2360-                    pindexLast->GetBlockHash().ToString(),
2361-                    pindexLast->nHeight);
2362+            LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "Large reorg, won't direct fetch to %s (%d)\n",

dergoegge commented at 4:09 pm on September 20, 2022:

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.

jonatack commented at 3:33 pm on January 26, 2023:

Yes, good point that any changes from conditional to unconditional logging should be separated out.

in src/logging.cpp:130 in 118c7567f6 outdated

124@@ -125,9 +125,9 @@ bool BCLog::Logger::WillLogCategory(BCLog::LogFlags category) const
125 
126 bool BCLog::Logger::WillLogCategoryLevel(BCLog::LogFlags category, BCLog::Level level) const
127 {
128-    // Log messages at Warning and Error level unconditionally, so that
129-    // important troubleshooting information doesn't get lost.
130-    if (level >= BCLog::Level::Warning) return true;
131+    // Log the Info, Warning, and Error message levels unconditionally, so that
132+    // important troubleshooting information isn't lost.
133+    if (level >= BCLog::Level::Info) return true;

dergoegge commented at 4:21 pm on September 20, 2022:

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)

jonatack commented at 3:38 pm on January 26, 2023:

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.

jonatack commented at 5:13 pm on January 11, 2024:

Noting here that this change was done in #28318, resolving.

achow101 marked this as a draft on Oct 12, 2022

DrahtBot commented at 1:44 am on January 10, 2023: contributor

There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

jonatack commented at 6:57 pm on January 10, 2023: contributor

Updating and rebasing.

ajtowns commented at 11:06 pm on January 23, 2023: contributor

I guess I’ve got conceptual questions about this.

It seems very intrusive to change LogPrint(NET, "hi"); to LogPrintLevel(NET, DEBUG, "hi"); everywhere. Why not just make LogPrint default to DEBUG instead of NONE and keep things brief? Likewise, why have LogPrintLevel(INFO) instead of just LogInfo and LogWarning etc?
Why is m_category_log_levels a map guarded by a mutex? We only have three levels: tracing, debugging and unconditional – why not just have two atomic bitfields: m_trace_categories and m_debug_categories. Then your test is:

0if (level == TRACE) return (m_trace_categories.load(relaxed) & category) != 0;
1else if (level == DEBUG) return (m_debug_categories.load(relaxed) & category) != 0;
2else return true;

[EDIT to add] I guess it’s not clear to me why you’d ever want to say LogPrintLevel(NET, INFO, "foo") rather than LogPrintf("foo")? I guess in general, if the logging is unconditional anyway, I don’t see what value there is in associating it with a category; likewise, adding a warning/error/debug/trace tag seems useful, but an “info” tag doesn’t seem useful? So having LogPrintf(msg) ; LogWarning(msg) ; LogError(msg) ; LogPrint(CATEGORY, msg) ; LogTrace(CATEGORY, msg) would make more sense to me (maybe deprecating LogPrintf in favour of LogInfo or similar over time, eventually with a scripted-diff to finish off)

jonatack commented at 3:30 pm on January 26, 2023: contributor

Thanks for the detailed feedback, @ajtowns. Will update with it in mind; looks like it will be helpful in simplifying things.

achow101 referenced this in commit 630756cac0 on Mar 23, 2023

sidhujag referenced this in commit 433494e1d3 on Mar 24, 2023

jonatack commented at 11:09 am on April 25, 2023: contributor

Closing temporarily so that I can re-open it – am still interested to finish this.

jonatack closed this on Apr 25, 2023

jonatack commented at 12:26 pm on August 31, 2023: contributor

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.

jonatack reopened this on Aug 31, 2023

jonatack commented at 4:35 pm on August 31, 2023: contributor

Why not just make LogPrint default to DEBUG instead of NONE

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 net logging, noisy high-frequency low-level p2p messages in this parent become trace, and lower-frequency, high-level events like peer disconnect/discourage become debug.

I guess it’s not clear to me why you’d ever want to say LogPrintLevel(NET, INFO, "foo") rather than LogPrintf("foo")? I guess in general, if the logging is unconditional anyway, I don’t see what value there is in associating it with a category

“info” tag doesn’t seem useful?

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.

maybe deprecating LogPrintf in favour of LogInfo or similar over time, eventually with a scripted-diff to finish off

The idea is for Log(category, level) to subsume all the other macros, to have just one.

DrahtBot commented at 0:48 am on November 30, 2023: contributor

There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

fanquake referenced this in commit 9fa8eda8af on Jan 16, 2024

jonatack commented at 4:20 pm on April 8, 2024: contributor

Closing to be able to re-open it.

jonatack closed this on Apr 8, 2024

Severity-based logging – parent PR #25203

Code Coverage

Reviews

Conflicts