[#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 62 additions and 27 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 {
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))
}
l.setMinLogLevel()
}
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)
}
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)
}
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,20 +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) {
Review

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

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

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
Review
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
}
if !c.settings.DefaultEnabled(entry.Level) {
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 {
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)
}
}
for _, field := range extra {
if field.Key == logs.TagFieldName && field.Type == zapcore.StringType {
return c.settings.LevelEnabled(field.String, entry.Level)
}
}

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