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

Call GetMetricData api per region instead of per instance #11882

Merged
merged 7 commits into from
Apr 25, 2019
Merged

Call GetMetricData api per region instead of per instance #11882

merged 7 commits into from
Apr 25, 2019

Conversation

kaiyan-sheng
Copy link
Contributor

@kaiyan-sheng kaiyan-sheng commented Apr 19, 2019

In current ec2 metricset code, we are constructing metricDataQueries per region per instance for GetMetricData cloudwatch api call. Because we have some metadata from DescribeInstances api call for each instance to add into each event. It will be more efficient/cheaper to improve ec2 metricset to construct metricDataQueries per region but not per instance.

closes #11820

@kaiyan-sheng kaiyan-sheng requested a review from a team as a code owner April 19, 2019 19:05
@kaiyan-sheng kaiyan-sheng self-assigned this Apr 19, 2019
@kaiyan-sheng kaiyan-sheng added Team:Integrations Label for the Integrations team Metricbeat Metricbeat needs_backport PR is waiting to be backported to other branches. v7.0.0 labels Apr 19, 2019
Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

Left some minor comments. From what I understand you launch a bunch of metric queries (for every EC2 instance) with the same call.

I'm wondering if it's possible to launch a query for all instance ids, without setting dimensions in it?

x-pack/metricbeat/module/aws/ec2/ec2.go Outdated Show resolved Hide resolved
x-pack/metricbeat/module/aws/aws.go Show resolved Hide resolved
@kaiyan-sheng
Copy link
Contributor Author

Left some minor comments. From what I understand you launch a bunch of metric queries (for every EC2 instance) with the same call.

I'm wondering if it's possible to launch a query for all instance ids, without setting dimensions in it?

@exekias Sorry I definitely need to put in more explanation here. So the goal for this PR is to reduce the number of svc.GetMetricDataRequest api call, which is hidden in getMetricDataPerRegion function in x-pack/metricbeat/module/aws/utils.go. Without the changes in this PR, svc.GetMetricDataRequest gets called per region per EC2 instance. With the changes, svc.GetMetricDataRequest gets called per region. Since we still want to keep the metadata for each EC2 instance, describeInstances api is called per region. The result from describeInstances is stored into a map<instanceID><describeInstanceOutput>. Then this map is passed into createCloudWatchEvents function with the output from svc.GetMetricDataRequest to construct events for each EC2 instance.

I guess one more thing we can do is to remove the for loop for regions for svc.GetMetricDataRequest. But that loop will still exist for describeInstances because this api call can only be made per region.

@exekias
Copy link
Contributor

exekias commented Apr 24, 2019

This is already an improvement, I'm ok to getting this in and experiment more later. I want to play a little bit with the cloudwatch metricset for better understanding.

Left a comment, let's tackle that and this is ready to go

@kaiyan-sheng
Copy link
Contributor Author

I spent some more time testing if we can make cloudwatch api calls without specifying ec2 instanceID. Unfortunately the answer is no. I also checked if we can make cloudwatch api calls without region specified, the answer is no too.

Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

Thank you for doing the additional testing!

@exekias
Copy link
Contributor

exekias commented Apr 25, 2019

I think we can skip backport for this one, as it's not fixing a critical bug but improving the behavior

@exekias exekias added v7.2.0 and removed needs_backport PR is waiting to be backported to other branches. v7.0.0 labels Apr 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Metricbeat Metricbeat review Team:Integrations Label for the Integrations team v7.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve efficiency on metricsets in aws module GetMetricData call
4 participants