go-grpc-prometheus
go-grpc-prometheus copied to clipboard
Decouple registration from grpc.Server implementation
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:
- I could see a signature of
RegisterServiceInfo (info grpc.ServiceInfo)that pre registers the methods for that service only andRegister(or an external callee) could do the iteration themselves. - I wasn't sure the best way to add tests as the current ones have global test setup that call
Registerand then one test that checks metrics registered so it wasn't apparent how to structure single calls toRegisterServiceInfowithout introducing ordering dependencies in the existing tests.
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?
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.
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.
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 addressed feedback and updated for some changes to master since I was last playing around here.
Codecov Report
Merging #17 into master will not change coverage. The diff coverage is
100%.
@@ 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 dataPowered by Codecov. Last update 39de438...f7df608. Read the comment docs.
this needs a rebase
@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.
@ppg can we have your CLA to be signed? Otherwise we cannot merge it ):
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!