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

[#195] Add tags support #195

Merged
alexvanin merged 3 commits from KurlesHS/frostfs-http-gw:feat/add_tags_support into master 2025-02-07 11:47:39 +00:00
Conversation 1 Commits 3 Files changed 22 +572 -295
KurlesHS commented 2025-01-22 10:12:06 +00:00
Member
Copy link
  1. All log messages have been tagged.
  2. The ability to specify which tags to include in the log output has been added.
1. All log messages have been tagged. 2. The ability to specify which tags to include in the log output has been added.
KurlesHS added 1 commit 2025-01-22 10:12:06 +00:00
Add tags support
Some checks failed
/ DCO (pull_request) Failing after 33s
Details
/ Vulncheck (pull_request) Successful in 44s
Details
/ Builds (pull_request) Successful in 54s
Details
/ Lint (pull_request) Failing after 56s
Details
/ Tests (pull_request) Successful in 51s
Details
/ OCI image (pull_request) Successful in 1m28s
Details
09a2574155 
Signed-off-by: Aleksey Kravchenko <al.kravchenko@yadro.com>
requested reviews from storage-services-developers, storage-services-committers 2025-01-22 10:12:06 +00:00
KurlesHS changed title from [#XX] Add tags support to [#195] Add tags support 2025-01-22 10:12:28 +00:00
KurlesHS force-pushed feat/add_tags_support from 09a2574155 to 425ba2ef62 2025-01-22 10:14:57 +00:00 Compare
KurlesHS force-pushed feat/add_tags_support from 425ba2ef62 to 2ab44ad555 2025-01-22 10:17:59 +00:00 Compare
KurlesHS force-pushed feat/add_tags_support from 2ab44ad555 to 35e736257b 2025-01-22 10:19:00 +00:00 Compare
KurlesHS self-assigned this 2025-01-22 10:29:15 +00:00
nzinkevich reviewed 2025-01-23 06:23:37 +00:00
config/config.yaml Outdated
@ -23,0 +23,4 @@
tags:
- name: "app"
- name: "config"
- name: "datapath"
nzinkevich commented 2025-01-23 06:22:07 +00:00
Member
Copy link

tags block should be under logger

`tags` block should be under `logger`
👍 1
KurlesHS commented 2025-01-23 06:46:50 +00:00
Author
Member
Copy link

tags block should be under logger

oops...

> `tags` block should be under `logger` oops...
KurlesHS force-pushed feat/add_tags_support from 35e736257b to 7705035e8b 2025-01-23 06:45:38 +00:00 Compare
nzinkevich commented 2025-01-23 10:32:10 +00:00
Member
Copy link

We also should add config example in config.env (same way as HTTP_GW_SERVER_... does)

We also should add config example in `config.env` (same way as `HTTP_GW_SERVER_...` does)
👍 1
dkirillov reviewed 2025-01-23 12:42:45 +00:00
cmd/http-gw/app.go Outdated
@ -582,4 +641,3 @@
return
}
if lvl, err := getLogLevel(a.cfg); err != nil {
dkirillov commented 2025-01-23 12:40:04 +00:00
Member
Copy link

Why do we drop this? tagsConfig.update doesn't reload this param

Why do we drop this? `tagsConfig.update` doesn't reload this param
KurlesHS commented 2025-01-23 13:52:52 +00:00
Author
Member
Copy link

We no longer need the "app.logLevel" variable, so there is no point in reading the "logger.level" parameter in this place. However, in "fetchLogTagsConfig", the "logger.level" parameter is used to set the logging level for tags for which a level has not been explicitly set.

We no longer need the "app.logLevel" variable, so there is no point in reading the "logger.level" parameter in this place. However, in "fetchLogTagsConfig", the "logger.level" parameter is used to set the logging level for tags for which a level has not been explicitly set.
dkirillov commented 2025-01-24 11:56:34 +00:00
Member
Copy link

Well, we still have some untagged logs e.g.

Lines 600 to 601 in KurlesHS/frostfs-http-gw@a51358b
prm.SetLogger(a.log)
prmTree.SetLogger(a.log)


Line 1248 in TrueCloudLab/frostfs-sdk-go@9d7f7bd
c.logger.Log(level, msg, fields...)

So I would expect we still can affect their appearance by SIGHUP

Well, we still have some untagged logs e.g. https://git.frostfs.info/KurlesHS/frostfs-http-gw/src/commit/a51358bb169003dc40a9e79e5ef0bb9834d64914/cmd/http-gw/settings.go#L600-L601 https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/src/commit/9d7f7bd04f63cf373a50c7e88e815cb332b561b2/pool/pool.go#L1248 So I would expect we still can affect their appearance by SIGHUP
KurlesHS commented 2025-01-24 14:47:02 +00:00
Author
Member
Copy link

I decided to rename the tagsConfig structure to logLevelConfig and extend it with a new logLevel zap.AtomicLevel variable that will be responsible for the global logging level. I also updated the code in `tagsConfig.update' to reflect this change.

I decided to rename the `tagsConfig` structure to `logLevelConfig` and extend it with a new `logLevel zap.AtomicLevel` variable that will be responsible for the global logging level. I also updated the code in `tagsConfig.update' to reflect this change.
dkirillov commented 2025-01-27 06:55:25 +00:00
Member
Copy link

Probably we should do the same for other repositories to keep this logic consistent

Probably we should do the same for other repositories to keep this logic consistent
dkirillov commented 2025-01-27 13:56:00 +00:00
Member
Copy link

If something goes wrong with global log level we don't update tags config

Lines 151 to 154 in KurlesHS/frostfs-http-gw@22d7962
ll, err := getLogLevel(cfg)
if err != nil {
return err
}

I suppose we can use something like that:

type	logLevelConfig struct {
		logLevel   zap.AtomicLevel
		tagsConfig *tagsConfig
}

type	tagsConfig struct {
		tagLogs sync.Map
}


func newLogLevelConfig(lvl zap.AtomicLevel, tagsConfig *tagsConfig) *logLevelConfig {
	return &logLevelConfig{
		logLevel:   lvl,
		tagsConfig: tagsConfig,
	}
}

func (l *logLevelConfig) update(cfg *viper.Viper, log *zap.Logger) {
	if lvl, err := getLogLevel(cfg.GetString(cfgLoggerLevel)); err != nil {
		log.Warn(logs.LogLevelWontBeUpdated, zap.Error(err), logs.TagField(logs.TagConfig))
	} else {
		l.logLevel.SetLevel(lvl)
	}

	if err := l.tagsConfig.update(cfg); err != nil {
		log.Warn(logs.TagsLogConfigWontBeUpdated, zap.Error(err), logs.TagField(logs.TagConfig))
	}
}
If something goes wrong with global log level we don't update tags config https://git.frostfs.info/KurlesHS/frostfs-http-gw/src/commit/22d7962039c67ae2870875b6a5c3d22f2908aa7f/cmd/http-gw/app.go#L151-L154 I suppose we can use something like that: ```golang type logLevelConfig struct { logLevel zap.AtomicLevel tagsConfig *tagsConfig } type tagsConfig struct { tagLogs sync.Map } func newLogLevelConfig(lvl zap.AtomicLevel, tagsConfig *tagsConfig) *logLevelConfig { return &logLevelConfig{ logLevel: lvl, tagsConfig: tagsConfig, } } func (l *logLevelConfig) update(cfg *viper.Viper, log *zap.Logger) { if lvl, err := getLogLevel(cfg.GetString(cfgLoggerLevel)); err != nil { log.Warn(logs.LogLevelWontBeUpdated, zap.Error(err), logs.TagField(logs.TagConfig)) } else { l.logLevel.SetLevel(lvl) } if err := l.tagsConfig.update(cfg); err != nil { log.Warn(logs.TagsLogConfigWontBeUpdated, zap.Error(err), logs.TagField(logs.TagConfig)) } } ```
👍 1
dkirillov marked this conversation as resolved
cmd/http-gw/app.go Outdated
@ -52,3 +53,3 @@
ctx context.Context
log *zap.Logger
logLevel zap.AtomicLevel
tagsConfig *tagsConfig
dkirillov commented 2025-01-23 12:37:47 +00:00
Member
Copy link

Can we move this filed to appSettings?

Can we move this filed to `appSettings`?
👍 1
dkirillov marked this conversation as resolved
config/config.yaml Outdated
@ -30,2 +30,4 @@
thereafter: 100
interval: 1s
tags:
- name: "app"
dkirillov commented 2025-01-23 12:41:18 +00:00
Member
Copy link

Can we add the same examples to config.env also?

Can we add the same examples to `config.env` also?
👍 1
dkirillov marked this conversation as resolved
internal/logs/logs.go Outdated
@ -3,1 +16,4 @@
return zap.String(TagFieldName, tag)
}
const (
dkirillov commented 2025-01-23 12:42:39 +00:00
Member
Copy link

Let's group logs by their tags. See https://git.frostfs.info/TrueCloudLab/frostfs-s3-lifecycler/src/branch/master/internal/logs/logs.go

Let's group logs by their tags. See https://git.frostfs.info/TrueCloudLab/frostfs-s3-lifecycler/src/branch/master/internal/logs/logs.go
👍 1
dkirillov marked this conversation as resolved
KurlesHS force-pushed feat/add_tags_support from 7705035e8b to a51358bb16 2025-01-23 15:12:48 +00:00 Compare
KurlesHS force-pushed feat/add_tags_support from a51358bb16 to 22d7962039 2025-01-24 13:49:17 +00:00 Compare
dkirillov reviewed 2025-01-27 06:50:51 +00:00
cmd/http-gw/app.go Outdated
@ -983,2 +1049,3 @@
zap.Int64("new_value", softMemoryLimit),
zap.Int64("old_value", previous))
zap.Int64("old_value", previous),
logs.TagField(logs.TagApp))
dkirillov commented 2025-01-27 06:36:33 +00:00
Member
Copy link

