[#51] add address to logs #322
No reviewers
Labels
No labels
P0
P1
P2
P3
good first issue
pool
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No milestone
No project
No assignees
4 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-sdk-go#322
Loading…
Add table
Reference in a new issue
No description provided.
Delete branch "pogpp/frostfs-sdk-go:feature/51_improve_logger"
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?
Signed-off-by: Pavel Pogodaev p.pogodaev@yadro.com
@ -167,3 +168,3 @@
return true, nil
}
return true, err
return true, fmt.Errorf("try put single '%s': %w", it.address, err)
There are plenty of other methods in pool. Why do we change only this one?
f3ede1fc56
toa7fa225b7e
a7fa225b7e
to5b92015d60
@ -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()
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
@ -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)
ListAllRelations
(and others) already contains address in error, returned from underlying methodsreturn nil, fmt.Errorf("failed to get raw object header by addr: %s %w", cp.address(), err)
And here:
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.:
return oid.ID{}, relations.ErrNoLeftSibling
And:
return nil, relations.ErrNoSplitInfo
5b92015d60
to5f110ec850
5f110ec850
to16284cad49
@ -167,3 +168,3 @@
return true, nil
}
return true, err
return true, fmt.Errorf("try put single '%s': %w", it.address, err)
There are other places where error is returned. Why do we update only this?
@ -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)
It seems HeadObject returns an err with cc.endpoint already included
@ -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)
SearchObjects's err already had an endpoint info
return fmt.Errorf("search object via client %s: %w", cc.endpoint, err)
@ -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)
Same
16284cad49
to602c0c7b2e
602c0c7b2e
to574f24a85c
574f24a85c
to7275a46996
@ -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)
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.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")
orfmt.Errorf("address '%s': %w")
could be really enough.WDYT?
@ -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)
Suggestion, not change request
Here the error occurred not on
tryPutSingle
but onObjectPutSingle
. 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
orput single to '%s': %w
. The same rule for other changes throughout this PR@ -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()
Actually we can't write this:
because in
p.HeadObject
we also invokep.connection()
and actual client that is used for making request can differ from that we have already got in first line (cp, _ := p.connection()
)Still not fixed.
Example above it's how we can not write. And the same for other similar places
@pogpp ^
@ -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()))
Actually we can update
cl.close
return c.client.Conn().Close()
so it returns error with address inside
@ -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()))
The same here. We can make
p.getNewTreeClient
(in particularnewTreeCl.dial
) return error with address inside@ -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)
Actually using
cc.endpoint
here is ineffective. Because only error that can be returned fromp.initCallContext
isno healthy client
and it can be return before we setcc.endpoint
so it always be empty7275a46996
toa5ffa32436
a5ffa32436
toeb9147c208
eb9147c208
toed6e71bfe2
ed6e71bfe2
to3bcf3469bc
@ -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)
Please either:
@ -128,3 +128,3 @@
return nil
}
return c.client.Conn().Close()
return 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())
@ -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))
The same we can add with using
client.dial
(above and in some other place) if we changedial
so it set address into errorWe have to update
treeClient.dial
so it returns error with address insidehere
return err
and here
return fmt.Errorf("healthcheck tree service: %w", err)
3bcf3469bc
tofbc676ed6f
fbc676ed6f
to62fd8d003b
62fd8d003b
to5f90bb7d3b
@ -2513,3 +2513,3 @@
}
return err
return fmt.Errorf("initial call via client '%s' : %w", cp.address(), err)
What is this?
Error here is always
nil
but you turn it into non-nil error.5f90bb7d3b
toa7423bdf85
a7423bdf85
to45f8074b89
45f8074b89
tof444280b99
f444280b99
to0039a8fcac
@ -128,3 +128,3 @@
return nil
}
return c.client.Conn().Close()
return fmt.Errorf("address '%s': %w", c.address, c.client.Conn().Close())
What if
c.client.Conn().Close()
returnsnil
? I suppose we don't want to return non-nil error in this case0039a8fcac
to85ed03f330
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.