separate shortpoll and longpoll metrics #6
Conversation
| defer func() { | ||
| s.m.APILatency.WithLabelValues("GetMessages", in.QueueId.Namespace, in.QueueId.Name).Observe(float64(time.Now().Sub(start))) | ||
| s.m.APILatency.WithLabelValues(apiName, in.QueueId.Namespace, in.QueueId.Name).Observe(float64(time.Now().Sub(start))) |
There was a problem hiding this comment.
Instead of replacing apiName it seems that it'd be better to add a label saying it was a short poll? My main concern is that previously the apiName matches the actual gRPC api name -- now it doesn't
There was a problem hiding this comment.
I agree it's not ideal, but adding a label has its problem too: it means adding the label to all other apis e.g. ‘PublishMessage’, which doesn’t has the poll length parameter. It can be confusing too. My feeling is that the apiName just serves as a label to differential different api calls, so an additional name isn't going to cause true problem. It also means we don't need to change current grafana metrics, the new metric will shows up naturally in existing graphs.
There was a problem hiding this comment.
that being said, I don't have a strong preference here, so if you do believe adding a label is better, I can do it
This is for https://phab.wish.com/T162969
I suspect some clients are using short polls to poll message which is not efficient. This PR Use a separate apiName to separate shortpolls from long polls (I've thought of using an additional label but this shortpoll/longpoll is specific to the GetMessages api, so this seems better).