search

grpc-ecosystem / go-grpc-prometheus

Prometheus monitoring for your gRPC Go servers.
Apache License 2.0
1.33k stars 164 forks source link

Support custom prometheus registerer #37

Closed sagikazarmark closed 6 years ago

sagikazarmark commented 6 years ago

Currently all metrics are exposed in the global registerer instance. Would be nice if one could provide a custom registerer instead of using a global one.

mwitkow commented 6 years ago

Happy to take a PR :)

sagikazarmark commented 6 years ago

Looks like this would require a major rewrite or force us to set a global instance which is exactly what I'm trying to avoid. For now my solution is to merge the global gatherer with a custom instance. But nevertheless it would be nice if global state could be avoided.

brancz commented 6 years ago

This is already possible. Quick example that I threw together, that we should probably document :slightly_smiling_face: 

r := prometheus.NewRegistry()
grpcMetrics := grpc_prometheus.NewServerMetrics()
gs := grpc.NewServer(
    grpc.StreamInterceptor(grpcMetrics.StreamServerInterceptor()),
    grpc.UnaryInterceptor(grpcMetrics.UnaryServerInterceptor()),
)

as := api.NewAPIServer()
pb.RegisterAPIServer(gs, as)

grpcMetrics.Register(r)
grpcMetrics.InitializeMetrics(as)

Similarly this exists for client metrics.

sagikazarmark commented 6 years ago

Oh, nice, although it looks like it has never been released. Can we expect a new tag in the near future?

sagikazarmark commented 6 years ago

@mwitkow @brancz any chance for a release?

brancz commented 6 years ago

