feat: Add named config overrides #659
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #659 +/- ##
===========================================
- Coverage 61.68% 57.21% -4.47%
===========================================
Files 123 143 +20
Lines 14925 16383 +1458
===========================================
+ Hits 9206 9374 +168
- Misses 4859 6127 +1268
- Partials 860 882 +22
|
jsimnz
force-pushed
the
jsimnz/feat/I413-log-config-overrides
branch
3 times, most recently
from
ceca67d
to
a945835
Compare
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
jsimnz
added
feature
cli/root.go
Outdated
| @@ -126,3 +136,162 @@ func init() { | |||
| log.FatalE(context.Background(), "Could not bind api.address", err) | |||
| } | |||
| } | |||
|
|
|||
| // parses and then configures the given config.Config logging subconfig. | |||
There was a problem hiding this comment.
nitpick:
| // parses and then configures the given config.Config logging subconfig. | |
| // parseAndConfigLog parses and then configures the given config.Config logging subconfig. |
cli/root.go
Outdated
| log.FatalE(ctx, "couldn't parse kv bool", err) | ||
| } | ||
| logcfg.Color = boolValue | ||
| } |
There was a problem hiding this comment.
suggestion: should handle the case of invalid field type.
For example I like how this fails:
./build/defradb start --logger "defra.cli,level=debu,output=stdout,format=json"
We get:
2022-07-25T05:58:53.931-0400, FATAL, defra.cli, Could not get logging config, {"Error": "couldn't convert override config: invalid log level: debu"}
Note: level=debu is bad.
OR for ./build/defradb start --logger "defra.cli,level=debug=,output=stdout,format=json"
We get:
2022-07-25T06:02:22.807-0400, FATAL, defra.cli, level was not provided as <key>=<value> pair, {"pair": "level=debug="}
Note: level=debug= is bad.
But for this it doesn't fail and actually starts the server:
./build/defradb start --logger "defra.cli,leva=debug,output=stdout,format=json"
Note: leva is an invalid field type.
There was a problem hiding this comment.
For the first two of your examples, it does get validated later on, so I didn't bother adding validation here, as that would just be doubling up the work, and result in approx the same error message.
There was a problem hiding this comment.
Sorry, I should have been clearer. The first 2 I was praising because they get validated (even if it's later). I was suggesting to validate the 3rd (last point) as well.
There was a problem hiding this comment.
Gotcha, sounds good, will do!
cli/root.go
Outdated
|
|
||
| type logParamSetterStringFn func(*config.LoggingConfig, string) | ||
|
|
||
| // type logParamSetterBoolFn func(*config.LoggingConfig, bool) |
There was a problem hiding this comment.
part of the LEAVE NOW comment, changed to make more clear
| log3 = logging.MustNewLogger(testLogger3) | ||
| ) | ||
|
|
||
| // todo - add test asserting that logger logs to file by default |
There was a problem hiding this comment.
question: finishing this todo part of this PR? if not suggest linking to an issue.
|
|
||
| // todo - add test asserting that logger logs to file by default | ||
|
|
||
| func TestCLILogsToStderrGivenNamedLogLevel(t *testing.T) { |
There was a problem hiding this comment.
suggestion: Maybe an overkill, but can quickly add tests for some error cases (invalid use, to see exit / fatels are called properly).
There was a problem hiding this comment.
This will be part of the larger CLI tests to be added in 0.4. This is just ensuring that given the log calls, the correct output is produced based on the logger config, of which, one of them is an Error call already.
config/config_test.go
Outdated
| cfg.Logging.Level = "debug" | ||
| cfg.Logging.Format = "json" | ||
| cfg.Logging.Stacktrace = true | ||
| cfg.Logging.OutputPath = "stdout" |
There was a problem hiding this comment.
nitpick: Maybe add this stdout file to the .gitignore
There was a problem hiding this comment.
or have it in a temp. directory.
There was a problem hiding this comment.
this shouldn't create a file. When zap sees this it just know the output goes to stdout
There was a problem hiding this comment.
In that case Idk why in my local directory a file called stdout was created.
There was a problem hiding this comment.
odd, ill check, but yea it shouldn't :)
There was a problem hiding this comment.
I know why. Will create a PR to fix in a few minutes
There was a problem hiding this comment.
There was a problem hiding this comment.
joke: I hope your PR removes the only reference to 666 in our code base :P
There was a problem hiding this comment.
Can confirm #666 fixes it this.
|
question: Why prefer |
When I picked up the branch from Andy, he had changed it. I wasn't 100% aware why so I just sent with it, can change it tho |
I see. For now unless there is argument or preference, I suggest we keep it as Log and not Logging. |
| // by the Apache License, Version 2.0, included in the file | ||
| // licenses/APL.txt. | ||
|
|
||
| package inline_array |
There was a problem hiding this comment.
suggestion: change pkg name to something meaningful
There was a problem hiding this comment.
lol, no idea this was here :D (andy code)
| setup() | ||
| cli.Execute() | ||
| predicate() | ||
| log1.Flush() |
There was a problem hiding this comment.
suggestion: flush as part of predicate func
There was a problem hiding this comment.
Prefer to keep the predicate as simple as possible. The Flush isn't strictly necessary, its just good practise for tests.
config/config_test.go
Outdated
| cfg.Logging.Level = "debug" | ||
| cfg.Logging.Format = "json" | ||
| cfg.Logging.Stacktrace = true | ||
| cfg.Logging.OutputPath = "stdout" |
There was a problem hiding this comment.
or have it in a temp. directory.
cli/root.go
Outdated
| "logoutput", cfg.Log.OutputPath, | ||
| "Log output path", | ||
| "logger", "", | ||
| "named logger parameter override. usage: --logger <name>,level=<level>,output=<output>,etc...", |
There was a problem hiding this comment.
nitpick: IMO is more readable if first word is capitalized
There was a problem hiding this comment.
additional nitpick: I suggest using either ... or etc and not both
cli/root.go
Outdated
|
|
||
| rootCmd.PersistentFlags().String( | ||
| "logoutput", cfg.Logging.OutputPath, | ||
| "log output path", |
There was a problem hiding this comment.
nitpick: IMO is more readable if first word of flag description is capitalized
cli/root.go
Outdated
| // parses and then configures the given config.Config logging subconfig. | ||
| // we use log.Fatal instead of returning an error because we can't gurantee | ||
| // atomic updates, its either everything is properly set, or we Fatal() | ||
| func parseAndConfigLog(ctx context.Context, cfg *config.LoggingConfig, cmd *cobra.Command) { |
There was a problem hiding this comment.
suggestion: func to exist in cli.go instead
cli/root.go
Outdated
| } | ||
| } | ||
|
|
||
| func parseAndConfigLogAllParams(ctx context.Context, cfg *config.LoggingConfig, kvs string) { |
There was a problem hiding this comment.
suggestion: func to exist in cli.go instead
There was a problem hiding this comment.
Now that its also used in init.go yea it can move.
cli/root.go
Outdated
| // we use log.Fatal instead of returning an error because we can't gurantee | ||
| // atomic updates, its either everything is properly set, or we Fatal() | ||
| func parseAndConfigLog(ctx context.Context, cfg *config.LoggingConfig, cmd *cobra.Command) { | ||
| // apply logger configuration at the end |
There was a problem hiding this comment.
thought: to my eyes, this comment is superfluous
There was a problem hiding this comment.
but also why not just put the deferred code directly at the end instead of using defer?
There was a problem hiding this comment.
Good catch, there was more branching before so defer was used to ensure at various return points things get cleaned up, but the branching has been reduced, so youre absolutely correct.
cli/start.go
Outdated
| @@ -47,31 +46,9 @@ var startCmd = &cobra.Command{ | |||
| Short: "Start a DefraDB node", | |||
| Long: "Start a new instance of DefraDB node.", | |||
| // Load the root config if it exists, otherwise create it. | |||
There was a problem hiding this comment.
suggestion: If PersistentPreRunE goes, its comment should go as well
There was a problem hiding this comment.
I like the new config overide flags :)
There are a few question and suggestions but nothing major. At this point I strongly disagree with the log.Fatal "need" as documented in the parseAndConfigLog function doc. I don't see any reason for not returning an error.
| assert.Len(t, logLines, 3) | ||
| } | ||
|
|
||
| func captureLogLines(t *testing.T, setup func(), predicate func()) []string { |
There was a problem hiding this comment.
thought: the fact that this works here is beyond me. I tried this in the logging unit test and the pipe wouln't catch anything. This is why I had to create other functions as suggested in the zap community to be able to catch log outputs to stderr.
There was a problem hiding this comment.
hmm, well, it does work 😂. Can't speak to the approach you were using before?
|
Suggestion: in the cli package, prefer Feedback logging methods to normal logging ones. |
Should be done in a seperate PR, as theres more than a few spots on the CLI outside of scope of this PR that would change to Feedback |
jsimnz
force-pushed
the
jsimnz/feat/I413-log-config-overrides
branch
from
d0c2a98
to
f8e4491
Compare
| } | ||
|
|
||
| rootCmd.PersistentFlags().String( | ||
| "logger", "", | ||
| "Named logger parameter override. usage: --logger <name>,level=<level>,output=<output>,...", |
There was a problem hiding this comment.
suggestion: usage can be capitalized
|
|
||
| func parseAndConfigLogAllParams(ctx context.Context, cfg *config.LoggingConfig, kvs string) error { | ||
| if kvs == "" { | ||
| return nil //nothing todo |
There was a problem hiding this comment.
nitpick: skip the comment or change todo to to do
|
issue: Previously, the behavior of the |
I suggest that for this PR, the simplest fix for this is not to remove the implementation of the |
Good catch! I had noticed the removal and thought I was going to see it moved somewhere else. Then forgot about it while reviewing. |
oops, I did meant to handle this correctly when I was considating the PreRun in root. Great catch! CLI integration tests will def be a priority next release! |
cli/start.go
Outdated
|
|
||
| // parse loglevel overrides | ||
| // we use `cfg.Logging.Level` as an argument since the viper.Bind already handles | ||
| // binding the flags / EnvVars to the struct |
There was a problem hiding this comment.
nitpik: this comment creeped back into your PR 🧟♂️
There was a problem hiding this comment.
thats the consequence of copy/pasting xD
cli/start.go
Outdated
| err = cfg.Load(rootDir) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to load config: %w", err) | ||
| } |
There was a problem hiding this comment.
nitpick:
if err := cfg.Load(rootDir); err != nil {
return fmt.Errorf("failed to load config: %w", err)
}
jsimnz
force-pushed
the
jsimnz/feat/I413-log-config-overrides
branch
from
5ae2080
to
a59c90b
Compare
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=debug will set the global level for all loggers to error and then the defra.cli level to debug specifically. It supports an unbounded number of named logger values. Additionally, introduces a new cli flag --logger which 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=json will set the level to debug, output to stdout, and format to json for the logger named defra.cli. Co-authored-by: Andrew Sisley <[email protected]>
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: