These are the next two commits from #25203.
Add LogPrintfCategory to log unconditionally while prefixing the output with the passed category name. Add documentation and a unit test, and update the lint-logs.py and lint-format-strings.py scripts.
Replace the log messages that manually print a category, with LogPrintfCategory. In upcoming commits, it will likely be used in many other cases, such as to replace LogPrintf where it makes sense.
114@@ -114,7 +115,8 @@ BOOST_FIXTURE_TEST_CASE(logging_LogPrintMacros, LogSetup)
115 "[net:debug] foo7: bar7",
116 "[net:info] foo8: bar8",
117 "[net:warning] foo9: bar9",
118- "[net:error] foo10: bar10"};
119+ "[net:error] foo10: bar10",
120+ "[validation] foo11: bar11"};
0 "[validation] foo11: bar11",
1 };
nit, to make future changes less verbose
prefixing the output with the passed category name.
- add documentation
- add a unit test
- update lint-logs.py
- update lint-format-strings.py
Here we update only the log messages that manually print a category.
In upcoming commits, LogPrintCategory will likely be used in many
other cases, such as to replace `LogPrintf` where it makes sense.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
Concept ACK
I do think this is an intermediate state. I imagine at some point we’ll want to have a category and log level for every message, so the need to log messages with just a category goes away again.
ACK ecff20db286e2f5d3afe32cfaae72de69d34d23c
Just reviewed the code and it’s good. I built and ran it (signet) and checked debug.log, lgtm.
Ex:
02022-06-08T17:03:23Z Script verification uses 7 additional threads
12022-06-08T17:03:23Z scheduler thread start
22022-06-08T17:03:23Z [http] creating work queue of depth 16
32022-06-08T17:03:23Z Using random cookie authentication.
42022-06-08T17:03:23Z Generated RPC authentication cookie /Users/brunogarcia/Library/Application Support/Bitcoin/signet/.cookie
52022-06-08T17:03:23Z [http] starting 4 worker threads
62022-06-08T17:03:23Z Using wallet directory /Users/brunogarcia/Library/Application Support/Bitcoin/signet/wallets
[category:all] for unconditional logging might be unnecessarily verbose, and (b) all doesn’t seem useful as a severity level that a user can set in our logging (it would do the same as setting debug currently), though I could be overlooking a use case.
Right, my understanding was that the None category and level would eventually go away, and it’d be more like logging systems in other applications. That it’s an intermediate solution until the existing messages are “sorted out”. And after threshold configuration (like #25287) was added. But I could be wrong.
In any case, permanent or not, I’m ok with this as intermediate solution.
I guess the statements under discussion are mostly:
I can see how they are useful unconditionally to make later debugging easier, which would be my reason to keep them unconditional. However, I can also see how there might be a user only interested in Warning/Error logs, not in Info logs. So I guess one alternative is to change None to Info and make it the default severity. (That is probably also the way python does it)
LogPrintfCategory. They probably could all be changed to Error, Warning, or Info. The default debug log would be shockingly quiet, though.
I can see how they are useful unconditionally to make later debugging easier, which would be my reason to keep them unconditional.
Yes, I don’t disagree. It does feel somewhat strange to me to have log messages that figure higher than Error in severity, but aren’t errors. But if used sparingly for this purpose it makes sense.
So I guess one alternative is to change None to Info and make it the default severity. (That is probably also the way python does it)
The default debug log would be shockingly quiet, though.
I think the default logging should remain the same. So if we use Info as default log level for what is now unconditional, for most categories we’d want to have the standard threshold at Info. For some (leveldb, qt) it could deviate, becuase they are spammy.
Users would currently need to turn on debug logging though, to see Info logging.
Edit: right, change Info to be unconditional. Hmm. That may need some shifting, though in the latest pass I moved most non-essential logging into debug anyway.
Users would currently need to turn on debug logging though, to see Info logging.
Yes. The current hardcoded LogAcceptCategory thresholding implementation is silly I temporarily added it for leveldb category, basically, until there’s more fine-grained configuration possible. The default threshold for non-spammy cagtegories should probably be Info and higher.
I think the current LogPrint messages should be of level Debug, and current LogPrintf should be Info or higher. I’m fine with having an unconditional None level for marker messages as @MarcoFalke says.