[Metricbeat] Add support for multiple regions in GCP by gpop63 · Pull Request #32964 · elastic/beats

gpop63 · 2022-09-01T21:34:05Z

What does this PR do?

Adds support for multiple regions in GCP.

Why is it important?

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.

Author's Checklist

[ ]

How to test this PR locally

Related issues

Closes [Metricbeat] Allow to specify more than one regions in googlecloud module #17835

Use cases

Screenshots

Logs

mergify · 2022-09-01T21:34:40Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @gpop63? 🙏.
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 · 2022-09-01T21:58:42Z

💚 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: 2022-09-09T19:07:00.861+0000
Duration: 51 min 52 sec

Test stats 🧪

Test	Results
Failed	0
Passed	1316
Skipped	87
Total	1403

💚 Flaky test report

Tests succeeded.

🤖 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!)

endorama

A couple of things I think needs to be improved before merging, but overall 👍

endorama · 2022-09-07T13:54:34Z

x-pack/metricbeat/module/gcp/metrics/metrics_requester.go


 	switch serviceName {
+	case gcp.ServiceCompute:
+		if r.config.Region != "" && r.config.Zone != "" {


This condition should take into account the case where region and regions are both configured

endorama · 2022-09-07T13:56:11Z

x-pack/metricbeat/module/gcp/metrics/metrics_requester.go

 		}

-		f = fmt.Sprintf(`%s AND resource.labels.location = "%s"`, f, r.config.Region)
+		return f


Is this return needed? If not I would remove it there is just one return at the end (where we also log the full filter).

endorama · 2022-09-07T13:56:46Z

x-pack/metricbeat/module/gcp/metrics/metrics_requester.go

+			f = fmt.Sprintf("%s AND %s", f, regionsFilter)
+		}
 	case gcp.ServiceGKE:
 		if r.config.Region != "" && r.config.Zone != "" {


This cThis condition should take into account the case where region and regions are both configured

endorama · 2022-09-07T13:57:15Z

x-pack/metricbeat/module/gcp/metrics/metrics_requester.go

 	case gcp.ServiceStorage:
-		if r.config.Region == "" {
-			return f
+		switch {


Here should be added the warning about region taking precedence over regions

endorama · 2022-09-07T13:58:37Z

@gpop63 I notice we use a if/break pattern that screams refactoring. What do you think about following up with a PR that changes that for if/elseif/else or switch/case like you did in gcp.ServiceStorage?

* add regions config setting and pass it as argument * add services region resource label constants * add buildRegionsFilter and update getFilterForMetric logic * add getFilterForMetric and buildRegionsFilter tests * add changelog entry * minor changes for golangci-lint * add warn logs and remove redundant return * add missing argument in warnf log (cherry picked from commit 3bcefab)

endorama · 2022-09-15T08:36:04Z

I'm going to backport this to 7.17, otherwise this change would make our integration not compatible with the 7.x branch.

* add regions config setting and pass it as argument * add services region resource label constants * add buildRegionsFilter and update getFilterForMetric logic * add getFilterForMetric and buildRegionsFilter tests * add changelog entry * minor changes for golangci-lint * add warn logs and remove redundant return * add missing argument in warnf log (cherry picked from commit 3bcefab) # Conflicts: # x-pack/metricbeat/module/gcp/metrics/compute/metadata.go # x-pack/metricbeat/module/gcp/metrics/metrics_requester.go # x-pack/metricbeat/module/gcp/metrics/metrics_requester_test.go # x-pack/metricbeat/module/gcp/metrics/metricset.go

endorama · 2022-09-15T16:05:52Z

Removing backport as backport is not needed.

kaiyan-sheng · 2022-10-18T17:43:37Z

CHANGELOG-developer.next.asciidoc

 - Add regex support for drop_fields processor.
 - Improve compatibility and reduce flakyness of Python tests {pull}31588[31588]
 - Added `.python-version` file {pull}32323[32323]
+- Add support for multiple regions in GCP {pull}32964[32964]


Just saw this changelog is under developer changelog, should this be under CHANGELOG-next?

You're right! @gpop63 may you please add this line to CHANGELOG.next with a separate PR?

* add regions config setting and pass it as argument * add services region resource label constants * add buildRegionsFilter and update getFilterForMetric logic * add getFilterForMetric and buildRegionsFilter tests * add changelog entry * minor changes for golangci-lint * add warn logs and remove redundant return * add missing argument in warnf log

gpop63 added 5 commits September 2, 2022 00:28

add regions config setting and pass it as argument

54164d3

add services region resource label constants

f079f12

add buildRegionsFilter and update getFilterForMetric logic

2f27442

add getFilterForMetric and buildRegionsFilter tests

31d8fa1

add changelog entry

2f73eb0

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

gpop63 added the Team:Cloud-Monitoring Label for the Cloud Monitoring team label Sep 1, 2022

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

mergify bot assigned gpop63 Sep 1, 2022

minor changes for golangci-lint

f492e82

gpop63 marked this pull request as ready for review September 1, 2022 22:44

gpop63 requested a review from a team as a code owner September 1, 2022 22:44

endorama requested changes Sep 7, 2022

View reviewed changes

add warn logs and remove redundant return

ecc40c6

gpop63 force-pushed the add_gcp_multiple_regions branch from 5883353 to ecc40c6 Compare September 9, 2022 13:21

elastic deleted a comment from cla-checker-service bot Sep 9, 2022

endorama added the backport-v8.4.0 Automated backport with mergify label Sep 9, 2022

endorama approved these changes Sep 9, 2022

View reviewed changes

add missing argument in warnf log

c3866cd

gpop63 merged commit 3bcefab into elastic:main Sep 11, 2022

mergify bot mentioned this pull request Sep 11, 2022

[8.4](backport #32964) [Metricbeat] Add support for multiple regions in GCP #33046

Closed

gpop63 mentioned this pull request Sep 11, 2022

[Metricbeat] GCP refactor getFilterForMetric ifelse to switch #33047

Merged

6 tasks

endorama mentioned this pull request Sep 12, 2022

[GCP] support multiple regions when gathering metrics elastic/integrations#4197

Merged

11 tasks

kaiyan-sheng removed the backport-v8.4.0 Automated backport with mergify label Sep 12, 2022

endorama added the backport-7.17 Automated backport to the 7.17 branch with mergify label Sep 15, 2022

mergify bot mentioned this pull request Sep 15, 2022

[7.17](backport #32964) [Metricbeat] Add support for multiple regions in GCP #33098

Closed

endorama removed the backport-7.17 Automated backport to the 7.17 branch with mergify label Sep 15, 2022

kaiyan-sheng reviewed Oct 18, 2022

View reviewed changes

gpop63 mentioned this pull request Nov 17, 2022

[metricbeat] [gcp] fix regions feature changelog entry #33716

Merged

6 tasks

Conversation

gpop63 commented Sep 1, 2022

What does this PR do?

Why is it important?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

Uh oh!

mergify bot commented Sep 1, 2022

Uh oh!

elasticmachine commented Sep 1, 2022 • edited by jenkins-beats-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

Uh oh!

endorama left a comment

Choose a reason for hiding this comment

Uh oh!

endorama Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

endorama Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

endorama Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

endorama Sep 7, 2022

Choose a reason for hiding this comment

Uh oh!

endorama commented Sep 7, 2022

Uh oh!

endorama commented Sep 15, 2022

Uh oh!

endorama commented Sep 15, 2022

Uh oh!

kaiyan-sheng Oct 18, 2022

Choose a reason for hiding this comment

Uh oh!

endorama Nov 15, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

elasticmachine commented Sep 1, 2022 •

edited by jenkins-beats-ci bot

Loading