feat: Add named logger configuration #658
Closed
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
…regular logging.Config
4 tasks
jsimnz
added a commit
that referenced
this pull request
Jul 31, 2022
There was a bug in the #658 that got merged which effectively broke the entire feature. Tests didn't catch it as they were accidentally poorly defined, as they would pass with the default logger settings as well as the custom ones set in the test. There are two fixes in this PR that resolves the problem. During review, we decided to switch back the Config.Log from Config.Logger, but the viper Bind calls were still using Bind(logger.loglevel) (old name). Additionally, the validate call on the config.Log struct previously always returned nil (no errors), but was changed recently to validate the available options. But, if the --loglevel option is provided it will set the config.Log.Level to error,defra.cli=info,... for example, which would fail validation. I have no moved the validation rules, as it will be validated anyways later in the call stack (in the ToLoggerConfig). Lastly, it fixes #694, as an accidental oversight, potentially during a rebase/merge-conflict.
shahzadlone
pushed a commit
to shahzadlone/defradb
that referenced
this pull request
Feb 23, 2024
There was a bug in the sourcenetwork#658 that got merged which effectively broke the entire feature. Tests didn't catch it as they were accidentally poorly defined, as they would pass with the default logger settings as well as the custom ones set in the test. There are two fixes in this PR that resolves the problem. During review, we decided to switch back the Config.Log from Config.Logger, but the viper Bind calls were still using Bind(logger.loglevel) (old name). Additionally, the validate call on the config.Log struct previously always returned nil (no errors), but was changed recently to validate the available options. But, if the --loglevel option is provided it will set the config.Log.Level to error,defra.cli=info,... for example, which would fail validation. I have no moved the validation rules, as it will be validated anyways later in the call stack (in the ToLoggerConfig). Lastly, it fixes sourcenetwork#694, as an accidental oversight, potentially during a rebase/merge-conflict.
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.
Relevant issue(s)
Resolves #413
note: This is a duplicate of #656 intentionally as there was some issue with the Github Actions not being triggered correctly
Description
Supports a new format for configuring loggers that targets specific named loggers. It uses a comma-seperated-value (CSV) list of keyvalues.
eg:
--loglevel error,defra.cli=debugwill set the global level for all loggers to
errorand then thedefra.clilevel todebugspecifically.It supports an unbounded number of named logger values.
Additionally, introduces a new cli flag
--loggerwhich uses the same format, but for all fields of a specific logger, instead of individual fields of many loggers.eg:
--logger defra.cli,level=debug,output=stdout,format=jsonwill set the level to
debug, output tostdout, and format tojsonfor the logger nameddefra.cli.Tasks
How has this been tested?
Some basic integration tests in
tests/integration/cliSpecify the platform(s) on which this was tested: