-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
separate openshift_template_instance_status_condition_total and openshift_template_instance_total metrics #16588
separate openshift_template_instance_status_condition_total and openshift_template_instance_total metrics #16588
Conversation
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.
A couple of comments basically just soliciting @smarterclayton's confirmation that this is really what we want to do, but if so then it lgtm.
pkg/template/controller/metrics.go
Outdated
Name: "openshift_template_instance_total", | ||
Help: "Counts TemplateInstance objects by condition type and status", | ||
Name: "openshift_template_instance_info", | ||
Help: "Information about templateinstance.", |
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.
i know this is copied from KSM but i just want to call @smarterclayton's attention to it because it seems like an unhelpful name+description for a metric that is really "total number of template instances".
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.
Lack of sleep may be impairing my judgment, but I'll say it:, KSM is just wrong IMO in this regard; it should end in _total
pkg/template/controller/metrics.go
Outdated
templateInstancesTotal.WithLabelValues(string(cond.Type), string(cond.Status)).Inc() | ||
templateInstanceStatusCondition.WithLabelValues(templateInstance.Namespace, templateInstance.Name, string(cond.Type), "true").Set(boolFloat64(cond.Status == kapi.ConditionTrue)) | ||
templateInstanceStatusCondition.WithLabelValues(templateInstance.Namespace, templateInstance.Name, string(cond.Type), "false").Set(boolFloat64(cond.Status == kapi.ConditionFalse)) | ||
templateInstanceStatusCondition.WithLabelValues(templateInstance.Namespace, templateInstance.Name, string(cond.Type), "unknown").Set(boolFloat64(cond.Status == kapi.ConditionUnknown)) |
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.
again i understand (after talking to @jim-minter) that this follows KSM, but it seems weird to guarantee we are going to report a datapoint for every combination of condition values, for each template instance. Reporting "0" doesn't seem terribly interesting for a condition=foo if we're also reporting "1" for condition=bar.
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.
(and on that same note it seems heavy handed to report N datapoints for every template instance, rather than just a few datapoints that roll up the totals for each condition). Again, understand this follows the KSM pattern, but want to call @smarterclayton's attention to it.
…hift_template_instance_total metrics
1b00e63
to
0fb610f
Compare
@bparees @smarterclayton updated... |
@smarterclayton so the reason we (or at least I?) prefer this implementation is that creating a metric per condition would mean having to change metric code if we add new conditions. Whereas this implementation is dynamic w/ respect to the set of valid conditions. So given the choice of 2 metrics (one for total, one that includes the condition+conditional_value labels) or N metrics (one metric per condition, each metric having a condition_value label), I prefer 2 metrics. |
/retest |
/retest |
1 similar comment
/retest |
@bparees bump |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bparees, jim-minter The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Automatic merge from submit-queue. |
follow-up from #16455
@smarterclayton @bparees ptal
@gabemontero fyi