TrueCloudLab/frostfs-node
20
1
Fork 28
Code Issues 106 Pull requests 4 Projects Releases 2 Packages 4 Activity Actions

audit: Fix duplicated request logs #1659

Merged
fyrchik merged 1 commit from elebedeva/frostfs-node:fix/duplicated-logs into master 2025-03-06 11:57:30 +00:00
Conversation 0 Commits 1 Files changed 1 +1 -1
1 changed files with 1 additions and 1 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
pkg/services/object/audit.go
View file

@ -163,7 +163,7 @@ func (a *auditPutStream) Send(ctx context.Context, req *object.PutRequest) error
if err != nil {
a.failed = true
}
if !errors.Is(err, util.ErrAbortStream) { // CloseAndRecv will not be called, so log here
if err != nil && !errors.Is(err, util.ErrAbortStream) { // CloseAndRecv will not be called, so log here
fyrchik commented 2025-03-03 06:29:54 +00:00
Outdated
Review
Copy link

Could you explain, why this was called multiple times?
I would expect that if there is an error, that is it, finish.

Could you explain, **why** this was called multiple times? I would expect that if there is an error, that is it, finish.
fyrchik commented 2025-03-03 06:30:30 +00:00
Outdated
Review
Copy link

The explanation should be a part of the commit message.

The explanation should be a part of the commit message.
elebedeva commented 2025-03-06 09:41:08 +00:00
Outdated
Review
Copy link

Updated commit message.

When we do object put with audit enabled we get several entries in logs: with and without object id.

object put request is logged in 2 places:

  1. (*auditPutStream) CloseAndRecv() - when the client closes the request stream or when stream gets aborted.
  2. (*auditPutStream) Send() - when stream was NOT aborted.

Send() does error check for ErrAbortStream because if there is any other error - CloseAndRecv will not be called and there won't be any audit log about failed request.
It led to logging on every object chunck put, even if err == nil.

Updated commit message. When we do `object put` with audit enabled we get several entries in logs: with and without object id. `object put` request is logged in 2 places: 1. `(*auditPutStream) CloseAndRecv()` - when the client closes the request stream or when stream gets aborted. 2. `(*auditPutStream) Send()` - when stream was NOT aborted. `Send()` does error check for `ErrAbortStream` because if there is any other error - CloseAndRecv will not be called and there won't be any audit log about failed request. It led to logging on every object chunck put, even if `err == nil`.
fyrchik commented 2025-03-06 10:19:54 +00:00
Outdated
Review
Copy link

Ok, this PR is approved by me.

But why do we call CloseAndRecv() only for this specific error and not for others? It seems there could be multiple logic errors, and if CloseAndRecv() does some kind of a graceful shutdown, we should always do it.

Ok, this PR is approved by me. But why do we call `CloseAndRecv()` only for this specific error and not for others? It seems there could be multiple logic errors, and if `CloseAndRecv()` does some kind of a graceful shutdown, we should always do it.
dstepanov-yadro commented 2025-03-06 10:51:34 +00:00
Outdated
Review
Copy link

See

Line 54 in 4c8f958
if err := stream.Send(gStream.Context(), patchReq); err != nil {

I don't know why this was done this way.

See https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/4c8f9580a16eb5d30320d670849585fe25c1d04d/pkg/network/transport/object/grpc/service.go#L54 I don't know why this was done this way.
fyrchik commented 2025-03-06 11:20:44 +00:00
Outdated
Review
Copy link

Did some digging
It was introduced in 7f5fb130c0 (diff-f9fb9a9419164dc9a10f6de9003e9b0d771cfa48)
And it seems we use it to treat signature error as a status error.

Did some digging It was introduced in https://git.frostfs.info/TrueCloudLab/frostfs-node/commit/7f5fb130c006337de41e5b141d8861bdb581a2d4#diff-f9fb9a9419164dc9a10f6de9003e9b0d771cfa48 And it seems we use it to treat signature error as a status error.
audit.LogRequestWithKey(ctx, a.log, objectGRPC.ObjectService_Put_FullMethodName, a.key,
audit.TargetFromContainerIDObjectID(a.containerID, a.objectID),
!a.failed)