[AWS] Fix ec2 host.cpu.usage + include instance metadata and rate metrics for linked monitoring accounts

[kaiyan-sheng]

What does this PR do?

This PR is fixing two bugs:

1. calculating host.cpu.usage metric for EC2: apparently it is looking for the wrong metric name. It should be aws.ec2.metrics.CPUUtilization.avg.
2. addHostFields and calculateRate functions are getting skipped when collecting metrics from linked accounts: because when adding metadata, we are making an additional API call to list all EC2 instances in that account and use that list/return value to add metadata. During this process, we only add metadata to instances from the monitoring account because DescribeInstances does not have access to linked source accounts. This case, we should move addHostFields and calculateRate functions to the beginning when adding meta data. This way, both monitoring account and linked source accounts instances will have host fields and rate. The actual metadata can be added later just for the instances in monitoring account.

Checklist

• My code follows the style guidelines of this project
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have made corresponding change to the default configuration files
• I have added tests that prove my fix is effective or that my feature works
• I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Related issues

• Relates [AWS] Add support for collecting metrics from linked cloudwatch accounts using cross-account monitoring  integrations#6253

[mergify]

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @kaiyan-sheng? 🙏.
For such, you'll need to label your PR with:

• The upcoming major version of the Elastic Stack
• The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

• backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

[elasticmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2023-06-15T19:17:46.561+0000

• Duration: 74 min 10 sec

Test stats 🧪

Test	Results
Failed	0
Passed	1490
Skipped	110
Total	1600

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

• /package : Generate the packages and run the E2E tests.

• /beats-tester : Run the installation tests with beats-tester.

• run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

[tommyers-elastic]

thanks kaiyan! why does moving addHostFields and calculateRate to the top of the loop have an effect on linked account instances? the instances still wont appear in the list returned from getInstancesPerRegion?

[kaiyan-sheng]

@tommyers-elastic WDYT about adding the value from dynamic label ${PROP('Period')} into aws.cloudwatch.period field? Is it worth keeping the value? I added it to help calculate the rate metrics for EC2.

[mergify]

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b fix_ec2_cpu upstream/fix_ec2_cpu
git merge upstream/main
git push upstream fix_ec2_cpu

[tommyers-elastic]

does this always exist in the dimensions? should we check the error here just in case?

[kaiyan-sheng]

Yeah good point! Will add it!

[kaiyan-sheng]

I did some testing on this, when aws.dimensions.InstanceId doesn't exist, dimInstanceID returns as nil. Then events[eventIdentifier].RootFields.Put("cloud.instance.id", dimInstanceID) will just put nil as the value for cloud.instance.id which won't show up in the event.

[tommyers-elastic]

😬 that wouldn't be ideal - i wonder if we could fall back to the instance ID we from the loop var?

[kaiyan-sheng]

I added the error check here to make sure only put value to cloud.instance.id when aws.dimensions.InstanceId exists. We can't use instance ID from the inner for loop var because that for loop is going through all the instances metadata from DescribeInstances API. We are trying to use the two for loops to match events with the dimension InstanceId to the same ID from DescribeInstances.

[tommyers-elastic]

@kaiyan-sheng if the period isn't used for anything in kibana, i don't think we should add it. we can always add it later if we find a need.

since we now have period and account name as dyamic labels, do we have any idea if this adds any additional overhead to the getmetricdata calls?

[kaiyan-sheng]

since we now have period and account name as dyamic labels, do we have any idea if this adds any additional overhead to the getmetricdata calls?

@tommyers-elastic Good point! I made a bash script and ran the AWS GetMetricData CLI call several times for both with and without getting the dynamic labels. The average time spent on GetMetricData is about the same.

for example some data points look like this:

Average Time without dynamic labels: 1040 ms
Average Time with dynamic label accountID and period: 1033 ms

and

Average Time without dynamic labels: 1036 ms
Average Time with dynamic label accountID and period: 1043 ms

By looking at several sets of output, I think we can say adding the dynamic label doesn't add overhead on the API call.