Add metrics for RPC backend#100
Conversation
Signed-off-by: Chengxuan Xing <chengxuan.xing@kaleido.io>
Signed-off-by: Chengxuan Xing <chengxuan.xing@kaleido.io>
EnriqueL8
left a comment
EnriqueL8
left a comment
There was a problem hiding this comment.
Looks good, one important question on batch vs single op per request metrics
| var batchRes []*RPCResponse | ||
| log.L(ctx).Debugf("RPC batch[%d] -->", len(ops)) | ||
| rpcStartTime := time.Now() | ||
| httpStart := time.Now() |
There was a problem hiding this comment.
rpcStartTime? not http no?
There was a problem hiding this comment.
also keeping things consistent
| SetBody(batchReqs). | ||
| SetResult(&batchRes). | ||
| Post("") | ||
| httpDuration := time.Since(httpStart) |
There was a problem hiding this comment.
ditto
| httpDuration := time.Since(httpStart) | |
| requestDuration := time.Since(httpStart) |
| errMsg := i18n.NewError(ctx, signermsgs.MsgRPCRequestFailed, err).Error() | ||
| log.L(ctx).Errorf("RPC batch[%d] <-- ERROR: %s", len(ops), errMsg) | ||
| for _, op := range ops { | ||
| recordRPCRequest(ctx, op.Method, statusTransportError, httpDuration) |
There was a problem hiding this comment.
This modifies the metrics no? you cannot compare a batch metric op to a single op right? That will give you odd metrics ?
There was a problem hiding this comment.
@EnriqueL8 Yes, and it's deliberate that the batch timing and single request timing are tracked under the same metric. The metric are grouped by json_rpc method and status.
if users decide to mix usage of batch / non-batch for the same query, the timing of a JSON_RPC method will be hard to track.
However, I don't believe not tracking batch requests in the proposed metrics is correct either. So I added a batch label just in case there is a need to separate them, and also added an extra metric for batch size.
Signed-off-by: Chengxuan Xing <chengxuan.xing@kaleido.io>
…-metrics Signed-off-by: Chengxuan Xing <chengxuan.xing@kaleido.io>
2 participants
Adding basic metrics for RPC backend.
Example dashboard: