BUILD-345: Added csi shared resource metrics to telemetry by alicerum · Pull Request #1477 · openshift/cluster-monitoring-operator

alicerum · 2021-11-12T11:31:46Z

We need to receive some new metrics from the telemetry, for CSI shared
resource driver and operator. Amount of shared resourced (secrets and
config maps) and amount of shares mapped to pods, both succeeded and
failed.

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

alicerum · 2021-11-12T12:09:33Z

/assign @jan--f

alicerum · 2021-11-12T18:44:40Z

/retest

bwplotka

LGTM, just docs nits.

bwplotka · 2021-11-15T11:43:15Z

+    #
+    # owners: (@openshift/team-build-api)
+    #
+    # openshift:openshift_csi_share_configmap_total:sum measures total amount of secrets shared by csi shared resource driver.


Suggested change

# openshift:openshift_csi_share_configmap_total:sum measures total amount of secrets shared by csi shared resource driver.

# openshift_csi_share_secret_total measures total amount of secrets shared by csi shared resource driver.

bwplotka · 2021-11-15T11:43:50Z

+    #
+    # owners: (@openshift/team-build-api)
+    #
+    # openshift:openshift_csi_share_configmap_total:sum measures total amount of secrets shared by csi shared resource driver.


Suggested change

# openshift:openshift_csi_share_configmap_total:sum measures total amount of secrets shared by csi shared resource driver.

# openshift_csi_share_secret_total measures total amount of secrets shared by csi shared resource driver.

simonpasquier · 2021-11-16T09:29:15Z

+    # owners: (@openshift/team-build-api)
+    #
+    # openshift:openshift_csi_share_configmap_total:sum measures total amount of config maps shared by csi shared resource driver.
+    - '{__name__="openshift_csi_share_configmap_total"}'


this looks like a gauge so it shouldn't have the _total suffix (reserved for counters). openshift_csi_shared_configmaps would be a better name.

we actuall use total suffix for a bunch of other gauges. should i go back to the operator and change this metric name everywhere? this will require another pr and we weirdly don't have consistency here.

Ideally i'd recommend that you update all metrics and make them consistent with the instrumentation guidelines. Now I understand that it may not be possible for the project.

simonpasquier · 2021-11-16T09:29:40Z

+    #
+    # owners: (@openshift/team-build-api)
+    #
+    # openshift:openshift_csi_share_configmap_total:sum measures total amount of config maps shared by csi shared resource driver.


this doesn't match the metric name

i think i forgot to rebuild the docs. thanks!

simonpasquier · 2021-11-16T09:34:25Z

+    # owners: (@openshift/team-build-api)
+    #
+    # openshift:openshift_csi_share_mount_total:sum measures total amount of csi shared resources mounted in the pods.
+    - '{__name__="openshift_csi_share_mount_total"}'


about this metric, it's usually better to track requests and failures in different counters (see https://promcon.io/2017-munich/slides/best-practices-and-beastly-pitfalls.pdf, page 10). For instance, openshift_csi_share_mount_requests_total and openshift_csi_share_mount_failures_total.

I don't see this recommendation to use different metrics instead of different labels in the slides. Slides do suggest that it's better to use failure_total and requests_total instead of failure_total and success_total though. Do you think we should go all the way back to already merged metrics and change them again?
@gabemontero wdyt?

Again i would recommend that you follow the recommendations. The idea of tracking requests and failures is that you can later alert on the percentage of operations failing.

This was discussed on the PR introducing this and I recommended the label way of solving this.

Calculating error rates should still be quite convenient with this setup rate(openshift_csi_share_mount_failures_total{succeeded="false"}[5m]) / rate(openshift_csi_share_mount_failures_total[5m]), though admittedly not as short as following the best practice.

@alicerum Since I approved the current state, I'd be happy to propose the change in the exporter.

@jan--f After discussion with colleagues we decided it's better to go along with best practices, so i'm going to split them into total and failed series without labels. Thank you!

See openshift/csi-driver-shared-resource#78 if you prefer to go with @simonpasquier's recommendation.

simonpasquier · 2021-11-16T09:34:33Z

+    # owners: (@openshift/team-build-api)
+    #
+    # openshift:openshift_csi_share_configmap_total:sum measures total amount of secrets shared by csi shared resource driver.
+    - '{__name__="openshift_csi_share_secret_total"}'


alicerum · 2021-12-15T07:13:35Z

@jan--f please take a look at this, I've updated the metrics according to their new names and rebased the PR.

jan--f · 2021-12-20T08:18:20Z

/lgtm

We need to receive some new metrics from the telemetry, for CSI shared resource driver and operator. Amount of shared resourced (secrets and config maps) and amount of shares mapped to pods, both succeeded and failed.

alicerum · 2022-01-04T08:39:16Z

/retest

alicerum · 2022-01-04T18:43:57Z

/retest

openshift-ci · 2022-01-04T20:30:19Z

@alicerum: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-single-node	`becd444`	link	false	`/test e2e-aws-single-node`

