Import k8s metrics stability framework #1844
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/M
|
cc @dgrisonnet |
|
/assign |
| @@ -52,6 +56,11 @@ func NewFamilyGenerator(name string, help string, metricType metric.Type, deprec | |||
| return f | |||
| } | |||
|
|
|||
| // NewFamilyGenerator creates new FamilyGenerator instances. | |||
| func NewFamilyGenerator(name string, help string, metricType metric.Type, deprecatedVersion string, generateFunc func(obj interface{}) *metric.Family) *FamilyGenerator { | |||
| return NewFamilyGeneratorWithStability(name, help, metricType, basemetrics.ALPHA, deprecatedVersion, generateFunc) | |||
There was a problem hiding this comment.
I'm not sure why all metrics are being set to alpha?
There was a problem hiding this comment.
Not all.
In replacing NewFamilyGenerator with NewFamilyGeneratorWithStability.
NewFamilyGenerator will be deleted once all replacements are done.
There was a problem hiding this comment.
Ah, okay. So is it right to say that for now we are setting just the node metrics to alpha? Also, what state were they in before? Experimental?
There was a problem hiding this comment.
It seems only two of these metrics were experimental before. So IIUC, if the node metrics are being set to alpha, should it only be for these two?
| kube_node_annotations | Gauge | Kubernetes annotations converted to Prometheus labels
| kube_node_role | Gauge | The role of a cluster node
There was a problem hiding this comment.
what state were they in before? Experimental?
Yes, they are mapped into k8s stability level:
Experimental -> Alpha
Stable -> Stable
So IIUC, if the node metrics are being set to alpha, should it only be for these two?
Working in progress for other stable metrics. Because there are many test codes need to be changed.
I planned to create 2+ CLs. The first CL (this CL) to import stability framework, other CLs migrate other NewFamilyGenerator
to NewFamilyGeneratorWithStability.
There was a problem hiding this comment.
Added all STABLE metrics just now.
There was a problem hiding this comment.
Thanks, if everything got refactored is it possible to keep the old function name?
There was a problem hiding this comment.
Oh, only stable metrics are using NewFamilyGeneratorWithStability now.
Will create another CL to migrate non-stable metrics to use NewFamilyGeneratorWithStability. Because this way has less merge conflicts in this CL.
k8s-ci-robot
added
size/L
CatherineF-dev
force-pushed
the
import-k8s-metrics-stability
branch
2 times, most recently
from
35dec67
to
dc23fad
Compare
k8s-ci-robot
added
size/XXL
CatherineF-dev
force-pushed
the
import-k8s-metrics-stability
branch
from
3e02fb7
to
1ae1e99
Compare
|
QQ: do we have other comments? Or could someone approve this CL? |
There was a problem hiding this comment.
One small comment from my side, but otherwise it looks good to me. Great work @CatherineF-dev 👍
k8s-ci-robot
added
the
needs-rebase
CatherineF-dev
force-pushed
the
import-k8s-metrics-stability
branch
from
1ae1e99
to
ea90693
Compare
k8s-ci-robot
removed
the
needs-rebase
CatherineF-dev
force-pushed
the
import-k8s-metrics-stability
branch
from
ea90693
to
5995c1e
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CatherineF-dev, dgrisonnet 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 |
k8s-ci-robot
added
the
approved
What this PR does / why we need it: #1833
How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality) No
Which issue(s) this PR fixes:
Fixes #1833