@Bplotka how do you feel about a new release? I think I'd be ok with that, but would like to identify if there are any breaking changes, as I'm not too familiar with all the changes since the last release (it's been 1.5 years).

bwplotka commented 6 years ago

Yea, let's review what we have and paste into some changelog maybe. Based on that we can tell if it deserves v2 or just v1.2

Can schedule some time for this during next days, but it would be manual process for me as well (:

fengzixu commented 6 years ago

It seems like that we should review events which happend last time. A important thing shuold be discussed is "Revising our project README doc.", we can make it easier understand to freshmen.

brancz commented 6 years ago

@Bplotka I was mainly referring to whether you are in the know of anything major that needs to be worked on. I should be able to reserve some time today to write up a change log and see whether that’s compatible with the latest release.

knweiss commented 6 years ago

@brancz If there will be a new major version may I suggest we take a look at the current static analysis/linter status of the package? Esp golint's warning regarding the APIs and package name:

client.go:6:1:warning: don't use an underscore in package name (golint)
client_metrics.go:1:1:warning: don't use an underscore in package name (golint)
client_metrics.go:120:1:warning: comment on exported method ClientMetrics.StreamClientInterceptor should be of the form "StreamClientInterceptor ..." (golint)
client_reporter.go:4:1:warning: don't use an underscore in package name (golint)
client_test.go:4:1:warning: don't use an underscore in package name (golint)
client_test.go:210:3:warning: should replace count += 1 with count++ (golint)
metric_options.go:1:1:warning: don't use an underscore in package name (golint)
metric_options.go:7:6:warning: exported type CounterOption should have comment or be unexported (golint)
metric_options.go:18:1:warning: exported function WithConstLabels should have comment or be unexported (golint)
metric_options.go:24:6:warning: exported type HistogramOption should have comment or be unexported (golint)
metric_options.go:31:1:warning: exported function WithHistogramConstLabels should have comment or be unexported (golint)
server.go:6:1:warning: don't use an underscore in package name (golint)
server_metrics.go:1:1:warning: don't use an underscore in package name (golint)
server_metrics.go:157:6:warning: func streamRpcType should be streamRPCType (golint)
server_reporter.go:4:1:warning: don't use an underscore in package name (golint)
server_test.go:4:1:warning: don't use an underscore in package name (golint)
server_test.go:96:6:warning: range var testId should be testID (golint)
server_test.go:233:3:warning: should replace count += 1 with count++ (golint)
util.go:4:1:warning: don't use an underscore in package name (golint)

FWIW there are other linter warnings reported by gometalinter but these could be fixed after a major version bump, too:

client_reporter.go:41:34:warning: code can be expvar.Var (interfacer)
server_reporter.go:41:34:warning: code can be expvar.Var (interfacer)
client.go:38:15:warning: error return value not checked (prom.Register(DefaultClientMetrics.clientHandledHistogram)) (errcheck)
server.go:47:15:warning: error return value not checked (prom.Register(DefaultServerMetrics.serverHandledHistogram)) (errcheck)
server_metrics.go:193:55:warning: error return value not checked (metrics.serverStartedCounter.GetMetricWithLabelValues(methodType, serviceName, methodName)) (errcheck)
server_metrics.go:194:58:warning: error return value not checked (metrics.serverStreamMsgReceived.GetMetricWithLabelValues(methodType, serviceName, methodName)) (errcheck)
server_metrics.go:195:54:warning: error return value not checked (metrics.serverStreamMsgSent.GetMetricWithLabelValues(methodType, serviceName, methodName)) (errcheck)
server_metrics.go:197:58:warning: error return value not checked (metrics.serverHandledHistogram.GetMetricWithLabelValues(methodType, serviceName, methodName)) (errcheck)
server_metrics.go:200:56:warning: error return value not checked (metrics.serverHandledCounter.GetMetricWithLabelValues(methodType, serviceName, methodName, code.String())) (errcheck)
server_test.go:247:2:warning: prometheus.Handler is deprecated: Please note the issues described in the doc comment of InstrumentHandler. You might want to consider using promhttp.Handler instead (which is not instrumented, but can
 be instrumented with the tooling provided in package promhttp).  (SA1019) (megacheck)
util.go:40:5:warning: should omit comparison to bool constant, can be simplified to !mInfo.IsClientStream (S1002) (megacheck)
util.go:40:38:warning: should omit comparison to bool constant, can be simplified to !mInfo.IsServerStream (S1002) (megacheck)
util.go:43:5:warning: should omit comparison to bool constant, can be simplified to mInfo.IsClientStream (S1002) (megacheck)
util.go:43:37:warning: should omit comparison to bool constant, can be simplified to !mInfo.IsServerStream (S1002) (megacheck)
util.go:46:5:warning: should omit comparison to bool constant, can be simplified to !mInfo.IsClientStream (S1002) (megacheck)
util.go:46:38:warning: should omit comparison to bool constant, can be simplified to mInfo.IsServerStream (S1002) (megacheck)

I can contribute some fixes if we agree this is worthwhile.

fengzixu commented 6 years ago

@knweiss @Bplotka I think that if we decide to fix those warnings, we should determine what is suitable check tool for our project. Because variety of check tools often report various warnings.

brancz commented 6 years ago

@fengzixu agreed. And @knweiss if we are doing a major release, then yes we should consider what changes we want to make.

knweiss commented 6 years ago

@fengzixu My suggestion is to consider using golint and megacheck (which bundles staticcheck, gosimple, and unused) as the reports are sensible in my experience. Travis-CI integration would be great.

brancz commented 6 years ago

I opened #43 to discuss the new release. Let me know what you think, and I encourage everyone to quickly double check my statement about backward compatibility to ensure we're not falsely pushing out a new minor version if it's not actually compatible.

brancz commented 6 years ago

Closing here as non global registry is already possible. Everything regarding a new release is discussed in #43

fengzixu commented 6 years ago

Can i ask a question about global registerer instance? Except a global variable pattern is bad, anyone else disadvantages in using global registerer instance? I want to use a custom register in our team, but i don't have some strong proofs to persuade my partners to follow this good pattern.

fengzixu commented 6 years ago

@brancz @sagikazarmark @Bplotka

brancz commented 6 years ago

I've seen a number of projects that register global metrics (also libraries) that then pollute the global metrics library, the worst about this is that often you don't even know what information you expose. We prefer non-global registries because we want to know explicitly what we instrument (also note that registering things twice can cause panics). Also as an example: minikube in Kubernetes combines all components into a single binary, as this uses a global metrics registry, you get API metrics in the scheduler - very confusing.

fengzixu commented 6 years ago

According to your explanations, i summarized some key points about why non-global registry at below.

  1. You should exactly know what metrics are exposed. Because an apparent bug about registering a same metric twice appear easily.
  2. We should separate different metrics between different components in our project. Mixed metrics make us very confusing.

Except above two points, is there anything missing? @brancz

brancz commented 6 years ago

plus any article you can find on why globals should be avoided :) otherwise yes that's a good summary

bwplotka commented 6 years ago

Yea, agree with @brancz ^^ You know @fengzixu , there is a good way to convince your colleagues:

  1. Let's say you have metric app_http_requests_total somewhere hidden in your project and registered in file's func init() (because why not!)
  2. Sneakily add same metric (maybe a little bit obfuscated) "app_" + "http" + "_requests" +"_total" in yet another package somewhere deeply.
  3. Ask someone for help to debug this, because you have some weird panic.
  4. Get popcorn. Enjoy.

Basically, you will get panic that will lead you for example to the first app_http_request_total because it was registered as the second guy and you have no clue where was the first one.

You will laugh if you will know how many times I needed to debug similar issues, for example when someone accidentally reused http.DefaultServeMux and wanted to register /debug/pprof/ route. (psst, I will save your time here: for unknown reasons pprof register certain routes to default mux in init: https://github.com/golang/go/blob/release-branch.go1.9/src/net/http/pprof/pprof.go#L72-L76) (:

fengzixu commented 6 years ago

Your example for Golang http.DefaultServeMux is very suitable. Thanks for your explanations. I have convince my partners in our team. Thanks. @Bplotka

piotrkowalczuk commented 6 years ago

@sagikazarmark you can try promgrpc it allows what you need. It simply implements prometheus.Collector interface.

bwplotka commented 6 years ago

@piotrkowalczuk just curious, what's the difference between this project and yours https://github.com/piotrkowalczuk/promgrpc ? (:

piotrkowalczuk commented 6 years ago

@bwplotka Main differences:

brancz commented 6 years ago

How about we merge those things back into this code base and cut a 2.0? :)

bwplotka commented 6 years ago

Generally - all of those makes sense to me, let's discuss all differences in separate Issue to not mislead by talking in this one (: