logging: Simplify API for level based logging

ajtowns commented at 4:46 am on August 22, 2023: contributor

Replace LogPrint* functions with severity based logging functions:

LogInfo(...), LogWarning(...), LogError(...) for unconditional (uncategorised) logging (replaces LogPrintf)
LogDebug(CATEGORY, ...) and LogTrace(CATEGORY, ...) for conditional logging (replaces LogPrint)
LogPrintLevel(CATEGORY, LEVEL, ...) for when the level isn’t known in advance, or a category needs to be added for an info/warning/error log message (mostly unchanged, but rarely needed)

Logs look roughly as they do now with LogInfo not having an [info] prefix, and LogDebug having a [cat] prefix, rather than a [cat:debug] prefix. This removes BCLog::Level::None entirely – for LogFlags::NONE just use Level::Info, for any actual category, use Level::Debug.

Adds docs to developer-notes about when to use which level.

Adds -loglevelalways=1 option so that you get [net:debug], [all:info], [all:warning] etc, which might be helpful for automated parsing, or just if you like everything to be consistent. Defaults to off to reduce noise in the default config, and to avoid unnecessary changes on upgrades.

Changes the behaviour of LogPrintLevel(CATEGORY, BCLog::Level::Info, ...) to be logged unconditionally, rather than only being an additional optional logging level in addition to trace and debug. Does not change the behaviour of LogPrintLevel(NONE, Debug, ...) and LogPrintLevel(NONE, Trace, ...) being no-ops.

DrahtBot commented at 4:46 am on August 22, 2023: 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
ACK	maflcko, stickies-v, jamesob, achow101
Concept ACK	jonatack, mzumsande, glozow

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:

#27826 (validation: log which peer sent us a header by Sjors)
#27231 (Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs by jonatack)

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:51 am on August 22, 2023: contributor

This implements ~most of~ ~all of~ most of my comment #25203 (comment)

I figure the main benefit of categories is to be able to selectively turn logging on/off – but for unconditional logging, you don’t have that benefit, so might as well just drop the category entirely, and just have the message be descriptive enough directly. Note that this effectively reverts #25306.

It should be compatible with other in-progress PRs – it only deprecates the LogPrint* functions, it doesn’t break them. And it should be easy to finish the conversion with a scripted-diff replacing LogPrintf/LogPrint across the codebase.

This seems like a straightforward way of getting the logging stuff finished up.

cc @jonatack

ajtowns marked this as a draft on Aug 23, 2023

ajtowns force-pushed on Aug 23, 2023

DrahtBot added the label CI failed on Aug 23, 2023

DrahtBot removed the label CI failed on Aug 23, 2023

maflcko added the label Utils/log/libs on Aug 23, 2023

ajtowns force-pushed on Aug 29, 2023

ajtowns commented at 2:34 pm on August 31, 2023: contributor

sample scripted diff code:

0sed -i 's/LogPrintLevel(\(BCLog::[^,]*\), BCLog::Level::Debug,/LogDebug(\1,/' net_processing.cpp dbwrapper.cpp net.cpp node/txreconciliation.cpp validation.cpp
1
2sed -i 's/LogPrintLevel(\(BCLog::[^,]*\), BCLog::Level::Error, */LogError(/' net.cpp txmempool.cpp
3
4sed -i 's/LogPrintLevel(\(BCLog::[^,]*\), BCLog::Level::Warning, */LogWarning(/' net.cpp
5
6sed -i 's/LogPrintf(/LogInfo(/' net.cpp
7sed -i 's/LogPrint(\(BCLog::[^,]*\),/LogDebug(\1,/' net.cpp

stickies-v commented at 2:35 pm on August 31, 2023: contributor

Concept ACK

maflcko approved

maflcko commented at 2:47 pm on August 31, 2023: member

Approach ACK. Makes sense to keep heavily used code snippets short.

This should also fix the issue of Info/Warning/Error messages not having a clear category and forcing the dev to somehow come up with one (or explicitly having to pick None)

maflcko commented at 2:47 pm on August 31, 2023: member

Is this ready for review, or any reason why this is still in draft?

ajtowns marked this as ready for review on Aug 31, 2023

ajtowns commented at 2:58 pm on August 31, 2023: contributor

Is this ready for review, or any reason why this is still in draft?

I was hoping for early feedback from @jonatack and initially wasn’t happy with the original “break the -loglevel command line, do stuff, then replace it” commit order (that’s now fixed). But no reason it can’t be reviewed now. It could also be split up to separate out the -loglevel vs -trace change, or to defer the map to atomic change – ie, I guess if later commits don’t feel worth the review effort, feel free to stop early?

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

I’ll have a look later today but my initial read, to be confirmed, was Approach NACK in the direction, which I’ll detail more on, and that this would hinder progress more than help.

I’ve been waiting for the merge of the bug fix, tests and updated docs in #27231 to continue on the next steps, as I thought it would be merged quickly (as bugfixes normally would be) and which is still waiting on re-ACKs after taking everyone’s feedback. ISTM this conflicts with it, which doesn’t help either. It doesn’t make sense to conflict with that PR for this one IMO.

ajtowns marked this as a draft on Aug 31, 2023

ajtowns commented at 4:29 pm on August 31, 2023: contributor

Is this ready for review, or any reason why this is still in draft?

Err, perhaps it was because I made some more changes and didn’t push them? [EDIT: Oh, was just some additional lint things]

ajtowns force-pushed on Aug 31, 2023

ajtowns marked this as ready for review on Aug 31, 2023

ajtowns commented at 4:38 pm on August 31, 2023: contributor

ISTM this conflicts with it, which doesn’t help either. It doesn’t make sense to conflict with that PR for this one IMO.

The first 6 commits apply cleanly on top of it; though LogPrintLevel(BCLog::NONE, BCLog::Level::Info, "foo9: %s\n", "bar9"); // no-op is no-longer a no-op, so the tests need modifying.

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

The first 6 commits apply cleanly on top of it

Thanks, will review properly asap.

maflcko commented at 4:49 pm on August 31, 2023: member

Absent the behavior changes, assuming they are controversial, you could also just split out the naming stuff.

