generic ephemeral volume: add metrics #99115

pohly · 2021-02-16T12:58:32Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

As discussed during the production readiness review, a metric for the
PVC create operations is useful. The "ephemeral_volume" workqueue
metrics were already added in the initial implementation.

Special notes for your reviewer:

The new code follows the example set by the endpoints controller.

Does this PR introduce a user-facing change?

added `ephemeral_volume_controller_create[_failures]_total` counters to kube-controller-manager metrics

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/pohly/enhancements/blob/master/keps/sig-storage/1698-generic-ephemeral-volumes/README.md#monitoring-requirements

/sig storage

pohly · 2021-02-16T20:14:49Z

/retest

jsafrane · 2021-02-19T16:49:16Z

/assign @gnufied

gnufied · 2021-02-22T17:49:11Z

pkg/controller/volume/ephemeral/metrics/metrics.go

+			StabilityLevel: metrics.ALPHA,
+		},
+		[]string{"reason"},
+	)


Can you provide an example of how metrics appear in prometheus?

My tentative update of the KEP has such examples: https://github.com/kubernetes/enhancements/pull/2513/files

Is that what you were looking for or shall I add it here, for example to the commit message?

ah nvm. I also see this captured in release-notes.

gnufied · 2021-02-22T18:01:24Z

Before we merge this change - do we know this metric can be extracted from api-server logs or api-server somehow? I would think that api-server logs should contain some information about incoming PVC creation requests (may not be as metric directly).

pohly · 2021-02-22T18:27:25Z

I would think that api-server logs should contain some information about incoming PVC creation requests (may not be as metric directly).

The ask from the production readiness review was explicitly to add metrics. Extracting the same information via log analysis might work, but it's simply not the same thing.

gnufied · 2021-02-22T18:35:22Z

cc @kubernetes/sig-instrumentation-members @logicalhan

gnufied · 2021-02-22T19:31:41Z

The ask from the production readiness review was explicitly to add metrics. Extracting the same information via log analysis might work, but it's simply not the same thing.

I understand. I am bit concerned about creating approximately 20-25 new timeseries metric because reason is being used a a dimension.

pohly · 2021-02-22T20:13:31Z

I am bit concerned about creating approximately 20-25 new timeseries metric because reason is being used a a dimension.

I have no strong opinion either way. I can implement whatever is considered appropriate for this feature. We can also reduce the status to just success/failure, status just seemed a bit more flexible. If we use a boolean, what are a good name and values for such a dimension?

brancz · 2021-02-23T08:54:04Z

pkg/controller/volume/ephemeral/controller.go

+			reason = "Unknown"
+		}
+	}
+	ephemeralvolumemetrics.EphemeralVolumeCreate.WithLabelValues(reason).Inc()


Is reason practically bound here? What kind of values are we expecting here for this to be valuable?

brancz · 2021-02-23T08:55:46Z

pkg/controller/volume/ephemeral/controller.go

@@ -279,6 +283,14 @@ func (ec *ephemeralController) handleVolume(pod *v1.Pod, vol v1.Volume) error {
 		Spec: ephemeral.VolumeClaimTemplate.Spec,
 	}
 	_, err = ec.kubeClient.CoreV1().PersistentVolumeClaims(pod.Namespace).Create(context.TODO(), pvc, metav1.CreateOptions{})
+	reason := ""


Having reason possible to be an empty string is going to make querying this metric awkward. I think we probably want a label that distinguishes between success and failure, or have separate metrics for each of those.

I'm fine with a label success that has values yes and no. Is that okay or are there better names? I am not sure what is used elsewhere.

The best practice is to have two metrics one for failures and one for all. Then you can naturally query failures / total and understand the percentage. The number of series you produce is identical, so this doesn't produce any more load or anything on the metrics system than what we already have here.

I've changed to to count ephemeral_volume_controller_create_total and ephemeral_volume_controller_create_failures. Is that okay?

Note that my updated tests initially failed because GetCounterMetricValue didn't work for a Counter. I'm including the fix for that (= adding the missing Metric interface implementation).

Yep, that works!

fedebongio · 2021-02-23T21:06:11Z

/remove-sig api-machinery

pohly · 2021-02-23T22:22:11Z

/retest

pohly · 2021-03-02T07:14:15Z

@gnufied I had to rebase to get rid of the modified BUILD file. Can you lgtm again?

@brancz Can I get an lgtm from you, too? logicalhan is out.

pohly · 2021-03-02T07:15:15Z

/remove-sig api-machinery

brancz · 2021-03-02T08:44:13Z

/lgtm
/approve

pohly · 2021-03-02T08:56:19Z

/retest

brancz

Just a tiny naming change to align with guidelines

brancz · 2021-03-02T10:00:42Z

pkg/controller/volume/ephemeral/metrics/metrics.go

