Logging cleanup #29798

vasild · 2024-04-03T11:53:20Z

Move special cases from LOG_CATEGORIES_BY_STR to GetLogCategory() (suggested here).
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).

This is a followup to #29419, addressing leftover suggestions + more.

DrahtBot · 2024-04-03T11:53:23Z

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	LarryRuane, ryanofsky
Concept ACK	kevkevinpal
Stale ACK	ajtowns

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:

#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
#26697 (logging: use bitset for categories by LarryRuane)
#17783 (util: Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior by ryanofsky)
#17581 (refactor: Remove settings merge reverse precedence code by ryanofsky)
#17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
#17493 (util: Forbid ambiguous multiple assignments in config file by ryanofsky)

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.

src/logging.cpp

jonatack · 2024-04-03T12:42:44Z

See #27231.

vasild · 2024-04-03T15:44:27Z

See #27231.

There you mention:

I'm going to open an alternate pull that does what you suggest...

Is the current PR doing what you meant? E.g. drop that help text and return an error for "0" ("none" already returns an error in master)?

ryanofsky

Code review ACK d98ca056a82264884bc1aa2da3af540a73ee8f26

Nice cleanups! Main feedback I have is to more clearly indicate which commits are refactoring, and which have behavior changes. Would suggest putting "logging, refactor:" in refactoring commit titles and "logging:" in commits that change behavior. Also I would consider dropping any behavior changes that aren't obvious improvments and doing them in separate PRs so they have more visibility and are not lost in "logging cleanup."

src/logging.cpp

src/logging.h

src/logging.cpp

kevkevinpal

Concept ACK

just a question is there any reason to keep BCLog::NONE in logging.h and not completely delete it I did
grep --exclude-dir=".deps" -nri "BCLog::NONE" ./src/ --binary-files=without-match
and
grep --exclude-dir=".deps" -nri "LogFlags::NONE" ./src/ --binary-files=without-match
and only see it being used in ./src/logging.{h,cpp} and ./src//qt/transactiondesc.cpp

src/logging.cpp

vasild · 2024-05-05T17:29:30Z

d98ca056a8...bee22409ea: rebase and address suggestions

Edit: @kevkevinpal, I am not sure about dropping BCLog::NONE on the technical side, but what is sure is that it would further increase the scope of this PR, so I am leaving it as it is :)

DrahtBot · 2024-05-05T17:53:59Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/24605970251}

vasild · 2024-05-05T17:55:06Z

bee22409ea...df2422c5f9: fix CI

this compiles locally but not on the CI:

ret.emplace_back(category, WillLogCategory(flag));

so instead use:

ret.push_back(LogCategory{.category = category, .active = WillLogCategory(flag)});

ryanofsky

Code review ACK df2422c5f91e5b1b42d6b718cc2527dc413712fc. Looks good, this gets rid of some unnecessary complexity and strange behavior in logging RPC.

src/logging.cpp

ajtowns · 2024-07-03T17:41:16Z

Consider also updating the help text for -debug and -debugexclude:

--- a/src/init/common.cpp
+++ b/src/init/common.cpp
@@ -27,9 +27,9 @@ void AddLoggingArgs(ArgsManager& argsman)
 {
     argsman.AddArg("-debuglogfile=<file>", strprintf("Specify location of debug log file (default: %s). Relative paths will be prefixed by a net-specific datadir location. Pass -nodebuglogfile to disable writing the log to a file.", DEFAULT_DEBUGLOGFILE), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-debug=<category>", "Output debug and trace logging (default: -nodebug, supplying <category> is optional). "
-        "If <category> is not supplied or if <category> = 1, output all debug and trace logging. <category> can be: " + LogInstance().LogCategoriesString() + ". This option can be specified multiple times to output multiple categories.",
+        "If <category> is not supplied or if <category> is 1 or \"all\", output all debug logging. If <category> is 0 or \"none\", any other categories are ignored. Other valid values for <category> are: " + LogInstance().LogCategoriesString() + ". This option can be specified multiple times to output multiple categories.",
         ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
-    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);
+    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. This takes priority over \"-debug\"", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);

ajtowns · 2024-07-03T17:53:13Z

ACK df2422c5f91e5b1b42d6b718cc2527dc413712fc ; looks like it behaves correctly to me

LarryRuane

ACK (but needs rebase, will re-ACK), this will make #26697 simpler (so I think the current PR should merge first). Comments are minor, feel free to ignore.

src/logging.cpp

vasild · 2024-08-02T12:43:32Z

df2422c5f9...397211b573: rebase due to conflicts

ryanofsky

Code review ACK 397211b573921d2e9d34906ca7720ca2092b8bd7, just since rebase since last review.

@vasild AJ's documentation suggestion also seems like it would be an improvement
#29798 (comment) (though I didn't look into the details and verify it is accurate).

src/logging.cpp

LarryRuane

ACK 397211b573921d2e9d34906ca7720ca2092b8bd7
I will re-ACK if you make that one small suggested change.

Make special cases explicit in GetLogCategory() and LogCategoryToStr() functions. Simplify the LOG_CATEGORIES_BY_STR and LOG_CATEGORIES_BY_FLAG mappings and LogCategoriesList() function. This makes the maps `LOG_CATEGORIES_BY_STR` and `LOG_CATEGORIES_BY_FLAG` consistent (one is exactly the opposite of the other).

