Skip to content
This repository was archived by the owner on Apr 18, 2023. It is now read-only.

Decouple registration from grpc.Server implementation #17

Open
ppg wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Decouple registration from grpc.Server implementation #17

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ab08d00
Add RegisterServiceInfo to decouple Register from grpc.Server.
ppg Mar 29, 2017
a47cc0d
Accept Server interface for Register to allow flexibility.
ppg Mar 29, 2017
8d96c0f
[CR] Use private interface and only support Register
ppg Sep 1, 2017
1cfc0ff
Merge remote-tracking branch 'upstream/master'
ppg Sep 1, 2017
d50de4f
[CR] Address feedback on godoc comments
ppg Nov 16, 2017
f7df608
Merge remote-tracking branch 'upstream/master'
ppg May 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,22 @@ import (
"google.golang.org/grpc"
)

// PreregisterServices 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 *grpc.Server) {
serviceInfo := server.GetServiceInfo()
// Server defines the interface that is required for grpc_prometheus to Register.
type Server interface {
Copy link
Member

@mwitkow mwitkow Apr 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't have to be a public interface. It can be private.

Copy link
Author

@ppg ppg Apr 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@mwitkow mwitkow May 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does it break people? It will happily accept grpc.Server, which is fine?

GetServiceInfo() map[string]grpc.ServiceInfo
}

// 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) {
Copy link
Member

@mwitkow mwitkow Apr 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@ppg ppg Apr 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@mwitkow mwitkow May 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as Register accepts something that *grpc.Server implements, it is fine.

Copy link
Author

@ppg ppg May 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

RegisterServiceInfo(server.GetServiceInfo())
}

// RegisterServiceInfo takes a map of gRPC ServiceInfo objects and per-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 RegisterServiceInfo(serviceInfo map[string]grpc.ServiceInfo) {
for serviceName, info := range serviceInfo {
for _, mInfo := range info.Methods {
preRegisterMethod(serviceName, &mInfo)
Expand Down