[#606] Support log tagging #606

Merged
alexvanin merged 5 commits from nzinkevich/frostfs-s3-gw:feature/log_tagging into master 2025-02-11 15:15:43 +00:00
Member

Signed-off-by: Nikita Zinkevich n.zinkevich@yadro.com

Signed-off-by: Nikita Zinkevich <n.zinkevich@yadro.com>
nzinkevich self-assigned this 2025-01-23 10:47:27 +00:00
nzinkevich added 1 commit 2025-01-23 10:47:28 +00:00
[#XX] Support log tagging
Some checks failed
/ DCO (pull_request) Failing after 34s
/ Builds (pull_request) Successful in 1m15s
/ Vulncheck (pull_request) Successful in 1m14s
/ Lint (pull_request) Failing after 1m13s
/ Tests (pull_request) Successful in 1m9s
/ OCI image (pull_request) Successful in 2m16s
cc4db2cf96
Signed-off-by: Nikita Zinkevich <n.zinkevich@yadro.com>
requested reviews from storage-services-developers, storage-services-committers 2025-01-23 10:47:28 +00:00
nzinkevich changed title from [#XX] Support log tagging to [#606] Support log tagging 2025-01-23 10:47:48 +00:00
nzinkevich force-pushed feature/log_tagging from cc4db2cf96 to 94b1c6ffd0 2025-01-23 10:48:13 +00:00 Compare
nzinkevich force-pushed feature/log_tagging from 94b1c6ffd0 to fc6e4423cd 2025-01-23 11:59:27 +00:00 Compare
dkirillov reviewed 2025-01-24 13:50:51 +00:00
@ -302,3 +302,3 @@
args.IfUnmodifiedSince = httpTime
} else {
log.Warn(logs.FailedToParseHTTPTime, zap.String(api.IfUnmodifiedSince, headers.Get(api.IfUnmodifiedSince)), zap.Error(err))
log.Warn(logs.FailedToParseHTTPTime, zap.String(api.IfUnmodifiedSince, headers.Get(api.IfUnmodifiedSince)), zap.Error(err), logs.TagField(logs.TagApp))
Member

I would mark this as datapath

I would mark this as `datapath`
dkirillov marked this conversation as resolved
@ -94,2 +94,3 @@
zap.String("cnrID", corsBkt.CID.EncodeToString()),
zap.String("objID", addr.Object().EncodeToString()))
zap.String("objID", addr.Object().EncodeToString()),
logs.TagField(logs.TagExternalStorageTree))
Member

Tag should be logs.TagExternalStorage I suppose

Tag should be `logs.TagExternalStorage` I suppose
dkirillov marked this conversation as resolved
@ -852,3 +854,3 @@
}
n.reqLogger(ctx).Info(logs.ResolveBucket, zap.Stringer("cid", cnrID))
n.reqLogger(ctx).Info(logs.ResolveBucket, zap.Stringer("cid", cnrID), logs.TagField(logs.TagExternalStorageTree))
Member

Should we use external_blockain tag here?

Should we use `external_blockain` tag here?
dkirillov marked this conversation as resolved
@ -81,2 +81,3 @@
zap.String("cid", lifecycleBkt.CID.EncodeToString()),
zap.String("oid", addr.Object().EncodeToString()))
zap.String("oid", addr.Object().EncodeToString()),
logs.TagField(logs.TagExternalStorageTree),
Member

Probably it would be better to use external_storage

Probably it would be better to use `external_storage`
dkirillov marked this conversation as resolved
@ -213,3 +213,3 @@
encInfo := FormEncryptionInfo(multipartInfo.Meta)
if err := p.Info.Encryption.MatchObjectEncryption(encInfo); err != nil {
n.reqLogger(ctx).Warn(logs.MismatchedObjEncryptionInfo, zap.Error(err))
n.reqLogger(ctx).Warn(logs.MismatchedObjEncryptionInfo, zap.Error(err), logs.TagField(logs.TagApp))
Member

Should it be datapath?

Should it be `datapath`?
dkirillov marked this conversation as resolved
@ -291,3 +297,3 @@
n.reqLogger(ctx).Debug(logs.UploadPart,
zap.String("multipart upload", p.Info.UploadID), zap.Int("part number", p.PartNumber),
zap.Stringer("cid", bktInfo.CID), zap.Stringer("oid", createdObj.ID))
zap.Stringer("cid", bktInfo.CID), zap.Stringer("oid", createdObj.ID), logs.TagField(logs.TagExternalStorage))
Member

