Start recording cloud provider metrics for AWS #43477

gnufied · 2017-03-21T20:27:44Z

What this PR does / why we need it:

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

Which issue this PR fixes

Release note:

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

k8s-reviewable · 2017-03-21T20:27:50Z

This change is

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

/assign @justinsb

gnufied · 2017-03-28T22:56:28Z

cc @justinsb @jingxu97 can we review this? the other PR for gce is almost ready for merge too.

gnufied · 2017-03-30T12:48:58Z

@piosz @brancz can you also review this one please, while we are at it?

/assign @brancz

rootfs · 2017-03-30T17:23:05Z

LGTM

brancz

only a small one, after that looks good

brancz · 2017-03-31T07:35:30Z

pkg/cloudprovider/providers/aws/aws_metrics.go

+			Name: "aws_attach_volume_duration_seconds",
+			Help: "Latency of aws attach call",
+		},
+		[]string{"kube_namespace"},


kube_namespace -> namespace

gnufied · 2017-03-31T14:34:02Z

@brancz fixed. ptal

brancz · 2017-03-31T16:16:43Z

/lgtm

justinsb · 2017-03-31T17:19:45Z

Looks good from an AWS perspective.

My two concerns:

naming (asking in sig-instrumentation)
How do we account for exponential backoff

gnufied · 2017-03-31T17:49:40Z

We don't account for exponential backoff here. There is separate feature request @rootfs is working on which considers total attach time, which possibly will include any exponential backoff incurred.

discordianfish · 2017-04-03T14:45:40Z

pkg/cloudprovider/providers/aws/aws.go

+	requestTime := time.Now()
+	resp, err := s.ec2.AttachVolume(request)
+	timeTaken := time.Since(requestTime).Seconds()
+	recordAwsMetric("aws_attach_volume", timeTaken, "")


This will record duration for successful and failed calls in the same metric. I'd prefer only recording them for succesfull requests, but I know some people have different preferences.

discordianfish · 2017-04-03T14:47:12Z

pkg/cloudprovider/providers/aws/aws.go

@@ -626,16 +635,25 @@ func (s *awsSdkEC2) DescribeVolumes(request *ec2.DescribeVolumesInput) ([]*ec2.V
 		}
 		request.NextToken = nextToken
 	}
-
+	timeTaken := time.Since(requestTime).Seconds()
+	recordAwsMetric("aws_describe_volume", timeTaken, "")


Here it's only recording in the success case. I think it should be consist either way.

discordianfish · 2017-04-03T14:53:07Z

@justinsb Regarding the naming, from a prometheus perspective it doesn't matter, it's the same number of timeseries it needs to handle. I personally slightly prefer having one metric as you suggested, mainly for easier aggregation.

justinsb · 2017-04-19T06:39:56Z

So I read our guidelines and the discussion on the design and it sounds like everything is OK @gnufied. While it would be nice to see the total requests, I think that might also be optimizing for our purposes as developers rather than end-users. I expect in most cases if someone is hitting a rate limit it would be pretty easy to send a canned query and observe that one line is much higher than the others.

Splitting out success & failures sounds like we can either do it with more dimensions or maybe more names, but that we can do that later.

So I'm going to approve so we can start collecting data :-)

/approve

gnufied · 2017-04-21T13:08:34Z

@justinsb my plan is still to amend the metric names to have action as dimension. I opened the design PR for same - kubernetes/community#507

In the meanwhile @bowei has gone ahead and amended GCE metrics - #44510

I like the idea of recording errors and successes separately. I think we can do that here too. But there is too much disagreement on what dimensions to report. @bowei for example have dropped namespace in his GCE PR and replaced them with zone and node dimensions.

What I would like to do is have some consensus about this and then we can fix both GCE and AWS metrics to be on similar lines.

brancz · 2017-04-27T12:31:08Z

This is probably a good idea, we should adapt here if we decide to change it in the proposal.

Lets start recording storage metrics for AWS.

gnufied · 2017-04-28T01:02:02Z

@brancz fixed the metric names as per suggestion.

