[#51] add address to logs #322

Open
pogpp wants to merge 1 commit from pogpp/frostfs-sdk-go:feature/51_improve_logger into master
Member

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

Signed-off-by: Pavel Pogodaev <p.pogodaev@yadro.com>
pogpp added 1 commit 2025-01-24 09:42:03 +00:00
[#51] add address to logs
All checks were successful
DCO / DCO (pull_request) Successful in 27s
Code generation / Generate proto (pull_request) Successful in 35s
Tests and linters / Tests (pull_request) Successful in 50s
Tests and linters / Lint (pull_request) Successful in 1m24s
f3ede1fc56
Signed-off-by: Pavel Pogodaev <p.pogodaev@yadro.com>
requested reviews from storage-core-committers, storage-core-developers, storage-services-committers, storage-services-developers 2025-01-24 09:42:03 +00:00
pogpp was assigned by dkirillov 2025-01-24 13:22:09 +00:00
dkirillov reviewed 2025-01-24 13:24:26 +00:00
@ -167,3 +168,3 @@
return true, nil
}
return true, err
return true, fmt.Errorf("try put single '%s': %w", it.address, err)
Member

There are plenty of other methods in pool. Why do we change only this one?

There are plenty of other methods in pool. Why do we change only this one?
dkirillov marked this conversation as resolved
pogpp force-pushed feature/51_improve_logger from f3ede1fc56 to a7fa225b7e 2025-01-30 12:11:57 +00:00 Compare
pogpp force-pushed feature/51_improve_logger from a7fa225b7e to 5b92015d60 2025-01-30 12:12:55 +00:00 Compare
nzinkevich requested changes 2025-01-30 13:38:57 +00:00
pool/pool.go Outdated
@ -2670,6 +2670,10 @@ func (p *Pool) DeleteObject(ctx context.Context, prm PrmObjectDelete) error {
prmCtx.useDefaultSession()
prmCtx.useVerb(session.VerbObjectDelete)
prmCtx.useAddress(prm.addr)
cp, err := p.connection()
Member

Why do we declare this if do not make any requests with it? It's not guaranteed that endpoint would be the same as here

Why do we declare this if do not make any requests with it? It's not guaranteed that endpoint would be the same as here
pool/pool.go Outdated
@ -2678,3 +2682,3 @@
relatives, err := relations.ListAllRelations(ctx, p, prm.addr.Container(), prm.addr.Object(), tokens)
if err != nil {
return fmt.Errorf("failed to collect relatives: %w", err)
return fmt.Errorf("failed to collect relatives via client %s: %w", cp.address(), err)
Member

ListAllRelations (and others) already contains address in error, returned from underlying methods

Line 3230 in 5b92015
return nil, fmt.Errorf("failed to get raw object header by addr: %s %w", cp.address(), err)


And here:

Line 3282 in 5b92015
return oid.ID{}, fmt.Errorf("failed to read split chain member's header by addr: %s %w", cp.address(), err)


Why do we add endpoint to error again?

Maybe we should add endpoint info in places e.g.:

Line 3287 in 5b92015
return oid.ID{}, relations.ErrNoLeftSibling

And:

Line 3228 in 5b92015
return nil, relations.ErrNoSplitInfo

`ListAllRelations` (and others) already contains address in error, returned from underlying methods https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/src/commit/5b92015d603c07c447c04823b6099f6ad190a097/pool/pool.go#L3230 And here: https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/src/commit/5b92015d603c07c447c04823b6099f6ad190a097/pool/pool.go#L3282 Why do we add endpoint to error again? Maybe we should add endpoint info in places e.g.: https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/src/commit/5b92015d603c07c447c04823b6099f6ad190a097/pool/pool.go#L3287 And: https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/src/commit/5b92015d603c07c447c04823b6099f6ad190a097/pool/pool.go#L3228
pogpp force-pushed feature/51_improve_logger from 5b92015d60 to 5f110ec850 2025-02-03 12:45:34 +00:00 Compare
pogpp force-pushed feature/51_improve_logger from 5f110ec850 to 16284cad49 2025-02-03 12:46:46 +00:00 Compare
requested review from nzinkevich 2025-02-03 12:46:55 +00:00
dkirillov reviewed 2025-02-04 10:02:43 +00:00
@ -167,3 +168,3 @@
return true, nil
}
return true, err
return true, fmt.Errorf("try put single '%s': %w", it.address, err)
Member

There are other places where error is returned. Why do we update only this?

There are other places where error is returned. Why do we update only this?
dkirillov marked this conversation as resolved
nzinkevich reviewed 2025-02-06 07:10:01 +00:00
pool/pool.go Outdated
@ -3241,3 +3249,3 @@
res, err := p.HeadObject(ctx, prm)
if err != nil {
return nil, fmt.Errorf("failed to get linking object's header: %w", err)
return nil, fmt.Errorf("failed to get linking object's header by addr: %s %w", cp.address(), err)
Member

It seems HeadObject returns an err with cc.endpoint already included

It seems HeadObject returns an err with cc.endpoint already included
pool/pool.go Outdated
@ -3291,3 +3307,3 @@
res, err := p.SearchObjects(ctx, prm)
if err != nil {
return nil, fmt.Errorf("failed to search objects by split ID: %w", err)
return nil, fmt.Errorf("failed to search objects by split ID by addr %s: %w", cp.address(), err)
Member

SearchObjects's err already had an endpoint info

Line 2900 in 16284ca
return fmt.Errorf("search object via client %s: %w", cc.endpoint, err)

SearchObjects's err already had an endpoint info https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/src/commit/16284cad497c1661d58f8b55b4eb2c371e7f699b/pool/pool.go#L2900
pool/pool.go Outdated
@ -3323,3 +3343,3 @@
resSearch, err := p.SearchObjects(ctx, prm)
if err != nil {
return nil, fmt.Errorf("failed to find object children: %w", err)
return nil, fmt.Errorf("failed to find object children by addr %s: %w", cp.address(), err)
Member

Same

Same
pogpp force-pushed feature/51_improve_logger from 16284cad49 to 602c0c7b2e 2025-02-10 09:39:23 +00:00 Compare
pogpp force-pushed feature/51_improve_logger from 602c0c7b2e to 574f24a85c 2025-02-10 09:39:54 +00:00 Compare
pogpp force-pushed feature/51_improve_logger from 574f24a85c to 7275a46996 2025-02-25 09:34:14 +00:00 Compare
aarifullin reviewed 2025-02-25 18:15:17 +00:00
pool/pool.go Outdated
@ -3269,3 +3281,3 @@
idMember, ok := res.PreviousID()
if !ok {
return oid.ID{}, relations.ErrNoLeftSibling
return oid.ID{}, fmt.Errorf("failed to get sibling via client %s %w", cp.address(), relations.ErrNoLeftSibling)
Member
Suggestion, not change request

TBH, stuffing error messages with so many details doesn't look reasonable for me.

For instance, let's have a look at GetLeftSibling usage.

Line 2678 in c3f7378
relatives, err := relations.ListAllRelations(ctx, p, prm.addr.Container(), prm.addr.Object(), tokens)

idMember, err = rels.GetLeftSibling(ctx, cnrID, idMember, tokens)

As you can see, we may get an error with such message:
failed to collect relatives: failed to read split chain member's header: failed to get sibling via client...
So, for me fmt.Errorf("address: %s: %w") or fmt.Errorf("address '%s': %w") could be really enough.

WDYT?

##### Suggestion, not change request TBH, stuffing error messages with so many details doesn't look reasonable for me. For instance, let's have a look at `GetLeftSibling` usage. https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/src/commit/c3f7378887a476db9695812ea0fb89b1ecde6b78/pool/pool.go#L2678 https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/src/commit/c3f7378887a476db9695812ea0fb89b1ecde6b78/object/relations/relations.go#L92 As you can see, we may get an error with such message: `failed to collect relatives: failed to read split chain member's header: failed to get sibling via client...` So, for me `fmt.Errorf("address: %s: %w")` or `fmt.Errorf("address '%s': %w")` could be really enough. WDYT?
aarifullin reviewed 2025-02-25 18:19:40 +00:00
@ -152,3 +153,3 @@
res, err := it.client.ObjectPutSingle(ctx, cliPrm)
if err != nil && status.Code(err) == codes.Unimplemented {
return false, err
return false, fmt.Errorf("try put single '%s': %w", it.address, err)
Member
Suggestion, not change request

Here the error occurred not on tryPutSingle but on ObjectPutSingle. I'd prefer to not duplicate a method name in error message within this method (tryPutSingle ... try put single).
Either address '%s': %w/address: %s: %w or put single to '%s': %w. The same rule for other changes throughout this PR

##### Suggestion, not change request Here the error occurred not on `tryPutSingle` but on `ObjectPutSingle`. I'd prefer to not duplicate a method name in error message within **this** method (`tryPutSingle` ... `try put single`). Either `address '%s': %w`/`address: %s: %w` or `put single to '%s': %w`. The same rule for other changes throughout this PR
dkirillov reviewed 2025-02-27 08:33:13 +00:00
pool/pool.go Outdated
@ -3197,6 +3197,10 @@ func (p *Pool) GetSplitInfo(ctx context.Context, cnrID cid.ID, objID oid.ID, tok
var addr oid.Address
addr.SetContainer(cnrID)
addr.SetObject(objID)
cp, err := p.connection()
Member

Actually we can't write this:

cp, _ := p.connection()

_, err := p.HeadObject(ctx, prm)

// log err with address from 'cp'

because in p.HeadObject we also invoke p.connection() and actual client that is used for making request can differ from that we have already got in first line (cp, _ := p.connection())

Actually we can't write this: ```golang cp, _ := p.connection() _, err := p.HeadObject(ctx, prm) // log err with address from 'cp' ``` because in `p.HeadObject` we also invoke `p.connection()` and actual client that is used for making request can differ from that we have already got in first line (`cp, _ := p.connection()`)
Member

Still not fixed.

Example above it's how we can not write. And the same for other similar places

Still not fixed. Example above it's how we **can not** write. And the same for other similar places
Member

@pogpp ^

@pogpp ^
dkirillov marked this conversation as resolved
dkirillov reviewed 2025-02-27 08:37:52 +00:00
@ -677,3 +677,3 @@
for _, cl := range p.clientMap {
if closeErr := cl.close(); closeErr != nil {
p.log(zapcore.ErrorLevel, "close client connection", zap.Error(closeErr))
p.log(zapcore.ErrorLevel, "close client connection", zap.Error(closeErr), zap.String("addr", cl.endpoint()))
Member

Actually we can update cl.close

return c.client.Conn().Close()

so it returns error with address inside

Actually we can update `cl.close` https://git.frostfs.info/pogpp/frostfs-sdk-go/src/commit/7275a469960619f418ea8b4cf64d89108d1a30cd/pool/tree/client.go#L130 so it returns error with address inside
dkirillov marked this conversation as resolved
dkirillov reviewed 2025-02-27 08:41:28 +00:00
@ -963,3 +963,3 @@
if err != nil {
finErr = finalError(finErr, err)
p.log(zap.DebugLevel, "failed to create tree client", zap.String("request_id", reqID), zap.Int("remaining attempts", attempts))
p.log(zap.DebugLevel, "failed to create tree client", zap.String("request_id", reqID), zap.Int("remaining attempts", attempts), zap.String("addr", treeCl.endpoint()))
Member

The same here. We can make p.getNewTreeClient (in particular newTreeCl.dial) return error with address inside

The same here. We can make `p.getNewTreeClient` (in particular `newTreeCl.dial`) return error with address inside
dkirillov marked this conversation as resolved
dkirillov reviewed 2025-02-27 08:44:04 +00:00
pool/pool.go Outdated
@ -2770,3 +2770,3 @@
err := p.initCallContext(&cc, prm.prmCommon, prmContext{})
if err != nil {
return obj, err
return obj, fmt.Errorf("head object via client %s: %w", cc.endpoint, err)
Member

Actually using cc.endpoint here is ineffective. Because only error that can be returned from p.initCallContext is no healthy client and it can be return before we set cc.endpoint so it always be empty

Actually using `cc.endpoint` here is ineffective. Because only error that can be returned from `p.initCallContext` is `no healthy client` and it can be return before we set `cc.endpoint` so it always be empty
dkirillov marked this conversation as resolved
pogpp force-pushed feature/51_improve_logger from 7275a46996 to a5ffa32436 2025-02-28 06:02:40 +00:00 Compare
pogpp force-pushed feature/51_improve_logger from a5ffa32436 to eb9147c208 2025-02-28 09:29:03 +00:00 Compare
pogpp force-pushed feature/51_improve_logger from eb9147c208 to ed6e71bfe2 2025-02-28 09:34:41 +00:00 Compare
pogpp force-pushed feature/51_improve_logger from ed6e71bfe2 to 3bcf3469bc 2025-02-28 11:20:00 +00:00 Compare
dkirillov requested changes 2025-03-03 09:26:36 +00:00
pool/pool.go Outdated
@ -2770,3 +2770,3 @@
err := p.initCallContext(&cc, prm.prmCommon, prmContext{})
if err != nil {
return obj, err
return obj, fmt.Errorf("head object : %w", err)
Member

Please either:

  • Add the same log as in patch `fmt.Errorf("init call context: %w", err) and for other methods too
  • Not change it at all
Please either: * Add the same log as in patch `fmt.Errorf("init call context: %w", err) and for other methods too * Not change it at all
dkirillov marked this conversation as resolved
@ -128,3 +128,3 @@
return nil
}
return c.client.Conn().Close()
return fmt.Errorf("address '%s', %w", c.address, c.client.Conn().Close())
Member

Please, use : in msg: fmt.Errorf("address '%s': %w", c.address, c.client.Conn().Close())

Please, use `:` in msg: `fmt.Errorf("address '%s': %w", c.address, c.client.Conn().Close())`
dkirillov marked this conversation as resolved
@ -1065,3 +1065,3 @@
// Tree pool with netmap support does not operate with background goroutine, so we have to close connection immediately.
if err = newTreeCl.close(); err != nil {
p.log(zap.WarnLevel, "failed to close recently dialed tree client", zap.String("address", addr.URIAddr()), zap.Error(err))
p.log(zap.WarnLevel, "failed to close recently dialed tree client", zap.Error(err))
Member

The same we can add with using client.dial (above and in some other place) if we change dial so it set address into error

The same we can add with using `client.dial` (above and in some other place) if we change `dial` so it set address into error
Member

We have to update treeClient.dial so it returns error with address inside
here


and here

return fmt.Errorf("healthcheck tree service: %w", err)

We have to update `treeClient.dial` so it returns error with address inside here https://git.frostfs.info/pogpp/frostfs-sdk-go/src/commit/fbc676ed6fcadb34410db0139ed18bd7034b5c2d/pool/tree/client.go#L52 and here https://git.frostfs.info/pogpp/frostfs-sdk-go/src/commit/fbc676ed6fcadb34410db0139ed18bd7034b5c2d/pool/tree/client.go#L56
dkirillov marked this conversation as resolved
pogpp force-pushed feature/51_improve_logger from 3bcf3469bc to fbc676ed6f 2025-03-03 20:01:38 +00:00 Compare
pogpp force-pushed feature/51_improve_logger from fbc676ed6f to 62fd8d003b 2025-03-04 12:55:15 +00:00 Compare
pogpp force-pushed feature/51_improve_logger from 62fd8d003b to 5f90bb7d3b 2025-03-04 14:52:35 +00:00 Compare
dkirillov reviewed 2025-03-05 06:25:38 +00:00
pool/pool.go Outdated
@ -2513,3 +2513,3 @@
}
return err
return fmt.Errorf("initial call via client '%s' : %w", cp.address(), err)
Member

What is this?
Error here is always nil but you turn it into non-nil error.

What is this? Error here is always `nil` but you turn it into non-nil error.
dkirillov marked this conversation as resolved
pogpp force-pushed feature/51_improve_logger from 5f90bb7d3b to a7423bdf85 2025-03-05 10:36:55 +00:00 Compare
pogpp force-pushed feature/51_improve_logger from a7423bdf85 to 45f8074b89 2025-03-06 14:00:42 +00:00 Compare
pogpp force-pushed feature/51_improve_logger from 45f8074b89 to f444280b99 2025-03-07 15:30:03 +00:00 Compare
pogpp force-pushed feature/51_improve_logger from f444280b99 to 0039a8fcac 2025-03-10 09:07:09 +00:00 Compare
dkirillov reviewed 2025-03-11 06:56:25 +00:00
@ -128,3 +128,3 @@
return nil
}
return c.client.Conn().Close()
return fmt.Errorf("address '%s': %w", c.address, c.client.Conn().Close())
Member

What if c.client.Conn().Close() returns nil? I suppose we don't want to return non-nil error in this case

What if `c.client.Conn().Close()` returns `nil`? I suppose we don't want to return non-nil error in this case
pogpp force-pushed feature/51_improve_logger from 0039a8fcac to 85ed03f330 2025-03-11 12:06:28 +00:00 Compare
All checks were successful
DCO / DCO (pull_request) Successful in 25s
Required
Details
Code generation / Generate proto (pull_request) Successful in 29s
Required
Details
Tests and linters / Tests (pull_request) Successful in 40s
Required
Details
Tests and linters / Lint (pull_request) Successful in 1m32s
Required
Details
This pull request doesn't have enough approvals yet. 0 of 2 approvals granted.
This branch is out-of-date with the base branch
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/51_improve_logger:pogpp-feature/51_improve_logger
git checkout pogpp-feature/51_improve_logger
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-core-committers
TrueCloudLab/storage-core-developers
No milestone
No project
No assignees
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-sdk-go#322
No description provided.