Ensure tctl outputs all debug log messages
#12807
Merged
Conversation
This file contains 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
strideynet
changed the title
tctl outputs all debug log messages
gabrielcorado
approved these changes
May 23, 2022
There was a problem hiding this comment.
LGTM. As an alternative, we could set the configuration default logger to the global one (currently, we're creating a new logger), which would make more sense to me, since we're always going to set this.
|
That would equally make sense to me, but I wasn't too sure if that was going to start causing unexpected behaviour elsewhere (e.g intentionally silenced log messages start being output), so I went with this implementation as it felt a bit less risky with my current knowledge of the codebase. |
tcsc
approved these changes
May 25, 2022
strideynet
force-pushed
the
strideynet/tctl-debug-logging-fix
branch
from
7fbbc06 to
6eb2de8
Compare
hatched
approved these changes
May 25, 2022
strideynet
force-pushed
the
strideynet/tctl-debug-logging-fix
branch
from
6eb2de8 to
b54ac22
Compare
timothyb89
approved these changes
May 25, 2022
strideynet
force-pushed
the
strideynet/tctl-debug-logging-fix
branch
from
b54ac22 to
5248eeb
Compare
|
@strideynet See the table below for backport results.
|
This was referenced May 25, 2022
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.
Closes #12801
Whilst
tctlconfigures the global logger when-dis provided, it previously injected autils.NewLogger()into components which use thecfg.Logfield (as this is the default provided byservice.MakeDefaultConfig()and this meant some debug log messages were not output. This change injectslogrus.StandardLogger()into those components, so the configured log level/output rules take affect. See #12801 for an example of a log message that is not correctly output.An alternative implementation would be to instantiate a new logger, and configure it the same as
utils.InitLoggerconfigures it. I'm also open to this is you consider that preferable.