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

[GCP] support multiple regions when gathering metrics #4197

Merged
merged 14 commits into from
Nov 22, 2022
Merged

[GCP] support multiple regions when gathering metrics #4197

merged 14 commits into from
Nov 22, 2022

Conversation

endorama
Copy link
Member

@endorama endorama commented Sep 12, 2022

What does this PR do?

Add regions parameter to data streams.

It also updates Kibana min version to 8.5.0, as Metricbeat 8.5.0 is required for the underlying feature: elastic/beats#32964

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

Author's Checklist

regions setting should be added to these data streams:

  • compute
  • dataproc
  • firestore
  • gke
  • loadbalancing_metrics
  • redis
  • storage

How to test this PR locally

Related issues

Screenshots

@endorama endorama marked this pull request as ready for review September 12, 2022 15:56
@endorama endorama requested review from a team as code owners September 12, 2022 15:56
@endorama
Copy link
Member Author

I would merge this after the feature freeze for 8.5.0, so we don't block other possible changes compatible with 8.4.0.

@elasticmachine
Copy link

elasticmachine commented Sep 12, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-11-14T14:43:09.385+0000

  • Duration: 20 min 11 sec

Test stats 🧪

Test Results
Failed 0
Passed 52
Skipped 0
Total 52

🤖 GitHub comments

Expand to view the GitHub comments

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

  • /test : Re-trigger the build.

@elasticmachine
Copy link

elasticmachine commented Sep 12, 2022

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (5/5) 💚
Files 100.0% (5/5) 💚 2.314
Classes 100.0% (5/5) 💚 2.314
Methods 90.099% (91/101) 👎 -1.526
Lines 95.752% (1375/1436) 👍 4.048
Conditionals 100.0% (0/0) 💚

@tommyers-elastic
Copy link
Contributor

hey @endorama - one comment from me - i think it's kinda confusing seeing region and regions in the config. shouldn't we simply replace region with regions?

Screenshot 2022-09-20 at 13 37 40

@tommyers-elastic tommyers-elastic self-requested a review September 20, 2022 12:39
@tommyers-elastic tommyers-elastic added Team:Cloud-Monitoring Label for the Cloud Monitoring team Integration:gcp Google Cloud Platform labels Sep 20, 2022
@endorama
Copy link
Member Author

endorama commented Sep 20, 2022

@tommyers-elastic I agree. The reason behind this aws "removing region is a breaking change", but I did not consider that we can remove it from the UI without removing it from the gcp.metrics module. Less confusing UI, same features. What do you think?

@elasticmachine
Copy link

elasticmachine commented Sep 21, 2022

🚀 Benchmarks report

Package gcp 👍(2) 💚(1) 💔(2)

Expand to view
Data stream Previous EPS New EPS Diff (%) Result
dns 3690.04 2564.1 -1125.94 (-30.51%) 💔
loadbalancing_logs 5208.33 3424.66 -1783.67 (-34.25%) 💔

To see the full report comment with /test benchmark fullreport

@endorama
Copy link
Member Author

I went on and removed region setting from data stream with regions

@tommyers-elastic
Copy link
Contributor

agreed that removing from the UI is correct way forward. thanks

@endorama
Copy link
Member Author

With #4325 this PR shall add regions to redis data stream too

@endorama endorama marked this pull request as draft September 28, 2022 15:10
@tommyers-elastic
Copy link
Contributor

hey @endorama how come this PR moved to draft? is there something blocking us moving ahead on this? thanks!

@endorama
Copy link
Member Author

endorama commented Oct 6, 2022

@tommyers-elastic the reasoning has been:

@endorama
Copy link
Member Author

I tested the upgrade path from a previous version to this one.
I added region to the configuration and upgraded and this is the result:

  • region is not an hidden field in a policy
  • data collection still works, but the policy set region cannot longer be changed
  • regions can be set as expected

Due to this, users that are using region today would not be able to edit policies any more after upgrade. Data collection works and the workaround could be to create a new policy and unassign-reassign the agents to the new policy, but I'm not sure if this is ok from the user experience point of view.

@endorama
Copy link
Member Author

TL;DR: I'm going to reintroduce region with a message about the deprecation.

We had an internal discussion about this and we don't yet have a way to deprecate fields. Due to the upgrade issues and this missing functionality we all agreed that is best not to remove region to prevent what could be considered a breaking change.

@endorama endorama marked this pull request as ready for review October 20, 2022 09:46
Copy link
Contributor

@aspacca aspacca left a comment

Choose a reason for hiding this comment

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

LGTM

The relevant metricbeat change will be shipped with 8.5.0, so
this changeset needs minimum that kibana version to work as
expected.

elastic/beats#32964
This reverts commit 247a65d603df548fc66be04940a67c81cdb352df.
@endorama endorama merged commit 1947b55 into elastic:main Nov 22, 2022
@endorama endorama deleted the gcp-support-regions branch November 22, 2022 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Integration:gcp Google Cloud Platform Team:Cloud-Monitoring Label for the Cloud Monitoring team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants