-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: prepare for stable metrics static analysis by defining metric labels with same format #2258
Conversation
cc @dgrisonnet to review |
Forcing a particular coding style will make it a bit harder for contributors, but if we really have to do that, I am fine with it. Ideally if we go that route, we should reflect it in https://github.com/kubernetes/kube-state-metrics/blob/main/docs/developer/guide.md, to documented the new coding style. Also, there are a bunch more areas of the code that has this kind of definition, so it would be better to do it in one swoop: https://github.com/search?q=repo%3Akubernetes%2Fkube-state-metrics+LabelKeys%3A+%5B%5Dstring&type=code |
Similar to static analysis in k/k repo, it extracts all metric labels for each STABLE metric and generate a metric file containing metric name and metric labels. During presubmit, it regenerates this file and compare with previously generated file. Since the way of defining metric labels are constrained a little, it's easier to extract metric labels. For example, it will get label "container" for this metric.
|
3b9b607
to
08772b3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
08772b3
to
51de33f
Compare
New changes are detected. LGTM label has been removed. |
51de33f
to
7b457fd
Compare
Okay, will update other files:
|
cc @mrueg for review. |
93a2aa4
to
cda4a15
Compare
Yeah I know this is the general goal, but could you explain how this refactor makes it easier to parse? |
A good question. Before kube-state-metrics/internal/store/pod.go Lines 136 to 164 in 14e935a
Now kube-state-metrics/internal/store/pod.go Lines 137 to 167 in 5ff07b5
kube-state-metrics/internal/store/pod.go Lines 136 to 164 in 14e935a
kube-state-metrics/internal/store/endpoint.go Lines 167 to 195 in 14e935a
kube-state-metrics/internal/store/replicaset.go Lines 167 to 211 in 14e935a
kube-state-metrics/internal/store/service.go Lines 59 to 76 in 14e935a
cc @dgrisonnet |
5ff07b5
to
37015ff
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: CatherineF-dev, logicalhan 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 |
Will use #2294 instead. It's an end-to-end example on verifying stable metrics. |
…ls with same format
What this PR does / why we need it:
KSM metrics are defined freely, which increases the difficulties of static analysis.
Will add a static analysis to make sure each metric defines label in this way:
How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality) N/A
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #1833
[x] certificatesigningrequest.go
[x] clusterrolebinding.go
[x] clusterrole.go
[x] configmap.go
[x] cronjob.go
[x] daemonset.go
[x] deployment.go
[x] endpoint.go
[x] endpointslice.go
[x] horizontalpodautoscaler.go
[x] ingressclass.go
[x] ingress.go
[x] job.go
[x] lease.go
[x] limitrange.go
[x] mutatingwebhookconfiguration.go
[x] namespace.go
[x] networkpolicy.go
[x] node.go
[x] persistentvolumeclaim.go
[x] persistentvolume.go
[x] poddisruptionbudget.go
[x] pod.go
[x] replicaset.go
[x] replicationcontroller.go
[x] resourcequota.go
[x] rolebinding.go
[x] role.go
[x] secret.go
[x] serviceaccount.go
[x] service.go
[x] statefulset.go
[x] storageclass.go
[x] validatingwebhookconfiguration.go
[x] volumeattachment.go