RPC requests accounting #426
Conversation
| @@ -507,13 +532,15 @@ func (s *DispersalServer) GetBlobStatus(ctx context.Context, req *pb.BlobStatusR | |||
|
|
|||
| requestID := req.GetRequestId() | |||
| if len(requestID) == 0 { | |||
| s.metrics.HandleInvalidArgRpcRequest("GetBlobStatus") | |||
| s.metrics.HandleInvalidArgRequest("GetBlobStatus") | |||
There was a problem hiding this comment.
These should be removed later: quorum tracking doesnt' apply to GetBlobStatus
There was a problem hiding this comment.
Why can't we remove it now?
There was a problem hiding this comment.
It can be a sanity check for the new metric with new v.s. old comparison
| @@ -614,6 +646,7 @@ func (s *DispersalServer) RetrieveBlob(ctx context.Context, req *pb.RetrieveBlob | |||
|
|
|||
| origin, err := common.GetClientAddress(ctx, s.rateConfig.ClientIPHeader, 2, true) | |||
| if err != nil { | |||
| s.metrics.HandleInvalidArgRpcRequest("RetrieveBlob") | |||
| s.metrics.HandleInvalidArgRequest("RetrieveBlob") | |||
There was a problem hiding this comment.
These should be removed later: quorum tracking doesnt' apply to RetrieveBlob
disperser/apiserver/server.go
Outdated
| @@ -413,11 +433,13 @@ func (s *DispersalServer) checkRateLimitsAndAddRatesToHeader(ctx context.Context | |||
|
|
|||
| globalRates, ok := s.rateConfig.QuorumRateInfos[param.QuorumID] | |||
| if !ok { | |||
| s.metrics.HandleInternalFailureRpcRequest("DisperseBlobAuthenticated") | |||
There was a problem hiding this comment.
checkRateLimits... is called by both "DisperseBlob" and "DispserseBlobAuthenticated". Maybe we need to pass in the apiMethod argument here as elsewhere.
disperser/apiserver/server.go
Outdated
| return nil, api.NewInternalError(fmt.Sprintf("missing confirmation information: %s", err.Error())) | ||
| } | ||
|
|
||
| s.metrics.HandleSuccessfulRpcRequest("GetBlobStatus") | ||
|
|
||
| s.logger.Debug("isConfirmed", "metadata", metadata, "isConfirmed", isConfirmed) |
There was a problem hiding this comment.
Unrelated, but should we remove this debug call? Seems like metadata could be large.
There was a problem hiding this comment.
Switched to just log metadataKey
| return nil, api.NewInternalError(fmt.Sprintf("ratelimiter error: %v", err)) | ||
| } | ||
| if !allowed { | ||
| s.metrics.HandleRateLimitedRpcRequest("RetrieveBlob") | ||
| s.metrics.HandleFailedRequest(codes.ResourceExhausted.String(), "", 0, "RetrieveBlob") |
There was a problem hiding this comment.
Would we also remove this quorum specific metrics? (here and below)
There was a problem hiding this comment.
I'd do it in followup, making the goal of this PR focused
It's straightforward and just avoid updating the metric for each quorum |
|
A little late, but I'm confused about why some of the non-RPC metrics loop over the quorums and others do not: vs s.metrics.HandleInvalidArgRequest("DisperseBlobAuthenticated") How are the blob-specific metrics meant to be used here? |
Yea, this is another issue of current metric: it's not consistent. As a result, the number shows on this metric is not well defined. |
Why are these changes needed?
The current
NumBlobRequestsis counting the same RPC requests multiple times (one for each quorum). This is not right for API accounting.Checks