[#224] Refactor logger tag configuration #224

Open
pogpp wants to merge 1 commit from pogpp/frostfs-http-gw:feature/improve_logs into master
7 changed files with 59 additions and 28 deletions

View file

@ -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

View file

@ -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

I suppose we need min level. Otherwise this #224 (comment) still doesn't work

I suppose we need min level. Otherwise this https://git.frostfs.info/TrueCloudLab/frostfs-http-gw/pulls/224#issuecomment-71179 still doesn't work
Outdated
Review

tested on devenv - fixed

tested on devenv - fixed
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

I suggest this

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) {

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

We can don't change signature. If we write

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)
}
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

I would return not atomic, but just level

func (t *tagsConfig) DefaultLevel() zapcore.Level {
	return t.defaultLvl.Level()
}

or don't return level but accept it (similart to levelEnabled)

func (t *tagsConfig) DefaultEnabled(lvl zapcore.Level) bool {
	return t.defaultLvl.Enabled(lvl)
}
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

Should be

t.defaultLvl.SetLevel(ll)
Should be ```golang t.defaultLvl.SetLevel(ll) ```
Outdated
Review

In such scenario we will have panic on integration test

In such scenario we will have panic on integration test

Why so?

Why so?

Using t.defaultLvl = zap.NewAtomicLevelAt(ll) leads to data race

Using `t.defaultLvl = zap.NewAtomicLevelAt(ll)` leads to data race
}
t.defaultLvl.SetLevel(ll)
return nil
}

View file

@ -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

Why !c.shouldSkip? It seems we still need c.shouldSkip

Why `!c.shouldSkip`? It seems we still need `c.shouldSkip`

Oh, I see, you've changed

return c.settings.LevelEnabled(field.String, entry.Level)

But then we still have false

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

return c.settings.LevelEnabled(field.String, entry.Level)


and above.

and don't add ! here

if !c.shouldSkip(entry, fields, c.extra) {

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
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 {

```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

Probably we can pass both (fields, c.extra) to c.shouldSkip

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)
}
}

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.

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 {

View file

@ -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

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.

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`.

We should also handle empty tags, like tag1,,tag2, possibly by simply skipping them

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) {

View file

@ -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

View file

@ -30,8 +30,7 @@ logger:
thereafter: 100
interval: 1s
tags:
- name: app
- name: datapath
- names: app,datapath
dkirillov marked this conversation as resolved Outdated

What about changing config.env

HTTP_GW_LOGGER_TAGS_1_NAME=datapath

?

What about changing `config.env` https://git.frostfs.info/pogpp/frostfs-http-gw/src/commit/178cb19ea4b5651dd1a096f55b4e398fff52b51c/config/config.env#L24 ?
level: debug
server:

View file

@ -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

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