[#224] Refactor logger tag configuration #224
|
@ -7,6 +7,7 @@ This document outlines major changes between releases.
|
|||
### Added
|
||||
- Add handling quota limit reached error (#187)
|
||||
- Add slash clipping for FileName attribute (#174)
|
||||
- Add new format of tag names config
|
||||
|
||||
## [0.32.3] - 2025-02-05
|
||||
|
||||
|
|
|
@ -114,7 +114,8 @@ type (
|
|||
}
|
||||
|
||||
tagsConfig struct {
|
||||
tagLogs sync.Map
|
||||
tagLogs sync.Map
|
||||
defaultLvl zap.AtomicLevel
|
||||
}
|
||||
|
||||
logLevelConfig struct {
|
||||
|
@ -134,19 +135,34 @@ func newLogLevel(v *viper.Viper) zap.AtomicLevel {
|
|||
}
|
||||
|
||||
func newTagsConfig(v *viper.Viper, ll zapcore.Level) *tagsConfig {
|
||||
var t tagsConfig
|
||||
t := tagsConfig{defaultLvl: zap.NewAtomicLevelAt(ll)}
|
||||
if err := t.update(v, ll); err != nil {
|
||||
// panic here is analogue of the similar panic during common log level initialization.
|
||||
panic(err.Error())
|
||||
}
|
||||
|
||||
return &t
|
||||
}
|
||||
|
||||
func newLogLevelConfig(lvl zap.AtomicLevel, tagsConfig *tagsConfig) *logLevelConfig {
|
||||
return &logLevelConfig{
|
||||
cfg := &logLevelConfig{
|
||||
logLevel: lvl,
|
||||
tagsConfig: tagsConfig,
|
||||
}
|
||||
|
||||
cfg.setMinLogLevel()
|
||||
|
||||
return cfg
|
||||
}
|
||||
|
||||
func (l *logLevelConfig) setMinLogLevel() {
|
||||
l.tagsConfig.tagLogs.Range(func(_, value any) bool {
|
||||
dkirillov marked this conversation as resolved
Outdated
|
||||
v := value.(zapcore.Level)
|
||||
if v < l.logLevel.Level() {
|
||||
l.logLevel.SetLevel(v)
|
||||
}
|
||||
return true
|
||||
})
|
||||
}
|
||||
|
||||
func (l *logLevelConfig) update(cfg *viper.Viper, log *zap.Logger) {
|
||||
|
@ -159,17 +175,23 @@ func (l *logLevelConfig) update(cfg *viper.Viper, log *zap.Logger) {
|
|||
if err := l.tagsConfig.update(cfg, l.logLevel.Level()); err != nil {
|
||||
log.Warn(logs.TagsLogConfigWontBeUpdated, zap.Error(err), logs.TagField(logs.TagApp))
|
||||
}
|
||||
dkirillov marked this conversation as resolved
Outdated
dkirillov
commented
I suggest this
I suggest this
```diff
diff --git a/cmd/http-gw/app.go b/cmd/http-gw/app.go
index 8fc9fd2..f3638dd 100644
--- a/cmd/http-gw/app.go
+++ b/cmd/http-gw/app.go
@@ -145,36 +145,38 @@ func newTagsConfig(v *viper.Viper, ll zapcore.Level) *tagsConfig {
}
func newLogLevelConfig(lvl zap.AtomicLevel, tagsConfig *tagsConfig) *logLevelConfig {
- return &logLevelConfig{
- logLevel: minLogLevel(lvl, tagsConfig),
+ cfg := &logLevelConfig{
+ logLevel: lvl,
tagsConfig: tagsConfig,
}
+
+ cfg.setMinLogLevel()
+
+ return cfg
}
-func minLogLevel(lvl zap.AtomicLevel, config *tagsConfig) zap.AtomicLevel {
- minLvl := lvl
- config.tagLogs.Range(func(_, value any) bool {
+func (l *logLevelConfig) setMinLogLevel() {
+ l.tagsConfig.tagLogs.Range(func(_, value any) bool {
v := value.(zapcore.Level)
- if v < minLvl.Level() {
- minLvl = zap.NewAtomicLevelAt(v)
+ if v < l.logLevel.Level() {
+ l.logLevel.SetLevel(v)
}
return true
})
-
- return minLvl
}
func (l *logLevelConfig) update(cfg *viper.Viper, log *zap.Logger) {
- lvl, err := getLogLevel(cfg)
- if err != nil {
+ if lvl, err := getLogLevel(cfg); err != nil {
log.Warn(logs.LogLevelWontBeUpdated, zap.Error(err), logs.TagField(logs.TagApp))
+ } else {
+ l.logLevel.SetLevel(lvl)
}
if err := l.tagsConfig.update(cfg, l.logLevel.Level()); err != nil {
log.Warn(logs.TagsLogConfigWontBeUpdated, zap.Error(err), logs.TagField(logs.TagApp))
}
- l.logLevel.SetLevel(minLogLevel(zap.NewAtomicLevelAt(lvl), l.tagsConfig).Level())
+ l.setMinLogLevel()
}
func (t *tagsConfig) LevelEnabled(tag string, tgtLevel zapcore.Level) (bool, bool) {
```
|
||||
|
||||
l.setMinLogLevel()
|
||||
}
|
||||
dkirillov marked this conversation as resolved
Outdated
dkirillov
commented
We can don't change signature. If we write
We can don't change signature. If we write
```golang
func (t *tagsConfig) LevelEnabled(tag string, tgtLevel zapcore.Level) bool {
lvl, ok := t.tagLogs.Load(tag)
if !ok {
return t.defaultLvl.Enabled(tgtLevel)
}
return lvl.(zapcore.Level).Enabled(tgtLevel)
}
```
|
||||
|
||||
func (t *tagsConfig) LevelEnabled(tag string, tgtLevel zapcore.Level) bool {
|
||||
lvl, ok := t.tagLogs.Load(tag)
|
||||
if !ok {
|
||||
return false
|
||||
return t.defaultLvl.Enabled(tgtLevel)
|
||||
}
|
||||
|
||||
return lvl.(zapcore.Level).Enabled(tgtLevel)
|
||||
}
|
||||
dkirillov marked this conversation as resolved
Outdated
dkirillov
commented
I would return not atomic, but just level
or don't return level but accept it (similart to
I would return not atomic, but just level
```golang
func (t *tagsConfig) DefaultLevel() zapcore.Level {
return t.defaultLvl.Level()
}
```
or don't return level but accept it (similart to `levelEnabled`)
```golang
func (t *tagsConfig) DefaultEnabled(lvl zapcore.Level) bool {
return t.defaultLvl.Enabled(lvl)
}
```
|
||||
|
||||
func (t *tagsConfig) DefaultEnabled(lvl zapcore.Level) bool {
|
||||
return t.defaultLvl.Enabled(lvl)
|
||||
}
|
||||
|
||||
func (t *tagsConfig) update(cfg *viper.Viper, ll zapcore.Level) error {
|
||||
tags, err := fetchLogTagsConfig(cfg, ll)
|
||||
if err != nil {
|
||||
|
@ -194,6 +216,7 @@ func (t *tagsConfig) update(cfg *viper.Viper, ll zapcore.Level) error {
|
|||
for k, v := range tags {
|
||||
t.tagLogs.Store(k, v)
|
||||
dkirillov marked this conversation as resolved
Outdated
dkirillov
commented
Should be
Should be
```golang
t.defaultLvl.SetLevel(ll)
```
pogpp
commented
In such scenario we will have panic on integration test In such scenario we will have panic on integration test
dkirillov
commented
Why so? Why so?
dkirillov
commented
Using Using `t.defaultLvl = zap.NewAtomicLevelAt(ll)` leads to data race
|
||||
}
|
||||
t.defaultLvl.SetLevel(ll)
|
||||
|
||||
return nil
|
||||
}
|
||||
|
|
|
@ -40,7 +40,8 @@ type zapCoreTagFilterWrapper struct {
|
|||||||||||
}
|
||||||||||||
|
||||||||||||
type TagFilterSettings interface {
|
||||||||||||
LevelEnabled(tag string, lvl zapcore.Level) bool
|
||||||||||||
LevelEnabled(tag string, tgtLevel zapcore.Level) bool
|
||||||||||||
DefaultEnabled(lvl zapcore.Level) bool
|
||||||||||||
}
|
||||||||||||
|
||||||||||||
func (c *zapCoreTagFilterWrapper) Enabled(level zapcore.Level) bool {
|
||||||||||||
|
@ -63,24 +64,26 @@ func (c *zapCoreTagFilterWrapper) Check(entry zapcore.Entry, checked *zapcore.Ch
|
|||||||||||
}
|
||||||||||||
|
||||||||||||
func (c *zapCoreTagFilterWrapper) Write(entry zapcore.Entry, fields []zapcore.Field) error {
|
||||||||||||
if c.shouldSkip(entry, fields) || c.shouldSkip(entry, c.extra) {
|
||||||||||||
if c.shouldSkip(entry, fields, c.extra) {
|
||||||||||||
dkirillov marked this conversation as resolved
Outdated
dkirillov
commented
Why Why `!c.shouldSkip`? It seems we still need `c.shouldSkip`
dkirillov
commented
Oh, I see, you've changed
But then we still have
That means that we should skip log if we don't have tag in it. So I suppose don't remove
and don't add
Oh, I see, you've changed https://git.frostfs.info/pogpp/frostfs-http-gw/src/commit/9a02809ac7dbe9c998b0fc4d2f88c104e51f6f80/cmd/http-gw/logger.go#L81
But then we still have `false` https://git.frostfs.info/pogpp/frostfs-http-gw/src/commit/9a02809ac7dbe9c998b0fc4d2f88c104e51f6f80/cmd/http-gw/logger.go#L90
That means that we should skip log if we don't have tag in it.
But you skip it if you keep `if !c.shouldSkip(entry, fields, c.extra) {`
So I suppose don't remove `!` here https://git.frostfs.info/pogpp/frostfs-http-gw/src/commit/9a02809ac7dbe9c998b0fc4d2f88c104e51f6f80/cmd/http-gw/logger.go#L86
and above.
and don't add `!` here https://git.frostfs.info/pogpp/frostfs-http-gw/src/commit/9a02809ac7dbe9c998b0fc4d2f88c104e51f6f80/cmd/http-gw/logger.go#L67
dkirillov
commented
```diff
diff --git a/cmd/http-gw/logger.go b/cmd/http-gw/logger.go
index f01327c..195aa4e 100644
--- a/cmd/http-gw/logger.go
+++ b/cmd/http-gw/logger.go
@@ -64,11 +64,7 @@ func (c *zapCoreTagFilterWrapper) Check(entry zapcore.Entry, checked *zapcore.Ch
}
func (c *zapCoreTagFilterWrapper) Write(entry zapcore.Entry, fields []zapcore.Field) error {
- if !c.shouldSkip(entry, fields, c.extra) {
- return nil
- }
-
- if !c.settings.DefaultEnabled(entry.Level) {
+ if c.shouldSkip(entry, fields, c.extra) {
return nil
}
@@ -78,16 +74,16 @@ func (c *zapCoreTagFilterWrapper) Write(entry zapcore.Entry, fields []zapcore.Fi
func (c *zapCoreTagFilterWrapper) shouldSkip(entry zapcore.Entry, fields []zap.Field, extra []zap.Field) bool {
for _, field := range fields {
if field.Key == logs.TagFieldName && field.Type == zapcore.StringType {
- return c.settings.LevelEnabled(field.String, entry.Level)
+ return !c.settings.LevelEnabled(field.String, entry.Level)
}
}
for _, field := range extra {
if field.Key == logs.TagFieldName && field.Type == zapcore.StringType {
- return c.settings.LevelEnabled(field.String, entry.Level)
+ return !c.settings.LevelEnabled(field.String, entry.Level)
}
}
- return false
+ return !c.settings.DefaultEnabled(entry.Level)
}
func (c *zapCoreTagFilterWrapper) Sync() error {
```
|
||||||||||||
return nil
|
||||||||||||
}
|
||||||||||||
|
||||||||||||
return c.core.Write(entry, fields)
|
||||||||||||
}
|
||||||||||||
|
||||||||||||
func (c *zapCoreTagFilterWrapper) shouldSkip(entry zapcore.Entry, fields []zap.Field) bool {
|
||||||||||||
func (c *zapCoreTagFilterWrapper) shouldSkip(entry zapcore.Entry, fields []zap.Field, extra []zap.Field) bool {
|
||||||||||||
dkirillov marked this conversation as resolved
Outdated
dkirillov
commented
Probably we can pass both ( Probably we can pass both (`fields`, `c.extra`) to `c.shouldSkip`
|
||||||||||||
for _, field := range fields {
|
||||||||||||
if field.Key == logs.TagFieldName && field.Type == zapcore.StringType {
|
||||||||||||
if !c.settings.LevelEnabled(field.String, entry.Level) {
|
||||||||||||
return true
|
||||||||||||
}
|
||||||||||||
break
|
||||||||||||
return !c.settings.LevelEnabled(field.String, entry.Level)
|
||||||||||||
}
|
||||||||||||
}
|
||||||||||||
KurlesHS
commented
Current
the first Current `Write` implementation issue: if extra fields contain an app tag with `info` level, while:
- default level is set to `fatal`
- current level is set to `info`
the first `shouldSkip` call will return true, causing messages with tags that should be logged to be skipped.
|
||||||||||||
for _, field := range extra {
|
||||||||||||
if field.Key == logs.TagFieldName && field.Type == zapcore.StringType {
|
||||||||||||
return !c.settings.LevelEnabled(field.String, entry.Level)
|
||||||||||||
}
|
||||||||||||
}
|
||||||||||||
|
||||||||||||
return false
|
||||||||||||
return !c.settings.DefaultEnabled(entry.Level)
|
||||||||||||
}
|
||||||||||||
|
||||||||||||
func (c *zapCoreTagFilterWrapper) Sync() error {
|
||||||||||||
|
|
|
@ -113,7 +113,7 @@ const (
|
|||
|
||||
cfgLoggerTags = "logger.tags"
|
||||
cfgLoggerTagsPrefixTmpl = cfgLoggerTags + ".%d."
|
||||
cfgLoggerTagsNameTmpl = cfgLoggerTagsPrefixTmpl + "name"
|
||||
cfgLoggerTagsNameTmpl = cfgLoggerTagsPrefixTmpl + "names"
|
||||
cfgLoggerTagsLevelTmpl = cfgLoggerTagsPrefixTmpl + "level"
|
||||
|
||||
// Wallet.
|
||||
|
@ -516,8 +516,8 @@ func fetchLogTagsConfig(v *viper.Viper, defaultLvl zapcore.Level) (map[string]za
|
|||
res := make(map[string]zapcore.Level)
|
||||
|
||||
for i := 0; ; i++ {
|
||||
name := v.GetString(fmt.Sprintf(cfgLoggerTagsNameTmpl, i))
|
||||
if name == "" {
|
||||
tagNames := v.GetString(fmt.Sprintf(cfgLoggerTagsNameTmpl, i))
|
||||
if tagNames == "" {
|
||||
break
|
||||
}
|
||||
|
||||
|
@ -529,7 +529,12 @@ func fetchLogTagsConfig(v *viper.Viper, defaultLvl zapcore.Level) (map[string]za
|
|||
}
|
||||
}
|
||||
|
||||
res[name] = lvl
|
||||
for _, tagName := range strings.Split(tagNames, ",") {
|
||||
tagName = strings.TrimSpace(tagName)
|
||||
KurlesHS marked this conversation as resolved
Outdated
KurlesHS
commented
I think we should trim leading and trailing whitespace from all obtained tags because users may separate tags with commas and spaces, e.g., I think we should trim leading and trailing whitespace from all obtained tags because users may separate tags with commas and spaces, e.g., `app, datapath`.
KurlesHS
commented
We should also handle empty tags, like We should also handle empty tags, like `tag1,,tag2`, possibly by simply skipping them
|
||||
if len(tagName) != 0 {
|
||||
res[tagName] = lvl
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if len(res) == 0 && !v.IsSet(cfgLoggerTags) {
|
||||
|
|
|
@ -20,8 +20,9 @@ HTTP_GW_LOGGER_SAMPLING_ENABLED=false
|
|||
HTTP_GW_LOGGER_SAMPLING_INITIAL=100
|
||||
HTTP_GW_LOGGER_SAMPLING_THEREAFTER=100
|
||||
HTTP_GW_LOGGER_SAMPLING_INTERVAL=1s
|
||||
HTTP_GW_LOGGER_TAGS_0_NAME=app
|
||||
HTTP_GW_LOGGER_TAGS_1_NAME=datapath
|
||||
HTTP_GW_LOGGER_TAGS_0_NAMES=app,datapath
|
||||
HTTP_GW_LOGGER_TAGS_0_LEVEL=level
|
||||
HTTP_GW_LOGGER_TAGS_1_NAME=external_storage_tree
|
||||
|
||||
HTTP_GW_SERVER_0_ADDRESS=0.0.0.0:443
|
||||
HTTP_GW_SERVER_0_TLS_ENABLED=false
|
||||
|
|
|
@ -30,8 +30,7 @@ logger:
|
|||||
thereafter: 100
|
||||||
interval: 1s
|
||||||
tags:
|
||||||
- name: app
|
||||||
- name: datapath
|
||||||
- names: app,datapath
|
||||||
dkirillov marked this conversation as resolved
Outdated
dkirillov
commented
What about changing
? What about changing `config.env` https://git.frostfs.info/pogpp/frostfs-http-gw/src/commit/178cb19ea4b5651dd1a096f55b4e398fff52b51c/config/config.env#L24 ?
|
||||||
level: debug
|
||||||
|
||||||
server:
|
||||||
|
|
|
@ -176,10 +176,9 @@ logger:
|
|||
thereafter: 100
|
||||
interval: 1s
|
||||
tags:
|
||||
- name: "app"
|
||||
- names: "app,datapath"
|
||||
level: info
|
||||
- name: "datapath"
|
||||
- name: "external_storage_tree"
|
||||
- names: "external_storage_tree"
|
||||
```
|
||||
|
||||
| Parameter | Type | SIGHUP reload | Default value | Description |
|
||||
|
@ -199,14 +198,14 @@ parameter. Available tags:
|
|||
|
||||
```yaml
|
||||
tags:
|
||||
- name: "app"
|
||||
- names: "app,datapath"
|
||||
dkirillov marked this conversation as resolved
Outdated
dkirillov
commented
This change should be mentioned in CHANGELOG.md I suppose This change should be mentioned in CHANGELOG.md I suppose
|
||||
level: info
|
||||
```
|
||||
|
||||
| Parameter | Type | SIGHUP reload | Default value | Description |
|
||||
|-----------------------|------------|---------------|---------------------------|-------------------------------------------------------------------------------------------------------|
|
||||
| `name` | `string` | yes | | Tag name. Possible values see below in `Tag values` section. |
|
||||
| `level` | `string` | yes | Value from `logger.level` | Logging level for specific tag. Possible values: `debug`, `info`, `warn`, `dpanic`, `panic`, `fatal`. |
|
||||
| Parameter | Type | SIGHUP reload | Default value | Description |
|
||||
|-----------|------------|---------------|---------------------------|-------------------------------------------------------------------------------------------------------|
|
||||
| `names` | `[]string` | yes | | Tag names separated by `,`. Possible values see below in `Tag values` section. |
|
||||
| `level` | `string` | yes | Value from `logger.level` | Logging level for specific tag. Possible values: `debug`, `info`, `warn`, `dpanic`, `panic`, `fatal`. |
|
||||
|
||||
### Tag values
|
||||
|
||||
|
|
I suppose we need min level. Otherwise this #224 (comment) still doesn't work
tested on devenv - fixed