Decouple registration from grpc.Server implementation #17
Conversation
|
Hmm, I'm a bit confused as to what you're actually intending here? Do you want to: |
server.go
Outdated
| func Register(server *grpc.Server) { | ||
| serviceInfo := server.GetServiceInfo() | ||
| // Server defines the interface that is required for grpc_prometheus to Register. | ||
| type Server interface { |
There was a problem hiding this comment.
this doesn't have to be a public interface. It can be private.
There was a problem hiding this comment.
Is that preferred? Private or public, changing it breaks people, so its not like making it private is not exposing the contract, it just hides it vs. making it more explicit. It makes things like gomock harder too, as you'd have to re-define the interface yourself to get a usable mock.
There was a problem hiding this comment.
why does it break people? It will happily accept grpc.Server, which is fine?
server.go
Outdated
| // Register takes a gRPC server and pre-initializes all counters to 0. | ||
| // This allows for easier monitoring in Prometheus (no missing metrics), and | ||
| // should be called *after* all services have been registered with the server. | ||
| func Register(server Server) { |
There was a problem hiding this comment.
you don't need this to be separate from RegisterServiceInfo. You can just change the type of the existing Register from the concrete *grpc.Server to a private interface serviceInfoGetter.
There was a problem hiding this comment.
My thought process here was, ignoring back compat, its somewhat pointless to require the callee to have an implementation that responds to GetServiceInfo() map[string]grpc.ServiceInfo; instead, because this library only needs to know the services described by grpc.ServiceInfo objects so it can initialize the metrics, it would be more flexible to just allow those to be passed in directly, and the calllees would use whatever facilities they had to get them (GetServiceInfo() in the case of *grpc.Server, perhaps something else in other implementations). Instead of the current instructions for hooking up, they would have originally been:
// Initialize your gRPC server's interceptor.
myServer := grpc.NewServer(
grpc.StreamInterceptor(grpc_prometheus.StreamServerInterceptor),
grpc.UnaryInterceptor(grpc_prometheus.UnaryServerInterceptor),
)
// Register your gRPC service implementations.
myservice.RegisterMyServiceServer(s.server, &myServiceImpl{})
// After all your registrations, make sure all of the Prometheus metrics are initialized.
grpc_prometheus.Register(myServer.GetServiceInfo()) // <=== this is now requiring only what it cares about
// Register Prometheus metrics handler.
http.Handle("/metrics", prometheus.Handler())
However, in order to not break existing consumers I didn't want to change Register, so I figured second best options was to expose the more general register function that most people should use (RegisterServiceInfo) and then Register is a convenience function for back compat specifically with *grpc.Server.
There was a problem hiding this comment.
As long as Register accepts something that *grpc.Server implements, it is fine.
There was a problem hiding this comment.
I'm trying to fix (in a backwards compatible way) the RPC registration path to be more generalized (and thus more generally useful), as well as trying to avoid contorting my non-*grpc.Server code to pretend that it's that object. The current code base for registering RPCs (and the interface only change for Regsiter) is saying:
in order to register RPCs give me an object that I can call an arbitrary method on to return a description of those RPCs
What I believe it should say instead is:
in order to register RPCs give me a description of those RPCs
That stops this library from randomly picking a method name of GetServiceInfo that all consumers must implement and instead spells out exactly what it needs: map[string]grpc.ServiceInfo, i.e. "a description of those RPCs".
With RegisterServiceInfo it fulfills that more generalized registration functionality. Any code that followed the server-side usage example can simply do:
grpc_prometheus.RegisterServiceInfo(myServer.GetServiceInfo())
And literally any other non-*grpc.Server code can do:
var infos map[string]grpc.ServiceInfo
// collect infos here from whatever is servicing them, however it is appropriate to.
grpc_prometheus.RegisterServiceInfo(infos)
If we're willing to change to an interface to make more generally useful, why not also address the original overly constrained path so that people can migrate away from Register?
|
For the overall question, option B; I have something consuming RPCs that isn't a |
|
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). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
I'm ok with implementing a private interface that |
|
@mwitkow addressed feedback and updated for some changes to master since I was last playing around here. |
Codecov Report
@@ Coverage Diff @@
## master #17 +/- ##
=======================================
Coverage 78.59% 78.59%
=======================================
Files 8 8
Lines 271 271
=======================================
Hits 213 213
Misses 49 49
Partials 9 9
Continue to review full report at Codecov.
|
server_metrics.go
Outdated
| @@ -129,9 +129,9 @@ func (m *ServerMetrics) StreamServerInterceptor() func(srv interface{}, ss grpc. | |||
| } | |||
|
|
|||
| // InitializeMetrics initializes all metrics, with their appropriate null | |||
| // value, for all gRPC methods registered on a gRPC server. This is useful, to | |||
| // value, for all gRPC methods registered on a prometheusServer. This is useful, to | |||
There was a problem hiding this comment.
Please keep this as gRPC server.
server.go
Outdated
| GetServiceInfo() map[string]grpc.ServiceInfo | ||
| } | ||
|
|
||
| // Register takes a prometheusServer and pre-initializes all counters to 0. |
There was a problem hiding this comment.
In general please talk about gRPC.Server and mention ("or any compatible type"), so it is clear for most users.
|
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 |
|
@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 Sorry for the confusion, but it's needed for the project sustainability. Cheers! |
Hi,
I was hoping this library could decouple the registration for monitoring from the specific implementation of
*grpc.Serverthat 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 viaRegisterServiceInfoso that someone could register arbitrary services. In addition, we can define what is important for aserverinRegister, 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 exposeGetServiceInfo()to describe what services I consume.A couple notes:
RegisterServiceInfo (info grpc.ServiceInfo)that pre registers the methods for that service only andRegister(or an external callee) could do the iteration themselves.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.