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 add_cloud_metadata to not overwrite cloud field #11612

Merged
merged 9 commits into from
Apr 9, 2019
Merged

Change add_cloud_metadata to not overwrite cloud field #11612

merged 9 commits into from
Apr 9, 2019

Conversation

kaiyan-sheng
Copy link
Contributor

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

When aws module is enabled, ec2 metricset will collect data with cloud.* included. If metricbeat is running on GCP or other cloud platforms, with the current add_cloud_metadata processor will overwrite the cloud.* fields from ec2 metricset.

This PR is to disable the overwrite part for add_cloud_metadata. When cloud.* exists already, add_cloud_metadata will not overwrite cloud fields.

closes #11305

@kaiyan-sheng kaiyan-sheng requested a review from a team as a code owner April 2, 2019 18:42
@kaiyan-sheng kaiyan-sheng added Team:Integrations Label for the Integrations team discuss Issue needs further discussion. review labels Apr 2, 2019
@kaiyan-sheng kaiyan-sheng self-assigned this Apr 2, 2019
ruflin
ruflin requested changes Apr 3, 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.

+1 on this change. See comments inside.

libbeat/processors/add_cloud_metadata/provider_aws_ec2_test.go Show resolved Hide resolved
libbeat/processors/add_cloud_metadata/add_cloud_metadata.go Outdated Show resolved Hide resolved
ruflin
ruflin reviewed Apr 4, 2019
View reviewed changes
libbeat/processors/add_cloud_metadata/provider_aws_ec2_test.go Show resolved Hide resolved
libbeat/processors/add_cloud_metadata/provider_aws_ec2_test.go
"cloud.provider": "ec2",
},
common.MapStr{
"cloud.provider": "ec2",
Copy link
Member

Choose a reason for hiding this comment

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

Really expected? Not even sure what ES will do with such a document.

andrewkroh
andrewkroh requested changes Apr 4, 2019
View reviewed changes
libbeat/processors/add_cloud_metadata/add_cloud_metadata.go Outdated
// cloud fields. For example aws module writes cloud.instance.* to events already, with overwrite=false,
// add_cloud_metadata should not overwrite these fields with new values.
if !p.initData.overwrite {
for key := range event.Fields {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to do this? Are we expecting to be dealing with keys containing dots?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andrewkroh Yeah this is to deal with https://github.com/elastic/beats/pull/11612/files/38badaa57f14922562520ee38de7b9b5aba0aacf#diff-46124ff4b7e0a195eaff6cbeb8a90293R117 if keys containing dot 🤔 For example "cloud.provider": "ec2" instead of "cloud": common.MapStr{"provider": "ec2"}

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 event.Fields.HasKey would already to the right thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ruflin event.Fields.HasKey only check if cloud field exists but it doesn't check if cloud field value is null or not. I'm thinking to still use event.GetValue and compare value with nil.

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.

@kaiyan-sheng Sorry, forgot to publish my recent review comments.

For your question about dotted key check: I general I would say yes but as it's an issue not only affecting cloud, I suggest we keep it as is in this PR and open a follow up issue.

For the tests which from my perspective test the "wrong behaviour" let's keep them but add a comment that this is what happens currently but should be changed. So in case we fix it later we don't stumble over this test and are like: Oh, that is the way it should work.

libbeat/processors/add_cloud_metadata/add_cloud_metadata.go Outdated
// cloud fields. For example aws module writes cloud.instance.* to events already, with overwrite=false,
// add_cloud_metadata should not overwrite these fields with new values.
if !p.initData.overwrite {
for key := range event.Fields {
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 event.Fields.HasKey would already to the right thing?

libbeat/processors/add_cloud_metadata/provider_aws_ec2_test.go Show resolved Hide resolved
@kaiyan-sheng kaiyan-sheng mentioned this pull request Apr 8, 2019
Closed
@kaiyan-sheng
Copy link
Contributor Author

@ruflin Thanks for the suggestion. I just created #11697 for the follow up issue.

andrewkroh
andrewkroh approved these changes Apr 9, 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.

LGTM. I left some minor documentation edits, but nothing that I need to review again.

CHANGELOG.next.asciidoc Show resolved Hide resolved
libbeat/docs/processors-using.asciidoc Outdated Show resolved Hide resolved
libbeat/docs/processors-using.asciidoc Outdated Show resolved Hide resolved
ruflin
ruflin approved these changes Apr 9, 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.

One minor doc adjustment. Overall LGTM.

Could you rebase on master to get the build back to green?

I think we should also backport this to 7.0 as I would consider this a bug.

libbeat/docs/processors-using.asciidoc Outdated Show resolved Hide resolved
@kaiyan-sheng kaiyan-sheng added backport needs_backport PR is waiting to be backported to other branches. v7.0.0 and removed backport labels Apr 9, 2019
@kaiyan-sheng kaiyan-sheng merged commit 17edc90 into elastic:master Apr 9, 2019
@kaiyan-sheng kaiyan-sheng deleted the cloud_metadata_overwrite branch April 9, 2019 18:01
@kaiyan-sheng kaiyan-sheng mentioned this pull request Apr 9, 2019
Merged
kaiyan-sheng added a commit that referenced this pull request Apr 11, 2019
@kaiyan-sheng
Cherry-pick to 7.0: Change add_cloud_metadata to not overwrite cloud …
587707d 
…field (#11612) (#11730)

* Change add_cloud_metadata to not overwrite cloud field (#11612)

(cherry picked from commit 17edc90)
@kaiyan-sheng kaiyan-sheng removed needs_backport PR is waiting to be backported to other branches. v7.0.0 labels Apr 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@exekias exekias exekias left review comments

@ruflin ruflin ruflin approved these changes

@andrewkroh andrewkroh andrewkroh approved these changes

Assignees

@kaiyan-sheng kaiyan-sheng

Labels
discuss Issue needs further discussion. review Team:Integrations Label for the Integrations team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[metricbeat/aws] When running Metricbeat from GCP cloud.* fields get overwritten
5 participants
@kaiyan-sheng @ruflin @exekias @andrewkroh @alvarolobato