I don’t see the benefit of making the code overly verbose and typing LogPrintLevel(BCLog::Level::Warning for every log when it is trivial to add an alias like LogWarning( and using it in new code should be uncontroversial.

I don’t have an opinion on the category and I think anything is probably fine there.

jonatack commented at 2:18 am on September 1, 2023: contributor

Some thoughts, commit-by-commit.

068de7abc5bda9fd9928d714 The idea behind the severity level logging is to ultimately have only one macro like Log(category, level) with one consistent, simple interface for everyone – both developers updating logging in the code and people who read the logs – and remove all the other macros when we’re ready, simplifying the logging macros, tests, and benchmarks. This commit not only would add several more macros, but also with different interfaces for developers to remember or look up, rather than just one.

https://github.com/bitcoin/bitcoin/pull/28318/commits/94e1b46776e68f26f530c23123e9cecdb661de8a This would move the category from the prefix back into the log message string on an ad hoc basis, would be less consistent, and IMO a step backward.

https://github.com/bitcoin/bitcoin/pull/28318/commits/f20bfc3d95be659cbfa502151dd960c44d315988 Non-urgent approach ACK, I can pull it into the parent in #25203 if you like.

https://github.com/bitcoin/bitcoin/pull/28318/commits/b3bf2fafa678fbde303e7cccff70e3b2a6b66627 This would be a step back, I think, away from consistent explicit logging, to requiring remembering more implicit mental context baggage (“why do I see categories for these logs and not for those?”) Also, the prefix allows not needing to put the category manually into the log strings.

https://github.com/bitcoin/bitcoin/pull/28318/commits/53a7f76feb8fee8a00bea245940e46d65892f692 Much of the value from severity level logging will come from making some of the noisy categories (net in particular, and maybe validation and mempool) much more useful by selecting which LogPrints become debug vs trace, and in general, by selecting which LogPrintfs become info, warning, or error.

4906f49f0932cc8f6244cff8cfbd79407031e7b2 is an incomplete version of 118c756 (#25203) in the parent pull.

https://github.com/bitcoin/bitcoin/pull/28318/commits/99ac8b174a373d204cd4f2d0d6c1f897d7646526 As discussed in #27231 (review), it seems to make sense to drop the NONE category and level enums from logging.{h,cpp} when we remove the deprecated macros (as opposed to 0/none values that are still needed at the init and rpc layers, cf #27231).

https://github.com/bitcoin/bitcoin/pull/28318/commits/cb580599e026c713f7b6abae7a7e873b07b89df8 I agree the config option interface is WIP; Klement Tan who was working on it took a job that diverted him from contributing. It seems incongruent to have -debug and -trace options but only -debugexclude. The plan discussed in one of the step PRs is to combine the functionality of the -debug and (help-debug) -loglevel options (perhaps to a renamed -log config option, which would be more consistent naming-wise with the logging RPC), and add log level functionality to that RPC. I haven’t done that in the parent PR yet, so it’s available to be done!

https://github.com/bitcoin/bitcoin/pull/28318/commits/451f2e76b3293836509c1865222c23ef7b4aacf2 Ok for avoiding a mutex. I have had the nagging thought for a long time that it may not be inconceivable for some users to prefer logging only errors and warnings, thus to avoid coding ourselves into a corner regarding info always being logged (which might apply to the preceding commit as well).

ajtowns commented at 7:46 am on September 1, 2023: contributor

068de7a The idea behind the severity level logging is to ultimately have only one macro like Log(category, level) with one consistent, simple interface for everyone – both developers updating logging in the code and people who read the logs

I think there’s a higher level goal than that: logs should be convenient and easy for developers and users. Hopefully that’s a boring platitude, so I’m going to assume you agree with that?

In particular, I don’t think we should prioritise “one function” if it makes reading/writing code tedious and inconvenient, and to me, reading/writing Log(BCLog::NET, BCLog::Level::Debug, ...) (with or without the current PrintLevel) is pretty ugly. I think it’s better to have multiple functions than the other ways I can think of removing the BCLog::Level:: boilerplate.

Second, for both users and developers, I think the mix of enabled/disabled categories and global/category-specific levels is super confusing. AFAICS at the highest level we only really want three things:

unconditional logging for high-importance information
subsystem specific debug logging that can be turned on (in production) to get detailed information about what’s going on
subsystem specific trace logging to get highly detailed logging that may have severe performance impacts to diagnose specific misbehaviours

94e1b46 This would move the category from the prefix back into the log message string on an ad hoc basis, would be less consistent, and IMO a step backward.

My reasoning here is that the usefulness of the “category” isn’t in telling us what bit of code triggered the log message (we have -logsourcelocations for that which is much more precise, and you can generally just grep for the message anyway), but in categorising log messages the user can disable. It’s useful for the user to see [net] because it tells them they can use -debug=net to enable it; it’s useful for the user to see [net:trace] because it tells them they also need to enable tracing to see that. It’s useful to see [warning] or [error] because those are things they probably want to take action on, and having a consistent tag means they can grep for them more easily. It’s not useful to see [net:info] because that doesn’t tell them anything they can action, it’s just noise that makes it harder to read the logs.

So I contend that approach is consistent, it’s just consistent over a different (in my opinion more valuable) attribute; namely that categories are solely for conditional logging that the user can disable on a per-subsystem basis.

f20bfc3 Non-urgent approach ACK, I can pull it into the parent in #25203 if you like.

(I don’t think any of this is “urgent”)

b3bf2fa This would be a step back, I think, away from consistent explicit logging, to requiring remembering more implicit mental context baggage (“why do I see categories for these logs and not for those?”) Also, the prefix allows not needing to put the category manually into the log strings.

That question is easy to answer: you see categories for log entries that you need to use config options to enable. Likewise in the code: you write BCLog::HTTP for LogDebug and LogTrace entries that will only be shown if the user specifically enables that log category, and you don’t write it when that’s not the case.

Compare:

LogInfo("Starting HTTP server with %d worker threads\n", rpcThreads);
LogPrintfCategory(BCLog::HTTP, "Starting server with %d worker threads\n", rpcThreads);
Log(BCLog::HTTP, BCLog::Level::Info, "Starting server with %d worker threads\n", rpcThreads);

and the log messages:

Starting HTTP server with 2 worker threads
[http] Starting server with 2 worker threads
[http:info] Starting server with 2 worker threads

For unconditional logging, just writing self-explanatory text is shorter and easier to understand.

For conditional logging, we have to tell the compiler the category, so the BCLog::HTTP part isn’t really avoidable, but having functions to cover the two levels of conditional logging at least eliminates the boiler plate.

53a7f76 Much of the value from severity level logging will come from making some of the noisy categories (net in particular, and maybe validation and mempool) much more useful by selecting which LogPrints become debug vs trace, and in general, by selecting which LogPrintfs become info, warning, or error.

I think it would make sense for the line between info/warning/error to be:

normally, it’s a category specific debug log. your log message probably just isn’t that important.
otherwise, for important but rare messages (eg on startup), use info.
for something where the admin probably wants to take action, upgrade that to warning (eg things that trigger SetMiscWarning, like your system time being set wrong; or if you’ve enabled tor but tor connections aren’t working)
use error for things severe enough that bitcoind will shutdown now (or perhaps just some subsystem, eg a particular wallet that’s discovered its db is corrupt/unreadable/unwritable)

That seems pretty easy for both developers and users to keep consistent.

(Actually, maybe it’s worth distinguishing “startup” and “recurring-info” messages – it’s useful and okay for startup to be a bit noisy; but info messages should be very rare. Something like LogInit(...) for unconditional startup logging, and LogImportant(...) for UpdateTip and new outbounds.)

The “if it’s usable in production, then do it as debug, only use trace for stuff that’s not usable in production” rule I suggested above would imply that it’d be better to split net into multiple categories rather than move it into trace, I think. Concretely, the “send: msg” / “recv: msg” logging in particular could be its own category. [net:trace] would only be useful for extra logging that’s not suitable for production, maybe like logging every w/txid that’s in an INV message, rather than just the first?

4906f49 is an incomplete version of 118c756 (#25203) in the parent pull.

I mean, that PR had been closed for 7 months… That change is also inconsistent with what you recommended in #27277 (review)

99ac8b1 As discussed in #27231 (comment), it seems to make sense to drop the NONE category and level enums from logging.{h,cpp} when we remove the deprecated macros (as opposed to 0/none values that are still needed at the init and rpc layers, cf #27231).

That commit is about dropping Level::None (the alternative that currently gives us “[net] foo” instead of “[net:debug] foo”), not the category BCLog::NONE which is what the 27231 discussion is about.

cb58059 I agree the config option interface is WIP; Klement Tan who was working on it took a job that diverted him from contributing. It seems incongruent to have -debug and -trace options but only -debugexclude. The plan discussed in one of the step PRs is to combine the functionality of the -debug and (help-debug) -loglevel options (perhaps to a renamed -log config option, which would be more consistent naming-wise with the logging RPC), and add log level functionality to that RPC. I haven’t done that in the parent PR yet, so it’s available to be done!

shrug That was mostly about giving an api that doesn’t expose the global logging level and didn’t break tests (which exclude libevent) (and that writing -debug=net -debug=mempool -log=net:trace -log=mempool:trace seems unnecessarily clunky). Another approach to consider might be replacing -debug=all -debugexclude=libevent with -debug=all -debug=-libevent, which would naturally generalise to the same syntax with -trace.

I presume the logging RPC would just change to accept ["net"] ["validation"] ["wallet"] to set net’s logging level to debug, validation’s to disabled/info, and wallet’s to debug+trace, and return {"net": true, "validation": false, "wallet": "trace"} or something.

451f2e7 Concept ACK on avoiding a mutex. I have had the nagging thought for a long time that it may not be inconceivable for some users to prefer logging only errors and warnings, thus to avoid coding ourselves into a corner regarding info always being logged (which might apply to the preceding commit as well).

I don’t think that’s a sensible concern: unconditional logging should be rare enough to not need disabling (eg, connecting to new outbounds, new headers, and new tips for about 400 entries a day), and at that point it’s better not to disable them as that gives the user a pretty clear indication their node is still working right. Even for users that only care about errors/warnings, having the UpdateTip context at least is likely useful for giving some clue what state the node was in when it encountered the problem.

DrahtBot added the label CI failed on Sep 3, 2023

DrahtBot removed the label CI failed on Sep 5, 2023

ajtowns force-pushed on Sep 12, 2023

ajtowns commented at 10:13 am on September 12, 2023: contributor

Rebased and rearranged.

I’ve added -loglevelalways to give users the choice between low-noise logging / what we have now, where you just have [net] or [warning] show up, or always having the full category/severity info prefixed [net:debug] or [all:info] showing up, with the idea that the extra “noise” there might be helpful for automated parsing of logs and similar (and some might just prefer it anyway). In particular, after this PR and with that option set, all log messages specify a category and severity now, so that part of the “severity-based-logging” goal can be considered complete.

I haven’t based this on #27231 but it should be easy to rebase either PR once the other has been merged – there’s a conflict in the tests, which have to be edited either way, since 27231 adds test coverage of behaviour that’s changed in this PR (namely info logging becoming unconditional).

I’ve dropped the -trace command line argument and the conversion from map to bit flags in an atomic int from this PR; still think stuff like that is worth doing, but it doesn’t need to all happen at once.

I’ve also added docs on when to use the different severities, which seem like a useful thing to have.

I think this should be ready for review/merge at this point.

DrahtBot added the label CI failed on Sep 23, 2023

DrahtBot removed the label CI failed on Sep 25, 2023

in src/logging.cpp:1 in 9f71029192 outdated

maflcko commented at 2:36 pm on December 14, 2023:

nit in 9f71029192282baa4f7be6b5124bb7bc2491ef84: Could mark commit as “refactor”, if it is one?

maflcko commented at 2:45 pm on December 14, 2023:

Also, could add a test in this commit about [info] that will change in the next commit?

ajtowns commented at 1:03 am on December 15, 2023:

Added a test; tagged as refactor for whatever that’s worth

in doc/developer-notes.md:932 in db5ed8682f outdated

928@@ -891,7 +929,7 @@ Strings and formatting
929     `wcstoll`, `wcstombs`, `wcstoul`, `wcstoull`, `wcstoumax`, `wcswidth`,
930     `wcsxfrm`, `wctob`, `wctomb`, `wctrans`, `wctype`, `wcwidth`, `wprintf`
931 
932-- For `strprintf`, `LogPrint`, `LogPrintf` formatting characters don't need size specifiers.
933+- For `strprintf`, `LogInfo`, `LogDebug` etc formatting characters don't need size specifiers.

maflcko commented at 3:04 pm on December 14, 2023:

nit in db5ed8682f99c64a97e598bda69147307b447820: comma before etc?

in doc/developer-notes.md:735 in db5ed8682f outdated

730+logging messages. They should be used as follows:
731+
732+- `LogDebug(BCLog::CATEGORY, fmt, params...)` is what you want
733+  most of the time, and it should be used for log messages that are
734+  useful for debugging and can reasonably be enabled on a production
735+  system. They will be logged if bitcoind is started with `-debug=category`

maflcko commented at 3:07 pm on December 14, 2023:

nit in https://github.com/bitcoin/bitcoin/commit/db5ed8682f99c64a97e598bda69147307b447820: Clarify “production system with enough free storage space”. IIRC ~1TB per 100 connection-years? So if you have 100 connections and run for a year, 1TB of your storage will be eaten by transaction relay net debug messages, or so.

in doc/developer-notes.md:742 in db5ed8682f outdated

737+
738+- `LogInfo(fmt, params...)` should only be used rarely, eg for startup
739+  messages or for infrequent and important events such as a new block
740+  tip being found or a new outbound connection being made. These log
741+  messages are unconditional so care must be taken that they can't be
742+  used by an attacker to fill up the disk.

maflcko commented at 3:08 pm on December 14, 2023:

nit in https://github.com/bitcoin/bitcoin/commit/db5ed8682f99c64a97e598bda69147307b447820: Replace “disk” with “storage” or “persistent storage”, so that non-spinning storage doesn’t feel excluded?

in doc/developer-notes.md:746 in db5ed8682f outdated

741+  messages are unconditional so care must be taken that they can't be
742+  used by an attacker to fill up the disk.
743+
744+- `LogError(fmt, params...)` should be used in place of `LogInfo` for
745+  severe problems that require the node (or a subsystem) to shut down
746+  entirely (eg, insufficient disk space).

maflcko commented at 3:09 pm on December 14, 2023:

same :)

in doc/developer-notes.md:756 in db5ed8682f outdated

751+  appears to be wrong, unknown soft fork appears to have activated).
752+
753+- `LogTrace(BCLog::CATEGORY, fmt, params...) should be used in place of
754+  `LogDebug` for log messages that would be unusable on a production
755+  system, eg due to being too noisy in normal use, or too resource
756+  intensive to process. These will be logged if bitcoind is started

maflcko commented at 3:11 pm on December 14, 2023:

nit in https://github.com/bitcoin/bitcoin/commit/db5ed8682f99c64a97e598bda69147307b447820: Replace bitcoind with “the program” or “Bitcoin Core”, so that bitcoin-node or bitcoin-qt do not feel excluded?

ajtowns commented at 1:00 am on December 15, 2023:

Reworded. Note there’s lots of places in that file that talk about “bitcoind”…

maflcko approved

maflcko commented at 3:14 pm on December 14, 2023: member

Feel free to ignore the nits.

very nice, ACK 757b4c7776f4d11b9860bc93c514d8fea5ef1db 🎺

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: very nice, ACK 757b4c7776f4d11b9860bc93c514d8fea5ef1db 🎺
3tV/vqW396UNXRNAWVvgywpEMaq3xHn2/FTivxxEFeBaKfl+dcQWfVKnAiEzqDqsI2B/NlTdjl939rlFW3hC1Bw==

DrahtBot requested review from stickies-v on Dec 14, 2023

DrahtBot requested review from jonatack on Dec 14, 2023

jamesob commented at 4:14 pm on December 14, 2023: member

Big concept ACK on the interface. This is a nice simplification.

ajtowns force-pushed on Dec 15, 2023

logging: refactor: pull prefix code out c5c76dc615

logging: make [cat:debug] and [info] implicit dfe98b6874

logging: Log Info messages unconditionally

Previously Info-level logging when a category was specified (via
LogPrintLevel) would only print the corresponding log message if
`-debug=category` were specified, while Info-level logging without a
category would always be printed. Make this more consistent by having
Info messages always be logged, whether they include a category or not.

ab34dc6012

logging: Drop BCLog::Level::None

Now that Info-level logging is always logged, there is no further
need for the "None" level, so remove it.

667ce3e329

logging: treat BCLog::ALL like BCLog::NONE 782bb6a056

logging: add -loglevelalways=1 option

This option tells the logging system to always include a "[cat:level]"
prefix, so [net] becomes [net:debug], LogInfo/LogPrint statements will have
an [all:info] prefix, and LogWarning and LogError logs will become
[all:warning] and [all:error]. This may be easier for automated parsing
of logs, particularly if additional prefixes such as thread or source
location are enabled.

fbd7642c8e

ajtowns force-pushed on Dec 15, 2023

in doc/developer-notes.md:736 in e50934f9b2 outdated

731+
732+- `LogDebug(BCLog::CATEGORY, fmt, params...)` is what you want
733+  most of the time, and it should be used for log messages that are
734+  useful for debugging and can reasonably be enabled on a production
735+  system (that has sufficient free storage space). They will be logged
736+  if bitcoind is started with `-debug=category` or `-debug=1`.

maflcko commented at 11:10 am on December 15, 2023:

nit (same): “with the startup options -debug....”

ajtowns commented at 6:00 am on December 20, 2023:

nit notted

maflcko approved

maflcko commented at 11:10 am on December 15, 2023: member

re-ACK 16dd9042ea81412607c0645e4a3cc152abdc4d9a 😹

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK 16dd9042ea81412607c0645e4a3cc152abdc4d9a 😹
3LERMjNl3P8a4k1TfQERdSsQsKXp6qmX2bKUeU8FCwbzPnULd2KQlAdsRWrQjWCs8Ju9RG4vBerN1q+9s/WCpCg==

DrahtBot requested review from jamesob on Dec 15, 2023

in src/logging.cpp:131 in ab34dc6012 outdated

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

mzumsande commented at 9:57 pm on December 15, 2023:

Just noting that this doesn’t change the the one existing use of BCLog::Level::Info in httpserver from non-default to default because InitHTTPServer() just disables libevent logging callbacks entirely unless BCLog::LIBEVENT isn’t enabled.

jonatack commented at 4:22 pm on January 11, 2024:

ab34dc6012351 The loglevel help wasn’t updated with this change (note that this commit is an incomplete copy of 118c756 (#25203) from the Severity-based logging parent PR). Fixed in #29230.

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

Possibly relevant comment: 118c756 (#25203)

in doc/developer-notes.md:729 in e50934f9b2 outdated

722@@ -723,6 +723,44 @@ General Bitcoin Core
723   - *Explanation*: If the test suite is to be updated for a change, this has to
724     be done first.
725 
726+Logging
727+-------
728+
729+The macros `LogInfo`, `LogDebug`, `LogTrace`, `LogWarning` and `LogError` are available for

mzumsande commented at 11:12 pm on December 18, 2023:

As long as they are used all over the place, I think that LogPrintf() and LogPrint() should be mentioned to avoid confusion (could explain that they are a legacy options no longer recommended for use). Especially since the current state could last a while, who knows how fast future PRs updating existing log entries with dozen of conflicts would get merged.

ajtowns commented at 5:58 am on December 20, 2023:

Added notes for these.

mzumsande commented at 6:26 pm on December 19, 2023: contributor

Concept ACK

I like the simplified interface, having to write something like LogPrintLevel(BCLog::NET, BCLog::Level::Error, ...) for each log would be annoying, plus I like that there wouldn’t be a widely-used function/macro that could log both unconditionally and conditionally depending on parameters, which is a bit of a footgun.

ajtowns force-pushed on Dec 20, 2023

logging: add LogError, LogWarning, LogInfo, LogDebug, LogTrace

These provide simple and clear ways to write the most common logging
operations:

    LogInfo("msg");
    LogDebug(BCLog::LogFlags::NET, "msg");

    LogError("msg");
    LogWarning("msg");
    LogTrace(BCLog::LogFlags::NET, "msg");

For cases where the level cannot be hardcoded, LogPrintLevel(category,
level, ...) remains available.

f7ce5ac08c

logging: Replace uses of LogPrintfCategory

Replace LogPrintfCategory with alternative unconditional log statements.

e60fc7d5d3

ajtowns force-pushed on Dec 20, 2023

maflcko commented at 10:42 am on December 20, 2023: member

re-ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c 🌚

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c 🌚
39u130MWdX3CjjdI8HvlKNLo4eN/W08af+mU03bsnYiaWXHNdB1H37mnLjVU/JuMDVYsSrpXqKW+evmJI2/CkAQ==

DrahtBot requested review from mzumsande on Dec 20, 2023

jonatack commented at 9:08 pm on January 6, 2024: contributor

I like the simplified interface, having to write something like LogPrintLevel(BCLog::NET, BCLog::Level::Error, ...) for each log would be annoying

For the severity level logging, I’d prefer to have a single macro like Log(category, level) with a consistent, simple interface, rather than 5 macros that require varying parameters. See #28318#pullrequestreview-1605951639, above.

in src/logging.h:200 in e60fc7d5d3

196@@ -194,7 +197,7 @@ namespace BCLog {
197         std::string LogLevelsString() const;
198 
199         //! Returns the string representation of a log level.
200-        std::string LogLevelToStr(BCLog::Level level) const;
201+        static std::string LogLevelToStr(BCLog::Level level);

stickies-v commented at 12:23 pm on January 8, 2024:

nit: would it make sense to just move this to BCLog, out of ::Logger?

 0diff --git a/src/init/common.cpp b/src/init/common.cpp
 1index 6560258ef5..0832845921 100644
 2--- a/src/init/common.cpp
 3+++ b/src/init/common.cpp
 4@@ -31,7 +31,7 @@ void AddLoggingArgs(ArgsManager& argsman)
 5         ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
 6     argsman.AddArg("-debugexclude=<category>", "Exclude debug and trace logging for a category. Can be used in conjunction with -debug=1 to output debug and trace logging for all categories except the specified category. This option can be specified multiple times to exclude multiple categories.", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
 7     argsman.AddArg("-logips", strprintf("Include IP addresses in debug output (default: %u)", DEFAULT_LOGIPS), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
 8-    argsman.AddArg("-loglevel=<level>|<category>:<level>", strprintf("Set the global or per-category severity level for logging categories enabled with the -debug configuration option or the logging RPC: %s (default=%s); warning and error levels are always logged. If <category>:<level> is supplied, the setting will override the global one and may be specified multiple times to set multiple category-specific levels. <category> can be: %s.", LogInstance().LogLevelsString(), LogInstance().LogLevelToStr(BCLog::DEFAULT_LOG_LEVEL), LogInstance().LogCategoriesString()), ArgsManager::DISALLOW_NEGATION | ArgsManager::DISALLOW_ELISION | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
 9+    argsman.AddArg("-loglevel=<level>|<category>:<level>", strprintf("Set the global or per-category severity level for logging categories enabled with the -debug configuration option or the logging RPC: %s (default=%s); warning and error levels are always logged. If <category>:<level> is supplied, the setting will override the global one and may be specified multiple times to set multiple category-specific levels. <category> can be: %s.", LogInstance().LogLevelsString(), BCLog::LogLevelToStr(BCLog::DEFAULT_LOG_LEVEL), LogInstance().LogCategoriesString()), ArgsManager::DISALLOW_NEGATION | ArgsManager::DISALLOW_ELISION | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
10     argsman.AddArg("-logtimestamps", strprintf("Prepend debug output with timestamp (default: %u)", DEFAULT_LOGTIMESTAMPS), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
11 #ifdef HAVE_THREAD_LOCAL
12     argsman.AddArg("-logthreadnames", strprintf("Prepend debug output with name of the originating thread (only available on platforms supporting thread_local) (default: %u)", DEFAULT_LOGTHREADNAMES), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
13diff --git a/src/logging.cpp b/src/logging.cpp
14index 42f100ded6..b1f0efbbd8 100644
15--- a/src/logging.cpp
16+++ b/src/logging.cpp
17@@ -202,7 +202,7 @@ bool GetLogCategory(BCLog::LogFlags& flag, const std::string& str)
18     return false;
19 }
20 
21-std::string BCLog::Logger::LogLevelToStr(BCLog::Level level)
22+std::string BCLog::LogLevelToStr(BCLog::Level level)
23 {
24     switch (level) {
25     case BCLog::Level::Trace:
26@@ -407,7 +407,7 @@ std::string BCLog::Logger::GetLogPrefix(BCLog::LogFlags category, BCLog::Level l
27 
28         // Only add separator if we have a category
29         if (has_category) s += ":";
30-        s += Logger::LogLevelToStr(level);
31+        s += LogLevelToStr(level);
32     }
33 
34     s += "] ";
35diff --git a/src/logging.h b/src/logging.h
36index 525e0aec6d..a2228ac7ba 100644
37--- a/src/logging.h
38+++ b/src/logging.h
39@@ -81,6 +81,9 @@ namespace BCLog {
40     };
41     constexpr auto DEFAULT_LOG_LEVEL{Level::Debug};
42 
43+    //! Returns the string representation of a log level.
44+    std::string LogLevelToStr(BCLog::Level level);
45+
46     class Logger
47     {
48     private:
49@@ -196,9 +199,6 @@ namespace BCLog {
50         //! Returns a string with all user-selectable log levels.
51         std::string LogLevelsString() const;
52 
53-        //! Returns the string representation of a log level.
54-        static std::string LogLevelToStr(BCLog::Level level);
55-
56         bool DefaultShrinkDebugFile() const;
57     };
58

in src/logging.h:240 in e60fc7d5d3

236@@ -234,22 +237,17 @@ static inline void LogPrintf_(const std::string& logging_function, const std::st
237 #define LogPrintLevel_(category, level, ...) LogPrintf_(__func__, __FILE__, __LINE__, category, level, __VA_ARGS__)
238 
239 // Log unconditionally.
240-#define LogPrintf(...) LogPrintLevel_(BCLog::LogFlags::NONE, BCLog::Level::None, __VA_ARGS__)
241+#define LogInfo(...) LogPrintLevel_(BCLog::LogFlags::ALL, BCLog::Level::Info, __VA_ARGS__)

stickies-v commented at 2:37 pm on January 8, 2024:

nit: it seems like we can easily avoid using a macro here, wouldn’t that be better?

0template <typename... Args>
1static inline void LogInfo(Args&&... args) { LogPrintLevel_(BCLog::LogFlags::ALL, BCLog::Level::Info, std::forward<Args>(args)...); }

ajtowns commented at 7:26 pm on January 8, 2024:

Until we can use std::source_location (see #28999 (comment)) we need to use macros in order to pass __func__, __FILE__ and __LINE__ information through.

stickies-v commented at 10:55 pm on January 8, 2024:

Gha forgout about that. Macro it is, thanks.

maflcko commented at 9:01 am on January 9, 2024:

If this is changed, the format string could also be consteval’d into a compile-time checked object to do the newline check (and maybe others), which is currently done by clang-tidy-plugin.

maflcko commented at 5:42 am on April 18, 2024:

Could also check if source_location fixes the annoyance on Windows to print the full path when logging:

02024-04-15T13:27:36.5198997Z  node0 2024-04-15T13:27:36.001614Z [D:\a\bitcoin\bitcoin\src\validation.cpp:2549] [ConnectBlock] [bench]     - Verify 0 txins: 0.06ms (0.000ms/txin) [0.00s (0.03ms/blk)]

in doc/developer-notes.md:738 in e60fc7d5d3

733+  most of the time, and it should be used for log messages that are
734+  useful for debugging and can reasonably be enabled on a production
735+  system (that has sufficient free storage space). They will be logged
736+  if the program is started with `-debug=category` or `-debug=1`.
737+  Note that `LogPrint(BCLog::CATEGORY, fmt, params...)` is a deprecated
738+  alias for `LogDebug`.

stickies-v commented at 5:06 pm on January 8, 2024:

nit: for consistency to avoid confusion

0  Note that `LogPrint(BCLog::CATEGORY, fmt, params...)` is a deprecated
1  alias for `LogDebug(fmt, params...)`.

stickies-v approved

stickies-v commented at 6:51 pm on January 8, 2024: contributor

ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c

After e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c, is there a reason not to remove LogPrintfCategory completely since it’s now only used in tests?

For the severity level logging, I’d prefer to have a single macro like Log(category, level) with a consistent, simple interface, rather than five macros.

In most other codebases I’ve worked on, having a per-level logging function was pretty standard. I think the approach suggested here is more developer-friendly than forcing usage of a single Log function, especially since the log levels are small in number and not likely to significantly change.

jonatack commented at 7:23 pm on January 8, 2024: contributor

forcing usage of a single Log function

That doesn’t make sense if the goal, like the title of this PR, is to simplify the API. One macro is simpler, and involves less code and documentation and test coverage, than five that take varying parameters.

I plan to re-review here soon. My work on severity-based logging in #25203 was set aside to first fix current bugs, add missing tests and improve the documentation in #27231 that has been open for ten months.

in src/logging.h:80 in 667ce3e329 outdated

76@@ -77,7 +77,6 @@ namespace BCLog {
77         Info,      // Default
78         Warning,
79         Error,
80-        None, // Internal use only

jamesob commented at 10:18 pm on January 8, 2024:

Minor thing, but there’s still a lingering reference in contrib/devtools/bitcoin-tidy/example_logprintf.cpp. Not sure if that needs to be updated or not.

in doc/developer-notes.md:740 in f7ce5ac08c outdated

735+  system (that has sufficient free storage space). They will be logged
736+  if the program is started with `-debug=category` or `-debug=1`.
737+  Note that `LogPrint(BCLog::CATEGORY, fmt, params...)` is a deprecated
738+  alias for `LogDebug`.
739+
740+- `LogInfo(fmt, params...)` should only be used rarely, eg for startup

jamesob commented at 10:38 pm on January 8, 2024:

https://github.com/bitcoin/bitcoin/pull/28318/commits/f7ce5ac08c669ac763e275bb7c82dcfb2b1b6c33

It’s somewhat wonky that e.g. LogInfo() doesn’t take a category, but e.g. LogDebug does, and in later commits we actually remove category information from info-equivalent log statements, which seems backwards to me. Ideally, LogInfo would optionally take a category. But maybe this can be done later?

stickies-v commented at 11:38 am on January 9, 2024:

Ideally, LogInfo would optionally take a category.

I had that intuition at first too, but then changed my mind after reading ajtowns’s comment stating:

My reasoning here is that the usefulness of the “category” isn’t in telling us what bit of code triggered the log message (we have -logsourcelocations for that which is much more precise, and you can generally just grep for the message anyway), but in categorising log messages the user can disable. It’s useful for the user to see [net] because it tells them they can use -debug=net to enable it; it’s useful for the user to see [net:trace] because it tells them they also need to enable tracing to see that. It’s useful to see [warning] or [error] because those are things they probably want to take action on, and having a consistent tag means they can grep for them more easily. It’s not useful to see [net:info] because that doesn’t tell them anything they can action, it’s just noise that makes it harder to read the logs.

So I contend that approach is consistent, it’s just consistent over a different (in my opinion more valuable) attribute; namely that categories are solely for conditional logging that the user can disable on a per-subsystem basis.

Given that LogInfo is unconditional, what would be your use case for adding a log category?

jamesob commented at 7:04 pm on January 9, 2024:

Given that LogInfo is unconditional, what would be your use case for adding a log category?

Simply that “info = unconditional” might not always be the case (and is an unusual convention to have hardcoded), and a user could rightly want to see only WARNING+ logs as well as, say, net or validation-related INFOs.

stickies-v commented at 11:43 am on January 11, 2024:

I think ajtowns addressed that quite well at the bottom of this comment:

I don’t think that’s a sensible concern: unconditional logging should be rare enough to not need disabling (eg, connecting to new outbounds, new headers, and new tips for about 400 entries a day), and at that point it’s better not to disable them as that gives the user a pretty clear indication their node is still working right. Even for users that only care about errors/warnings, having the UpdateTip context at least is likely useful for giving some clue what state the node was in when it encountered the problem.

Seeing warning/error-logs only is easy enough to do with grep, and info logging should be infrequent enough to not take up significant system resources.

jamesob commented at 2:45 pm on January 11, 2024:

@stickies-v it’s not about system resources; it’s about allowing the end user to articulate what they see. Not a big deal either way, but I think our current scheme is a little wonky if you compare it to more widely deployed systems e.g. the python logging module or log4j.

stickies-v commented at 4:31 pm on January 11, 2024:

it’s about allowing the end user to articulate what they see

which is trivial to do with grep? My point about system resources is that theoretically we could always log everything but that would use an unreasonable amount of system resources, hence the nuance.

ryanofsky commented at 8:21 pm on January 11, 2024:

which is trivial to do with grep?

This doesn’t seem trivial if some log messages have categories and some don’t. Like there is no way anymore to only see a list of all the messages only from wallet, or all the messages only from net_processing. I don’t think this is terrible, but it does seem weird, and I wonder if any log systems work like this where they go out of their way to define categories, but then choose not to include categories in the most important messages.

ajtowns commented at 9:13 pm on January 11, 2024:

This doesn’t seem trivial if some log messages have categories and some don’t.

If you want all log messages to have categories for easier grepping (or for importing into a database), there’s the -loglevelalways=1 option introduced in this PR.

Like there is no way anymore to only see a list of all the messages only from wallet,

If you want to focus on messages from some section of the code, then that’s what the -logsourcelocations=1 option is for.

Your wording suggests that you think this is a regression. It’s not: we’ve traditionally used LogPrintf() for non-debug messages which also doesn’t allow selecting only messages from the wallet, and does not add any consistent tagging.

I don’t think this is terrible, but it does seem weird, and I wonder if any log systems work like this where they go out of their way to define categories, but then choose not to include categories in the most important messages.

I don’t think it is actually terribly weird compared to log4j: the equivalent there would be setting up a single global logger, that does info/warn/error levels and always logs all messages, and a bunch of subsystem-specific loggers that do debug/trace levels and that default to off. I think it’d be roughly the same with python: have a global logger for info/warn/error (logging.warning("boo")), and child loggers for subsystems for debug/trace (logging.getLogger('net').debug('did something')).

I’m not really sure what else to say – I feel like the arguments stickies-v quoted above were pretty clear, and the result is both simple and useful, even if it seems “weird” or surprising.

jamesob commented at 9:18 pm on January 11, 2024:

To clarify, I don’t think that anyone is debating that this changeset is an improvement over the status quo. The ask (at least from my end) would be to unify the Log* interfaces and allow the optional specification of a category. That way, when migrating existing logging statements over, we’re not throwing away category information just because something is INFO. And we support users who want to see WARN only most of the time, but care about, say, net INFO messages. That doesn’t seem like a crazy thing to ask for.

ryanofsky commented at 1:04 pm on January 12, 2024:

IMO, being able to filter log messages and only see messages from a particular component is a useful thing to support. Current bitcoin behavior of defining categories and not actually using them is weird, and wanting to bake this behavior into the logging API seems even weirder. The python logger is very flexible and of course it allows you to log per-component or globally. But if you did both these things in a single python project, it would be confusing and weird there too.

maflcko commented at 1:40 pm on January 12, 2024:

No opinion on this thread, but I wanted to mention that it is not always possible to easily define a logging category right now. For example, log statements in the flatfile module may fit in the “blockstorage” or “indexes” category, depending on context. Thus, either the log category would have to be passed down the call stack before logging, or the log-string would have to be passed up the call stack before logging.

Also, some modules, such as args, have only a few log statements, so creating a category for each of them may be verbose. Though, that is probably less of a problem.

Again, no opinion on this. I think either solution is fine. PRs welcome, I guess?

ryanofsky commented at 9:58 pm on January 12, 2024:

Again, no opinion on this. I think either solution is fine. PRs welcome, I guess?

Thanks, I started playing around with a change that should make it easier to set the category without being verbose, and also start relying less on a global logging instance, which should be useful for kernel applications and tests: ae450f19032ba388beefc56677c8788d6783ce92. Will see how this looks and probably open a PR

ryanofsky commented at 10:12 pm on January 16, 2024:

Will see how this looks and probably open a PR

Opened #29256 with these changes. Code changes should be complete but there are some test failures so it is a draft.

jamesob approved

jamesob commented at 10:50 pm on January 8, 2024: member

ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c (jamesob/ackr/28318.1.ajtowns.logging_simplify_api_for)

This is a definite improvement in the new-style logging interface.

I built and reviewed each commit locally, and ran ./test/functional/feature_segwit.py --nocleanup and reviewed the resulting debug.log to sanity check.

Eventually it would be nice to unify the interface for each logging function (e.g. have the ability to specify category for Info), but personally what is written here would be an improvement over the slightly wordy LogPrintfCategory() equivalents that I’ve never found intuitive.

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c ([`jamesob/ackr/28318.1.ajtowns.logging_simplify_api_for`](https://github.com/jamesob/bitcoin/tree/ackr/28318.1.ajtowns.logging_simplify_api_for))
 4
 5This is a definite improvement in the new-style logging interface.
 6
 7I built and reviewed each commit locally, and ran `./test/functional/feature_segwit.py --nocleanup` and reviewed the resulting debug.log to sanity check.
 8
 9Eventually it would be nice to unify the interface for each logging function (e.g. have the ability to specify category for Info), but personally what is written here would be an improvement over the slightly wordy `LogPrintfCategory()` equivalents that I've never found intuitive.
10-----BEGIN PGP SIGNATURE-----
11
12iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmWcfI8ACgkQepNdrbLE
13TwWeeBAAiISGHmXYnPZNG+pefl/FmVCdsWNXlPfI1tmV5QNY5e0SwVYXCMCerKYU
14n7LQcLJv20rfGpHFclmSqV77eSoZfXTDEY7V+ySERpIWM83H3mPOtmC/KzczfoAd
15MklMaQ0GXeyqpZBkU9dstzYR2OvL8/X8Pg8Qsx9+90Cr318AAj+diXePaLyycbkQ
16mmbGn36oM69crn6KTilDoDxION1pqC/YAZBLyl9MQ57B4kwML/VM/iLZPUhg9yOf
17KbTJ0UVAeSMGrzYeUDlirx0K0KIiy0gEzby6YEPU60YM9YdHIBGIagaWpOIOHAEt
18RnWc3VzQryrS3PqHTeTe/PqnHFzc1ukQe1QTGQ7nCJW/RpVijOVjEwgAwPVaB0Zn
199DOZJBWY4WbnCGgfIMwQh35j+qg0Pc7omhmhFb7f6GftSWmEaOytcZef7peEBwyq
209ls7+rBOkkL5CW+8atU+UrRzRjSjvqIUFM14ks5cbgIGfvGzu0uGELaVkyYyuON4
21AKZC/kgN+MhYNRINWkZ3d3nVBulc4QqV+ClAMk8WjM8DKz8gstfwSDrCnpQTlBVT
22pN/nkGlAHzDeRFi2FypS5ObBo3LHRCzmhqnXbe6IKj+D+8EheFReCXEiuxMNcqpQ
23Sp1BmpgFRAe9x/CSUZhicYQThOteZvaDWmL9sz44zewMmk4IV8c=
24=Ho6t
25-----END PGP SIGNATURE-----

0Tested on Linux-6.6.8-arch1-1-x86_64-with-glibc2.38
1
2Configured with ./configure 'BDB_LIBS=-L/home/james/src/bitcoin/db4/lib -ldb_cxx-4.8' BDB_CFLAGS=-I/home/james/src/bitcoin/db4/include 'CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare ' --disable-gui-tests --disable-fuzz-binary --enable-wallet --enable-debug --with-daemon --enable-natpmp-default
3
4Compiled with /usr/bin/ccache /usr/bin/clang++ -std=c++17 -mavx -mavx2 -mpclmul -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare  -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=.  -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -msse4.1 -msse4.2 -msse4 -msha  i
5
6Compiler version: clang version 16.0.6

glozow commented at 11:33 am on January 9, 2024: member

Concept ACK based on the developer-notes. I’m a fan of having 1 not-too-verbose one that we use most of the time, and then a few others based on severity.

achow101 commented at 6:59 pm on January 10, 2024: member

ACK e60fc7d5d34f23cccbff6e4f5f3d716fa8dad50c

I also prefer the use of macros/functions for the different categories as it is much less verbose than LogPrintLevel(CAT, LEVEL, ...). This pattern is also similar to what many other logging frameworks do, e.g. python’s logging module.

DrahtBot requested review from glozow on Jan 10, 2024

achow101 merged this on Jan 10, 2024

achow101 closed this on Jan 10, 2024

ryanofsky referenced this in commit 262923c5b3 on Jan 12, 2024

ryanofsky referenced this in commit 1bdef2f620 on Jan 12, 2024

ryanofsky referenced this in commit 83cc80ba04 on Jan 13, 2024

ryanofsky referenced this in commit ae450f1903 on Jan 13, 2024

fanquake referenced this in commit 28ccc7003a on Jan 15, 2024

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

ryanofsky referenced this in commit 4311bde85e on Jan 16, 2024

ryanofsky referenced this in commit 25a5e0d26e on Jan 16, 2024

ryanofsky referenced this in commit 54ebd96256 on Jan 16, 2024

ryanofsky referenced this in commit 1ee3edec53 on Jan 17, 2024

ryanofsky referenced this in commit d402c90a5c on Jan 17, 2024

ryanofsky referenced this in commit 01a93086fd on Jan 17, 2024

ryanofsky referenced this in commit dd65128c7b on Jan 17, 2024

ryanofsky referenced this in commit 69898b20e7 on Jan 17, 2024

ryanofsky referenced this in commit 7c2b10f016 on Jan 17, 2024

ryanofsky referenced this in commit 705f795aee on Jan 17, 2024

ryanofsky referenced this in commit 91fc2cdaad on Jan 17, 2024

ryanofsky referenced this in commit 632bbb887a on Jan 17, 2024

ryanofsky referenced this in commit b5c54fc032 on Jan 17, 2024

ryanofsky referenced this in commit f929469779 on Jan 17, 2024

ryanofsky referenced this in commit d57a12f7f1 on Jan 17, 2024

ryanofsky referenced this in commit 15c08bcf35 on Jan 17, 2024

ryanofsky referenced this in commit 832f7f3678 on Jan 18, 2024

ryanofsky referenced this in commit 99feed7350 on Jan 18, 2024

ryanofsky referenced this in commit 65222071cc on Feb 27, 2024

ryanofsky referenced this in commit 0cd1616d47 on Feb 27, 2024

ryanofsky referenced this in commit 6c17a3562c on Feb 27, 2024

ryanofsky referenced this in commit 93d37f7d65 on Feb 27, 2024

ryanofsky referenced this in commit 576bdcbbde on Feb 27, 2024

ryanofsky referenced this in commit 5c4717a561 on Feb 27, 2024

ryanofsky referenced this in commit a3d57a58c6 on Feb 28, 2024

ryanofsky referenced this in commit f83e2b5781 on Feb 28, 2024

ryanofsky referenced this in commit 388a94d314 on Mar 28, 2024

ryanofsky referenced this in commit 4e46b1c52c on Mar 28, 2024

ryanofsky referenced this in commit 531b223763 on Mar 28, 2024

ryanofsky referenced this in commit febed22cc5 on Mar 28, 2024

laanwj commented at 11:15 am on April 30, 2024: member

https://github.com/bitcoin/bitcoin/commit/068de7abc5bda9fd9928d7145265ae3a3a851de0 The idea behind the severity level logging is to ultimately have only one macro like Log(category, level) with one consistent, simple interface for everyone – both developers updating logging in the code and people who read the logs – and remove all the other macros when we’re ready, simplifying the logging macros, tests, and benchmarks. This commit not only would add several more macros, but also with different interfaces for developers to remember or look up, rather than just one.

~~i very much agree with @jonatack here, imo ideally there would just be one unified logging function with category and level, not the cognitive overhead of all kinds of different macros.~~

logging: Simplify API for level based logging #28318

Code Coverage

Reviews

Conflicts