Configuration support for zap logger #52
Conversation
sudiptob2
force-pushed
the
feat/48/refine-zap-logger
branch
2 times, most recently
from
a692caf to
9bb1d77
Compare
sudiptob2
force-pushed
the
feat/48/refine-zap-logger
branch
from
9bb1d77 to
2cb4548
Compare
sudiptob2
force-pushed
the
feat/48/refine-zap-logger
branch
from
2cb4548 to
3f474e2
Compare
|
Pods in Kubernetes are ephemeral, and we don't use any persistent volumes to backup log files. In case of a Pod restart, the log files will be lost. Best practice is to log directly to STDOUT and STDERR and leave log managament, rotation... to Kubernetes. No reason to keep a log file or manually do rotation. |
|
It would be nice to add a config option to switch between human-readable logs and JSON logs. |
log/logger.go
Outdated
| MaxBackups: maxBackups, | ||
| MaxAge: maxAge, | ||
| Compress: compress, | ||
| Filename: config.LogFilePath, |
There was a problem hiding this comment.
check my comment, this is redundant
log/logger.go
Outdated
| logLevel = zap.InfoLevel | ||
| ) | ||
| type LogConfig struct { | ||
| LogFilePath string |
There was a problem hiding this comment.
Same for all file-related log config
log/logger.go
Outdated
| consoleEncoder := zapcore.NewConsoleEncoder(developmentCfg) | ||
| fileEncoder := zapcore.NewJSONEncoder(productionCfg) | ||
| var encoder zapcore.Encoder | ||
| if config.IsProduction { |
There was a problem hiding this comment.
this can be simplified:
encoder = zapcore.NewConsoleEncoder(developmentCfg)
if config.IsProduction {
encoder = zapcore.NewJSONEncoder(productionCfg)
}
There was a problem hiding this comment.
I'd rename IsProduction to UseJSONLogFormat
There was a problem hiding this comment.
In that case, can we entirely remove the development config? And just keep only one configuration. And then according to the log format flag, we can switch the Encoder
config := zap.NewProductionEncoderConfig()
var encoder zapcore.Encoder
encoder = zapcore.NewConsoleEncoder(config)
if config.IsProduction {
encoder = zapcore.NewJSONEncoder(config)
}There was a problem hiding this comment.
that makes sense, I always try to avoid naming something in an application based on environment name and keep things generic
sudiptob2
force-pushed
the
feat/48/refine-zap-logger
branch
from
ea16d1e to
d04494b
Compare
charts/values.yaml
Outdated
| log: | ||
| # -- Output type of the log, if true, log will be output in json format | ||
| useJSONFormat: true |
There was a problem hiding this comment.
I guess we could even shorten it to jsonFormat
log/logger.go
Outdated
| @@ -10,8 +10,8 @@ import ( | |||
| ) | |||
|
|
|||
| type LogConfig struct { | |||
| UseJSONFormat bool | |||
| LogLevel string | |||
| JsonFormat bool | |||
There was a problem hiding this comment.
| JsonFormat bool | |
| JSONFormat bool |
closes #48
Changes:
DEBUG, INFO, WARN, ERROR, DPANIC, PANIC, FATAL