add flag to enable logging output in json to consul-k8s #523
Conversation
There was a problem hiding this comment.
Nice work on this. Couple questions.
What about:
- acl-init
- tls-init
It looks like zapcore doesn't support trace (see snippet below) so how should we handle that since it's a valid value in the values file? We could make it use debug in that instance?
Other questions
- should we switch everything over to using zap logger and ditch hclog?
- should we also change where we're using c.UI to use the logger? Otherwise there will be non-json output mixed in I think?
// UnmarshalText unmarshals text to a level. Like MarshalText, UnmarshalText
// expects the text representation of a Level to drop the -Level suffix (see
// example).
//
// In particular, this makes it easy to configure logging levels using YAML,
// TOML, or JSON files.
func (l *Level) UnmarshalText(text []byte) error {
if l == nil {
return errUnmarshalNilLevel
}
if !l.unmarshalText(text) && !l.unmarshalText(bytes.ToLower(text)) {
return fmt.Errorf("unrecognized level: %q", text)
}
return nil
}
func (l *Level) unmarshalText(text []byte) bool {
switch string(text) {
case "debug", "DEBUG":
*l = DebugLevel
case "info", "INFO", "": // make the zero value useful
*l = InfoLevel
case "warn", "WARN":
*l = WarnLevel
case "error", "ERROR":
*l = ErrorLevel
case "dpanic", "DPANIC":
*l = DPanicLevel
case "panic", "PANIC":
*l = PanicLevel
case "fatal", "FATAL":
*l = FatalLevel
default:
return false
}
return true
}
@lkysow acl-init does not use the command Which brings us to your other 2 questions:
tls-init should be implemented in here? I see it at least, maybe my 👁️👁️ see into the future? |
|
@lkysow I did a bit of digging regarding your question about switching everything to Zap from hclog: I dug a bit into converting all hclog into logr.Logger (zap).
So I'll open an issue for 1-3 above, and maybe we can try to also solve number 4 somehow else.. And then we'll just go ahead with this PR in its current form after I've fixed the comments! |
There was a problem hiding this comment.
I think this PR looks great!
I agree with the comments above for moving over to zap in a follow up PR.
For the UI logs, will it break users ci setups if they turn on JSON logging and there are a few interspersed UI logs? Will it just be a few unparseable logs? Just wondering the impact to see whether it should belong in this PR or not. If you think it's reasonable to push that out to a follow up though, that sounds good too.
There was a problem hiding this comment.
(copied from helm, had a thought when reviewing that one) For a command like consul-sidecar for connect-injected pods that doesn't have its logLevel/logJSON set in the helm template, do you need to add something to this PR to invoke that internal command with the right flag based on the global value?
Oh great catch! I've plumbed that through in the handler now. |
Co-authored-by: Luke Kysow <[email protected]>
|
@kschoche I believe the Changelog somehow got placed in the wrong place after merging? It looks like it is now under Improvements under |
Changes proposed in this PR:
-log-levelto all consul-k8s commands that dont already have it which controls the log level.-log-jsonto all consul-k8s commands which controls whether or not the command will log its output in JSON.DevModefor Zap based logging (controller and connect-inject) so that we no longer print stack traces for anything below Error.This resolves Allow JSON logging for consul-k8s #516
How I've tested this PR:
Tested using a debug image:
Unit tests pass and when enabled we have logging like this:
How I expect reviewers to test this PR:
code review / check out and run tests with json logging enabled.
A second PR for consul-helm will drive these flags [https://github.com/hashicorp/consul-helm/pull/980/]
Note to reviewers: I'm curious about how to go about unit testing for this PR, thoughts?
Checklist: