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.

Sign up for GitHub

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

Jump to bottom

Change cloud.provider from ec2 to aws and from gce to gcp in add_cloud_metadata #11687

Merged
merged 3 commits into from
Apr 11, 2019
Merged

Change cloud.provider from ec2 to aws and from gce to gcp in add_cloud_metadata #11687

merged 3 commits into from
Apr 11, 2019

Conversation

kaiyan-sheng
Copy link
Contributor

@kaiyan-sheng kaiyan-sheng commented Apr 8, 2019 •
edited
Loading

With the change elastic/ecs#348 in ECS, add_cloud_metadata should change cloud.provider from ec2 to aws and from gce to gcp to match the example in https://www.elastic.co/guide/en/ecs/current/ecs-cloud.html

@kaiyan-sheng kaiyan-sheng requested a review from a team as a code owner April 8, 2019 04:10
@kaiyan-sheng kaiyan-sheng self-assigned this Apr 8, 2019
@kaiyan-sheng kaiyan-sheng mentioned this pull request Apr 8, 2019
Closed
@kaiyan-sheng kaiyan-sheng added the Team:Integrations Label for the Integrations team label Apr 8, 2019
ruflin
ruflin reviewed Apr 8, 2019
View reviewed changes
Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

@andrewkroh Could you have a look at this as you initially introduced this if I remember correctly?

libbeat/processors/add_cloud_metadata/provider_aws_ec2_test.go Outdated
@@ -82,7 +82,7 @@ func TestRetrieveAWSMetadata(t *testing.T) {

expected := common.MapStr{
"cloud": common.MapStr{
"provider": "ec2",
"provider": "aws",
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this is also going to break some of our dashboards?

As I would consider this a bug in 7 I'm good with getting this in even though it's a breaking change.

CHANGELOG.next.asciidoc Outdated Show resolved Hide resolved
libbeat/docs/processors-using.asciidoc Show resolved Hide resolved
@kaiyan-sheng
Copy link
Contributor Author

@ruflin @exekias Thanks for the review. Quick question, similar to this https://www.elastic.co/guide/en/ecs/current/ecs-cloud.html, GCE probably should be GCP instead? hmm

@ruflin
Copy link
Member

ruflin commented Apr 9, 2019

jenkins, test this

@exekias
Copy link
Contributor

exekias commented Apr 9, 2019

+1 to changing GCE to GCP too, hopefully in the same version

@ruflin
Copy link
Member

ruflin commented Apr 9, 2019

@kaiyan-sheng Could you double check that this does not break any dashboards / visualisatins?

@kaiyan-sheng kaiyan-sheng changed the title Change cloud.provider from ec2 to aws in add_cloud_metadata Change cloud.provider from ec2 to aws and from gce to gcp in add_cloud_metadata Apr 9, 2019
@kaiyan-sheng
Copy link
Contributor Author

@kaiyan-sheng Could you double check that this does not break any dashboards / visualisatins?

@ruflin Sorry I forgot to respond to you on this earlier. I did a search in metricbeat dashboards and filebeat dashboards to see if there's anywhere needed cloud.provider=ec2/gce and I didn't see any. So I assume it's safe here. And to be more sure about this, I'm trying to setup metricbeat/kibana/es on GCP to actually test it.

ruflin
ruflin approved these changes Apr 10, 2019
View reviewed changes
Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

LGTM. This will also need backports I assume?

@kaiyan-sheng kaiyan-sheng added needs_backport PR is waiting to be backported to other branches. v7.0.0 labels Apr 10, 2019
@kaiyan-sheng
Copy link
Contributor Author

LGTM. This will also need backports I assume?

Yes! Sorry I forgot to add the labels.

andrewkroh
andrewkroh approved these changes Apr 10, 2019
View reviewed changes
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

I'm fine with the changes. It's unfortunate that this didn't make it into 7.0.0 because now we have a breaking change in a minor.

CHANGELOG.next.asciidoc
@@ -27,6 +27,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d
*Metricbeat*

- Add new option `OpMultiplyBuckets` to scale histogram buckets to avoid decimal points in final events {pull}10994[10994]
- Change cloud.provider from ec2 to aws and from gce to gcp in add_cloud_metadata to align with ECS. {issue}10775[10775] {pull}11687[11687]
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change affect all Beats.

@kaiyan-sheng
Copy link
Contributor Author

@andrewkroh Yeah my bad... I didn't quite realize the timing there...

@kaiyan-sheng kaiyan-sheng merged commit 69f6aeb into elastic:master Apr 11, 2019
@kaiyan-sheng kaiyan-sheng deleted the cloud_metadata_doc branch April 11, 2019 03:21
@kaiyan-sheng kaiyan-sheng mentioned this pull request Apr 11, 2019
Merged
@kaiyan-sheng kaiyan-sheng removed the needs_backport PR is waiting to be backported to other branches. label Apr 11, 2019
kaiyan-sheng added a commit that referenced this pull request Apr 12, 2019
@kaiyan-sheng
Cherry-pick to 7.0: Change cloud.provider from ec2 to aws and from gc…
23c70f0 
…e to gcp in add_clou… (#11765)

* Change cloud.provider from ec2 to aws and from gce to gcp in add_cloud_metadata (#11687)
(cherry picked from commit 69f6aeb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewkroh andrewkroh andrewkroh approved these changes

@exekias exekias exekias approved these changes

@ruflin ruflin ruflin approved these changes

Assignees

@kaiyan-sheng kaiyan-sheng

Labels
libbeat review Team:Integrations Label for the Integrations team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kaiyan-sheng @ruflin @exekias @andrewkroh @alvarolobato