TrueCloudLab/frostfs-http-gw
17
1
Fork 18
Code Issues 9 Pull requests 2 Projects Releases 16 Activity Actions

[#224] Refactor logger tag configuration #224

Open
pogpp wants to merge 1 commit from pogpp/frostfs-http-gw:feature/improve_logs into master
pull from: pogpp/frostfs-http-gw:feature/improve_logs
merge into: TrueCloudLab:master
Conversation 0 Commits 1 Files changed 7 +61 -26
pogpp commented 2025-03-21 10:39:35 +00:00
Member
Copy link

Signed-off-by: Pavel Pogodaev p.pogodaev@yadro.com

Signed-off-by: Pavel Pogodaev <p.pogodaev@yadro.com>
pogpp added 1 commit 2025-03-21 10:39:36 +00:00
[] Refactor logger tag configuration
Some checks failed
/ DCO (pull_request) Failing after 33s
Details
/ Vulncheck (pull_request) Successful in 56s
Details
/ Builds (pull_request) Successful in 1m2s
Details
/ OCI image (pull_request) Successful in 1m21s
Details
/ Lint (pull_request) Successful in 2m51s
Details
/ Tests (pull_request) Successful in 1m43s
Details
/ Integration tests (pull_request) Successful in 7m17s
Details
71a9794222 
Signed-off-by: Pavel Pogodaev <p.pogodaev@yadro.com>
requested reviews from storage-services-developers, storage-services-committers 2025-03-21 10:39:36 +00:00
pogpp changed title from [] Refactor logger tag configuration to [#224] Refactor logger tag configuration 2025-03-21 10:39:55 +00:00
pogpp force-pushed feature/improve_logs from 71a9794222 to 4cc2c3f9ba 2025-03-21 10:40:48 +00:00 Compare
pogpp force-pushed feature/improve_logs from 4cc2c3f9ba to 6608423f74 2025-03-21 10:41:45 +00:00 Compare
pogpp was assigned by dkirillov 2025-03-21 11:28:12 +00:00
KurlesHS reviewed 2025-03-21 11:41:33 +00:00
cmd/http-gw/settings.go Outdated
@ -531,2 +531,3 @@
res[name] = lvl
for _, tagName := range strings.Split(tagNames, ",") {
res[tagName] = lvl
KurlesHS commented 2025-03-21 11:41:33 +00:00
Member
Copy link

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`.
👍 1
KurlesHS commented 2025-03-21 11:55:37 +00:00
Member
Copy link

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
👍 1
KurlesHS marked this conversation as resolved
pogpp force-pushed feature/improve_logs from 6608423f74 to 178cb19ea4 2025-03-21 12:38:27 +00:00 Compare
dkirillov requested changes 2025-03-21 13:14:38 +00:00
dkirillov left a comment
Member
Copy link

Actually such config:

logger:
  level: fatal
  tags:
    - names: app
      level: debug

Doesn't work as expected now. It won't print any message it their log level isn't fatal. But we want to print logs with app tag that have level debug

Actually such config: ```yaml logger: level: fatal tags: - names: app level: debug ``` Doesn't work as expected now. It won't print any message it their log level isn't fatal. But we want to print logs with `app` tag that have level `debug`
config/config.yaml Outdated
@ -32,3 +32,2 @@
tags:
- name: app
- name: datapath
- names: app,datapath
dkirillov commented 2025-03-21 13:10:39 +00:00
Member
Copy link

What about changing config.env

Line 24 in pogpp/frostfs-http-gw@178cb19
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 ?
👍 1
dkirillov marked this conversation as resolved
pogpp force-pushed feature/improve_logs from 178cb19ea4 to 38856cc81f 2025-03-24 08:36:44 +00:00 Compare
pogpp force-pushed feature/improve_logs from 38856cc81f to 8583dffb7c 2025-03-24 08:42:53 +00:00 Compare
pogpp force-pushed feature/improve_logs from 8583dffb7c to 65e0ac354a 2025-03-24 08:54:13 +00:00 Compare
pogpp force-pushed feature/improve_logs from 65e0ac354a to 81e525d6df 2025-03-24 10:38:09 +00:00 Compare
pogpp force-pushed feature/improve_logs from 81e525d6df to 06db52e53d 2025-03-24 12:25:33 +00:00 Compare
pogpp force-pushed feature/improve_logs from 06db52e53d to 901bf4171d 2025-03-24 13:59:49 +00:00 Compare
dkirillov requested changes 2025-03-24 14:27:58 +00:00
cmd/http-gw/app.go Outdated
@ -119,10 +119,15 @@ type (
logLevelConfig struct {
logLevel zap.AtomicLevel
maxLevel zap.AtomicLevel
dkirillov commented 2025-03-24 14:23:38 +00:00
Member
Copy link

Why do we need new field? Can we reuse existing one?

Why do we need new field? Can we reuse existing one?
👍 1
dkirillov marked this conversation as resolved
cmd/http-gw/app.go Outdated
@ -149,3 +156,4 @@
}
}
func maxLogLevel(lvl zap.AtomicLevel, config *tagsConfig) zap.AtomicLevel {
dkirillov commented 2025-03-24 14:20:16 +00:00
Member
Copy link

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
pogpp commented 2025-03-25 09:30:50 +00:00
Author
Member
Copy link

tested on devenv - fixed

tested on devenv - fixed
dkirillov marked this conversation as resolved
cmd/http-gw/app.go Outdated
@ -204,3 +226,3 @@
tagConfig := newTagsConfig(cfg.config(), logLevel.Level())
logConfig := newLogLevelConfig(logLevel, tagConfig)
log := pickLogger(cfg.config(), logConfig.logLevel, logSettings, tagConfig)
log := pickLogger(cfg.config(), *logConfig, logSettings, tagConfig)
dkirillov commented 2025-03-24 14:26:37 +00:00
Member
Copy link

Actually we can don't change this if logConfig.lovLevel be already computed min log level

Actually we can don't change this if `logConfig.lovLevel` be already computed min log level
👍 1
dkirillov marked this conversation as resolved
docs/gate-configuration.md Outdated
@ -200,3 +199,3 @@
```yaml
tags:
- name: "app"
- names: "app,datapath"
dkirillov commented 2025-03-24 14:16:43 +00:00
Member
Copy link

This change should be mentioned in CHANGELOG.md I suppose

This change should be mentioned in CHANGELOG.md I suppose
👍 1
dkirillov marked this conversation as resolved
KurlesHS reviewed 2025-03-25 06:50:52 +00:00
cmd/http-gw/logger.go Outdated
@ -73,24 +79,21 @@ func (c *zapCoreTagFilterWrapper) Write(entry zapcore.Entry, fields []zapcore.Fi
func (c *zapCoreTagFilterWrapper) shouldSkip(entry zapcore.Entry, fields []zap.Field) bool {
KurlesHS commented 2025-03-25 06:50:52 +00:00
Member
Copy link

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.
👍 1
pogpp force-pushed feature/improve_logs from 901bf4171d to a833607526 2025-03-25 09:30:01 +00:00 Compare
pogpp force-pushed feature/improve_logs from a833607526 to 0117adf231 2025-03-25 09:43:38 +00:00 Compare
pogpp force-pushed feature/improve_logs from 0117adf231 to c9d2ad947c 2025-03-25 09:44:14 +00:00 Compare
pogpp force-pushed feature/improve_logs from c9d2ad947c to 5800d48112 2025-03-25 15:06:19 +00:00 Compare
pogpp force-pushed feature/improve_logs from 5800d48112 to ce12a389c9 2025-03-26 13:57:55 +00:00 Compare
dkirillov reviewed 2025-03-27 11:03:03 +00:00
cmd/http-gw/app.go Outdated
@ -160,2 +174,4 @@
log.Warn(logs.TagsLogConfigWontBeUpdated, zap.Error(err), logs.TagField(logs.TagApp))
}
l.logLevel.SetLevel(minLogLevel(zap.NewAtomicLevelAt(lvl), l.tagsConfig).Level())
dkirillov commented 2025-03-27 09:31:37 +00:00
Member
Copy link

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) { ```
👍 1
cmd/http-gw/app.go Outdated
@ -162,3 +178,3 @@
}
func (t *tagsConfig) LevelEnabled(tag string, tgtLevel zapcore.Level) bool {
func (t *tagsConfig) LevelEnabled(tag string, tgtLevel zapcore.Level) (bool, bool) {
dkirillov commented 2025-03-27 10:58:40 +00:00
Member
Copy link

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) } ```
👍 1
cmd/http-gw/app.go Outdated
@ -171,0 +186,4 @@
return lvl.(zapcore.Level).Enabled(tgtLevel), true
}
func (t *tagsConfig) DefaultLevel() zap.AtomicLevel {
dkirillov commented 2025-03-27 11:00:36 +00:00
Member
Copy link

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) } ```
👍 1
cmd/http-gw/app.go Outdated
@ -194,6 +214,7 @@ func (t *tagsConfig) update(cfg *viper.Viper, ll zapcore.Level) error {
for k, v := range tags {
t.tagLogs.Store(k, v)
}
t.defaultLvl = zap.NewAtomicLevelAt(ll)
dkirillov commented 2025-03-27 09:34:17 +00:00
Member
Copy link

Should be

t.defaultLvl.SetLevel(ll)
Should be ```golang t.defaultLvl.SetLevel(ll) ```
pogpp commented 2025-03-28 11:34:46 +00:00
Author
Member
Copy link

In such scenario we will have panic on integration test

In such scenario we will have panic on integration test
cmd/http-gw/logger.go Outdated
@ -67,0 +71,4 @@
}
return c.core.Write(entry, fields)
}
shouldSkip, tagFound = c.shouldSkip(entry, c.extra)
dkirillov commented 2025-03-27 11:01:33 +00:00
Member
Copy link

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

Probably we can pass both (`fields`, `c.extra`) to `c.shouldSkip`
👍 1
pogpp force-pushed feature/improve_logs from ce12a389c9 to d94fa7add6 2025-03-28 11:33:45 +00:00 Compare
All checks were successful
/ DCO (pull_request) Successful in 31s
Required
Details
/ Builds (pull_request) Successful in 1m2s
Required
Details
/ Vulncheck (pull_request) Successful in 55s
Required
Details
/ OCI image (pull_request) Successful in 1m20s
Required
Details
/ Lint (pull_request) Successful in 2m3s
Required
Details
/ Tests (pull_request) Successful in 1m2s
Required
Details
/ Integration tests (pull_request) Successful in 5m42s
Required
Details
This pull request doesn't have enough approvals yet. 0 of 2 approvals granted.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u feature/improve_logs:pogpp-feature/improve_logs
git checkout pogpp-feature/improve_logs
Sign in to join this conversation.
Reviewers
No reviewers
dkirillov
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
good first issue

Good for newcomers

  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

No labels
P0
P1
P2
P3
good first issue
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
pogpp
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-http-gw#224
No description provided.
Delete branch "pogpp/frostfs-http-gw:feature/improve_logs"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?