Client: add metric for failed RPC calls to a consul server #4220
Conversation
There was a problem hiding this comment.
@guidoiaquinti Thanks for the PR. I just have the one question but otherwise everything looks good.
agent/consul/client.go
Outdated
| @@ -276,6 +276,7 @@ TRY: | |||
|
|
|||
| // Move off to another server, and see if we can retry. | |||
| c.logger.Printf("[ERR] consul: %q RPC failed to server %s: %v", method, server.Addr, rpcErr) | |||
| metrics.IncrCounter([]string{"client", "rpc", "failed"}, 1) | |||
There was a problem hiding this comment.
Would it be useful to add a label here for the server that the RPC failed on? It seems like it would be a good idea because the problem here is likely on the server that failed to do the RPC. What do you think @guidoiaquinti.
Maybe something like this:
metrics.IncrCounterWithLabels([]string{"client", "rpc", "failed"}, 1, []metrics.Label{{Name: "server", Value: server.ID}})
Not sure whether server.ID, server.Name or server.Addr.String() is most appropriate.
There was a problem hiding this comment.
server.Name gets my vote - it must be unique at any point in time and is human readable. Metric labels with UUIDs in are pretty unpleasant to use in practice for at-a-glance analysis.
There was a problem hiding this comment.
Sorry, I read your comment after pushing my commit. I've changed the label to server.Name as you suggested @banks
Note: we could also add both of them 🤷♂️
Add metric for failed RPC calls to a consul server