Add wrappers for prometheus.Counter/CounterVec and prometheus.Registry #77037
Conversation
|
/assign @brancz |
There was a problem hiding this comment.
This goes in the right direction I think. Could you just make sure that all the comments are full sentences, and are written in third person and present time (while during development the comments make sense as is, they won't make sense if we merge them and read them in 3 months :) ).
pkg/util/metrics/opts.go
Outdated
| type StabilityLevel string | ||
|
|
||
| const ( | ||
| ALPHA StabilityLevel = "ALPHA" |
There was a problem hiding this comment.
Should these be the only two? I feel like beta would be a useful state, where we say "we think these should be stable metrics, but we'd like to have everyone review this, and test it, and find out if it might still need tweaking".
There was a problem hiding this comment.
Yeah I mention beta state in the document. We can add beta state in a future iteration if there is a need for it. It’d be easy given the setup.
ead9369 to
1b3a7f0
Compare
11fdae4 to
0abbdd1
Compare
pkg/util/metrics/version_parser.go
Outdated
| ) | ||
|
|
||
| const ( | ||
| versionRegexpString = `^v(\d+\.\d+\.\d+)` |
There was a problem hiding this comment.
I don't know enough about the apimachineryversion.Info to know if we can rely on this.
@kubernetes/sig-api-machinery-pr-reviews
There was a problem hiding this comment.
I think it is really just legacy that it is actually called apimachineryversion.Info. The relevant scripts are build/release related.
cc/ @BenTheElder
4370d21 to
ce24dec
Compare
|
/test pull-kubernetes-verify |
|
|
||
| // DefaultGlobalRegistry is a stub for the global registry which prometheus client | ||
| // currently uses. | ||
| var DefaultGlobalRegistry = NewKubeRegistry() |
There was a problem hiding this comment.
we should start avoiding global variables. Each component should have such a registry.
There was a problem hiding this comment.
Yeah I agree. However it's mostly an orthogonal change to this particular effort. Definitely want to do it though.
|
/assign @sttts |
…. Also add wrapper around prometheus.Registry to customize control-flow
…on for registry version
8a5f27b to
91d3a79
Compare
|
Thanks for the great work! /lgtm |
staging/src/k8s.io/component-base/metrics/legacyregistry/registry.go
Outdated
Show resolved
Hide resolved
For future readers, the wrappers here appear to be for providing metadata about the metrics. /approve |
|
/hold holding for the condition check on second calls. I don't need a second look, feel free to remove the hold after handling it. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, logicalhan, sttts The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
re-applying /lgtm |
This PR implements the first part of the metrics control-plane stability KEP. Add initial wrappers around prometheus registry and prometheus counter metrics.
What type of PR is this?
/kind feature
/sig instrumentation