feat: include reason label to kube_deployment_status_condition
#2719
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
needs-triage
internal/store/deployment.go
Outdated
| metric.LabelValues = append([]string{string(c.Type)}, metric.LabelValues...) | ||
| reason := c.Reason | ||
| if _, ok := allowedDeploymentReasons[reason]; !ok { | ||
| reason = "" |
There was a problem hiding this comment.
should this be empty? or should this be "unknown"
CC: @rexagod
There was a problem hiding this comment.
I went with an empty string for cleaner queries, but I'm open to changing it if you want.
There was a problem hiding this comment.
it depends, is the reason ever empty? then we could use unknown to differ between an empty reason and something we don't recognize.
if the reason field is never empty, this approach is fine.
There was a problem hiding this comment.
yeah right, seems like reason field can be empty, I've changed it to unknown, thanks for pointing out!
|
/assign @mrueg |
k8s-ci-robot
added
triage/accepted
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Rishab87 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 |
|
@mrueg I've made the changes, can you have a look again? |
| metric.LabelKeys = []string{"condition", "status"} | ||
| metric.LabelValues = append([]string{string(c.Type)}, metric.LabelValues...) | ||
| reason := c.Reason | ||
| if _, ok := allowedDeploymentReasons[reason]; !ok { |
There was a problem hiding this comment.
if it's empty, will it still be unknown?
Can you create a test with the reason field being empty as well as with one test with a non allowed deployment reason?
There was a problem hiding this comment.
yeah that was the case earlier, fixed it if its empty then it will remain empty and will show unknown if reason is not in the allowed list, also added the tests for both cases.
|
@mrueg made changes, can you have another look? |
internal/store/deployment.go
Outdated
| @@ -183,8 +183,10 @@ func deploymentMetricFamilies(allowAnnotationsList, allowLabelsList []string) [] | |||
| metric := m | |||
|
|
|||
| reason := c.Reason | |||
| if _, ok := allowedDeploymentReasons[reason]; !ok { | |||
| reason = "unknown" | |||
| if reason != "" { | |||
There was a problem hiding this comment.
Can we add "" to the allowedDeploymentReasons instead?
|
@mrueg I've addressed the reviews, can you look again? |
| var ( | ||
| allowedDeploymentReasons = map[string]struct{}{ | ||
| "MinimumReplicasAvailable": {}, |
There was a problem hiding this comment.
Looking at these strings again
https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/deployment/util/deployment_util.go#L67
Can we refer to these here instead and create a complete list based on what's available in deployment_util.go?
| var ( | |
| allowedDeploymentReasons = map[string]struct{}{ | |
| "MinimumReplicasAvailable": {}, | |
| import deploymentutil "k8s.io/kubernetes/pkg/controller/deployment/util" | |
| var ( | |
| allowedDeploymentReasons = map[string]struct{}{ | |
| deploymentutil.MinimumReplicasAvailable: {}, |
What this PR does / why we need it:
Building upon the work of #2146, I've addressed the reviews and created this.
This PR adds reason label to
kube_deployment_status_conditionmetrics.It's necessary to distinguish between Progressing Deployment and Complete Deployment .
How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)
no changes in cardinality
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 #1991