[AWS] Use namespace for GetListMetrics when exists #41022

kaiyan-sheng · 2024-09-27T00:07:12Z

Proposed commit message

This PR is to change GetListMetricsOutput function (which calls the ListMetrics API) to use namespaces when namespaces are given in AWS CloudWatch configuration in Metricbeat.

Instead of returning all possible metrics from CloudWatch from ALL namespaces and filter later, we only need to query the ListMetrics API to get the metrics under the namespaces that we care about.

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.

mergify · 2024-09-27T00:07:51Z

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-8./d is the label to automatically backport to the 8./d branch. /d is the digit

mergify · 2024-09-27T00:07:52Z

backport-8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label and remove the backport-8.x label.

elasticmachine · 2024-09-27T16:57:54Z

Pinging @elastic/obs-ds-hosted-services (Team:obs-ds-hosted-services)

x-pack/metricbeat/module/aws/cloudwatch/cloudwatch.go

Kavindu-Dodan

I have this remark [1] otherwise, looks okay

[1] - https://github.com/elastic/beats/pull/41022/files#r1784739873

gizas · 2024-10-03T05:58:48Z

x-pack/metricbeat/module/aws/cloudwatch/cloudwatch.go

+		if len(namespaceDetailTotal) == 0 {
+			listMetricsOutput, err = aws.GetListMetricsOutput("*", regionName, m.Period, m.IncludeLinkedAccounts, m.OwningAccount, m.MonitoringAccountID, svcCloudwatch)
+			if err != nil {
+				m.Logger().Errorf("Error while retrieving the list of metrics for region %s and namespace %s: %w", regionName, "*", err)


Do we need to include the namespace in this error message?

Also I think that this is can be a good integration test to include it. WDYT?

The error message here shows namespace is * because in this if statement we are calling ListMetrics with * all namespaces. The else statement part is the one with a specific namespace.

@gizas, most integration tests start the metricset/input and check that they produce a non-zero number of events.

What kind of check do you have in mind?

What do you think about creating a dedicated issue to enhance the integration tests for the cloudwatch metricset?

@kaiyan-sheng, this change may increase the number of API calls because we're making one API call for each namespace in the config.

However, it shouldn't make a difference since I don't expect configurations to have more than 1-3 namespaces, right?

@zmoog #41146 FYI

@zmoog Yeah that's the original thought when we decide to use "*" instead of individual namespace in this API call. But I think at this point, the benefit of using specific namespaces with a higher number of API calls wins.
I will update the document here: https://www.elastic.co/guide/en/beats/metricbeat/current/metricbeat-module-aws.html#aws-api-requests

We actually already have tests around this but since we are mocking the ListMetrics API and what's returning from the API, the test is no use for distinguish namespaces. As we discussed earlier, I will merge this PR to get the fix in but look into the test later. Thanks!

@zmoog I tested the pr again with the config below to double check how many times this ListMetrics API will be called. Turned out we are calling it once because namespaces are the same in both parts under metrics:

- module: aws period: 60s metricsets: - cloudwatch data_granularity: 60s include_linked_accounts: true regions: - ap-southeast-1 metrics: - dimensions: - name: CanaryName value: canary1 namespace: CloudWatchSynthetics - dimensions: - name: CanaryName value: canary2 namespace: CloudWatchSynthetics

mergify · 2024-10-03T07:48:29Z

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 cloudwatch_delay upstream/cloudwatch_delay
git merge upstream/main
git push upstream cloudwatch_delay

zmoog · 2024-10-04T15:06:57Z

/test

zmoog · 2024-10-04T15:08:15Z

I added the aws label to trigger the integration tests.

girodav · 2024-10-07T14:10:31Z

Please keep in mind that:

Making multiple ListMetrics API calls may incur additional costs for the user, even though these APIs are generally not the main cost driver.
We should update the docs accordingly, stating when each API is called and how.

See this old PR for reference https://github.com/elastic/beats/pull/34055/files

mergify · 2024-10-07T18:03:09Z

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 cloudwatch_delay upstream/cloudwatch_delay
git merge upstream/main
git push upstream cloudwatch_delay

* Use namespace for GetListMetrics when exists (cherry picked from commit 36327a4)

* Use namespace for GetListMetrics when exists (cherry picked from commit 36327a4) Co-authored-by: kaiyan-sheng <[email protected]>

…xists (#41175) * [AWS] Use namespace for GetListMetrics when exists (#41022) * Use namespace for GetListMetrics when exists (cherry picked from commit 36327a4) --------- Co-authored-by: kaiyan-sheng <[email protected]>

Use namespace for GetListMetrics when exists

a419b52

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Sep 27, 2024

mergify bot assigned kaiyan-sheng Sep 27, 2024

mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Sep 27, 2024

kaiyan-sheng added Team:obs-ds-hosted-services Label for the Observability Hosted Services team backport-8.15 Automated backport to the 8.15 branch with mergify labels Sep 27, 2024

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Sep 27, 2024

kaiyan-sheng added 2 commits September 27, 2024 09:34

add changelog

4143efb

fix lint

95df0a6

kaiyan-sheng marked this pull request as ready for review September 27, 2024 16:57

kaiyan-sheng requested a review from a team as a code owner September 27, 2024 16:57

kaiyan-sheng requested a review from zmoog September 27, 2024 17:22

Kavindu-Dodan reviewed Oct 2, 2024

View reviewed changes

x-pack/metricbeat/module/aws/cloudwatch/cloudwatch.go Outdated Show resolved Hide resolved

Kavindu-Dodan approved these changes Oct 2, 2024

View reviewed changes

check len(namespaceDetailTotal) instead

d3be852

gizas reviewed Oct 3, 2024

View reviewed changes

zmoog added the aws Enable builds in the CI for aws cloud testing label Oct 4, 2024

gizas mentioned this pull request Oct 7, 2024

Enhance tests for cloudwatch metricset #41146

Open

kaiyan-sheng added 2 commits October 7, 2024 15:38

update documentation

f83c542

Merge branch 'main' into cloudwatch_delay

2c0dde2

zmoog approved these changes Oct 8, 2024

View reviewed changes

kaiyan-sheng merged commit 36327a4 into elastic:main Oct 8, 2024
33 checks passed

kaiyan-sheng deleted the cloudwatch_delay branch October 8, 2024 14:43

mergify bot pushed a commit that referenced this pull request Oct 8, 2024

[AWS] Use namespace for GetListMetrics when exists (#41022)

c370951

* Use namespace for GetListMetrics when exists (cherry picked from commit 36327a4)

mergify bot pushed a commit that referenced this pull request Oct 8, 2024

[AWS] Use namespace for GetListMetrics when exists (#41022)

b159d03

* Use namespace for GetListMetrics when exists (cherry picked from commit 36327a4)

This was referenced Oct 8, 2024

[8.15](backport #41022) [AWS] Use namespace for GetListMetrics when exists #41175

Merged

[8.x](backport #41022) [AWS] Use namespace for GetListMetrics when exists #41176

Merged

kaiyan-sheng added a commit that referenced this pull request Oct 8, 2024

[AWS] Use namespace for GetListMetrics when exists (#41022) (#41176)

1311999

* Use namespace for GetListMetrics when exists (cherry picked from commit 36327a4) Co-authored-by: kaiyan-sheng <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AWS] Use namespace for GetListMetrics when exists #41022

[AWS] Use namespace for GetListMetrics when exists #41022

kaiyan-sheng commented Sep 27, 2024 •

edited

Loading

mergify bot commented Sep 27, 2024

mergify bot commented Sep 27, 2024

elasticmachine commented Sep 27, 2024

Kavindu-Dodan left a comment

gizas Oct 3, 2024

gizas Oct 3, 2024

kaiyan-sheng Oct 3, 2024

zmoog Oct 4, 2024

zmoog Oct 4, 2024

gizas Oct 7, 2024

kaiyan-sheng Oct 7, 2024

kaiyan-sheng Oct 7, 2024 •

edited

Loading

kaiyan-sheng Oct 7, 2024

mergify bot commented Oct 3, 2024

zmoog commented Oct 4, 2024

zmoog commented Oct 4, 2024

girodav commented Oct 7, 2024 •

edited

Loading

mergify bot commented Oct 7, 2024

[AWS] Use namespace for GetListMetrics when exists #41022

[AWS] Use namespace for GetListMetrics when exists #41022

Conversation

kaiyan-sheng commented Sep 27, 2024 • edited Loading

Proposed commit message

Checklist

mergify bot commented Sep 27, 2024

mergify bot commented Sep 27, 2024

elasticmachine commented Sep 27, 2024

Kavindu-Dodan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaiyan-sheng Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Oct 3, 2024

zmoog commented Oct 4, 2024

zmoog commented Oct 4, 2024

girodav commented Oct 7, 2024 • edited Loading

mergify bot commented Oct 7, 2024

kaiyan-sheng commented Sep 27, 2024 •

edited

Loading

kaiyan-sheng Oct 7, 2024 •

edited

Loading

girodav commented Oct 7, 2024 •

edited

Loading