Logging improvements #2646
Open
Logging improvements #2646
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
Update the logging to use a logging manager singleton. It also keeps the generation of all logging messages in one location, but distributes the output control to individual targets that controls how content is displayed (file, stdout, stderr, etc). These changes alos remove the need to delete and re-initialize the logging during start-up. Additionally, this will allow us to fine-tune control of each individual output to have different severities (should we desire).
Collaborator
|
CI gfxreconstruct build queued with queue ID 638025. |
Collaborator
|
CI gfxreconstruct build # 8764 running. |
Collaborator
|
CI gfxreconstruct build # 8764 passed. |
Collaborator
|
CI gfxreconstruct build queued with queue ID 638072. |
Collaborator
|
CI gfxreconstruct build # 8766 running. |
Collaborator
|
CI gfxreconstruct build # 8766 failed. |
| { | ||
| // setting this to kDebugSeverity yields debug-output from parser | ||
| gfxrecon::util::Log::Init(gfxrecon::util::Log::kErrorSeverity); | ||
| gfxrecon::util::Log::Init(gfxrecon::util::LoggingSeverity::kDebug); |
Contributor
There was a problem hiding this comment.
this is not required here, same above w. kInfo. makes the unit-test quite verbose since SpirVParsingUtil uses GFXRECON_LOG_DEBUG to print a summary
| GFXRECON_BEGIN_NAMESPACE(gfxrecon) | ||
| GFXRECON_BEGIN_NAMESPACE(util) | ||
|
|
||
| const char g_process_tag[] = "gfxrecon"; |
Contributor
There was a problem hiding this comment.
constexpr char g_process_tag[] = "gfxrecon"; should also work
| return true; | ||
| } | ||
|
|
||
| bool LoggingManager::UpdateStdErrTarget(bool write_to_stderr, bool flush_after_write) |
Contributor
There was a problem hiding this comment.
UpdateStdErrTarget always returns true. should it rather return if state actually got changed?
like:
bool was_using_stderr = logging_targets_[kTarget_StdErr] && logging_targets_[kTarget_StdErr]->IsEnabled(); // LoggingTargetBase::IsEnabled() does not exist, but would be useful
...
return write_to_stderr != was_using_stderr;
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
These changes pave the way for improved logging:
NOTE: This is squashed because it was already pre-approved in another workflow (sorry for the large chunk).