Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use batch values API for CloudWatch PutMetric data call #960

Merged
merged 3 commits into from
Feb 20, 2020

Conversation

taraspos
Copy link
Contributor

Some time ago, AWS introduced the way to publish a batch of metric values in one call.

So there is no point in sending last(values) if we could send the whole batch.
This PR implements the sending of values batch,

@peterbourgon
Copy link
Member

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x900100]

goroutine 28 [running]:
github.com/go-kit/kit/metrics/cloudwatch.(*mockCloudWatch).PutMetricData(0xc0001d0900, 0xc00000e080, 0x0, 0x0, 0x0)
	/home/build/go/src/github.com/go-kit/kit/metrics/cloudwatch/cloudwatch_test.go:36 +0x150
github.com/go-kit/kit/metrics/cloudwatch.(*CloudWatch).Send.func5(0xc00019de60, 0xc000090840, 0xc00008e220, 0x1, 0x1)
	/home/build/go/src/github.com/go-kit/kit/metrics/cloudwatch/cloudwatch.go:251 +0x233
created by github.com/go-kit/kit/metrics/cloudwatch.(*CloudWatch).Send
	/home/build/go/src/github.com/go-kit/kit/metrics/cloudwatch/cloudwatch.go:246 +0x55a
FAIL	github.com/go-kit/kit/metrics/cloudwatch	0.025s

@taraspos
Copy link
Contributor Author

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x900100]

goroutine 28 [running]:
github.com/go-kit/kit/metrics/cloudwatch.(*mockCloudWatch).PutMetricData(0xc0001d0900, 0xc00000e080, 0x0, 0x0, 0x0)
	/home/build/go/src/github.com/go-kit/kit/metrics/cloudwatch/cloudwatch_test.go:36 +0x150
github.com/go-kit/kit/metrics/cloudwatch.(*CloudWatch).Send.func5(0xc00019de60, 0xc000090840, 0xc00008e220, 0x1, 0x1)
	/home/build/go/src/github.com/go-kit/kit/metrics/cloudwatch/cloudwatch.go:251 +0x233
created by github.com/go-kit/kit/metrics/cloudwatch.(*CloudWatch).Send
	/home/build/go/src/github.com/go-kit/kit/metrics/cloudwatch/cloudwatch.go:246 +0x55a
FAIL	github.com/go-kit/kit/metrics/cloudwatch	0.025s

Yup, sorry.
This is a problem in the tests, they are not ready for the absence of Value field.
Will fix!

@taraspos
Copy link
Contributor Author

@peterbourgon hey, fixing the tests took some effort and changes.
One way was just to take the latest value from the array of values for CloudWatch, but that doesn't seem right, so I had to change the tests to accept the array of values, could you please review and give your feedback, please?

@peterbourgon
Copy link
Member

@cam-stitt This is your package :D Feedback?

@trane9991 Did you see package cloudwatch2? Should this be ported there, too?

@taraspos
Copy link
Contributor Author

hmmm... I see that cloudwatch2 uses the StatisticsSet which allows pushing already calculated values, this is a different approach, not sure which one is better yet :)

Copy link
Contributor

@cam-stitt cam-stitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the implementation, but I think we can just tidy it a little and always use the batch request.

metrics/cloudwatch/cloudwatch.go Outdated Show resolved Hide resolved
})
}

if len(values) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could move this above the datum initialization to save an alloc, but not a big deal.

@peterbourgon peterbourgon merged commit 7dd0815 into go-kit:master Feb 20, 2020
@peterbourgon
Copy link
Member

Nice, thanks!

@sagikazarmark sagikazarmark added this to the v0.11.0 milestone Jun 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants