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

Decouple registration from grpc.Server implementation

Open ppg opened this issue 8 years ago • 10 comments

Hi,

I was hoping this library could decouple the registration for monitoring from the specific implementation of *grpc.Server that it's currently tied to. From what I can tell all that grpc prometheus cares about for registration is that it has a bunch of service info objects that describe what it can monitor. So it seems like we can expose direct access to that via RegisterServiceInfo so that someone could register arbitrary services. In addition, we can define what is important for a server in Register, namely that it can get said service info, via an interface so that current code works but that function is no longer tied explicitly to grpc's particular server implementation. In my particular use case I have a queue consumer that consumes the RPCs defined by the protobufs but my concrete type obviously isn't *grpc.Server, even though I can expose GetServiceInfo() to describe what services I consume.

A couple notes:

  1. I could see a signature of RegisterServiceInfo (info grpc.ServiceInfo) that pre registers the methods for that service only and Register (or an external callee) could do the iteration themselves.
  2. I wasn't sure the best way to add tests as the current ones have global test setup that call Register and then one test that checks metrics registered so it wasn't apparent how to structure single calls to RegisterServiceInfo without introducing ordering dependencies in the existing tests.

ppg avatar Mar 29 '17 02:03 ppg

Hmm, I'm a bit confused as to what you're actually intending here? Do you want to: a) be able to perform the monitoring pre-registration on multiple *grpc.Server objects? b) have your own implementation of grpc.Server and you have your own grpc.ServiceInfo laying around that you want to hook up into?

mwitkow avatar Apr 02 '17 09:04 mwitkow

For the overall question, option B; I have something consuming RPCs that isn't a *grpc.Server, but I have grpc.ServiceInfo items about what I can consume.

ppg avatar Apr 06 '17 17:04 ppg

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

googlebot avatar Apr 06 '17 17:04 googlebot

I'm ok with implementing a private interface that *grpc.Server implements (i.e. just the signature o f the method). This way Register will be backwards compatible.

mwitkow avatar May 06 '17 10:05 mwitkow

@mwitkow addressed feedback and updated for some changes to master since I was last playing around here.

ppg avatar Sep 01 '17 23:09 ppg

Codecov Report

Merging #17 into master will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #17   +/-   ##
=======================================
  Coverage   78.59%   78.59%           
=======================================
  Files           8        8           
  Lines         271      271           
=======================================
  Hits          213      213           
  Misses         49       49           
  Partials        9        9
Impacted Files Coverage Δ
server_metrics.go 80.19% <100%> (ø) :arrow_up:
server.go 100% <100%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 39de438...f7df608. Read the comment docs.

codecov-io avatar Sep 01 '17 23:09 codecov-io

this needs a rebase

brancz avatar Apr 18 '18 08:04 brancz

@ppg sorry for the massive delay in review. Let us know If you still want to have this change in new release of go-grpc-prometheus.

bwplotka avatar Apr 18 '18 09:04 bwplotka

@ppg can we have your CLA to be signed? Otherwise we cannot merge it ):

bwplotka avatar Jun 04 '18 08:06 bwplotka

Hi 👋🏽

Sorry for the massive, lag. We consolidating projects and moving to single repo where we have more control and awareness. We are moving this code base longer to https://github.com/grpc-ecosystem/go-grpc-middleware/tree/v2 and we moved existing state of Prometheus middleware to https://github.com/grpc-ecosystem/go-grpc-middleware/tree/v2/providers/openmetrics

This means that before we release v2 this is the best opportunity to shape new https://github.com/grpc-ecosystem/go-grpc-middleware/tree/v2/providers/openmetrics with whatever we want to change in API 🤗 If you want to change something effectively long term, I would suggest switching your PR and rebasing it on https://github.com/grpc-ecosystem/go-grpc-middleware/tree/v2/providers/openmetrics instead (notice v2 branch, not master!).

Sorry for the confusion, but it's needed for the project sustainability. Cheers!

bwplotka avatar Mar 15 '21 15:03 bwplotka