+	EphemeralVolumeCreateFailures = metrics.NewCounter(
+		&metrics.CounterOpts{
+			Subsystem:      EphemeralVolumeSubsystem,
+			Name:           "create_failures",


I apologize for not catching this earlier. The guidelines are to always end counters with _total so this one needs that as well.

Done, please take another look.

brancz · 2021-03-02T10:27:09Z

/lgtm
/approve

k8s-ci-robot · 2021-03-02T10:27:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brancz, pohly

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:

~~pkg/controller/volume/ephemeral/OWNERS~~ [pohly]
~~staging/src/k8s.io/component-base/metrics/OWNERS~~ [brancz]

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

As discussed during the production readiness review, a metric for the PVC create operations is useful. The "ephemeral_volume" workqueue metrics were already added in the initial implementation. The new code follows the example set by the endpoints controller.

GetCounterMetricValue has an unchecked conversion to metrics.Metric, something that didn't work for a *Counter because it didn't implement that interface.

A CounterVector with status as label may create unnecessary overhead and using the success case with the empty label value wasn't easy. It's better to have two seperate counters, one for total number of calls and one for failed calls.

pohly · 2021-03-02T11:04:14Z

/retest

brancz · 2021-03-02T12:14:36Z

/lgtm

fejta-bot · 2021-03-02T15:01:14Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2021-03-02T17:49:17Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot requested review from jsafrane and msau42 February 16, 2021 12:59

k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 16, 2021

pohly mentioned this pull request Feb 16, 2021

generic ephemeral volumes: document metrics and tests kubernetes/enhancements#2513

Merged

pohly force-pushed the ephemeral-volume-metrics branch from 6461c73 to a4174a7 Compare February 16, 2021 14:19

k8s-ci-robot assigned gnufied Feb 19, 2021

gnufied reviewed Feb 22, 2021

View reviewed changes

brancz reviewed Feb 23, 2021

View reviewed changes

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Feb 23, 2021

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 2, 2021

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Mar 2, 2021

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Mar 2, 2021

k8s-ci-robot added lgtm "Looks good to me", 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 Mar 2, 2021

brancz reviewed Mar 2, 2021

View reviewed changes

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 2, 2021

pohly force-pushed the ephemeral-volume-metrics branch from 5ef4b7d to c3add2d Compare March 2, 2021 10:18

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 2, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 2, 2021

pohly added 3 commits March 2, 2021 12:01

k8s.io/component-base: support GetCounterMetricValue for Counter

ef6f65b

GetCounterMetricValue has an unchecked conversion to metrics.Metric, something that didn't work for a *Counter because it didn't implement that interface.

generic ephemeral volume: simpler metrics

98f7529

A CounterVector with status as label may create unnecessary overhead and using the success case with the empty label value wasn't easy. It's better to have two seperate counters, one for total number of calls and one for failed calls.

pohly force-pushed the ephemeral-volume-metrics branch from c3add2d to 98f7529 Compare March 2, 2021 11:11

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 2, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 2, 2021

pohly mentioned this pull request Mar 2, 2021

generic ephemeral inline volumes kubernetes/enhancements#1698

Closed

k8s-ci-robot merged commit d1a2af5 into kubernetes:master Mar 2, 2021

k8s-ci-robot added this to the v1.21 milestone Mar 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

generic ephemeral volume: add metrics #99115

generic ephemeral volume: add metrics #99115

pohly commented Feb 16, 2021 •

edited

pohly commented Feb 16, 2021

jsafrane commented Feb 19, 2021

gnufied Feb 22, 2021

pohly Feb 22, 2021

gnufied Feb 22, 2021

gnufied commented Feb 22, 2021

pohly commented Feb 22, 2021

gnufied commented Feb 22, 2021

gnufied commented Feb 22, 2021

pohly commented Feb 22, 2021

brancz Feb 23, 2021

brancz Feb 23, 2021 •

edited

pohly Feb 23, 2021

brancz Feb 23, 2021

pohly Feb 23, 2021 •

edited

brancz Feb 24, 2021

fedebongio commented Feb 23, 2021

pohly commented Feb 23, 2021

pohly commented Mar 2, 2021

pohly commented Mar 2, 2021

brancz commented Mar 2, 2021

pohly commented Mar 2, 2021

brancz left a comment

brancz Mar 2, 2021

pohly Mar 2, 2021

brancz commented Mar 2, 2021

k8s-ci-robot commented Mar 2, 2021

pohly commented Mar 2, 2021

brancz commented Mar 2, 2021

fejta-bot commented Mar 2, 2021

fejta-bot commented Mar 2, 2021

generic ephemeral volume: add metrics #99115

generic ephemeral volume: add metrics #99115

Conversation

pohly commented Feb 16, 2021 • edited

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

pohly commented Feb 16, 2021

jsafrane commented Feb 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnufied commented Feb 22, 2021

pohly commented Feb 22, 2021

gnufied commented Feb 22, 2021

gnufied commented Feb 22, 2021

pohly commented Feb 22, 2021

Choose a reason for hiding this comment

brancz Feb 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly Feb 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedebongio commented Feb 23, 2021

pohly commented Feb 23, 2021

pohly commented Mar 2, 2021

pohly commented Mar 2, 2021

brancz commented Mar 2, 2021

pohly commented Mar 2, 2021

brancz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brancz commented Mar 2, 2021

k8s-ci-robot commented Mar 2, 2021

pohly commented Mar 2, 2021

brancz commented Mar 2, 2021

fejta-bot commented Mar 2, 2021

fejta-bot commented Mar 2, 2021

pohly commented Feb 16, 2021 •

edited

brancz Feb 23, 2021 •

edited

pohly Feb 23, 2021 •

edited