[#461] Configure logger sampling policy #495
|
@ -41,6 +41,8 @@ const (
|
||||||
defaultStreamTimeout = 10 * time.Second
|
defaultStreamTimeout = 10 * time.Second
|
||||||
defaultShutdownTimeout = 15 * time.Second
|
defaultShutdownTimeout = 15 * time.Second
|
||||||
|
|
||||||
|
defaultLoggerSamplerInterval = 1 * time.Second
|
||||||
|
|
||||||
defaultGracefulCloseOnSwitchTimeout = 10 * time.Second
|
defaultGracefulCloseOnSwitchTimeout = 10 * time.Second
|
||||||
|
|
||||||
defaultPoolErrorThreshold uint32 = 100
|
defaultPoolErrorThreshold uint32 = 100
|
||||||
|
@ -81,6 +83,11 @@ const ( // Settings.
|
||||||
cfgLoggerLevel = "logger.level"
|
cfgLoggerLevel = "logger.level"
|
||||||
dkirillov marked this conversation as resolved
Outdated
|
|||||||
cfgLoggerDestination = "logger.destination"
|
cfgLoggerDestination = "logger.destination"
|
||||||
|
|
||||||
|
cfgLoggerSamplingEnabled = "logger.sampling.enabled"
|
||||||
|
cfgLoggerSamplingInitial = "logger.sampling.initial"
|
||||||
|
cfgLoggerSamplingThereafter = "logger.sampling.thereafter"
|
||||||
|
cfgLoggerSamplingInterval = "logger.sampling.interval"
|
||||||
|
|
||||||
// HttpLogging.
|
// HttpLogging.
|
||||||
cfgHTTPLoggingEnabled = "http_logging.enabled"
|
cfgHTTPLoggingEnabled = "http_logging.enabled"
|
||||||
cfgHTTPLoggingMaxBody = "http_logging.max_body"
|
cfgHTTPLoggingMaxBody = "http_logging.max_body"
|
||||||
|
@ -788,6 +795,10 @@ func newSettings() *viper.Viper {
|
||||||
// logger:
|
// logger:
|
||||||
v.SetDefault(cfgLoggerLevel, "debug")
|
v.SetDefault(cfgLoggerLevel, "debug")
|
||||||
v.SetDefault(cfgLoggerDestination, "stdout")
|
v.SetDefault(cfgLoggerDestination, "stdout")
|
||||||
dkirillov marked this conversation as resolved
Outdated
dkirillov
commented
Should we disable it by default? Should we disable it by default?
|
|||||||
|
v.SetDefault(cfgLoggerSamplingEnabled, false)
|
||||||
|
v.SetDefault(cfgLoggerSamplingThereafter, 100)
|
||||||
|
v.SetDefault(cfgLoggerSamplingInitial, 100)
|
||||||
|
v.SetDefault(cfgLoggerSamplingInterval, defaultLoggerSamplerInterval)
|
||||||
|
|
||||||
// http logger
|
// http logger
|
||||||
v.SetDefault(cfgHTTPLoggingEnabled, false)
|
v.SetDefault(cfgHTTPLoggingEnabled, false)
|
||||||
|
@ -1031,9 +1042,9 @@ func pickLogger(v *viper.Viper) *Logger {
|
||||||
|
|
||||||
switch dest {
|
switch dest {
|
||||||
case destinationStdout:
|
case destinationStdout:
|
||||||
return newStdoutLogger(lvl)
|
return newStdoutLogger(v, lvl)
|
||||||
case destinationJournald:
|
case destinationJournald:
|
||||||
alexvanin marked this conversation as resolved
Outdated
alexvanin
commented
Why sampler configuration only affects stdout logger? I think both loggers can use it. Why sampler configuration only affects stdout logger? I think both loggers can use it.
|
|||||||
return newJournaldLogger(lvl)
|
return newJournaldLogger(v, lvl)
|
||||||
default:
|
default:
|
||||||
panic(fmt.Sprintf("wrong destination for logger: %s", dest))
|
panic(fmt.Sprintf("wrong destination for logger: %s", dest))
|
||||||
}
|
}
|
||||||
|
@ -1046,53 +1057,74 @@ func pickLogger(v *viper.Viper) *Logger {
|
||||||
// - parameterized level (debug by default)
|
// - parameterized level (debug by default)
|
||||||
// - console encoding
|
// - console encoding
|
||||||
// - ISO8601 time encoding
|
// - ISO8601 time encoding
|
||||||
|
// - sampling intervals
|
||||||
//
|
//
|
||||||
// and atomic log level to dynamically change it.
|
// and atomic log level to dynamically change it.
|
||||||
//
|
//
|
||||||
// Logger records a stack trace for all messages at or above fatal level.
|
// Logger records a stack trace for all messages at or above fatal level.
|
||||||
//
|
//
|
||||||
// See also zapcore.Level, zap.NewProductionConfig, zap.AddStacktrace.
|
// See also zapcore.Level, zap.NewProductionConfig, zap.AddStacktrace.
|
||||||
func newStdoutLogger(lvl zapcore.Level) *Logger {
|
func newStdoutLogger(v *viper.Viper, lvl zapcore.Level) *Logger {
|
||||||
c := zap.NewProductionConfig()
|
stdout := zapcore.AddSync(os.Stderr)
|
||||||
dkirillov marked this conversation as resolved
Outdated
dkirillov
commented
It seems before we use It seems before we use `stderr` (from zap production config). Is it ok that we change this?
|
|||||||
c.Level = zap.NewAtomicLevelAt(lvl)
|
level := zap.NewAtomicLevelAt(lvl)
|
||||||
c.Encoding = "console"
|
|
||||||
c.EncoderConfig.EncodeTime = zapcore.ISO8601TimeEncoder
|
|
||||||
|
|
||||||
l, err := c.Build(
|
consoleOutCore := zapcore.NewCore(newLogEncoder(), stdout, level)
|
||||||
zap.AddStacktrace(zap.NewAtomicLevelAt(zap.FatalLevel)),
|
|
||||||
)
|
consoleOutCore = samplingEnabling(v, consoleOutCore)
|
||||||
if err != nil {
|
|
||||||
panic(fmt.Sprintf("build zap logger instance: %v", err))
|
|
||||||
}
|
|
||||||
|
|
||||||
return &Logger{
|
return &Logger{
|
||||||
logger: l,
|
logger: zap.New(consoleOutCore, zap.AddStacktrace(zap.NewAtomicLevelAt(zap.FatalLevel))),
|
||||||
lvl: c.Level,
|
lvl: level,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
dkirillov marked this conversation as resolved
Outdated
dkirillov
commented
What about configuring this value (I don't mind current behavior)? What about configuring this value (I don't mind current behavior)?
Also we should mention all new params in `doc/configuration.md`
|
|||||||
func newJournaldLogger(lvl zapcore.Level) *Logger {
|
func newJournaldLogger(v *viper.Viper, lvl zapcore.Level) *Logger {
|
||||||
c := zap.NewProductionConfig()
|
level := zap.NewAtomicLevelAt(lvl)
|
||||||
c.EncoderConfig.EncodeTime = zapcore.ISO8601TimeEncoder
|
|
||||||
c.Level = zap.NewAtomicLevelAt(lvl)
|
|
||||||
|
|
||||||
encoder := zapjournald.NewPartialEncoder(zapcore.NewConsoleEncoder(c.EncoderConfig), zapjournald.SyslogFields)
|
encoder := zapjournald.NewPartialEncoder(newLogEncoder(), zapjournald.SyslogFields)
|
||||||
|
|
||||||
dkirillov marked this conversation as resolved
Outdated
dkirillov
commented
As I understand this won't work as expected because we have already built logger wit production config that has enabled sampling As I understand this won't work as expected because we have already built logger wit production config that has enabled sampling
|
|||||||
core := zapjournald.NewCore(c.Level, encoder, &journald.Journal{}, zapjournald.SyslogFields)
|
core := zapjournald.NewCore(level, encoder, &journald.Journal{}, zapjournald.SyslogFields)
|
||||||
coreWithContext := core.With([]zapcore.Field{
|
coreWithContext := core.With([]zapcore.Field{
|
||||||
zapjournald.SyslogFacility(zapjournald.LogDaemon),
|
zapjournald.SyslogFacility(zapjournald.LogDaemon),
|
||||||
zapjournald.SyslogIdentifier(),
|
zapjournald.SyslogIdentifier(),
|
||||||
zapjournald.SyslogPid(),
|
zapjournald.SyslogPid(),
|
||||||
})
|
})
|
||||||
|
|
||||||
|
coreWithContext = samplingEnabling(v, coreWithContext)
|
||||||
|
|
||||||
alexvanin marked this conversation as resolved
Outdated
alexvanin
commented
Can we set this before calling Build? You can set sampling config in Can we set this before calling Build? You can set sampling config in `c` as it is done in `c.Encoding` and `c.EncoderConfig`.
pogpp
commented
https://github.com/uber-go/zap/blob/master/config.go#L37
dkirillov
commented
We need rewrite initialization then (e.g. TrueCloudLab/frostfs-s3-lifecycler#21) We need rewrite initialization then (e.g. https://git.frostfs.info/TrueCloudLab/frostfs-s3-lifecycler/pulls/21)
|
|||||||
l := zap.New(coreWithContext, zap.AddStacktrace(zap.NewAtomicLevelAt(zap.FatalLevel)))
|
l := zap.New(coreWithContext, zap.AddStacktrace(zap.NewAtomicLevelAt(zap.FatalLevel)))
|
||||||
|
|
||||||
return &Logger{
|
return &Logger{
|
||||||
logger: l,
|
logger: l,
|
||||||
lvl: c.Level,
|
lvl: level,
|
||||||
}
|
}
|
||||||
alexvanin marked this conversation as resolved
Outdated
alexvanin
commented
One last suggestion. Both jouranld and stdout logger require:
Can we make separate function to get console encoder and reuse it in both constructors? It will make code similar and much easier to read. For example:
One last suggestion.
Both jouranld and stdout logger require:
* jsonEncoder
* level
Can we make separate function to get console encoder and reuse it in both constructors? It will make code *similar* and much easier to read.
For example:
```go
func consoleEncoder() zapcore.Encoder {
c := zap.NewProductionEncoderConfig()
c.EncodeTime = zapcore.ISO8601TimeEncoder
return zapcore.NewConsoleEncoder(c)
}
func newJournaldLogger(v *viper.Viper, lvl zapcore.Level) *Logger {
level := zap.NewAtomicLevelAt(lvl)
consoleEnc := consoleEncoder()
encoder := zapjournald.NewPartialEncoder(consoleEnc, zapjournald.SyslogFields)
core := zapjournald.NewCore(level, encoder, &journald.Journal{}, zapjournald.SyslogFields)
...
}
func newStdoutLogger(v *viper.Viper, lvl zapcore.Level) *Logger {
level := zap.NewAtomicLevelAt(lvl)
consoleEnc := consoleEncoder()
stdout := zapcore.AddSync(os.Stdout)
consoleOutCore := zapcore.NewCore(consoleEnc, stdout, level)
...
}
```
|
|||||||
}
|
}
|
||||||
|
|
||||||
|
func newLogEncoder() zapcore.Encoder {
|
||||||
|
c := zap.NewProductionEncoderConfig()
|
||||||
|
c.EncodeTime = zapcore.ISO8601TimeEncoder
|
||||||
dkirillov marked this conversation as resolved
Outdated
dkirillov
commented
Can we hide this condition in separate function? Currently, we have exactly the same 8 lines here and in Can we hide this condition in separate function? Currently, we have exactly the same 8 lines here and in `newStdoutLogger`
|
|||||||
|
|
||||||
|
return zapcore.NewConsoleEncoder(c)
|
||||||
|
}
|
||||||
|
|
||||||
|
func samplingEnabling(v *viper.Viper, core zapcore.Core) zapcore.Core {
|
||||||
|
// Zap samples by logging the first cgfLoggerSamplingInitial entries with a given level
|
||||||
|
// and message within the specified time interval.
|
||||||
|
// In the above config, only the first cgfLoggerSamplingInitial log entries with the same level and message
|
||||||
|
// are recorded in cfgLoggerSamplingInterval interval. Every other log entry will be dropped within the interval since
|
||||||
|
// cfgLoggerSamplingThereafter is specified here.
|
||||||
|
if v.GetBool(cfgLoggerSamplingEnabled) {
|
||||||
|
core = zapcore.NewSamplerWithOptions(
|
||||||
|
core,
|
||||||
|
v.GetDuration(cfgLoggerSamplingInterval),
|
||||||
|
v.GetInt(cfgLoggerSamplingInitial),
|
||||||
|
v.GetInt(cfgLoggerSamplingThereafter),
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
|
return core
|
||||||
|
}
|
||||||
|
|
||||||
func getLogLevel(v *viper.Viper) (zapcore.Level, error) {
|
func getLogLevel(v *viper.Viper) (zapcore.Level, error) {
|
||||||
dkirillov marked this conversation as resolved
Outdated
dkirillov
commented
Maybe just
Maybe just
```golang
return zapcore.NewConsoleEncoder(c)
```
|
|||||||
var lvl zapcore.Level
|
var lvl zapcore.Level
|
||||||
lvlStr := v.GetString(cfgLoggerLevel)
|
lvlStr := v.GetString(cfgLoggerLevel)
|
||||||
|
|
|
@ -51,6 +51,10 @@ S3_GW_CONFIG=/path/to/config/yaml
|
||||||
|
|
||||||
# Logger
|
# Logger
|
||||||
S3_GW_LOGGER_LEVEL=debug
|
S3_GW_LOGGER_LEVEL=debug
|
||||||
|
S3_GW_LOGGER_SAMPLING_ENABLED=false
|
||||||
|
S3_GW_LOGGER_SAMPLING_INITIAL=100
|
||||||
|
S3_GW_LOGGER_SAMPLING_THEREAFTER=100
|
||||||
|
S3_GW_LOGGER_SAMPLING_INTERVAL=1s
|
||||||
|
|
||||||
# HTTP logger
|
# HTTP logger
|
||||||
S3_GW_HTTP_LOGGING_ENABLED=false
|
S3_GW_HTTP_LOGGING_ENABLED=false
|
||||||
|
|
|
@ -55,6 +55,11 @@ vhs:
|
||||||
logger:
|
logger:
|
||||||
level: debug
|
level: debug
|
||||||
destination: stdout
|
destination: stdout
|
||||||
|
sampling:
|
||||||
|
enabled: false
|
||||||
|
initial: 100
|
||||||
|
thereafter: 100
|
||||||
alexvanin marked this conversation as resolved
Outdated
alexvanin
commented
`interval` is missing?
|
|||||||
|
interval: 1s
|
||||||
|
|
||||||
# log http request data (URI, headers, query, etc)
|
# log http request data (URI, headers, query, etc)
|
||||||
http_logging:
|
http_logging:
|
||||||
|
|
|
@ -370,12 +370,21 @@ server:
|
||||||
logger:
|
logger:
|
||||||
level: debug
|
level: debug
|
||||||
destination: stdout
|
destination: stdout
|
||||||
|
sampling:
|
||||||
|
enabled: false
|
||||||
alexvanin marked this conversation as resolved
Outdated
alexvanin
commented
`enabled` is missing?
alexvanin marked this conversation as resolved
Outdated
alexvanin
commented
backtick symbol? backtick symbol?
|
|||||||
|
initial: 100
|
||||||
|
thereafter: 100
|
||||||
|
interval: 1s
|
||||||
```
|
```
|
||||||
|
|
||||||
| Parameter | Type | SIGHUP reload | Default value | Description |
|
| Parameter | Type | SIGHUP reload | Default value | Description |
|
||||||
|---------------|----------|---------------|---------------|----------------------------------------------------------------------------------------------------|
|
|-----------------------|------------|---------------|---------------|----------------------------------------------------------------------------------------------------|
|
||||||
| `level` | `string` | yes | `debug` | Logging level.<br/>Possible values: `debug`, `info`, `warn`, `error`, `dpanic`, `panic`, `fatal`. |
|
| `level` | `string` | yes | `debug` | Logging level.<br/>Possible values: `debug`, `info`, `warn`, `error`, `dpanic`, `panic`, `fatal`. |
|
||||||
| `destination` | `string` | no | `stdout` | Destination for logger: `stdout` or `journald` |
|
| `destination` | `string` | no | `stdout` | Destination for logger: `stdout` or `journald` |
|
||||||
|
| `sampling.enabled` | `bool` | no | false | Sampling enabling flag. |
|
||||||
|
| `sampling.initial` | `int` | no | '100' | Sampling count of first log entries. |
|
||||||
|
| `sampling.thereafter` | `int` | no | '100' | Sampling count of entries after an `interval`. |
|
||||||
|
| `sampling.interval` | `duration` | no | '1s' | Sampling interval of messaging similar entries. |
|
||||||
|
|
||||||
|
|
||||||
### `http_logging` section
|
### `http_logging` section
|
||||||
|
|
We need flag that allow disable sampling at all