Issue #529: Partition API requests to CloudWatch into separate concur… by alpeb · Pull Request #540 · go-kit/kit

alpeb · 2017-05-30T17:08:56Z

…rent batches to circumvent the 20 data per request limit that they have

alpeb · 2017-05-30T20:58:44Z

@cam-stitt Looking forward for your comments.

cam-stitt · 2017-05-31T02:50:01Z

metrics/cloudwatch/cloudwatch.go

-		MetricData: datums,
-	})
-	return err
+	var tokens = make(chan struct{}, cw.numConcurrentRequests)


I'm not sure what the value of tokens is. I don't really see it being used.

It limits the number of concurrent requests. When the channel gets filled, tokens <- struct{}{} will block until a go routine finishes and releases a token through <-tokens

cam-stitt · 2017-05-31T02:52:07Z

metrics/cloudwatch/cloudwatch.go

+	var errors = make(chan error)
+	var n int
+
+	for len(datums) > 0 {


My personal preference would be to loop in jumps of numConcurrentRequests and then slicing datums up each time. Rough example: https://play.golang.org/p/slrBko_QdI

If you step through my code, you'll see it's pretty much equivalent to what you're proposing.

cam-stitt · 2017-05-31T02:53:33Z

metrics/cloudwatch/cloudwatch.go

+		}(batch)
+	}
+
+	var firstErr error


By returning only the first error, there may be some context lost as to what the overall problem was. Maybe we can create an Error type that actually contains multiple errors?

Sure. Would you agree on limiting the number of errors, or shall we return everything?

cam-stitt · 2017-05-31T02:54:34Z

Thanks a lot for this. It's a great start. I've put a few of my thoughts on there, but would love to get @peterbourgon's opinion.

cam-stitt · 2017-05-31T02:55:59Z

metrics/cloudwatch/cloudwatch.go

+		lim := min(len(datums), cw.numConcurrentRequests)
+		batch, datums = datums[:lim], datums[lim:]
+		n++
+		go func(batch []*cloudwatch.MetricDatum) {


I wonder if it's better to not make each PutMetricData in a goroutine, and just fire them and return the error that comes up. This means that the first request to error will stop any future requests from occurring, which might be nicer behaviour.

peterbourgon · 2017-06-03T15:37:08Z

metrics/cloudwatch/cloudwatch.go

+func (cw *CloudWatch) SetConcurrency(numConcurrentRequests int) *CloudWatch {
+	cw.numConcurrentRequests = numConcurrentRequests
+	return cw
+}


This should be an option in the constructor, rather than a method on the struct. I don't see any reason to make it modifiable during runtime. (It's also racy as implemented.)

Right, I wasn't sure how to handle optional parameters in go. Would this do?

func New(namespace string, svc cloudwatchiface.CloudWatchAPI, logger log.Logger, numConcurrentRequests ...int) *CloudWatch { numConcurrentRequestsValue := 10 if len(numConcurrentRequests) > 0 { numConcurrentRequestsValue = numConcurrentRequests[0] } return &CloudWatch{ namespace: namespace, numConcurrentRequests: numConcurrentRequestsValue, svc: svc, counters: map[string]*counter{}, gauges: map[string]*gauge{}, histograms: map[string]*histogram{}, logger: logger, } }

No, that's not the way. If I were you I'd just make it a non-optional parameter, and document a reasonable default in the function comment.

// New returns a blah blah. The client will emit at most // numConcurrent simultaneous requests to Amazon. // A good default value is 10. func New(namespace string, svc CloudWatchAPI, numConcurrent int, logger log.Logger) { ... }

peterbourgon · 2017-06-03T15:37:56Z

metrics/cloudwatch/cloudwatch.go

-		MetricData: datums,
-	})
-	return err
+	var tokens = make(chan struct{}, cw.numConcurrentRequests)


Similarly, this should be global per CloudWatch struct (so a member of the struct), not per-metric (scoped to NewCounter), right?

Sorry, I don't understand what you mean. This is inside Send(), why are you referring to NewCounter?

Sorry, my mistake—but my point stands: putting the semaphore here means there are numConcurrentRequest simultanous requests per Send invocation, so if the user invokes Send 10 times then you get 10x numConcurrentRequests going out. So in reality I think we want the semaphore to be in the struct.

Gotcha 💯

peterbourgon · 2017-06-03T15:42:12Z

metrics/cloudwatch/cloudwatch.go