brancz · 2017-04-28T07:51:11Z

/lgtm

k8s-github-robot · 2017-04-28T07:51:33Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

~~pkg/cloudprovider/providers/aws/OWNERS~~ [justinsb]

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-04-28T08:35:17Z

Automatic merge from submit-queue

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

k8s-github-robot assigned jingxu97 Mar 21, 2017

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

k8s-ci-robot assigned justinsb Mar 21, 2017

gnufied force-pushed the cloudprovider-aws-metrics branch from 144ebe1 to 4b58f71 Compare March 28, 2017 22:55

k8s-github-robot added 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

k8s-ci-robot assigned brancz Mar 30, 2017

gnufied mentioned this pull request Mar 30, 2017

Cloudprovider metrics for storage kubernetes/enhancements#182

Closed

23 tasks

brancz suggested changes Mar 31, 2017

View reviewed changes

gnufied force-pushed the cloudprovider-aws-metrics branch from 4b58f71 to 5220193 Compare March 31, 2017 14:33

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

discordianfish reviewed Apr 3, 2017

View reviewed changes

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 19, 2017

gnufied mentioned this pull request Apr 21, 2017

Add metrics to all major gce operations (latency, errors) #44510

Merged

gnufied force-pushed the cloudprovider-aws-metrics branch from 5220193 to 181b23b Compare April 26, 2017 19:34

Start recording cloud provider metrics for AWS

f2aa330

Lets start recording storage metrics for AWS.

gnufied force-pushed the cloudprovider-aws-metrics branch from 181b23b to f2aa330 Compare April 27, 2017 19:26

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

k8s-github-robot merged commit 9afeabb into kubernetes:master Apr 28, 2017

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

Start recording cloud provider metrics for AWS #43477

Start recording cloud provider metrics for AWS #43477

gnufied commented Mar 21, 2017 •

edited

Loading

k8s-reviewable commented Mar 21, 2017

gnufied commented Mar 21, 2017

gnufied commented Mar 28, 2017

gnufied commented Mar 30, 2017

rootfs commented Mar 30, 2017

brancz left a comment

brancz Mar 31, 2017

gnufied commented Mar 31, 2017

brancz commented Mar 31, 2017

justinsb commented Mar 31, 2017

gnufied commented Mar 31, 2017

discordianfish Apr 3, 2017

discordianfish Apr 3, 2017

discordianfish commented Apr 3, 2017

justinsb commented Apr 19, 2017

gnufied commented Apr 21, 2017 •

edited

Loading

brancz commented Apr 27, 2017

gnufied commented Apr 28, 2017

brancz commented Apr 28, 2017

k8s-github-robot commented Apr 28, 2017

k8s-github-robot commented Apr 28, 2017

Start recording cloud provider metrics for AWS #43477

Start recording cloud provider metrics for AWS #43477

Conversation

gnufied commented Mar 21, 2017 • edited Loading

k8s-reviewable commented Mar 21, 2017

gnufied commented Mar 21, 2017

gnufied commented Mar 28, 2017

gnufied commented Mar 30, 2017

rootfs commented Mar 30, 2017

brancz left a comment

Choose a reason for hiding this comment

brancz Mar 31, 2017

Choose a reason for hiding this comment

gnufied commented Mar 31, 2017

brancz commented Mar 31, 2017

justinsb commented Mar 31, 2017

gnufied commented Mar 31, 2017

discordianfish Apr 3, 2017

Choose a reason for hiding this comment

discordianfish Apr 3, 2017

Choose a reason for hiding this comment

discordianfish commented Apr 3, 2017

justinsb commented Apr 19, 2017

gnufied commented Apr 21, 2017 • edited Loading

brancz commented Apr 27, 2017

gnufied commented Apr 28, 2017

brancz commented Apr 28, 2017

k8s-github-robot commented Apr 28, 2017

k8s-github-robot commented Apr 28, 2017

gnufied commented Mar 21, 2017 •

edited

Loading

gnufied commented Apr 21, 2017 •

edited

Loading