-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Feature/cloudwatch #1032
Closed
Closed
Feature/cloudwatch #1032
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
86b30ef
Implement Run and Collect on CloudWatch collector
94095ad
Send tags to CloudWatch
f88740b
Add CloudWatch collector and update dependencies
490c953
Add CloudWatch dependencies
61e853d
Comment code and fix CloudWatch upper bound limit
6210619
Change log level from debug to warn on an error sending metrics to Cl…
92eb9b5
Log warning message when not all tags are send to CloudWatch
478f797
Simplify ticker instantiation
1ef4bb5
Sort imports with goimports
d2c2891
Sort imports in collectors with goimports
1f1f119
Add comment to public methods on CloudWatch collector's methods
263d4ff
Simplify CloudWatch constructor
d03c2e3
Add comment to CloudWatch's Collector
0105adf
Adapt comments in CloudWatch collector to golint
53b8235
Fix error on collector comment
1f9ed8d
Remove race condition in test
039ccd9
Refactor to avoid return private types
3da79ad
Add CloudWatch to the upcoming release notes
a175594
Apply suggestions from code review
db789b8
Add test for method reportSamples in cloudwatch client
2c7556a
Reports samples to cloudwatch outside lock zone
0e46b5a
Add missing cloudwatch interface
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,115 @@ | ||
package awscloudwatch | ||
|
||
import ( | ||
"github.com/aws/aws-sdk-go/aws" | ||
"github.com/aws/aws-sdk-go/aws/session" | ||
"github.com/aws/aws-sdk-go/service/cloudwatch" | ||
"github.com/aws/aws-sdk-go/service/cloudwatch/cloudwatchiface" | ||
"github.com/pkg/errors" | ||
"github.com/sirupsen/logrus" | ||
) | ||
|
||
type client struct { | ||
cloudwatchiface.CloudWatchAPI | ||
namespace string | ||
endpoint string | ||
} | ||
|
||
// ClientFactory returns a function that creates the AWS CloudWatch client | ||
func ClientFactory(namespace string) func() (cloudWatchClient, error) { | ||
return func() (cloudWatchClient, error) { | ||
cw, err := newCloudWatchClient() | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
return &client{ | ||
CloudWatchAPI: cw, | ||
namespace: namespace, | ||
endpoint: cw.Endpoint, | ||
}, nil | ||
} | ||
} | ||
|
||
// newCloudWatchClient creates a new CloudWatch client | ||
func newCloudWatchClient() (*cloudwatch.CloudWatch, error) { | ||
sess, err := session.NewSession() | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
return cloudwatch.New(sess), nil | ||
} | ||
|
||
const maxMetricsPerCall = 20 | ||
|
||
// reportSamples reports samples to CloudWatch. | ||
// It sends samples in batches of max 20, which is the upper limit of metrics | ||
// accepted per request by CloudWatch | ||
func (c *client) reportSamples(samples []*sample) error { | ||
samplesSent := 0 | ||
var lastError error | ||
|
||
for samplesSent < len(samples) { | ||
input := &cloudwatch.PutMetricDataInput{Namespace: &c.namespace} | ||
upperLimit := samplesSent + maxMetricsPerCall | ||
if len(samples) < upperLimit { | ||
upperLimit = len(samples) | ||
} | ||
|
||
for _, s := range samples[samplesSent:upperLimit] { | ||
input.MetricData = append(input.MetricData, toMetricDatum(s)) | ||
samplesSent++ | ||
} | ||
|
||
_, err := c.PutMetricData(input) | ||
if err != nil { | ||
logrus.WithError(err).Warn("Error sending metrics to CloudWatch") | ||
lastError = err | ||
} | ||
} | ||
|
||
if lastError != nil { | ||
return errors.Wrap(lastError, "Error sending metrics") | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func (c *client) address() string { | ||
return c.endpoint | ||
} | ||
|
||
const maxNumberOfDimensions = 10 | ||
|
||
func toMetricDatum(s *sample) *cloudwatch.MetricDatum { | ||
datum := &cloudwatch.MetricDatum{ | ||
Value: &s.Value, | ||
MetricName: &s.Metric, | ||
Timestamp: &s.Time, | ||
} | ||
|
||
var dims []*cloudwatch.Dimension | ||
|
||
for name, value := range s.Tags { | ||
if len(dims) == maxNumberOfDimensions { | ||
logrus.WithField("tags", s.Tags). | ||
WithField("dimensions_included", dims). | ||
Warnf("More than 10 tags, just 10 will be reported to CloudWatch") | ||
break | ||
} | ||
|
||
if value != "" { | ||
dims = append(dims, &cloudwatch.Dimension{ | ||
Name: aws.String(name), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is very strange... 😕 This just returns a pointer to the value and I have no idea why AWS needs it... |
||
Value: aws.String(value), | ||
}) | ||
} | ||
} | ||
|
||
if len(dims) > 0 { | ||
datum.Dimensions = dims | ||
} | ||
|
||
return datum | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading the docs for creating an AWS session (https://godoc.org/github.com/aws/aws-sdk-go/aws/session#hdr-Creating_Sessions), this would load the AWS credentials from
~/.aws/credentials
or useAWS_*
environment variables? I've only skimmed the SDK docs, but that seems a bit messy to me... 😕Ideally, this output shouldn't rely on the presence of files on the filesystem or read environment variables directly from the OS. For one, that makes it harder to test, but my main concern is that this could get in the way of the future native distributed execution support we want to introduce in k6... Also, the environment variables here aren't prefixed with
K6_
, like the other config options are. But from skimming the AWS SDK docs, I'm not sure we can forbid the SDK code from actually doing all of that magic 😕If the k6 output configuration situation was otherwise perfect (and it's unfortunately not - #587, #883 😞 ), I would force the issue, but I'm not sure if we shouldn't accept this as-is and deprecate the direct options in the future? @mstoykov, thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only way that I see to avoid this issue is duplicate the private newSession. I know this is not an ideal solution, but at the same time I don't see a big problem neither. Looking at what
NewSessionWithOptions
does, for instance, seems that most of the functionality is around more flexible configuration.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a big fan of loading things automatically without prefixes ... Not certain if copying the
newSession
will be enough, but if it is ... maybe that is good idea.On a related note (of copying code) ... I am under the impression that implementing the whole PutMetric call in k6 instead of using the aws-sdk will not be ... all that much code given that it is just http request.