Skip to content

Collect Cloudwatch metrics from the same timestamp#11142

Merged
kaiyan-sheng merged 10 commits intoelastic:masterfrom
kaiyan-sheng:ec2_reload
Mar 13, 2019
Merged

Collect Cloudwatch metrics from the same timestamp#11142
kaiyan-sheng merged 10 commits intoelastic:masterfrom
kaiyan-sheng:ec2_reload

Conversation

@kaiyan-sheng
Copy link
Copy Markdown
Contributor

@kaiyan-sheng kaiyan-sheng commented Mar 7, 2019

  1. This PR is to address comment: Push events even when there's no cloudwatch data #11023 (comment)
  2. This PR also added checks for the fields that were missed previously in TestFetch.
  3. When testing, I found out
getMetricDataResults =  [{
  Id: "ec20",
  Label: "i-0c68eeb552231a8d0 StatusCheckFailed_System",
  StatusCode: Complete,
  Timestamps: [2019-03-08 20:02:00 +0000 UTC,2019-03-08 19:57:00 +0000 UTC],
  Values: [0,0]
} {
  Id: "ec21",
  Label: "i-0c68eeb552231a8d0 CPUUtilization",
  StatusCode: Complete,
  Timestamps: [2019-03-08 19:57:00 +0000 UTC],
  Values: [0.100564971751321]
}]

We are collecting data from 2019-03-08 19:57:00 +0000 UTC for example for CPUUtilization, then we need to collect the rest of the metrics from the same timestamp from Values array. FindTimestamp function finds this timestamp and CheckTimestampInArray function checks if input timestamp exists in array and if it exists, return the position. In this example, 2019-03-08 19:57:00 +0000 UTC exists in [2019-03-08 20:02:00 +0000 UTC,2019-03-08 19:57:00 +0000 UTC] and position is 1.

@kaiyan-sheng kaiyan-sheng requested review from a team as code owners March 7, 2019 21:07
@kaiyan-sheng kaiyan-sheng self-assigned this Mar 7, 2019
@kaiyan-sheng kaiyan-sheng added docs Team:Integrations Label for the Integrations team labels Mar 7, 2019
@kaiyan-sheng kaiyan-sheng added [zube]: In Review review needs_backport PR is waiting to be backported to other branches. v7.0.0 and removed [zube]: In Review docs labels Mar 7, 2019
Comment thread x-pack/metricbeat/module/aws/ec2/ec2.go Outdated
@kaiyan-sheng kaiyan-sheng changed the title Update aws documentation Update documentation and collect Cloudwatch metrics from the same timestamp Mar 11, 2019
@kaiyan-sheng kaiyan-sheng changed the title Update documentation and collect Cloudwatch metrics from the same timestamp Collect Cloudwatch metrics from the same timestamp Mar 11, 2019
Comment thread x-pack/metricbeat/module/aws/utils.go Outdated
Copy link
Copy Markdown
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

@kaiyan-sheng Left a few minor comments but thanks to your explanation today the code makes now much more sense. I kind of hope we could make it more readable moving forward but at the same time it's not too great what we get from Cloudwatch :-(

Comment thread x-pack/metricbeat/module/aws/ec2/ec2_integration_test.go Outdated
Comment thread x-pack/metricbeat/module/aws/utils.go Outdated
Comment thread x-pack/metricbeat/module/aws/utils_test.go Outdated
Comment thread x-pack/metricbeat/module/aws/ec2/ec2.go Outdated
Comment thread x-pack/metricbeat/module/aws/ec2/ec2.go Outdated
Comment thread x-pack/metricbeat/module/aws/ec2/ec2.go Outdated
@kaiyan-sheng
Copy link
Copy Markdown
Contributor Author

kaiyan-sheng commented Mar 13, 2019

After #11151 get merged, I will push a commit up to adopt FindTimestamp method in SQS and S3 metricsets. When backporting to 7.0, only the changes related to ec2 will be cherrypicked.

@kaiyan-sheng
Copy link
Copy Markdown
Contributor Author

jenkins, test this please

@kaiyan-sheng
Copy link
Copy Markdown
Contributor Author

jenkins, test this please

Copy link
Copy Markdown
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

LGTM

Comments can be addressed later.

if len(output.Values) == 0 {
continue
}
exists, timestampIdx := aws.CheckTimestampInArray(timestamp, output.Timestamps)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like this code is identical to the code in the s3 daily storage file? Perhaps later we can take it out.

if len(output.Values) == 0 {
continue
}
exists, timestampIdx := aws.CheckTimestampInArray(timestamp, output.Timestamps)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One more time the same code? :-)

@kaiyan-sheng kaiyan-sheng merged commit 822b95a into elastic:master Mar 13, 2019
@kaiyan-sheng kaiyan-sheng deleted the ec2_reload branch March 13, 2019 20:46
@kaiyan-sheng kaiyan-sheng removed needs_backport PR is waiting to be backported to other branches. v7.0.0 labels Mar 13, 2019
kaiyan-sheng pushed a commit that referenced this pull request Mar 15, 2019
#11142) (#11236)

* Collect Cloudwatch metrics from the same timestamp (#11142)

* Update aws documentation
* Add checks for instance fields in TestFetch
* Add tests for CheckTimestampInArray and FindTimestamp
* Update comment for CheckTimestampInArray
* Change test to table driven
* Adopt CheckTimestampInArray and FindTimestamp in S3 and SQS

(cherry picked from commit 822b95a)

* Backport changes for CheckEventField and compareType from e03f966
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review Team:Integrations Label for the Integrations team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants