metrics: support usage inside CSI driver

[pohly]

What type of PR is this?
/kind feature

What this PR does / why we need it:

A CSI driver is a gRPC server, which implies that the interceptor must
use a slightly different API. The extended CSIMetricsManager gets
exported with a suitable method for that.

To avoid counting the same operation twice in the same metric, CSI
sidecar and driver should use different subsystem names. CSI driver
authors may decide to declare metrics support as "stable" based on
other criteria than the Kubernetes-CSI sidecars. Both aspects are now
configurable.

Special notes for your reviewer:

These API changes are backwards-compatible.

Does this PR introduce a user-facing change?:

metrics support can also be used by CSI drivers

[pohly]

/assign @msau42

Can you perhaps review? This is based on what we discussed on Slack, so you have some context here.

[msau42]

So the order of the labels has to be in the same order as the keys? Would it be better to have a map passed in and order by keys?

Should we validate that the length of the values is the same as the length of the keys?

[pohly]

So the order of the labels has to be in the same order as the keys?

Yes.

Would it be better to have a map passed in and order by keys? Should we validate that the length of the values is the same as the length of the keys?

We could make the call more robust against incorrect usage; both of this would work. Map corresponds to the GetMetricWith method (https://godoc.org/github.com/prometheus/client_golang/prometheus#CounterVec.GetMetricWith), checking the length to With (https://godoc.org/github.com/prometheus/client_golang/prometheus#CounterVec.With). Either way, we then also should introduce an error return value and expect that the callers handle this.

I don't have a strong preference either way.

[pohly]

One further comment: labelValues ...string was also chosen because it's compatible with current code which doesn't pass any additional labels. We could introduce WithLabels and/or WithLabelValues methods that return a CSIMetricsManager which uses the additional labels when its RecordMetrics is invoked. RecordMetrics itself then has the same parameters as it has before this PR.

This is probably the best solution, but also the most complicated one. I'm wondering whether it is overkill.

@msau42 do you have any preferences?

[msau42]

I think it's less brittle to not depend on the ordering of the values to match what was configured. If we need to change the interface to return an error, then I would return a new method for those drivers that want to use this customization

[pohly]

I revised the API such that RecordMetrics is as before and additional labels get injects as in Prometheus with a wrapper. I also added tests.

[msau42]

Is this needed or can you just verify the lengths at the end (and keep the duplicate values check)

[pohly]

I replaced it with a single loop.

[msau42]

I'm not sure if we can make the assumption that the missing labels are all at the end.

[msau42]

I think we can avoid all of this logic if we just pass in the labels as a map generated in WithLabelValues()

[pohly]

You are right, that makes the code simpler. Change pushed.

[msau42]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msau42, 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:

• OWNERS [msau42,pohly]

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