Repository navigation
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. DetailsNeeds 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. DetailsInstructions 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! |
| // 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
| func (r *resultAdapter) Increment(ctx context.Context, code, method, host string) { | ||
| 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
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
k8s-ci-robot
added
lifecycle/rotten
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
|
@k8s-triage-robot: Closed this PR. DetailsIn response to this:
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. |
…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.