+	for len(datums) > 0 {
+		var batch []*cloudwatch.MetricDatum
+		lim := min(len(datums), cw.numConcurrentRequests)
+		batch, datums = datums[:lim], datums[lim:]


If I'm reading this right, you're sizing the batch as the minimum of (number of datums remaining) and (number of allowable concurrent requests)? That doesn't seem right—batch size and concurrent batches have nothing to do with each other, do they?

You're right, I should have instead gotten the min between len(datums) and 20 (the maximum number of datums allowed per request).

peterbourgon · 2017-06-03T15:43:04Z

metrics/cloudwatch/cloudwatch.go

+			})
+			<-tokens
+			errors <- err
+		}(batch)


Since you construct the batch within the loop scope, there's no reason to pass it as a parameter to this anonymous function.

Indeed 👍

peterbourgon · 2017-06-03T15:44:14Z

metrics/cloudwatch/cloudwatch.go

+				MetricData: batch,
+			})
+			<-tokens
+			errors <- err


Is there a reason you decrement the semaphore before returning the error along the errors chan? If you didn't need to do that, it would be nicer to express this as

tokens <- struct{}{} defer func() { <-tokens }() _, err := cw... errors <- err

Yep, that's better.

peterbourgon · 2017-06-03T15:47:14Z

metrics/cloudwatch/cloudwatch.go

+		if err := <-errors; err != nil && firstErr != nil {
+			firstErr = err
+		}
+	}


It's a little fragile, for a couple reasons:

n is only loosely coupled to the number of goroutines you launched, easy to get out of sync w/ maintenance

If you forget to do precisely as many reads from errors as goroutines you launched, you'll leak goroutines

Suggest precomputing batches, constructing errors as make(chan error, len(batches)), and only then launching the goroutines to process the batches. Finally, changing this loop to e.g.

for i := 0; i < cap(errors); i++ { if err := <-errors; err != nil ... { ... } }

Understood. I'll follow suit.

alpeb · 2017-06-05T15:03:54Z

Thanks for the detailed review! I'll respond to comments inline, and then I'll submit a new patch.

peterbourgon · 2017-06-30T07:05:51Z

examples/addsvc/cmd/addcli/main.go:155: multiple-value 
transportFactory.GetTransport() in single-value context

I think if you rebase you might get this fixed for you?

…rent batches to circumvent the 20 data per request limit that they have

…er-filling logic in cloudwatch_test.go

…ved semaphore into the cw struct and use defer when using it. Fixed data partitioning logic and separate batch creation from goroutines launching. Improved tests.

alpeb · 2017-07-03T20:44:31Z

Yep, after rebasing checks are passing 🎉

peterbourgon · 2017-07-03T20:46:47Z

@cam-stitt I just want to get a +1 from you before merging. All good from your POV?

cam-stitt · 2017-07-03T23:03:32Z

Yeah, looks good. Thanks @alpeb for jumping on this.

peterbourgon · 2017-07-04T09:37:12Z

Thanks all!

cam-stitt reviewed May 31, 2017

View reviewed changes

peterbourgon requested changes Jun 3, 2017

View reviewed changes

peterbourgon mentioned this pull request Jun 9, 2017

Cloudwatch/metrics API call errors because of duplicate label #528

Closed

alpeb added 3 commits July 3, 2017 15:30

Issue #529: Partition API requests to CloudWatch into separate concur…

24eddfe

…rent batches to circumvent the 20 data per request limit that they have

Issue #529: Extracted FillCounter() from TestCounter() to reuse count…

9e2127e

…er-filling logic in cloudwatch_test.go

Issue #529: replaced SetConcurrency() with new parameter in New(). Mo…

196891a

…ved semaphore into the cw struct and use defer when using it. Fixed data partitioning logic and separate batch creation from goroutines launching. Improved tests.

peterbourgon approved these changes Jul 3, 2017

View reviewed changes

peterbourgon merged commit 6a894fe into go-kit:master Jul 4, 2017

Uh oh!

Comments

Conversation

alpeb commented May 30, 2017

Uh oh!

alpeb commented May 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cam-stitt commented May 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterbourgon Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alpeb Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alpeb commented Jun 5, 2017

Uh oh!

peterbourgon commented Jun 30, 2017

Uh oh!

alpeb commented Jul 3, 2017

Uh oh!

peterbourgon commented Jul 3, 2017

Uh oh!

cam-stitt commented Jul 3, 2017

Uh oh!

peterbourgon commented Jul 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

peterbourgon Jun 5, 2017 •

edited

Loading

alpeb Jun 5, 2017 •

edited

Loading