kernel, logging: Pass Logger instances to kernel objects #30342
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30342. 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. |
This was referenced Jun 26, 2024
Closed
|
Concept ACK. |
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. Debug: https://github.com/bitcoin/bitcoin/runs/26722091796 |
|
Updated 4542335 -> db1b9f7 ( |
This was referenced Jun 27, 2024
Draft
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Jul 3, 2024
Change LogInstance() function to no longer allocate (and leak) a BCLog::Logger instance. Instead allow kernel applications to initialize their own logging instances that can be returned by LogInstance(). This change is somewhat useful by itself, but more useful in combination with bitcoin#30342. By itself, it gives kernel applications control over when the Logger is created and destroyed and allows new instances to replace old ones. In combination with bitcoin#30342 it allows multiple log instances to exist at the same time, and different output to be sent to different instances. This commit is built on top of bitcoin#30141 since it simplifies the implementation somewhat.
This was referenced Mar 13, 2025
Draft
Test will be extended in next commit to cover support for context objects.
Allow LogDebug(), LogTrace(), LogInfo(), LogWarning(), and LogError() macros to accept context arguments to provide more information in log messages and more control over logging to callers. This functionality is used in followup PRs: - bitcoin#30342 - To let libbitcoinkernel send output to specfic `BCLog::Logger` instances instead of a global instance, so output can be disambiguated and applications can have more control over logging. - bitcoin#30343 - To replace custom `WalletLogPrintf` calls with standard logging calls that automatically include wallet names and don't log everything at info level. This commit does not change behavior of current log prints or require them to be updated. It includes tests and documentation covering the new functionality. Co-Authored-By: Hodlinator <[email protected]>
Needed to fix errors like: const Context &GetContext(const Context &)': expects 1 arguments - 3 provided due to a compiler bug: https://stackoverflow.com/questions/5134523/msvc-doesnt-expand-va-args-correctly/5134656#5134656 Example CI failure: https://github.com/bitcoin/bitcoin/actions/runs/8072891468/job/22055528830?pr=29256
Disallow passing BCLog category constants to LogInfo(), LogWarning(), and LogError() macros, and disallow omitting BCLog categories when calling LogDebug() and LogTrace() macros. These restrictions have existed since the logging macros were added in bitcoin#28318 and not technically neccessary, but are believed to be useful to prevent log spam and prevent users from filtering out important messages based on category. Co-Authored-By: Hodlinator <[email protected]>
Pass Logger instances to BlockManager, CCoinsViewDB, CDBWrapper, ChainstateManager, and CoinsViews instances so libbitcoinkernel applications and test code have the option to control where log output goes instead of having all output sent to the global logger. This commit just passes the logger objects without using them. The next commit updates log print statements to use the new objects.
This is a mechanical change updating kernel code that currently uses the global log instance to log to local instances instead.
This allows libbitcoinkernel applications and test code to have the option to control where log output goes instead of having all output sent to the global logger.
Change LogInstance() function to no longer allocate (and leak) a BCLog::Logger instance. Instead allow kernel applications to initialize their own logging instances that can be returned by LogInstance(). The LogInstance() function is not actually used for the majority of logging in kernel code. Most kernel log output goes directly to BCLog::Logger instances specified when kernel objects like ChainstateManager and CTxMemPool are constructed, which allows applications to create multiple Logger instances and control which log output is sent to them. The only kernel code that does rely on LogInstance() is certain low level code in the util library, like the RNG seeder, that is not passed a specific instance and needs to rely on the global LogInstance() function. Other code outside the kernel library uses LogInstance() heavily, and may continue to do so. bitcoind and test code now just create a log instance before the first LogInstance() call, which it returns, so all behavior is the same as before.
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Apr 3, 2025
Allow LogDebug(), LogTrace(), LogInfo(), LogWarning(), and LogError() macros to accept context arguments to provide more information in log messages and more control over logging to callers. This functionality is used in followup PRs: - bitcoin#30342 - To let libbitcoinkernel send output to specfic `BCLog::Logger` instances instead of a global instance, so output can be disambiguated and applications can have more control over logging. - bitcoin#30343 - To replace custom `WalletLogPrintf` calls with standard logging calls that automatically include wallet names and don't log everything at info level. This commit does not change behavior of current log prints or require them to be updated. It includes tests and documentation covering the new functionality.
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Apr 3, 2025
Allow LogDebug(), LogTrace(), LogInfo(), LogWarning(), and LogError() macros to accept context arguments to provide more information in log messages and more control over logging to callers. This functionality is used in followup PRs: - bitcoin#30342 - To let libbitcoinkernel send output to specfic `BCLog::Logger` instances instead of a global instance, so output can be disambiguated and applications can have more control over logging. - bitcoin#30343 - To replace custom `WalletLogPrintf` calls with standard logging calls that automatically include wallet names and don't log everything at info level. This commit does not change behavior of current log prints or require them to be updated. It includes tests and documentation covering the new functionality.
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pass
Loggerinstances toBlockManager,CCoinsViewDB,CDBWrapper,Chainstate,ChainstateManager,CoinsViews, andCTxMemPoolclasses so libbitcoinkernel applications and tests have the option to control where log output goes instead of having all output sent to the global logger.This PR is an alternative to #30338. It is more limited because it only changes kernel code while leaving other parts of the codebase alone. It also takes the opportunity to migrate legacy LogPrint / LogPrintf calls to the new log macros introduced in #28318.
This is based on #29256. The non-base commits are:
1c58fc2adc30refactor: Pass Logger instances to kernel objects8517d317c235refactor: Log kernel output to local log instances3e859af7a1c7refactor: Pass Logger instance to CTxMemPoolec6d771d3b34kernel: Drop global Logger instance