Probably this should have config tag

Probably this should have `config` tag
👍 1
dkirillov marked this conversation as resolved
cmd/http-gw/settings.go Outdated
@ -563,3 +489,3 @@
}
l.Info(logs.SetCustomIndexPageTemplate)
l.Info(logs.SetCustomIndexPageTemplate, logs.TagField(logs.TagApp))
dkirillov commented 2025-01-27 06:38:04 +00:00
Member
Copy link

Maybe config tag is more appropriate to these three logs?

Maybe `config` tag is more appropriate to these three logs?
👍 1
dkirillov marked this conversation as resolved
internal/handler/browse.go Outdated
@ -274,3 +274,3 @@
if err != nil {
wg.Done()
log.Warn(logs.FailedToSumbitTaskToPool, zap.Error(err))
log.Warn(logs.FailedToSumbitTaskToPool, zap.Error(err), logs.TagField(logs.TagDatapath))
dkirillov commented 2025-01-27 06:39:51 +00:00
Member
Copy link

Probably this can have app tag. Anyway we should be consistent across other components

Probably this can have `app` tag. Anyway we should be consistent across other components
KurlesHS commented 2025-01-27 14:44:00 +00:00
Author
Member
Copy link

In the frostfs-s3-lifecycler service, such logic has been marked with the runtime tag. Maybe, in order to be consistent with other components, we should add this tag here despite the fact that there are no other messages for this tag? I've tagged this message as an app for now.

In the frostfs-s3-lifecycler service, such logic has been marked with the `runtime` tag. Maybe, in order to be consistent with other components, we should add this tag here despite the fact that there are no other messages for this tag? I've tagged this message as an `app` for now.
dkirillov marked this conversation as resolved
internal/handler/handler.go Outdated
@ -387,3 +390,3 @@
unescapedKey, err := url.QueryUnescape(oidURLParam)
if err != nil {
logAndSendBucketError(c, log, err)
logAndSendBucketError(c, log, err, logs.TagExternalStorage)
dkirillov commented 2025-01-27 06:48:20 +00:00
Member
Copy link

Why not datapath?

Why not `datapath`?
👍 1
dkirillov marked this conversation as resolved
internal/handler/upload.go Outdated
@ -60,3 +60,3 @@
bktInfo, err := h.getBucketInfo(ctx, scid, log)
if err != nil {
logAndSendBucketError(c, log, err)
logAndSendBucketError(c, log, err, logs.TagExternalStorageTree)
dkirillov commented 2025-01-27 06:49:25 +00:00
Member
Copy link

Why not external_storage or datapath?

Why not `external_storage` or `datapath`?
👍 1
dkirillov marked this conversation as resolved
internal/handler/utils.go Outdated
@ -79,3 +80,2 @@
func logAndSendBucketError(c *fasthttp.RequestCtx, log *zap.Logger, err error) {
log.Error(logs.CouldntGetBucket, zap.Error(err))
func logAndSendBucketError(c *fasthttp.RequestCtx, log *zap.Logger, err error, tag string) {
dkirillov commented 2025-01-27 06:46:32 +00:00
Member
Copy link

As alternative we could invoke this function like:

logAndSendBucketError(c, log.With(logs.TagField(logs.TagExternalStorage)), err)
As alternative we could invoke this function like: ```golang logAndSendBucketError(c, log.With(logs.TagField(logs.TagExternalStorage)), err) ```
👍 1
dkirillov marked this conversation as resolved
internal/logs/logs.go Outdated
@ -97,0 +17,4 @@
}
const (
ServiceIsRunning = "service is running" // Info in ../../metrics/service.go
dkirillov commented 2025-01-27 06:32:02 +00:00
Member
Copy link

Let's drop all comments

Let's drop all comments
👍 1
dkirillov commented 2025-01-27 06:33:32 +00:00
Member
Copy link

Probably we can add other comments (just to mention for which tag every group of logs belong)

Probably we can add other comments (just to mention for which tag every group of logs belong)
👍 1
dkirillov marked this conversation as resolved
internal/logs/logs.go Outdated
@ -97,0 +118,4 @@
)
const (
WrongObjectID = "wrong object id" // Error in ../../downloader/download.go
dkirillov commented 2025-01-27 06:44:59 +00:00
Member
Copy link

If this messages are unused we should drop it

If this messages are unused we should drop it
👍 1
dkirillov marked this conversation as resolved
internal/net/event_handler.go Outdated
@ -21,2 +22,3 @@
} else {
l.logger.Debug(logs.MultinetDialFail, zap.String("source", sourceIPString), zap.String("destination", address), zap.Error(err))
l.logger.Debug(logs.MultinetDialFail, zap.String("source", sourceIPString),
zap.String("destination", address), zap.Error(err), logs.TagField(logs.TagDatapath))
dkirillov commented 2025-01-27 06:50:37 +00:00
Member
Copy link

These logs should have app tag

These logs should have `app` tag
👍 1
dkirillov marked this conversation as resolved
dkirillov reviewed 2025-01-27 06:53:19 +00:00
cmd/http-gw/app.go Outdated
@ -96,6 +96,7 @@ type (
workerPoolSize int
mu sync.RWMutex
logLevelConfig *logLevelConfig
dkirillov commented 2025-01-27 06:53:19 +00:00
Member
Copy link

It seem we don't use mu to protect logLevelConfig so this field should be placed above mu

It seem we don't use `mu` to protect `logLevelConfig` so this field should be placed above `mu`
👍 1
dkirillov marked this conversation as resolved
KurlesHS force-pushed feat/add_tags_support from 22d7962039 to 83d14a6b38 2025-01-27 15:20:51 +00:00 Compare
KurlesHS force-pushed feat/add_tags_support from 83d14a6b38 to 1679da60b3 2025-01-28 12:27:08 +00:00 Compare
KurlesHS force-pushed feat/add_tags_support from 1679da60b3 to d97dd52939 2025-01-28 16:12:37 +00:00 Compare
dkirillov approved these changes 2025-01-30 08:00:49 +00:00
Dismissed
dkirillov left a comment
Member
Copy link

LGTM

LGTM
alexvanin added this to the v0.33.0 milestone 2025-02-04 13:22:41 +00:00
KurlesHS force-pushed feat/add_tags_support from d97dd52939 to eb5d1b40d6 2025-02-05 07:55:03 +00:00 Compare
KurlesHS dismissed dkirillov's review 2025-02-05 07:55:03 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

KurlesHS force-pushed feat/add_tags_support from eb5d1b40d6 to ec629ffb65 2025-02-05 07:58:35 +00:00 Compare
KurlesHS force-pushed feat/add_tags_support from ec629ffb65 to 436004ae0b 2025-02-06 11:05:28 +00:00 Compare
KurlesHS reviewed 2025-02-06 11:43:54 +00:00
internal/handler/handler.go Outdated
@ -331,2 +335,4 @@
cnrID, err := h.resolveContainer(ctx, containerName)
if err != nil {
log.Error(logs.CouldNotResolveContainerID, zap.Error(err), zap.String("cnrName", containerName),
logs.TagField(logs.TagDatapath))
KurlesHS commented 2025-02-06 11:41:11 +00:00
Author
Member
Copy link

The datapathtag is used because both DNS and NNS resolvers can be used here.

The `datapath`tag is used because both `DNS` and `NNS` resolvers can be used here.
KurlesHS force-pushed feat/add_tags_support from 436004ae0b to 57886d562f 2025-02-06 11:58:02 +00:00 Compare
dkirillov reviewed 2025-02-07 05:05:36 +00:00
config/config.yaml Outdated
@ -31,1 +31,4 @@
interval: 1s
tags:
- name: app
- name: config
dkirillov commented 2025-02-07 05:04:48 +00:00
Member
Copy link

We don't have this section anymore

We don't have this section anymore
👍 1
dkirillov marked this conversation as resolved
KurlesHS force-pushed feat/add_tags_support from 57886d562f to 6ca273e3ee 2025-02-07 06:05:51 +00:00 Compare
dkirillov approved these changes 2025-02-07 09:41:55 +00:00
Dismissed
alexvanin added 2 commits 2025-02-07 11:05:41 +00:00 
Signed-off-by: Alex Vanin <a.vanin@yadro.com>
[#195] Fix log record grouping
Some checks failed
/ DCO (pull_request) Successful in 32s
Details
/ Vulncheck (pull_request) Failing after 57s
Details
/ Builds (pull_request) Successful in 1m11s
Details
/ OCI image (pull_request) Successful in 1m12s
Details
/ Lint (pull_request) Successful in 2m8s
Details
/ Tests (pull_request) Successful in 1m14s
Details
/ Integration tests (pull_request) Successful in 5m55s
Details
6613a9f06c 
Signed-off-by: Alex Vanin <a.vanin@yadro.com>
alexvanin dismissed dkirillov's review 2025-02-07 11:05:41 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

alexvanin approved these changes 2025-02-07 11:06:06 +00:00
Dismissed
dkirillov reviewed 2025-02-07 11:24:27 +00:00
docs/gate-configuration.md Outdated
@ -187,0 +212,4 @@
* `app` - common application logs (enabled by default).
* `datapath` - main logic of application (enabled by default).
* `external_storage` - external interaction with storage node.
* `external_storage_tree` - external interaction with tree service in storage node.
dkirillov commented 2025-02-07 11:24:27 +00:00
Member
Copy link

We should also mention the default tags here

We should also mention the default tags here
👍 1
dkirillov marked this conversation as resolved
KurlesHS force-pushed feat/add_tags_support from 6613a9f06c to 6f94727ab6 2025-02-07 11:35:25 +00:00 Compare
KurlesHS dismissed alexvanin's review 2025-02-07 11:35:25 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

dkirillov approved these changes 2025-02-07 11:43:29 +00:00
alexvanin merged commit c509ce0b28 into master 2025-02-07 11:47:39 +00:00
alexvanin referenced this pull request from a commit 2025-02-07 11:47:40 +00:00
[#195] Add tags support
alexvanin referenced this pull request from a commit 2025-02-07 11:47:41 +00:00
[#195] Fix log record grouping
alexvanin deleted branch feat/add_tags_support 2025-02-07 11:47:41 +00:00
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
v0.33.0
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
KurlesHS
4 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#195
No description provided.
Delete branch "KurlesHS/frostfs-http-gw:feat/add_tags_support"

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?