-
Notifications
You must be signed in to change notification settings - Fork 361
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
Bug 1958367: Add recording rule for builds by strategy #1136
Bug 1958367: Add recording rule for builds by strategy #1136
Conversation
If possible, we'd like this rule backported to 4.6 so we can measure build usage in the active fleet. |
/cc @siamaksade |
/hold |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: adambkaplan, siamaksade 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 |
Reviewer note - recording rules with @simonpasquier should I open a BZ so we can backport the recording rule? |
jsonnet/rules.libsonnet
Outdated
@@ -485,6 +485,10 @@ local droppedKsmLabels = 'endpoint, instance, job, pod, service'; | |||
expr: 'sum(openshift_build_total{job="kubernetes-apiservers",phase="Error"})/(sum(openshift_build_total{job="kubernetes-apiservers",phase=~"Failed|Complete|Error"}))', | |||
record: 'build_error_rate', | |||
}, | |||
{ | |||
expr: 'sum by (strategy) (openshift_build_status_phase_total)', | |||
record: 'cluster:usage:openshift:build_by_strategy:sum', |
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.
👍
@adambkaplan, we will need a BZ for 4.8, 4.7 and 4.6. Since this PR is adding a new recording rule to telemeter, you'll need to add a comment about which team owns this rule as in https://github.com/openshift/cluster-monitoring-operator/blob/master/manifests/0000_50_cluster-monitoring-operator_04-config.yaml. The rule will also need to be validated by @smarterclayton before we can proceed. On a side note about allowing Unholding since the CI has been fixed. |
@adambkaplan: This pull request references Bugzilla bug 1958367, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
adb3b45
to
8c67e27
Compare
/assign @smarterclayton @dgrisonnet given that I'll need to add a comment to the telemeter config, I've renamed the rule so that we explicitly need to export it. |
@simonpasquier ran into this issue trying to get |
@adambkaplan Add a case for this feature OCP-41751 . Test this feature on cluster launched from pr, the metrics works well. /label qe-approved |
Aggregate the number of builds run on a cluster by strategy, and export the recording rule to Telemeter. This will help the Build API team determine which build strategies are actively being used on a cluster. The recording rule will produce a time series with the following strategy labels: - docker - source - jenkinspipeline - custom Updated documentation and telemeter_query
8c67e27
to
ec26e16
Compare
@openshift/openshift-team-monitoring ptal |
/retest |
/retest |
Can we skip the single-node test for this PR? |
/retest |
@adambkaplan the single-node job is optional |
/lgtm Leaving approval to @smarterclayton. |
/remove-approve |
@@ -368,6 +368,8 @@ spec: | |||
rules: | |||
- expr: sum(openshift_build_total{job="kubernetes-apiservers",phase="Error"})/(sum(openshift_build_total{job="kubernetes-apiservers",phase=~"Failed|Complete|Error"})) | |||
record: build_error_rate | |||
- expr: sum by (strategy) (openshift_build_status_phase_total) |
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.
one issue with summing counters is that the numbers would be off when a counter resets. It is recommended to rate/increase then sum rather.
- expr: sum by (strategy) (openshift_build_status_phase_total) | |
- expr: sum by (strategy) (rate(openshift_build_status_phase_total[5m])) |
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.
Is phase_total a counter? I thought it was a gauge? Agree if it's not a gauge this is a bit wierd.
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.
from what I can tell from the prometheus target metadata, it is a gauge. @adambkaplan usually the naming convention for Prometheus says that the _total
suffix is reserved to counters. The correct naming for your metric should be openshift_build_status_phases
since it's a gauge.
https://prometheus.io/docs/practices/naming/
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.
Metric is collected via openshift-state-metrics
- it's been around since 3.x and is marked STABLE in our GitHub docs. I'm fairly certain this name was chosen before the naming conventions were codified upstream.
https://github.com/openshift/openshift-state-metrics/blob/master/docs/build-metrics.md
Cardinality and appropriateness to be sent to telemetry approved, consider my part of approval granted. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan, dgrisonnet, siamaksade 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 |
/retest |
/retest Please review the full test history for this PR and help us cut down flakes. |
6 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
@adambkaplan: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@adambkaplan: All pull requests linked via external trackers have merged: Bugzilla bug 1958367 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cherry-pick release-4.7 |
@adambkaplan: #1136 failed to apply on top of branch "release-4.7":
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Aggregate the number of builds run on a cluster by strategy. This will
help the Build API team determine which build strategies are actively
being used on a cluster. This will produce a time series with the
following strategy labels: