✨Expose RegisterClientMetrics to register other clien… #2936
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Yesphet The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @Yesphet. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
needs-ok-to-test
|
@alvaroaleman @inteon Hi, I hope you’re doing well! Just a quick reminder to review this pr (#2936) submitted two weeks ago. Your feedback would be greatly appreciated! |
pkg/metrics/client_go_adapter.go
Outdated
| // register the metrics with client-go | ||
| clientmetrics.Register(clientmetrics.RegisterOpts{ | ||
| RequestResult: &resultAdapter{metric: requestResult}, | ||
| time.AfterFunc(30*time.Second, func() { |
There was a problem hiding this comment.
I am not a fan of this because it means you might not get metrics for the first thirty seconds of the binary. Another way I can think of is to maybe call this before first use in the controller or so, that would still allow to register them yourselves by doing it before you build your controllers and avoid this issue?
There was a problem hiding this comment.
Agreed, I moved this call into the NewServer function of metric server in 639312c
pkg/metrics/client_go_adapter.go
Outdated
| r.metric.WithLabelValues(code, method, host).Inc() | ||
| if r.customMetric != nil { | ||
| r.customMetric.Increment(ctx, code, method, host) |
There was a problem hiding this comment.
Sorry, I do not understand this. You both want to register them yourself and plug your own functionality into the existing adapter? Why isn't one of the two enough?
There was a problem hiding this comment.
Initially, the goal was to support custom requestResult metrics and keep the original rest_client_requests_total metrics. I changed the resultAdapter to the default value of RegisterOpts.RequestResult in 639312c
…gister client-go metrics in NewServer instead of init function
k8s-ci-robot
added
size/S
…t-go metrics
Users of controller-runtime can not register custom metrics of
client-gosincek8s.io/client-go/tools/metrics.Registercan only be called once and it's called in theinit()function ofpkg/metrics. So this PR exposeRegisterClientMetricsfor registering customclient-gometrics. If it is not called within the first 30s of startup, only the client latency metrics will be registered.