go-grpc-prometheus icon indicating copy to clipboard operation
go-grpc-prometheus copied to clipboard

Allow limiting grpc codes tracked in histrogram

Open brancz opened this issue 4 years ago • 3 comments

Latency alerts tend to be most focused on succeeding calls, and since errors can already be accurately I would like to allow to ignore failing requests from the histogram metrics.

I am a bit torn though on what to do:

  1. Make this configurable, I don't really like this because suddenly the same histogram metric of different processes may behave totally different from the next.
  2. Change the default, this might cause unexpected behavior for users that are already using the library today.

Since I'm unable to ignore failed requests from the histogram at all today, I'm going to mark this as a bug, as it's essentially of no use as it is for alerting purposes.

Wdyt @bwplotka ?

brancz avatar Dec 30 '20 09:12 brancz

Totally +100 to this. I was thinking about this as well. In fact I think we should care about user error, system error etc. Histograms per grpc code would be bit too cardinal IMO, but is also an option.

I have a proposition:

  • Finalize move of this lib to https://github.com/grpc-ecosystem/go-grpc-middlewares and implement new histograms as totally new major version of go-grpc-prometheus (and in different repo)

WDYT?

bwplotka avatar Dec 30 '20 14:12 bwplotka

  1. would be totally possible as a major version breaking change, so that sounds good to me. What's left to "finalize"? Happy to help out!

brancz avatar Dec 30 '20 16:12 brancz

Here is a plan: https://github.com/grpc-ecosystem/go-grpc-middleware/issues?q=is%3Aissue+is%3Aopen+label%3Av2

Literally, at any point now, we could release v2 officially. We already have release candidates. But I would love to have metrics in first so https://github.com/grpc-ecosystem/go-grpc-middleware/issues/346

Plus I think the last thing that has to be finalized is moving to newest protobuf Go bindings which I kind of stopped at some point doing, so help wanted: https://github.com/grpc-ecosystem/go-grpc-middleware/pull/321

Once https://github.com/grpc-ecosystem/go-grpc-middleware/issues/346 and https://github.com/grpc-ecosystem/go-grpc-middleware/pull/321 are in I believe we can ship v2. Plus users can already use v2 it's quite stable (except proto and grpc deps update). Both blockers are not connected as well you can help move this repo if you want 🤗

Maybe I will find time this or next week for https://github.com/grpc-ecosystem/go-grpc-middleware/pull/321

cc @johanbrandhorst

bwplotka avatar Dec 30 '20 16:12 bwplotka