log: deduplicate category names and improve logging.cpp #29419
Conversation
The code in `logging.cpp` needs to:
* Get the category name given the flag (e.g. `BCLog::PRUNE` -> `"prune"`)
* Get the flag given the category name (e.g. `"prune"` -> `BCLog::PRUNE`)
* Get the list of category names sorted in alphabetical order
Achieve this by using the proper std containers. The result is
* less code (this diff is +62 / -129)
* faster code (to linear search and no copy+sort)
* more maintainable code (the categories are no longer duplicated in
`LogCategories[]` and `LogCategoryToStr()`)
This behavior is preserved:
`BCLog::NONE` -> `""` (lookup by `LogCategoryToStr()`)
`""` -> `BCLog::ALL` (lookup by `GetLogCategory("")`)
Suggested by: David Gumberg (bitcoin#29415 (comment))
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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. |
|
crACK b0344c2 |
There was a problem hiding this comment.
ACK b0344c2
Reviewed code as part of #29415 confirmed no diff in the first commit, Second commit looks good as well (confirmed no usage in code of "util" log category). Built on arm/macOS and ran all functional and unit tests.
Show Signature
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK b0344c219a641b759fb0cc4f53afebe675b8ca27
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmXwZA4ACgkQ5+KYS2KJ
yTrJ1g/9Fz6E1A37SBb9FWrndC9xgTYLBzEBRtgy1+95yfmeLCpMUbY5Cjvp97I7
jhuSweOry0H5Z7SMtfvTMhd2AYxHuGs6Ygj8qt+tOhRj/vhI+mQ53Qu0Fm9nbLsp
AkGvN5kx3uGSYAgHkfqq+KsH3Q3KkeEjW9q3wX8aW5oW5XDGkTpTPOqMYCpuGhmk
Ih56P3D5I+NuI9e3hIrEPajnYvMz9f/aIeD6VEWGb9Jx19BHqX86sIJ3g38Ej7eI
gqH6l0RuGWnA10P7NGoSZBXdrvFAQ8hWNZUW87MjhfPodjjbRwTBwtYUWUnRc3cw
6kE07moevRoEcjs7NcCmI91pMk5NYdZdyyPHCWODMCLTjDVg5J3806tEFt8N1Klc
stxv4atBvMfVK7fgnjVSZG2mrJo08XU+VmY1hx0vc3jMnGk3rtziExN+vsPxYUM9
YjD4O/Z3yx9c83fA7wcBE6jqS6EN+u7URgxwDAjTYyjH4ajnReetc5m0LAA0fC4Q
uYKNShC+DQpfsmZw74/ROiVcRyKXUvI5O9EnlUJNIh/jM9OkPcbJkmURqTLRbCOE
x3/GSbQN+4+e+J3XPGIL1sPGH5YLOr2HLtIgMJZhYzSr53nKyHgLYRtrCTE9F7bl
A7+REDbe6oW63GdZv+hA5c0W0kR+LOaOqB/Hl2hrss1fIUL6LuM=
=yCfD
-----END PGP SIGNATURE-----
pinheadmz's public key is on keybase
There was a problem hiding this comment.
Code review ACK b0344c2. Nice cleanup! Having to maintain multiple copies of the same mapping seemed messy and a like a possible footgun. I checked old and new mappings in both directions and confirmed no behavior should be changing.
I do suspect the previous code was probably more efficient, but the new code could be adopted to use string_view and constexpr arrays if needed.
| switch (v) { | ||
| case BCLog::NONE: out.emplace(BCLog::NONE, ""); break; | ||
| case BCLog::ALL: out.emplace(BCLog::ALL, "all"); break; | ||
| default: out.emplace(v, k); |
There was a problem hiding this comment.
In commit "logging: remove unused BCLog::UTIL" (b0344c2)
Would be good to assert that emplace succeeded, and the map is consistent.
| case BCLog::NONE: out.emplace(BCLog::NONE, ""); break; | ||
| case BCLog::ALL: out.emplace(BCLog::ALL, "all"); break; |
There was a problem hiding this comment.
In commit "logging: remove unused BCLog::UTIL" (b0344c2)
I think it would make code more straightforward if we could drop these special cases here and avoid having discrepancies between LOG_CATEGORIES_BY_FLAG and LOG_CATEGORIES_BY_STR maps.
There are already special cases in existing GetLogCategory and LogCategoriesList functions so I think it would make more sense to put new special cases there and not introduce them in a third place. Would suggest:
--- a/src/logging.cpp
+++ b/src/logging.cpp
@@ -142,7 +142,6 @@ bool BCLog::Logger::DefaultShrinkDebugFile() const
}
static const std::map<std::string, BCLog::LogFlags> LOG_CATEGORIES_BY_STR{
- {"0", BCLog::NONE},
{"", BCLog::NONE},
{"net", BCLog::NET},
{"tor", BCLog::TOR},
@@ -175,7 +174,6 @@ static const std::map<std::string, BCLog::LogFlags> LOG_CATEGORIES_BY_STR{
{"txreconciliation", BCLog::TXRECONCILIATION},
{"scan", BCLog::SCAN},
{"txpackages", BCLog::TXPACKAGES},
- {"1", BCLog::ALL},
{"all", BCLog::ALL},
};
@@ -184,11 +182,8 @@ static const std::unordered_map<BCLog::LogFlags, std::string> LOG_CATEGORIES_BY_
[](const std::map<std::string, BCLog::LogFlags>& in) {
std::unordered_map<BCLog::LogFlags, std::string> out;
for (const auto& [k, v] : in) {
- switch (v) {
- case BCLog::NONE: out.emplace(BCLog::NONE, ""); break;
- case BCLog::ALL: out.emplace(BCLog::ALL, "all"); break;
- default: out.emplace(v, k);
- }
+ bool inserted{out.emplace(v, k).second};
+ assert(inserted);
}
return out;
}(LOG_CATEGORIES_BY_STR)
@@ -196,9 +191,12 @@ static const std::unordered_map<BCLog::LogFlags, std::string> LOG_CATEGORIES_BY_
bool GetLogCategory(BCLog::LogFlags& flag, const std::string& str)
{
- if (str.empty()) {
+ if (str.empty() || str == "1") {
flag = BCLog::ALL;
return true;
+ } else if (str == "0") {
+ flag = BCLog::NONE;
+ return true;
}
auto it = LOG_CATEGORIES_BY_STR.find(str);
if (it != LOG_CATEGORIES_BY_STR.end()) {
| {BCLog::IPC, "ipc"}, | ||
| static const std::map<std::string, BCLog::LogFlags> LOG_CATEGORIES_BY_STR{ | ||
| {"0", BCLog::NONE}, | ||
| {"", BCLog::NONE}, |
There was a problem hiding this comment.
In commit "log: deduplicate category names and improve logging.cpp" (d3b3af9)
I think it's good to keep the "" to NONE map entry in this commit, just to make it obvious this commit is not changing behavior. But in a followup, I think it would be good to drop this entry because it doesn't actually do anything useful, and it is confusing to have a map from "" to NONE in one place and ALL in another.
|
@ryanofsky, thanks for the review! Addressed your pending suggestions in #29798. |
a7432dd logging: clarify -debug and -debugexclude descriptions (Anthony Towns) 74dd33c rpc: make logging method reject "0" category and correct the help text (Vasil Dimov) 8c6f3bf logging, refactor: minor encapsulation improvement and use BCLog::NONE instead of 0 (Vasil Dimov) 160706a logging, refactor: make category special cases explicit (Ryan Ofsky) Pull request description: * Move special cases from `LOG_CATEGORIES_BY_STR` to `GetLogCategory()` (suggested [here](#29419 (comment))). * Remove `"none"` and `"0"` from RPC `logging` help because that help text was wrong. `"none"` resulted in an error and `"0"` was ignored itself (contrary to what the help text suggested). * Remove unused `LOG_CATEGORIES_BY_STR[""]` (suggested [here](#29419 (comment))). This is a followup to #29419, addressing leftover suggestions + more. ACKs for top commit: LarryRuane: ACK a7432dd ryanofsky: Code review ACK a7432dd. Only changes since last review are removing dead if statement and adding AJ's suggested -debug and -debugexclude help improvements, which look accurate and much more clear. Tree-SHA512: 41b997b06fccdb4c1d31f57d4752c83caa744cb3280276a337ef4a9b7012a04eb945071db6b8fad24c6a6cf8761f2f800fe6d8f3d8836f5b39c25e4f11c85bf0
Summary: Overview: This PR introduces a new macro, LogPrintLevel, that allows developers to add logs with the severity level. Additionally, it will also print the log category if it is specified. Sample log: ``` 2022-03-04T16:41:15Z [opencon] [net:debug] trying connection XX.XX.XXX.XXX:YYYYY lastseen=2.7hrs ``` This is a backport of [[bitcoin/bitcoin#24464 | core#24464]] and [[bitcoin/bitcoin#29419 | core#29419]] bitcoin/bitcoin@a829064 bitcoin/bitcoin@d3b3af9 (dedup category names and improve logging.cpp) bitcoin/bitcoin@e11cdc9 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D17217
Summary: Overview: This PR introduces a new macro, LogPrintLevel, that allows developers to add logs with the severity level. Additionally, it will also print the log category if it is specified. Sample log: ``` 2022-03-04T16:41:15Z [opencon] [net:debug] trying connection XX.XX.XXX.XXX:YYYYY lastseen=2.7hrs ``` This is a backport of [[bitcoin/bitcoin#24464 | core#24464]] and [[bitcoin/bitcoin#29419 | core#29419]] bitcoin/bitcoin@a829064 bitcoin/bitcoin@d3b3af9 (dedup category names and improve logging.cpp) bitcoin/bitcoin@e11cdc9 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D17217
The code in
logging.cppneeds to:BCLog::PRUNE->"prune")"prune"->BCLog::PRUNE)Achieve this by using the proper std containers. The result is
LogCategories[]andLogCategoryToStr())This behavior is preserved:
BCLog::NONE->""(lookup byLogCategoryToStr())""->BCLog::ALL(lookup byGetLogCategory(""))Also remove unused
BCLog::UTIL.These changes (modulo the
BCLog::UTILremoval) are part of #29415 but they make sense on their own and would be good to have them, regardless of the fate of #29415. Also, if this is merged, that would reduce the size of #29415, thus the current standalone PR.