Full PR test history. Your PR dashboard.

Details

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.

simonpasquier · 2022-01-05T13:12:03Z

/skip

gabemontero · 2022-01-05T15:14:22Z

/assign @prietyc123

for qe-approved label
I do no think there is a qe impact for you, as development and PM will be consuming telemetry for insight into future work. @adambkaplan of course chime in if you disagree.

/assign @rolfedh
for docs approved label .... looking at the existing docs, there is just a general description of what telemetry provides, with no discussion of what specific components are enabled, so there should be no docs impact for this

/label px-approved

gabemontero · 2022-01-05T15:16:51Z

@simonpasquier @jan--f @bwplotka given your prior efforts reviewing this, I'm inclined to let one of you do the LGTM, but if you all are otherwise too busy, I think @alicerum has pulled in you comments, and I can do it.

thanks

rolfedh · 2022-01-05T18:16:51Z

/label docs-approved

prietyc123 · 2022-01-06T06:09:35Z

/label qe-approved

jan--f · 2022-01-06T08:01:45Z

Thanks, this looks great!
/lgtm

openshift-ci · 2022-01-06T08:02:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alicerum, bwplotka, jan--f

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [jan--f]

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

gabemontero · 2022-01-06T15:26:14Z

/label docs-approved

Fleet capability choices should make their way up to Red Hat via Insights uploads. But some clusters have Telemetry enabled but Insights disabled [1]. For these clusters, we'll want this data in Telemetry, and getting it into metrics (this commit) is the first step. After this lands, we'll need to update the monitoring operator like [2] to get these shipped up off of cluster. [1]: openshift/enhancements#922 (comment) [2]: openshift/cluster-monitoring-operator#1477

openshift-ci Bot requested review from raptorsun and slashpai November 12, 2021 11:32

openshift-ci Bot assigned jan--f Nov 12, 2021

bwplotka approved these changes Nov 15, 2021

View reviewed changes

simonpasquier reviewed Nov 16, 2021

View reviewed changes

openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 23, 2021

jan--f mentioned this pull request Nov 24, 2021

metrics: split openshift_csi_share_mount_total into total and failures openshift/csi-driver-shared-resource#78

Merged

alicerum force-pushed the csi-shared-resource-metrics branch 2 times, most recently from bb908ed to cf52b18 Compare December 14, 2021 16:36

openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 14, 2021

alicerum force-pushed the csi-shared-resource-metrics branch from cf52b18 to 55dd2be Compare December 14, 2021 16:46

openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 18, 2021

openshift-ci Bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 20, 2021

Added csi shared resource metrics to telemetry

becd444

We need to receive some new metrics from the telemetry, for CSI shared resource driver and operator. Amount of shared resourced (secrets and config maps) and amount of shares mapped to pods, both succeeded and failed.

alicerum force-pushed the csi-shared-resource-metrics branch from 55dd2be to becd444 Compare January 3, 2022 12:04

openshift-ci Bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 3, 2022

openshift-ci Bot assigned prietyc123 and rolfedh Jan 5, 2022

openshift-ci Bot added the px-approved Signifies that Product Support has signed off on this PR label Jan 5, 2022

openshift-ci Bot added the qe-approved Signifies that QE has signed off on this PR label Jan 6, 2022

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jan 6, 2022

openshift-ci Bot added the docs-approved Signifies that Docs has signed off on this PR label Jan 6, 2022

openshift-merge-robot merged commit 375ec36 into openshift:master Jan 6, 2022

wking mentioned this pull request Mar 23, 2022

pkg/cvo/metrics: Add a cluster_version_capability metric openshift/cluster-version-operator#755

Merged

	# openshift:openshift_csi_share_configmap_total:sum measures total amount of secrets shared by csi shared resource driver.
	# openshift_csi_share_secret_total measures total amount of secrets shared by csi shared resource driver.

Uh oh!

Conversation

alicerum commented Nov 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alicerum commented Nov 12, 2021

Uh oh!

alicerum commented Nov 12, 2021

Uh oh!

bwplotka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alicerum Nov 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alicerum commented Dec 15, 2021

Uh oh!

jan--f commented Dec 20, 2021

Uh oh!

alicerum commented Jan 4, 2022

Uh oh!

alicerum commented Jan 4, 2022

Uh oh!

openshift-ci Bot commented Jan 4, 2022

Uh oh!

simonpasquier commented Jan 5, 2022

Uh oh!

gabemontero commented Jan 5, 2022

Uh oh!

gabemontero commented Jan 5, 2022

Uh oh!

rolfedh commented Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prietyc123 commented Jan 6, 2022

Uh oh!

jan--f commented Jan 6, 2022

Uh oh!

openshift-ci Bot commented Jan 6, 2022

Uh oh!

gabemontero commented Jan 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

alicerum commented Nov 12, 2021 •

edited

Loading

alicerum Nov 23, 2021 •

edited

Loading

rolfedh commented Jan 5, 2022 •

edited

Loading