Non default registry #20
Conversation
Codecov Report
@@ Coverage Diff @@
## master #20 +/- ##
==========================================
- Coverage 86.36% 82.66% -3.71%
==========================================
Files 5 7 +2
Lines 154 248 +94
==========================================
+ Hits 133 205 +72
- Misses 13 35 +22
Partials 8 8
Continue to review full report at Codecov.
|
|
I have now plugged this into an application and can confirm that this indeed works. |
|
Wow, that's a massive PR. But it looks good overall. Please add a comments to |
server.go
Outdated
| } | ||
| return err | ||
| // EnableHandlingTimeHistogram turns on recording of handling time of RPCs for server-side interceptors. | ||
| // Histogram metrics can be very expensive for Prometheus to retain and query. |
There was a problem hiding this comment.
Please add a note that this only works for the DefaultServerMetrics.
server.go
Outdated
| return err | ||
| // 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. |
There was a problem hiding this comment.
Please add a note that this only works for DefaultServerMetrics.
server.go
Outdated
| // Histogram metrics can be very expensive for Prometheus to retain and query. | ||
| func EnableHandlingTimeHistogram(opts ...HistogramOption) { | ||
| DefaultServerMetrics.EnableHandlingTimeHistogram(opts...) | ||
| prom.Register(DefaultServerMetrics.serverHandledHistogram) |
There was a problem hiding this comment.
how about we move this to EnableHandlingTimeHistogram istelf?
There was a problem hiding this comment.
Or will that die hard because of double registration?
There was a problem hiding this comment.
It's unclear that the method actually instruments a metric, so I wanted to keep it out of the underlying method, but keep the existing interface.
Aside from that, yes registration is expected to be unique:
// Register registers a new Collector to be included in metrics
// collection. It returns an error if the descriptors provided by the
// Collector are invalid or if they — in combination with descriptors of
// already registered Collectors — do not fulfill the consistency and
// uniqueness criteria described in the documentation of metric.Desc.
client_metrics.go
Outdated
| "google.golang.org/grpc/codes" | ||
| ) | ||
|
|
||
| type ClientMetrics struct { |
There was a problem hiding this comment.
please add comments here and how it relates to `Default
brancz
force-pushed
the
non-default-registry
branch
from
e3873b9
to
afb525b
Compare
brancz
force-pushed
the
non-default-registry
branch
from
afb525b
to
42b3df7
Compare
|
Added documentation everywhere, let me know what you think. |
|
@mwitkow ping? |
|
Looks good, thank you! :) |
This PR allows instrumenting the gRPC metrics with a custom metrics registry instead of the default Prometheus registry. This is useful if you don't want random
initfunctions to register metrics on your registry, but have control over it.Additionally it allow the histogram buckets to be chosen on a per server/service basis, rather than globally.
I have yet to actually plug this into an application to try it out, but the tests are passing so I'm confident that it works properly. I was careful to keep the existing interface, to not break existing users, but it would be good to validate this. Also if the current usage can be broken I have other ideas on how to potentially change it.
Let me know what you want me to do in terms of the license header. I'm working out of Germany and a copyright transfer is not possible by law.
@mwitkow