Severity-based logging -- parent PR #25203

pull jonatack wants to merge 22 commits into bitcoin:master from jonatack:logging-updates changing 38 files +704 −562
  1. jonatack commented at 8:08 PM on May 24, 2022: member

    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
  2. 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,

    LogPrintLevel(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?

    --- a/src/logging.h
+++ b/src/logging.h
@@ -198,6 +198,7 @@ static inline void LogPrintf_(const std::string& logging_function, const std::st
 
 #define LogPrintf(...) LogPrintLevel_(BCLog::LogFlags::NONE, BCLog::Level::None, __VA_ARGS__)
+#define LogPrintfCategory(category, ...) LogPrintLevel_(category, BCLog::Level::None, __VA_ARGS__)

--- a/src/i2p.cpp
+++ b/src/i2p.cpp
@@ -376,7 +376,7 @@ void Session::CreateIfNotCreatedAlready()
 
-    LogPrintf("I2P: SAM session created: session id=%s, my address=%s\n", m_session_id,
+    LogPrintfCategory(BCLog::I2P, "SAM session created: session id=%s, my address=%s\n", m_session_id,

    debug log

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

  3. 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
  4. jonatack force-pushed on May 24, 2022
  5. DrahtBot added the label Refactoring on May 24, 2022
  6. 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
  7. jonatack force-pushed on May 25, 2022
  8. jonatack commented at 11:13 PM on May 25, 2022: member

    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.

  9. DrahtBot commented at 9:23 AM on May 26, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    <!--021abf342d371248e50ceaed478a90ca-->

    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.

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  10. jonatack force-pushed on May 26, 2022
  11. jonatack force-pushed on May 26, 2022
  12. jonatack force-pushed on May 26, 2022
  13. 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
  14. DrahtBot added the label Needs rebase on May 27, 2022
  15. jonatack force-pushed on May 27, 2022
  16. DrahtBot removed the label Needs rebase on May 27, 2022
  17. 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:

    diff --git a/src/logging.cpp b/src/logging.cpp
index f1a86f0dce..3f316f0101 100644
--- a/src/logging.cpp
+++ b/src/logging.cpp
@@ -187,8 +187,6 @@ bool GetLogCategory(BCLog::LogFlags& flag, const std::string& str)
 std::string LogLevelToStr(BCLog::Level level)
 {
     switch (level) {
-    case BCLog::Level::None:
-        return "none";
     case BCLog::Level::Debug:
         return "debug";
     case BCLog::Level::Info:
@@ -197,6 +195,8 @@ std::string LogLevelToStr(BCLog::Level level)
         return "warning";
     case BCLog::Level::Error:
         return "error";
+    case BCLog::Level::None:
+        return "";
     }
     assert(false);
 }

    diff --git a/src/logging.h b/src/logging.h
index 8a896b6b33..f8e55b88b2 100644
--- a/src/logging.h
+++ b/src/logging.h
@@ -69,10 +69,10 @@ namespace BCLog {
     };
     enum class Level {
         Debug = 0,
-        None = 1,
-        Info = 2,
-        Warning = 3,
-        Error = 4,
+        Info,
+        Warning,
+        Error,
+        None,
     };
    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.

  18. jonatack force-pushed on Jun 2, 2022
  19. 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
  20. jonatack renamed this:
    log: dedup categories, add macro, use severity-based logging
    logging: update to severity-based logging
    on Jun 2, 2022
  21. jonatack marked this as ready for review on Jun 2, 2022
  22. jonatack force-pushed on Jun 2, 2022
  23. jonatack force-pushed on Jun 2, 2022
  24. 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.

  25. jonatack force-pushed on Jun 2, 2022
  26. jonatack force-pushed on Jun 3, 2022
  27. klementtan commented at 11:26 AM on June 3, 2022: contributor

    Approach ACK

  28. laanwj commented at 1:33 PM on June 3, 2022: member

    Concept ACK

  29. maflcko referenced this in commit 45d8b1e94a on Jun 7, 2022
  30. DrahtBot added the label Needs rebase on Jun 7, 2022
  31. jonatack force-pushed on Jun 7, 2022
  32. jonatack commented at 2:05 PM on June 7, 2022: member

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

  33. DrahtBot removed the label Needs rebase on Jun 7, 2022
  34. DrahtBot added the label Needs rebase on Jun 7, 2022
  35. jonatack force-pushed on Jun 8, 2022
  36. DrahtBot removed the label Needs rebase on Jun 8, 2022
  37. 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.

  38. laanwj referenced this in commit 9e4fbebcc8 on Jun 14, 2022
  39. DrahtBot added the label Needs rebase on Jun 14, 2022
  40. sidhujag referenced this in commit 78150ec7b6 on Jun 15, 2022
  41. Onyeali approved
  42. jonatack force-pushed on Jun 29, 2022
  43. jonatack force-pushed on Jun 29, 2022
  44. DrahtBot removed the label Needs rebase on Jun 29, 2022
  45. jonatack force-pushed on Jul 1, 2022
  46. jonatack force-pushed on Jul 1, 2022
  47. jonatack commented at 1:38 PM on July 1, 2022: member

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

  48. DrahtBot added the label Needs rebase on Jul 4, 2022
  49. jonatack force-pushed on Jul 8, 2022
  50. jonatack force-pushed on Jul 8, 2022
  51. DrahtBot removed the label Needs rebase on Jul 8, 2022
  52. DrahtBot added the label Needs rebase on Jul 18, 2022
  53. 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
  54. 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
  55. doc: release note for -loglevel configuration option f036d993ec
  56. 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
  57. logging: print wallet name more clearly to distinguish from category/level 361f186993
  58. logging: unconditionally log Info severity level messages
    in addition to Warning and Error ones.
    118c7567f6
  59. jonatack force-pushed on Jul 20, 2022
  60. jonatack commented at 2:54 PM on July 20, 2022: member

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

  61. DrahtBot removed the label Needs rebase on Jul 20, 2022
  62. refactor: replace hardcoded LogLevelsList vector with a programmatic one
    built from the BCLog:Level enum class, as done in GetNetworkNames().
    ce3e64c660
  63. refactor: use std::optional for GetLogCategory() df84a4f1f6
  64. refactor: deduplicate LogCategory code 74bacb381f
  65. tor: log messages from torcontrol with category and debug level dd20c44af6
  66. i2p: log messages from I2P with category and debug level fc0a50e92a
  67. refactor: remove unused i2p::Session::Log() template method 43315a0f69
  68. 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
  69. 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
  70. 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
  71. 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
  72. 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
  73. logging: replace LogPrintf ADDRMAN messages with category and debug level 1116d90a46
  74. logging: use severity-based logging for LogPrint ADDRMAN messages e97f1664aa
  75. logging: replace mempool LogPrintf messages with category and debug level dc13d42146
  76. logging: replace MEMPOOL LogPrint messages with category and debug level 5db4361e70
  77. doc: update logging in fuzzing.md Honggfuzz code 10461c5e01
  78. jonatack force-pushed on Jul 20, 2022
  79. DrahtBot commented at 2:26 PM on July 26, 2022: contributor

    <!--cf906140f33d8803c4a75a2196329ecb-->

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

    <sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>

  80. DrahtBot added the label Needs rebase on Jul 26, 2022
  81. 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.

  82. vasild commented at 12:29 PM on August 18, 2022: contributor

    Approach ACK

  83. jonatack closed this on Aug 22, 2022
  84. maflcko commented at 10:43 AM on August 22, 2022: member

    Concept ACK

  85. jonatack deleted the branch on Aug 22, 2022
  86. achow101 referenced this in commit 7281fac2e0 on Sep 1, 2022
  87. jonatack renamed this:
    logging: update to severity-based logging
    Severity-based logging -- parent PR
    on Sep 2, 2022
  88. jonatack restored the branch on Sep 2, 2022
  89. jonatack commented at 10:33 AM on September 2, 2022: member

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

  90. jonatack reopened this on Sep 2, 2022
  91. 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.

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

  93. achow101 marked this as a draft on Oct 12, 2022
  94. DrahtBot commented at 1:44 AM on January 10, 2023: contributor

    <!--13523179cfe9479db18ec6c5d236f789-->

    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.
  95. jonatack commented at 6:57 PM on January 10, 2023: member

    Updating and rebasing.

  96. ajtowns commented at 11:06 PM on January 23, 2023: contributor

    I guess I've got conceptual questions about this.

    1. 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?
    2. 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:
    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;
    1. [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)
  97. jonatack commented at 3:30 PM on January 26, 2023: member

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

  98. achow101 referenced this in commit 630756cac0 on Mar 23, 2023
  99. sidhujag referenced this in commit 433494e1d3 on Mar 24, 2023
  100. jonatack commented at 11:09 AM on April 25, 2023: member

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

  101. jonatack closed this on Apr 25, 2023
  102. jonatack commented at 12:26 PM on August 31, 2023: member

    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.

  103. jonatack reopened this on Aug 31, 2023
  104. jonatack commented at 4:35 PM on August 31, 2023: member

    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.

  105. DrahtBot commented at 12:48 AM on November 30, 2023: contributor

    <!--13523179cfe9479db18ec6c5d236f789-->

    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.
  106. fanquake referenced this in commit 9fa8eda8af on Jan 16, 2024
  107. jonatack commented at 4:20 PM on April 8, 2024: member

    Closing to be able to re-open it.

  108. jonatack closed this on Apr 8, 2024
  109. PastaPastaPasta referenced this in commit ec47cb9f87 on Feb 17, 2025
  110. bitcoin locked this on Apr 8, 2025
Contributors
jonatacklaanwjDrahtBotmaflckoklementtandergoeggeOnyealiw0xltvasildajtowns
Labels
RefactoringNeeds rebase
Linked (view graph)
#20576 RFC on logging improvements#24464 logging: Add severity level to logs#25286 scripted-diff: remove duplicate categories from LogPrint output#25293 logging: remove duplicate categories from LogPrint output#25306 logging: add LogPrintfCategory to log unconditionally with category#25614 Severity-based logging, step 2#26957 bench: update logging benchmarks#27231 Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs#27632 Raise on invalid -debug and -loglevel config options#28599 Gathering Priorities for 27.0#28642 Voting on Priority Projects for 27.0#29230 doc: update -loglevel help to add `info` to the always logged levels
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-02 15:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me