This change is 

/assign @saad-ali @nicksardo

@k8s-bot non-cri e2e test this

@saad-ali -- assign to someone for review?

/assign @gnufied

Thanks for the fixes here @bowei. @gnufied from the storage-sig will review the storage changes.

CC @brancz to review on behalf of sig-instrumentation; @kubernetes/sig-storage-pr-reviews as FYI

cc @brancz

@bowei can you explain -

"Metrics label tuple would have resulted in many independent
histograms stored, one for each disk. (Did not aggregate well)." ?

The old metrics only had namespace as dimensions and this PR replaces that with zone and node as dimension. So likely old PR will create 1 time series for 1 metric but this PR will create 2000 time series for 1 metric.

Also --

"- Time duration tracked was of the initial API call, not the entire
operation."

What you mean by entire operation? Do you mean we capture error and success metrics separately?

1. If you look at the metrics created before this PR:

bowei@e2e-test-bowei-master ~ $ curl localhost:10252/metrics 2>/dev/null | grep _disk_ | grep Inf   
gce_attach_disk_duration_seconds_bucket{namespace="e2e-test-bowei-dynamic-pvc-02c15253-1f0f-11e7-8e35-42010a280002",le="+Inf"} 1
gce_attach_disk_duration_seconds_bucket{namespace="e2e-test-bowei-dynamic-pvc-1c0aba8f-1f0f-11e7-8e35-42010a280002",le="+Inf"} 1
gce_attach_disk_duration_seconds_bucket{namespace="e2e-test-bowei-dynamic-pvc-de1c20ab-1f0e-11e7-8e35-42010a280002",le="+Inf"} 1
... (many entries)

You will end up keeping a histogram for each PVC that is created.

The number of separate stats kept by Prometheus is the number of unique tuples for the given metric. For disk, I aggregate based on (zone, node), which means the metrics tracked will be the same as the number of nodes in the cluster.

After this change, we see the following, aggregating per node.

bowei@e2e-test-bowei-master ~ $ curl localhost:10252/metrics 2>/dev/null | grep cloud | grep Inf
cloudprovider_gce_disk_duration_seconds_bucket{function="attach",node="e2e-test-bowei-minion-group-3wd9",zone="us-central1-b",le="+Inf"} 1
cloudprovider_gce_disk_duration_seconds_bucket{function="attach",node="e2e-test-bowei-minion-group-52ql",zone="us-central1-b",le="+Inf"} 2
cloudprovider_gce_routes_duration_seconds_bucket{function="create",le="+Inf"} 41

2. Most of the GCP operations that change state return immediately after the change has been submitted, but the completion of the API call is signaled by polling the API for the state change. When you attach the duration timer to the HTTP transaction context, you will be timing the API submission, not the duration of the state change. We can track completion time of the HTTP request/response but it seems to be more interesting to track the time taken by the overall state change.

@gnufied responded

@bowei I don't have a prometheus install handy but depending on metrics scrapper, some dimensions (such as node) are automatically added when metrics are collected. It is entirely possible I am wrong. can you please check?

@gnufied: the Prometheus scraper will add the node the metric was scraped from, which in this case will be the master node where the controller was run, not the target of the operation (e.g. the node that the PV is being mounted on).

@bowei I think we have bunch of parallel threads going on about metric naming and dimensions. I opened a PR for aws here - #43477 in which @justinsb proposed that we report api action as a dimension rather than part of the metric name.

You have also dropped namespace from the reported metrics. I am not sure using e2e tests is a good example since it creates lot of ephemeral namespaces.

There is also a design proposal which should be amended if we are going to change metric names - https://github.com/kubernetes/community/blob/master/contributors/design-proposals/cloudprovider-storage-metrics.md

Can we have a quick call today sometime and discuss this offline? I would ask @justinsb to be present as well, so as we can discuss a common naming scheme for all cloudprovider metrics.

@bowei @justinsb does 1.00PM EST works for you guys?

Sorry I was on vacation, is there a summary of your offline discussion, or what was the outcome? Do you want me to hold off on reviewing for now?

Me, @chauncey(Jonathan) @bowei and @justinsb discussed this offline. Following things were finalized:

1. As agreed previously API action name will be moved to a dimension and will not be part of metric name.
2. The actual meaning of metrics between cloudproviders might be subtly different. For example - aws_create_disk could just mean time it took to submit the API request, whereas gce_create_disk could mean - time it took to submit the API request and waiting for API action to take effect. These low level metrics, shouldn't be used to measure relative performance of cloudprovider (namely - if creating a disk is faster in AWS or GCE). It was agreed that, it is upto cloudprovider maintainers to decide how they are going to emit metrics.
3. For now, we are going to drop the namespace requirement from the metrics. Namespace information isn't available in most call sites anyways. It was discussed that, we may amend metric emission to pass Context objects which can hold additional dimensions.

@justinsb @bowei let me know, if I missed anything.

Thanks @gnufied -- that is a good summary. Are you going to update the metrics doc as well?

@gnufied -- oh, did you guys decide on the specific naming?

"cloudprovider_{gce,aws,...}_{disk, }"?

Or something else...

@bowei we didn't really had a chance to get consensus around metric name. But I have updated the design proposal kubernetes/community#507 to use cloudprovider_gce_api_duration_seconds as a name and api_request as a dimension.

@gnufied please take a look, I updated the metrics to fit with the discussed metrics proposal

@bowei lets wait for @brancz to have a look by tomorrow. It looks all good from me.

Same thing applies here as I mentioned here.

@brancz @gnufied -- fixed the naming as requested

/lgtm

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, gnufied

Automatic merge from submit-queue (batch tested with PRs 44124, 44510)