Bug 1958367: Add recording rule for builds by strategy #1136

adambkaplan · 2021-04-27T19:23:14Z

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:

docker
source
jenkinspipeline
custom

I added CHANGELOG entry for this change.
No user facing changes, so no entry in CHANGELOG was needed.

adambkaplan · 2021-04-27T19:29:29Z

If possible, we'd like this rule backported to 4.6 so we can measure build usage in the active fleet.

adambkaplan · 2021-04-27T19:29:41Z

/cc @siamaksade

simonpasquier · 2021-04-28T07:59:49Z

/hold
until the CI is fixed

openshift-ci-robot · 2021-04-28T12:32:17Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: adambkaplan, siamaksade
To complete the pull request process, please assign dgrisonnet after the PR has been reviewed.
You can assign the PR to them by writing /assign @dgrisonnet in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

adambkaplan · 2021-04-28T12:55:36Z

Reviewer note - recording rules with cluster:usage prefix are automatically exported to Telemeter.

@simonpasquier should I open a BZ so we can backport the recording rule?

sbose78 · 2021-04-28T20:41:20Z

jsonnet/rules.libsonnet

@@ -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',


dgrisonnet · 2021-05-05T15:34:19Z

@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 '{__name__=~"cluster:usage:.*"} in the telemeter config. I am not aware of the exact context behind this, but wouldn't it be better to allow each rule manually instead of grouping them with this regex? The benefit would be to improve the tractability of the ownership of these rules which is currently disregarded. cc @simonpasquier

Unholding since the CI has been fixed.
/unhold

openshift-ci · 2021-05-07T18:11:55Z

@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

bug is open, matching expected state (open)
bug target release (4.8.0) matches configured target release for branch (4.8.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @xiuwang

In response to this:

Bug Bug 1958367: Add recording rule for builds by strategy

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.

adambkaplan · 2021-05-07T18:27:42Z

/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.

adambkaplan · 2021-05-07T18:44:42Z

@simonpasquier ran into this issue trying to get make generate happy. #1153

xiuwang · 2021-05-11T04:20:38Z

@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

adambkaplan · 2021-05-13T19:54:05Z

@openshift/openshift-team-monitoring ptal

adambkaplan · 2021-05-25T12:14:53Z

/retest

adambkaplan · 2021-05-26T13:53:06Z

/retest

adambkaplan · 2021-05-26T19:51:54Z

Can we skip the single-node test for this PR?

adambkaplan · 2021-05-26T19:53:16Z

/retest

dgrisonnet · 2021-05-27T08:16:27Z

@adambkaplan the single-node job is optional

dgrisonnet · 2021-05-27T08:20:06Z

/lgtm

Leaving approval to @smarterclayton.

dgrisonnet · 2021-05-27T08:24:41Z

/remove-approve

simonpasquier · 2021-05-27T08:55:49Z

assets/cluster-monitoring-operator/prometheus-rule.yaml

@@ -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)


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.

Suggested change

- expr: sum by (strategy) (openshift_build_status_phase_total)

- expr: sum by (strategy) (rate(openshift_build_status_phase_total[5m]))

Is phase_total a counter? I thought it was a gauge? Agree if it's not a gauge this is a bit wierd.

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/

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

smarterclayton · 2021-05-27T13:43:18Z

Cardinality and appropriateness to be sent to telemetry approved, consider my part of approval granted.

dgrisonnet · 2021-06-01T08:56:28Z

/approve

openshift-ci · 2021-06-01T08:56:43Z

[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:

~~OWNERS~~ [dgrisonnet]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

dgrisonnet · 2021-06-01T08:56:53Z

/retest

openshift-bot · 2021-06-01T10:48:14Z

/retest