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
7408a876e4outdated
375@@ -376,7 +376,7 @@ void Session::CreateIfNotCreatedAlready()
376 m_session_id = session_id;
377 m_control_sock = std::move(sock);
378379- 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,
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.
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
#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 force-pushed
on May 26, 2022
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
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 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
jonatack force-pushed
on Jun 2, 2022
in
src/logging.h:238
in
3168b62549outdated
205206+// Log unconditionally.
207 #define LogPrintf(...) LogPrintLevel_(BCLog::LogFlags::NONE, BCLog::Level::None, __VA_ARGS__)
208209+// Log unconditionally, prefixing the output with the passed category name.
210+#define LogPrintfCategory(category, ...) LogPrintLevel_(category, BCLog::Level::None, __VA_ARGS__)
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
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
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 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
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 optionf036d993ec
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/level361f186993
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 code74bacb381f
tor: log messages from torcontrol with category and debug leveldd20c44af6
i2p: log messages from I2P with category and debug levelfc0a50e92a
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 level1116d90a46
logging: use severity-based logging for LogPrint ADDRMAN messagese97f1664aa
logging: replace mempool LogPrintf messages with category and debug leveldc13d42146
logging: replace MEMPOOL LogPrint messages with category and debug level5db4361e70
doc: update logging in fuzzing.md Honggfuzz code10461c5e01
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
8248529713outdated
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
118c7567f6outdated
124@@ -125,9 +125,9 @@ bool BCLog::Logger::WillLogCategory(BCLog::LogFlags category) const
125126 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:
[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
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: 2024-10-04 13:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me