Added std type to logging config. #2060
Conversation
Added demux-ing of logging to stdout/err working around sirupsen/logrus#403 Turned off the default logging in favour of using hooks always. Added fallback, if no stdout/err logger was configured, add one automatically. This prevents unexpected lack of logging after an upgrade in case the user's configuration file is not updated. Fixes: #2054 Signed-off-by: Martin Ashby <[email protected]>
internal/log_unix.go
Outdated
| // If something fails here it means that the logging was improperly configured, | ||
| // so we just exit with the error | ||
| func SetupHookLogging(hooks []config.LogrusHook, componentName string) { | ||
| func SetupLogging(hooks []config.LogrusHook, componentName string) { |
There was a problem hiding this comment.
Renaming this function will break building on Windows.
There was a problem hiding this comment.
Fair. Reverting this.
internal/log_unix.go
Outdated
| @@ -29,6 +31,17 @@ import ( | |||
| // so we just exit with the error | |||
| func SetupHookLogging(hooks []config.LogrusHook, componentName string) { | |||
| logrus.SetReportCaller(true) | |||
| logrus.SetFormatter(&utcFormatter{ | |||
There was a problem hiding this comment.
If you move this back to internal/log.go and undo the changes to setup/base/base.go, I'm happy with this. :)
The same formatting should be applied on both windows and unix, so it makes sense to keep it in files which are shared between both platforms. Fixes #2060 (comment)
internal/log_unix.go
Outdated
| default: | ||
| logrus.Fatalf("Unrecognised logging hook type: %s", hook.Type) | ||
| } | ||
| } | ||
| if !stdLogAdded { | ||
| logrus.Info("No std logger config found. Enabling at INFO level by default") |
There was a problem hiding this comment.
I'm not really sure if we need to log this? It just seems like noise.
There was a problem hiding this comment.
Yep, I was pretty much just using it for a quick test. Could take it out.
| setupStdLogHook(logrus.InfoLevel) | ||
| } | ||
| // Hooks are now configured for stdout/err, so throw away the default logger output | ||
| logrus.SetOutput(ioutil.Discard) |
There was a problem hiding this comment.
I'm somewhat wondering whether something like logrus.SetLevel(logrus.PanicLevel) is better here, since otherwise we're allocating log buffers just to write them to a discarding writer, which seems like a waste of allocations?
There was a problem hiding this comment.
I'm not sure: wouldn't this set the level for all loggers?
There was a problem hiding this comment.
I'm not sure to be honest. Just a thought / might be worth trying.
There was a problem hiding this comment.
It doesn't work, I'm afraid. Calling SetLevel(logrus.PanicLevel) simply disables all logging
This explains why this code here is required:
Line 41 in 61406a6
I think this is just a limitation of logrus.
There was a problem hiding this comment.
That's unfortunate. We'll just have to go with io.Discard anyway then.
Added demux-ing of logging to stdout/err working around
sirupsen/logrus#403
Turned off the default logging in favour of using hooks always.
Added fallback, if no stdout/err logger was configured, add one
automatically. This prevents unexpected lack of logging after an
upgrade in case the user's configuration file is not updated.
Fixes: #2054
Pull Request Checklist
✔️sytest-whitelistas specified in docs/sytest.mdSigned-off-by:
Your Name <[email protected]>