Probably this should be datapath

Probably this should be `datapath`
dkirillov marked this conversation as resolved
@ -481,2 +488,3 @@
zap.String("uploadKey", p.Info.Key),
zap.Error(err))
zap.Error(err),
logs.TagField(logs.TagExternalStorage))
Member

Probably this can be datapath

Probably this can be `datapath`
@ -299,0 +298,4 @@
n.reqLogger(ctx).Debug(logs.FailedToDeleteObject,
zap.Stringer("cid", p.BktInfo.CID),
zap.Stringer("oid", createdObj.ID),
logs.TagField(logs.TagApp),
Member

Why not external_storage?

Why not `external_storage`?
dkirillov marked this conversation as resolved
@ -313,0 +316,4 @@
n.reqLogger(ctx).Debug(logs.FailedToDeleteObject,
zap.Stringer("cid", p.BktInfo.CID),
zap.Stringer("oid", createdObj.ID),
logs.TagField(logs.TagApp))
Member

Why not external_storage?

Why not `external_storage`?
dkirillov marked this conversation as resolved
@ -140,3 +140,3 @@
containerID, err := resolveContainerID(ctx, reqInfo.BucketName)
if err != nil {
reqLogOrDefault(ctx, log).Debug(logs.FailedToResolveCID, zap.Error(err))
reqLogOrDefault(ctx, log).Debug(logs.FailedToResolveCID, zap.Error(err), logs.TagField(logs.TagApp))
Member

external_blockchain?

`external_blockchain`?
dkirillov marked this conversation as resolved
@ -202,2 +202,3 @@
zap.String("resource", res), zap.Any("request properties", requestProps),
zap.Any("resource properties", resourceProps))
zap.Any("resource properties", resourceProps),
logs.TagField(logs.TagApp),
Member

Should we use datapath tag?

Should we use `datapath` tag?
dkirillov marked this conversation as resolved
cmd/s3-gw/app.go Outdated
@ -697,16 +747,17 @@ func (a *App) initTracing(ctx context.Context) {
Version: version.Version,
}
log := a.log.With(logs.TagField(logs.TagConfig))
Member

I would use app tag

I would use `app` tag
dkirillov marked this conversation as resolved
cmd/s3-gw/app.go Outdated
@ -799,2 +851,2 @@
prm.SetLogger(a.log)
prmTree.SetLogger(a.log)
prm.SetLogger(log)
prmTree.SetLogger(log)
Member

Actually, I'm not sure if we need to pass app tagged logger to pool. Probably we need separate tag for that

Actually, I'm not sure if we need to pass `app` tagged logger to pool. Probably we need separate tag for that
Author
Member

Why not using external_storage_*?

Why not using `external_storage_*`?
dkirillov marked this conversation as resolved
@ -435,1 +435,3 @@
l.Fatal(logs.FailedToParseDefaultDefaultLocationConstraint, zap.String("policy", defaultPlacementPolicy))
l.Fatal(logs.FailedToParseDefaultDefaultLocationConstraint,
zap.String("policy", defaultPlacementPolicy),
logs.TagField(logs.TagConfig),
Member

Probably any Fatal must be app tagged

@alexvanin what do you think?

Probably any `Fatal` must be `app` tagged @alexvanin what do you think?
dkirillov marked this conversation as resolved
@ -509,3 +518,3 @@
regionPolicyMap, err := readRegionMap(filepath)
if err != nil {
l.Warn(logs.FailedToReadRegionMapFilePolicies, zap.String("file", filepath), zap.Error(err))
l.Warn(logs.FailedToReadRegionMapFilePolicies, zap.String("file", filepath), zap.Error(err), logs.TagField(logs.TagApp))
Member

This should have config tag. In other places too.

Add app tag to logs that initialize something app related (not warn/info about config value/parsing etc).

This should have `config` tag. In other places too. Add `app` tag to logs that initialize something app related (not warn/info about config value/parsing etc).
dkirillov marked this conversation as resolved
@ -636,3 +649,3 @@
if len(defaultNamespaces) == 0 {
defaultNamespaces = defaultDefaultNamespaces
l.Warn(logs.DefaultNamespacesCannotBeEmpty, zap.Strings("namespaces", defaultNamespaces))
l.Warn(logs.DefaultNamespacesCannotBeEmpty, zap.Strings("namespaces", defaultNamespaces), logs.TagField(logs.TagApp))
Member

This should have config tag.

This should have `config` tag.
dkirillov marked this conversation as resolved
@ -660,3 +673,3 @@
nsConfig, err := readNamespacesConfig(v.GetString(cfgNamespacesConfig))
if err != nil {
l.Warn(logs.FailedToParseNamespacesConfig, zap.Error(err))
l.Warn(logs.FailedToParseNamespacesConfig, zap.Error(err), logs.TagField(logs.TagApp))
Member

This should have config tag.

This should have `config` tag.
dkirillov marked this conversation as resolved
@ -674,3 +687,3 @@
if len(overrideDefaults) > 0 {
l.Warn(logs.DefaultNamespaceConfigValuesBeOverwritten)
l.Warn(logs.DefaultNamespaceConfigValuesBeOverwritten, logs.TagField(logs.TagApp))
Member

This should have config tag.

This should have `config` tag.
dkirillov marked this conversation as resolved
@ -721,3 +734,3 @@
if address == "" {
l.Warn(logs.SkipEmptyAddress)
l.Warn(logs.SkipEmptyAddress, logs.TagField(logs.TagApp))
Member

This should have config tag

This should have `config` tag
dkirillov marked this conversation as resolved
@ -737,2 +750,3 @@
zap.String("address", address),
zap.Float64("weight", weight))
zap.Float64("weight", weight),
logs.TagField(logs.TagApp),
Member

This should have config tag

This should have `config` tag
dkirillov marked this conversation as resolved
@ -3,1 +16,4 @@
return zap.String(TagFieldName, tag)
}
const (
Member
Please group log message by tag groups https://git.frostfs.info/TrueCloudLab/frostfs-s3-lifecycler/src/branch/master/internal/logs/logs.go
dkirillov marked this conversation as resolved
nzinkevich force-pushed feature/log_tagging from fc6e4423cd to f4901b830f 2025-01-30 07:03:09 +00:00 Compare
nzinkevich force-pushed feature/log_tagging from f4901b830f to 14458cfdaf 2025-01-30 08:18:42 +00:00 Compare
nzinkevich force-pushed feature/log_tagging from 14458cfdaf to 2f4f60b2ff 2025-01-30 08:47:34 +00:00 Compare
dkirillov requested changes 2025-02-04 09:24:53 +00:00
dkirillov left a comment
Member

Missed logs

Missed logs * https://git.frostfs.info/nzinkevich/frostfs-s3-gw/src/commit/2f4f60b2ff9e707d443862bba782e8718341a341/api/layer/multipart_upload.go#L619 * https://git.frostfs.info/nzinkevich/frostfs-s3-gw/src/commit/2f4f60b2ff9e707d443862bba782e8718341a341/api/handler/patch.go#L145 *
@ -109,3 +109,3 @@
bktInfo, err := h.getBucketInfo(ctx, reqInfo.BucketName)
if err != nil {
h.reqLogger(ctx).Warn(logs.GetBucketInfo, zap.Error(err))
h.reqLogger(ctx).Warn(logs.GetBucketInfo, zap.Error(err), logs.TagField(logs.TagExternalBlockchain))
Member

Probably external_object_storage be better

Probably `external_object_storage` be better
Member

I still think we should use external_object_storage tag here

I still think we should use `external_object_storage` tag here
@ -545,2 +545,3 @@
zap.Stringer("cid", p.BktInfo.CID),
zap.Stringer("oid", objInfo.ObjectInfo.ID))
zap.Stringer("oid", objInfo.ObjectInfo.ID),
logs.TagField(logs.TagExternalStorageTree),
Member

I would use datapath

I would use `datapath`
dkirillov marked this conversation as resolved
cmd/s3-gw/app.go Outdated
@ -816,3 +868,3 @@
p, err := pool.NewPool(prm)
if err != nil {
a.log.Fatal(logs.FailedToCreateConnectionPool, zap.Error(err))
log.Fatal(logs.FailedToCreateConnectionPool, zap.Error(err), logs.TagField(logs.TagApp))
Member

Please, don't use log package. Use a.log

Please, don't use `log` package. Use `a.log`
dkirillov marked this conversation as resolved
@ -671,3 +695,3 @@
copiesNums[constraint] = vector32
l.Info(logs.ConstraintAdded, zap.String("location", constraint), zap.Strings("copies numbers", vector))
l.Info(logs.ConstraintAdded, zap.String("location", constraint), zap.Strings("copies numbers", vector), logs.TagField(logs.TagApp))
Member

It's better to use config tag

It's better to use `config` tag
dkirillov marked this conversation as resolved
@ -60,6 +60,12 @@ logger:
initial: 100
thereafter: 100
interval: 1s
tags:
Member
We should add new param to docs https://git.frostfs.info/nzinkevich/frostfs-s3-gw/src/branch/feature/log_tagging/docs/configuration.md
dkirillov marked this conversation as resolved
@ -540,3 +540,3 @@
ind := latest.GetLatestNodeIndex()
if latest.IsSplit() {
c.reqLogger(ctx).Error(logs.BucketSettingsNodeHasMultipleIDs, zap.Uint64s("ids", latest.ID))
c.reqLogger(ctx).Error(logs.BucketSettingsNodeHasMultipleIDs, zap.Uint64s("ids", latest.ID), logs.TagField(logs.TagApp))
Member

I'm not sure if it should be app tag.

Below we use external_storage_tree and datapath for the same case. Let's pick one

I'm not sure if it should be `app` tag. Below we use `external_storage_tree` and `datapath` for the same case. Let's pick one
dkirillov marked this conversation as resolved
nzinkevich force-pushed feature/log_tagging from 2f4f60b2ff to 9056e93a54 2025-02-04 12:57:34 +00:00 Compare
nzinkevich force-pushed feature/log_tagging from 9056e93a54 to f71e35850f 2025-02-04 12:59:51 +00:00 Compare
nzinkevich force-pushed feature/log_tagging from f71e35850f to e1ee48827d 2025-02-06 10:21:53 +00:00 Compare
nzinkevich force-pushed feature/log_tagging from e1ee48827d to e4d4ac075c 2025-02-06 10:24:29 +00:00 Compare
nzinkevich force-pushed feature/log_tagging from e4d4ac075c to 75cdff9036 2025-02-06 10:28:50 +00:00 Compare
nzinkevich force-pushed feature/log_tagging from 75cdff9036 to 86b638adf0 2025-02-06 10:37:05 +00:00 Compare
dkirillov reviewed 2025-02-06 14:44:03 +00:00
@ -63,0 +63,4 @@
tags:
- name: "app"
level: "debug"
- name: "config"
Member

We don't have config tag now (the same for .env and configuration.md files)

We don't have config tag now (the same for .env and configuration.md files)
dkirillov marked this conversation as resolved
alexvanin added this to the v0.33.0 milestone 2025-02-07 12:34:58 +00:00
nzinkevich force-pushed feature/log_tagging from 86b638adf0 to 4c20b224bb 2025-02-11 07:03:28 +00:00 Compare
nzinkevich force-pushed feature/log_tagging from 4c20b224bb to 969d36ff6b 2025-02-11 07:05:54 +00:00 Compare
nzinkevich force-pushed feature/log_tagging from 969d36ff6b to 7905aa3bf3 2025-02-11 07:32:28 +00:00 Compare
nzinkevich force-pushed feature/log_tagging from 7905aa3bf3 to e7f620f137 2025-02-11 12:12:27 +00:00 Compare
requested review from dkirillov 2025-02-11 12:12:48 +00:00
alexvanin added 3 commits 2025-02-11 13:47:53 +00:00
Signed-off-by: Alex Vanin <a.vanin@yadro.com>
Signed-off-by: Alex Vanin <a.vanin@yadro.com>
[#606] Reorganize some log tags
Some checks failed
/ DCO (pull_request) Successful in 32s
/ Vulncheck (pull_request) Successful in 1m12s
/ Builds (pull_request) Successful in 58s
/ OCI image (pull_request) Successful in 2m4s
/ Lint (pull_request) Successful in 2m23s
/ Tests (pull_request) Successful in 1m13s
/ Vulncheck (push) Successful in 42s
/ Builds (push) Successful in 1m12s
/ Lint (push) Successful in 2m25s
/ Tests (push) Successful in 1m7s
/ OCI image (push) Failing after 11m37s
ee46382a68
Signed-off-by: Alex Vanin <a.vanin@yadro.com>
alexvanin approved these changes 2025-02-11 13:49:32 +00:00
alexvanin merged commit ee46382a68 into master 2025-02-11 15:15:43 +00:00
alexvanin referenced this pull request from a commit 2025-02-11 15:15:44 +00:00
alexvanin referenced this pull request from a commit 2025-02-11 15:15:45 +00:00
alexvanin referenced this pull request from a commit 2025-02-11 15:15:45 +00:00
alexvanin referenced this pull request from a commit 2025-02-11 15:15:46 +00:00
alexvanin deleted branch feature/log_tagging 2025-02-11 15:15:56 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
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-s3-gw#606
No description provided.