…E instead of 0 * Make the standalone function `LogCategoryToStr()` private inside `logging.cpp` (aka `static`) - it is only used in that file. * Make the method `Logger::GetLogPrefix()` `private` - it is only used within the class. * Use `BCLog::NONE` to initialize `m_categories` instead of `0`. We later check whether it is `BCLog::NONE` (in `Logger::DefaultShrinkDebugFile()`).

Current logging RPC method documentation claims to accept "0" and "none" categories, but the "none" argument is actually rejected and the "0" argument is ignored. Update the implementation to refuse both categories, and remove the help text claiming to support them.

vasild · 2024-08-04T04:47:27Z

397211b573...a7432dd6ed: pick #29798 (comment) and #29798 (comment)

LarryRuane · 2024-08-04T15:23:14Z

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.

ryanofsky · 2024-08-05T01:08:46Z

Merged with 2 current acks and a stale ack since changes after the stale ack were pretty minor (rebase, dropping dead code, updating help as suggested).

vasild mentioned this pull request Apr 3, 2024

log: deduplicate category names and improve logging.cpp #29419

Merged

maflcko reviewed Apr 3, 2024

View reviewed changes

src/logging.cpp Outdated Show resolved Hide resolved

DrahtBot added the CI failed label Apr 3, 2024

DrahtBot mentioned this pull request Apr 4, 2024

Broadcast own transactions only via short-lived Tor or I2P connections #29415

Merged

4 tasks

DrahtBot removed the CI failed label Apr 5, 2024

ryanofsky approved these changes Apr 5, 2024

View reviewed changes

src/logging.cpp Outdated Show resolved Hide resolved

src/logging.h Outdated Show resolved Hide resolved

src/logging.cpp Outdated Show resolved Hide resolved

src/logging.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Apr 9, 2024

logging: use bitset for categories #26697

Closed

kevkevinpal reviewed Apr 10, 2024

View reviewed changes

src/logging.cpp Outdated Show resolved Hide resolved

vasild force-pushed the logging_cleanup branch from d98ca05 to bee2240 Compare May 5, 2024 17:29

vasild force-pushed the logging_cleanup branch from bee2240 to df2422c Compare May 5, 2024 17:53

DrahtBot added the CI failed label May 5, 2024

DrahtBot removed the CI failed label May 5, 2024

ryanofsky approved these changes May 6, 2024

View reviewed changes

src/logging.cpp Outdated Show resolved Hide resolved

DrahtBot requested a review from kevkevinpal May 6, 2024 17:12

DrahtBot added CI failed and removed CI failed labels May 22, 2024

ryanofsky mentioned this pull request Jul 3, 2024

logging: Simplify edge cases in logging configuration #30384

Closed

This was referenced Jul 4, 2024

Early logging improvements #30386

Merged

kernel, logging: Pass Logger instances to kernel objects #30342

Draft

DrahtBot added the Needs rebase label Jul 26, 2024

LarryRuane reviewed Jul 27, 2024

View reviewed changes

src/logging.cpp Outdated Show resolved Hide resolved

src/logging.cpp Outdated Show resolved Hide resolved

src/logging.cpp Outdated Show resolved Hide resolved

vasild force-pushed the logging_cleanup branch from df2422c to 397211b Compare August 2, 2024 12:42

DrahtBot removed the Needs rebase label Aug 2, 2024

ryanofsky approved these changes Aug 2, 2024

View reviewed changes

src/logging.cpp Outdated Show resolved Hide resolved

DrahtBot requested review from LarryRuane and ajtowns August 2, 2024 17:15

LarryRuane approved these changes Aug 2, 2024

View reviewed changes

ryanofsky and others added 4 commits August 4, 2024 06:42

logging: clarify -debug and -debugexclude descriptions

a7432dd

vasild force-pushed the logging_cleanup branch from 397211b to a7432dd Compare August 4, 2024 04:45

DrahtBot requested a review from ryanofsky August 4, 2024 15:23

ryanofsky approved these changes Aug 5, 2024

View reviewed changes

ryanofsky merged commit 55d1994 into bitcoin:master Aug 5, 2024

vasild deleted the logging_cleanup branch August 12, 2024 04:34

bitcoin locked and limited conversation to collaborators Aug 12, 2025

Logging cleanup #29798

Logging cleanup #29798

Uh oh!

Conversation

vasild commented Apr 3, 2024

Uh oh!

DrahtBot commented Apr 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Uh oh!

jonatack commented Apr 3, 2024

Uh oh!

vasild commented Apr 3, 2024

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevkevinpal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasild commented May 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 5, 2024

Uh oh!

vasild commented May 5, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ajtowns commented Jul 3, 2024

Uh oh!

ajtowns commented Jul 3, 2024

Uh oh!

LarryRuane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vasild commented Aug 2, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LarryRuane left a comment

Choose a reason for hiding this comment

Uh oh!

vasild commented Aug 4, 2024

Uh oh!

LarryRuane commented Aug 4, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Aug 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

DrahtBot commented Apr 3, 2024 •

edited

Loading

ryanofsky left a comment •

edited

Loading

vasild commented May 5, 2024 •

edited

Loading