Implement API usage metrics for gce storage #40338

gnufied · 2017-01-24T02:20:24Z

What this PR does / why we need it:

This PR implements support for emitting metrics from GCE about storage operations.

Which issue this PR fixes

Release note:

Add support for emitting metrics from GCE cloudprovider about storage operations.

k8s-reviewable · 2017-01-24T02:20:32Z

This change is

k8s-ci-robot · 2017-01-24T02:45:40Z

Jenkins GCI GKE smoke e2e failed for commit 6a88f38. Full PR test history.

cc @gnufied, your PR dashboard

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

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.

k8s-ci-robot · 2017-01-24T03:37:45Z

Jenkins GKE smoke e2e failed for commit 6a88f38. Full PR test history.

cc @gnufied, your PR dashboard

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

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.

sttts · 2017-01-26T14:50:31Z

pkg/cloudprovider/providers/gce/gce.go

@@ -2930,3 +2981,8 @@ func lastComponent(s string) string {
 	}
 	return s
 }
+
+// Gets the time since the specified start in microseconds.
+func SinceInMicroseconds(start time.Time) float64 {


why public?

microsecondsSince

sttts · 2017-01-26T14:52:14Z

pkg/cloudprovider/providers/gce/gce.go

@@ -2493,7 +2533,9 @@ func (gce *GCECloud) doDeleteDisk(diskToDelete string) error {
 		return err
 	}

-	deleteOp, err := gce.service.Disks.Delete(gce.projectID, disk.Zone, disk.Name).Do()
+	dc := contextWithNamespace(diskToDelete, "gce_disk_delete")
+	deleteOp, err := gce.service.Disks.


IMO ugly indention

sttts · 2017-01-26T14:55:44Z

pkg/cloudprovider/providers/gce/gce.go

@@ -113,6 +116,12 @@ type Config struct {

 type DiskType string

+// ApiWithNamespace stores api and namespace in context
+type ApiWithNamespace struct {


why public?

sttts · 2017-01-26T14:57:11Z

pkg/cloudprovider/providers/gce/gce.go

+	apiNamespace, ok := t.(ApiWithNamespace)
+
+	if ok {
+		apiResponseReceived := func(resp *http.Response) {


if !ok { return nil }

sttts · 2017-01-26T14:59:33Z

pkg/cloudprovider/providers/gce/gce.go

+	apiNamespace, ok := t.(ApiWithNamespace)
+
+	if ok {
+		apiResponseReceived := func(resp *http.Response) {


return func(....)

sttts · 2017-01-26T15:00:27Z

pkg/cloudprovider/providers/gce/gce.go

+	if ok {
+		apiResponseReceived := func(resp *http.Response) {
+			timeTaken := SinceInMicroseconds(requestTime)
+			gceMetricMap[apiNamespace.apiCall].


are we sure that gceMetricMap[apiNamespace.apiCall] is never nil?

sttts · 2017-01-26T15:02:58Z

pkg/cloudprovider/providers/gce/gce.go

@@ -30,6 +30,8 @@ import (

 	"gopkg.in/gcfg.v1"

+	"golang.org/x/net/context"


no need for empty line above? And why not merge the non-kube blocks?

Thank you for reviewing this. I pushed this as a WIP and used it as a starting point for CloudProvider metrics proposal. I will clean it up and add more checks for nil and stuff.

I will give you another shout when it is ready for review.

gnufied · 2017-03-20T21:19:27Z

@sttts I have fixed the review comments and this PR is ready for review. Also adding @piosz @jingxu97

gnufied · 2017-03-20T21:19:36Z

/assign @jingxu97 @piosz

jingxu97 · 2017-03-20T21:41:40Z

cc @bowei

mikedanese · 2017-03-21T12:43:35Z

/approve

piosz · 2017-03-22T08:43:48Z

LGTM
cc @fabxc @brancz

piosz · 2017-03-22T08:45:48Z

pkg/cloudprovider/providers/gce/gce_metrics.go

@@ -0,0 +1,76 @@
+/*
+Copyright 2014 The Kubernetes Authors.


brancz · 2017-03-22T09:19:48Z

pkg/cloudprovider/providers/gce/gce_metrics.go

+var gceMetricMap = map[string]*prometheus.HistogramVec{
+	"gce_instance_list": prometheus.NewHistogramVec(
+		prometheus.HistogramOpts{
+			Name:    "gce_instance_list",


note the naming conventions for histograms: https://prometheus.io/docs/practices/histograms/

gce_instance_list_duration_seconds would be applicable here. Note that metrics are also meant to be in the base unit. Generally following the instrumentation guide.

Same applies to all other metrics introduced here.

Do you mean - these metrics should only be collected in seconds? not in ms as done here?

I couldn't find any info. about that in linked documentation

Sorry, pointed at the wrong resources there, this is what I meant: https://prometheus.io/docs/practices/naming/#metric-names

brancz · 2017-03-22T09:20:23Z

pkg/cloudprovider/providers/gce/gce_metrics.go

+			Help:    "Latency of instance listing calls in ms",
+			Buckets: prometheus.ExponentialBuckets(100, 2, 10),
+		},
+		[]string{"kube_namespace"},


According to the instrumentation guide, this should be just namespace.

gnufied · 2017-03-28T20:25:21Z

@brancz fixed all review comments. ptal

This PR implements tracking of GCE API usage via prometheus metrics.

brancz · 2017-03-29T06:55:41Z

lgtm 👍

gnufied · 2017-03-29T13:35:54Z

@brancz @piosz someone needs to "/lgtm" and "/approve" for this PR to get merged.

cc @saad-ali

rootfs · 2017-03-29T13:36:28Z

pkg/cloudprovider/providers/gce/gce.go

+	}
+
+	apiResponseReceived := func(resp *http.Response) {
+		timeTaken := time.Since(requestTime).Seconds()


are seconds too high for latency?

According to https://prometheus.io/docs/practices/naming/#metric-names metrics should use base units. I guess we should be okay, since we are reporting seconds as floats anyways.

brancz · 2017-03-30T05:51:35Z

/lgtm

k8s-github-robot · 2017-03-30T05:51:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brancz, gnufied, mikedanese

Needs approval from an approver in each of these OWNERS Files:

~~pkg/cloudprovider/providers/gce/OWNERS~~ [mikedanese]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-03-30T19:42:01Z

Automatic merge from submit-queue

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 24, 2017

k8s-github-robot assigned sttts Jan 24, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Jan 24, 2017

sttts reviewed Jan 26, 2017

View reviewed changes

k8s-github-robot assigned mikedanese and unassigned sttts Jan 30, 2017

apelisse assigned sttts Jan 31, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 17, 2017

gnufied force-pushed the cloudprovider-gce-metrics branch from 6a88f38 to 2ec3618 Compare March 20, 2017 21:17

gnufied changed the title ~~[WIP] Implement API usage metrics for gce storage~~ Implement API usage metrics for gce storage Mar 20, 2017

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

k8s-ci-robot assigned jingxu97 and piosz Mar 20, 2017

gnufied force-pushed the cloudprovider-gce-metrics branch from 2ec3618 to 0e5e8a1 Compare March 20, 2017 21:37

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 20, 2017

gnufied force-pushed the cloudprovider-gce-metrics branch from 0e5e8a1 to 7e6da70 Compare March 21, 2017 01:16

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

gnufied force-pushed the cloudprovider-gce-metrics branch from 7e6da70 to 76392ee Compare March 21, 2017 03:13

sttts removed their assignment Mar 21, 2017

mikedanese removed their assignment Mar 21, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 21, 2017

piosz reviewed Mar 22, 2017

View reviewed changes

brancz suggested changes Mar 22, 2017

View reviewed changes

gnufied force-pushed the cloudprovider-gce-metrics branch from 76392ee to 59c0773 Compare March 22, 2017 22:49

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 24, 2017

gnufied force-pushed the cloudprovider-gce-metrics branch from 59c0773 to 04c7ed9 Compare March 28, 2017 20:24

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

Implement API usage metrics for gce

c4aaf47

This PR implements tracking of GCE API usage via prometheus metrics.

gnufied force-pushed the cloudprovider-gce-metrics branch from 04c7ed9 to c4aaf47 Compare March 28, 2017 20:33

rootfs reviewed Mar 29, 2017

View reviewed changes

k8s-ci-robot assigned brancz Mar 30, 2017

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

k8s-github-robot added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Mar 30, 2017

childsb removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Mar 30, 2017

k8s-github-robot merged commit 449a13c into kubernetes:master Mar 30, 2017

gnufied mentioned this pull request Mar 30, 2017

Cloudprovider metrics for storage kubernetes/enhancements#182

Closed

23 tasks

saad-ali mentioned this pull request Jun 7, 2017

Need metrics in volume manager and controllers #36818

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement API usage metrics for gce storage #40338

Implement API usage metrics for gce storage #40338

gnufied commented Jan 24, 2017 •

edited

Loading

k8s-reviewable commented Jan 24, 2017

k8s-ci-robot commented Jan 24, 2017

k8s-ci-robot commented Jan 24, 2017

sttts Jan 26, 2017

sttts Jan 26, 2017

sttts Jan 26, 2017

sttts Jan 26, 2017

sttts Jan 26, 2017

sttts Jan 26, 2017

sttts Jan 26, 2017

sttts Jan 26, 2017

gnufied Jan 26, 2017

sttts Jan 26, 2017

gnufied commented Mar 20, 2017

gnufied commented Mar 20, 2017

jingxu97 commented Mar 20, 2017

mikedanese commented Mar 21, 2017

piosz commented Mar 22, 2017

piosz Mar 22, 2017

gnufied Mar 28, 2017

brancz Mar 22, 2017 •

edited

Loading

gnufied Mar 22, 2017

brancz Mar 23, 2017

brancz Mar 22, 2017

gnufied Mar 28, 2017

gnufied commented Mar 28, 2017

brancz commented Mar 29, 2017

gnufied commented Mar 29, 2017

rootfs Mar 29, 2017

gnufied Mar 29, 2017

brancz commented Mar 30, 2017

k8s-github-robot commented Mar 30, 2017

k8s-github-robot commented Mar 30, 2017

		@@ -30,6 +30,8 @@ import (

		"gopkg.in/gcfg.v1"

		"golang.org/x/net/context"

Implement API usage metrics for gce storage #40338

Implement API usage metrics for gce storage #40338

Conversation

gnufied commented Jan 24, 2017 • edited Loading

k8s-reviewable commented Jan 24, 2017

k8s-ci-robot commented Jan 24, 2017

k8s-ci-robot commented Jan 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnufied commented Mar 20, 2017

gnufied commented Mar 20, 2017

jingxu97 commented Mar 20, 2017

mikedanese commented Mar 21, 2017

piosz commented Mar 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brancz Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnufied commented Mar 28, 2017

brancz commented Mar 29, 2017

gnufied commented Mar 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brancz commented Mar 30, 2017

k8s-github-robot commented Mar 30, 2017

k8s-github-robot commented Mar 30, 2017

gnufied commented Jan 24, 2017 •

edited

Loading

brancz Mar 22, 2017 •

edited

Loading