-
Notifications
You must be signed in to change notification settings - Fork 165
Decouple registration from grpc.Server implementation #17
base: master
Are you sure you want to change the base?
Changes from 2 commits
ab08d00
a47cc0d
8d96c0f
1cfc0ff
d50de4f
f7df608
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you don't need this to be separate from There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
However, in order to not break existing consumers I didn't want to change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As long as There was a problem hiding this comment. Choose a reason for hiding this commentThe 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-
What I believe it should say instead is:
That stops this library from randomly picking a method name of With
And literally any other non-
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 |
||
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) | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?