Fix log level handling #93
Conversation
This seems like the most reasonable default. We no longer need to take a lock to read the global log level, so we might as well drop the lock entirely.
That way, the reader gets a nice EOF instead of a "read on closed pipe" error.
This shouldn't be necessary, but it can't hurt.
|
cc @iand |
setup.go
Outdated
| // primaryFormat is the default format of the primary core used for logging | ||
| var primaryFormat LogFormat = ColorizedOutput | ||
| // defaultFormat is the default format of the primary core used for logging | ||
| var defaultFormat LogFormat = ColorizedOutput |
There was a problem hiding this comment.
I originally named them primaryFormat/primaryLevel because they correspond to the format and level used by the primary core which. It's not really a default
There was a problem hiding this comment.
primaryFormat makes sense. primaryLevel isn't quite correct because the level for the primary logger needs to be debug.
pipe.go
Outdated
| // By default, it: | ||
| // | ||
| // 1. Logs JSON. | ||
| // 2. Logs at the Debug level. However, unless SetLogLevel is called on a |
There was a problem hiding this comment.
I think it's better to set it to be the same as the primary logger's level by default. The user will then see the same logs that are being logged to stderr or the output file. They still have an option to inspect debug messages from subsystems that have had their log level decreased.
Either way the comment about "only error messages will be logged" is slightly confusing because if they have specified GOLOG_LOG_LEVEL=Info then the primary core will be emitting log events at that level and above.
There was a problem hiding this comment.
I think it's better to set it to be the same as the primary logger's level by default.
It is. The primary log level is debug and must be debug. Otherwise, calling SetLogLevel("some-subsystem", "debug") won't work because, while the messages will make it through the subsystem's logger, they'll get stopped by the primary logger because the primary logger will still filter out all messages with levels below ERROR.
(but I agree that the comment is really confusing)
|
Thanks for the review! |
Currently, we filter messages at two layers:
However, the final log level was set to ERROR. That meant that reducing the level on subsystem loggers didn't actually do anything. This meant
SetLogLeveldidn't work. This change sets the default log level for pipe readers and the default logging core to DEBUG.This change also changes the default pipe reader format to JSON to avoid taking a lock. IMO, this should be less surprising regardless as it's more predictable.
I'm not entirely happy about how we're handing log levels but this should be good enough for now. Ideally, we'd have #92.