Add replace_fields config option in add_host_metadata for replacing host fields by kaiyan-sheng · Pull Request #20490 · elastic/beats

kaiyan-sheng · 2020-08-07T00:35:06Z

What does this PR do?

This PR adds replace_fields config option in add_host_metadata for replacing host fields. By default, replace_fields equals to true to make sure this PR is not a breaking change.

Why is it important?

We are adding common host fields into several modules in Metricbeat, such as host.id and host.name. But add_host_metadata processor would overwrite host.id field without this PR.

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.

How to test this PR locally

This PR should work for any module/metricset. I'm using aws module as an example here:

Make sure Add host inventory metrics to ec2 metricset #20171 is in the code and then enable aws module by
./metricbeat modules enable aws
Change aws config modules.d/aws.yml to:

- module: aws
  period: 5m
  credential_profile_name: elastic-beats
  metricsets:
    - ec2

Make sure in metricbeat.yml, add_host_metadata processor is enabled and replace_fields is set to false:

processors:
  - add_host_metadata:
      replace_fields: false

Start Metricbeat with ./metricbeat -e
Check metrics sent to ES from Kibana discover. Metrics should have host.* fields only from ec2 metricset but not any fields from add_host_metadata such as host.hostname, host.architecture, host.os.* etc.

Related issues

#20464

elasticmachine · 2020-08-07T02:40:21Z

💚 Build Succeeded

Expand to view the summary

Build stats

Build Cause: [Pull request #20490 updated]
Start Time: 2020-08-14T16:50:49.857+0000
Duration: 65 min 24 sec

Test stats 🧪

Test	Results
Failed	0
Passed	18002
Skipped	1827
Total	19829

elasticmachine · 2020-08-10T17:40:39Z

Pinging @elastic/integrations-platforms (Team:Platforms)

libbeat/processors/add_host_metadata/add_host_metadata.go

kaiyan-sheng · 2020-08-12T02:53:09Z

@jsoriano I consider this is a breaking change. Is it ok to add this in next release?

jsoriano · 2020-08-12T07:58:39Z

@jsoriano I consider this is a breaking change. Is it ok to add this in next release?

@kaiyan-sheng I agree that this is a breaking change, but it seems needed for the inventory efforts, and I think that for the cases we will be breaking it will be more a fix than a breaking change 🙂

Apart of "breaking" the case of add_host_metadata overwriting fields added by modules, is there any other case that you think we would be affecting with this change?

Something else we can do to mitigate possible problems is to add a replace_fields option to add_host_metadata, that defaults to false, and can be set to true to have the previous behaviour.

We can also add, in the note you are adding in add_host_metadata documentation, a line about using add_observer_metadata instead of add_host_metadata if the beat is being used to monitor external systems.

kaiyan-sheng · 2020-08-13T02:40:53Z

Apart of "breaking" the case of add_host_metadata overwriting fields added by modules, is there any other case that you think we would be affecting with this change?

Hmm not what I can think of. I will post it here if I see any other case.

Something else we can do to mitigate possible problems is to add a replace_fields option to add_host_metadata, that defaults to false, and can be set to true to have the previous behaviour.

Good point! I added replace_host_fields config parameter for this and set default as false. If we want to keep this change not a breaking change, we can set default as true. WDYT? @jsoriano

My thought is, we can keep it default as true right now to match the previous/original behavior and then switch the default to false in 8.0.

We can also add, in the note you are adding in add_host_metadata documentation, a line about using add_observer_metadata instead of add_host_metadata if the beat is being used to monitor external systems.

Done. Thank you!

libbeat/processors/add_host_metadata/add_host_metadata.go

jsoriano · 2020-08-13T10:25:12Z

Good point! I added replace_host_fields config parameter for this and set default as false. If we want to keep this change not a breaking change, we can set default as true. WDYT? @jsoriano

My thought is, we can keep it default as true right now to match the previous/original behavior and then switch the default to false in 8.0.

Ok, let's avoid making it a breaking change by now, we can still reconsider this before 7.10. I was convinced that it was better to make the breaking change, and I still think it would be for Metricbeat now that we are adding more logic in modules for the host inventory. But then I have thought on Filebeat and I am not so sure. There are probably SIEM use cases where log parsing is adding host information, but additional network information is still wanted. And also, we are adding this processor by default in the Kubernetes manifests for Filebeat.

Btw (naming is complicated), I find having host in replace_host_fields a bit redundant. Wdyt about naming it replace_fields?

libbeat/processors/add_host_metadata/add_host_metadata.go

kaiyan-sheng · 2020-08-13T17:05:50Z

Ok, let's avoid making it a breaking change by now, we can still reconsider this before 7.10. I was convinced that it was better to make the breaking change, and I still think it would be for Metricbeat now that we are adding more logic in modules for the host inventory. But then I have thought on Filebeat and I am not so sure. There are probably SIEM use cases where log parsing is adding host information, but additional network information is still wanted. And also, we are adding this processor by default in the Kubernetes manifests for Filebeat.

Done. replace_fields config defaults to true for now to keep the same behavior.
@andrewkroh Do you have concerns about this? TIA!

Btw (naming is complicated), I find having host in replace_host_fields a bit redundant. Wdyt about naming it replace_fields?

Done, yeah I was thinking replace_host_fields is more clear but it is a bit redundant.

jsoriano

LGTM, added some nitpicking comments.

We can decide to make it a breaking change later.

libbeat/processors/add_host_metadata/add_host_metadata.go

libbeat/processors/add_host_metadata/add_host_metadata_test.go

jsoriano

Generator errors in CI have been fixed in master by #20586. The other errors are flaky ones.

kaiyan-sheng · 2020-08-14T18:18:59Z

Merging this PR. If we decide to change replace_fields default to false, we can create a separate PR later.

…ost fields (#20490) (#20617) * add replace_host_fields config param (cherry picked from commit 7bb31e6)

andrewkroh · 2020-08-25T11:05:05Z

libbeat/processors/add_host_metadata/add_host_metadata.go

+		return false
+	}
+
+	hostFieldsMap := hostFields.(common.MapStr)


This will panic if the host is not a common.MapStr. Since Beats like Filebeat can handle arbitrary data it should be defensive (for example see

beats/metricbeat/mb/event.go

Lines 207 to 216 in 95626b8

func tryToMapStr(v interface{}) (common.MapStr, bool) {

switch m := v.(type) {

case common.MapStr:

return m, true

case map[string]interface{}:

return common.MapStr(m), true

default:

return nil, false

}

}

).

Thanks @andrewkroh for pointing this out! I will create a separate PR to fix this issue.

This is the PR: #20791

…ost fields (elastic#20490) * add replace_host_fields config param

Change add_host_metadata to not overwrite if host fields already exist

f3a4d2e

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Aug 7, 2020

kaiyan-sheng self-assigned this Aug 7, 2020

kaiyan-sheng added in progress Pull request is currently in progress. Team:Platforms Label for the Integrations - Platforms team labels Aug 7, 2020

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Aug 7, 2020

kaiyan-sheng added 3 commits August 7, 2020 10:57

undo remove host.name from libbeat

4f6c33e

Merge remote-tracking branch 'upstream/master' into host_name

c4ac6e1

add changelog

1e8e6d5

kaiyan-sheng mentioned this pull request Aug 10, 2020

add_host_metadata processor overwrites host.id field added by modules #20464

Closed

update add_host_metadata doc

dee70c9

kaiyan-sheng marked this pull request as ready for review August 10, 2020 17:40

kaiyan-sheng added needs_backport PR is waiting to be backported to other branches. review test-plan Add this PR to be manual test plan and removed in progress Pull request is currently in progress. labels Aug 10, 2020

ChrsMark reviewed Aug 11, 2020

View reviewed changes

libbeat/processors/add_host_metadata/add_host_metadata.go Outdated Show resolved Hide resolved

remove host.name and check host field length

8d8b4e7

kaiyan-sheng requested a review from ChrsMark August 11, 2020 18:12

add replace_host_fields config param

569c349

ChrsMark reviewed Aug 13, 2020

View reviewed changes

libbeat/processors/add_host_metadata/add_host_metadata.go Outdated Show resolved Hide resolved

jsoriano reviewed Aug 13, 2020

View reviewed changes

libbeat/processors/add_host_metadata/add_host_metadata.go Outdated Show resolved Hide resolved

kaiyan-sheng added 2 commits August 13, 2020 10:26

change config option to replace_fields and default to true

ba72694

Merge remote-tracking branch 'upstream/master' into host_name

8c37b27

update changelog

e971439

jsoriano reviewed Aug 13, 2020

View reviewed changes

jsoriano approved these changes Aug 13, 2020

View reviewed changes

Simplify skipAddingHostMetadata and add unit test for no host field case

f25052a

jsoriano approved these changes Aug 14, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into host_name

336a4b9

kaiyan-sheng changed the title ~~Change add_host_metadata to not overwrite if host fields already exist~~ Add replace_fields config option in add_host_metadata for replacing host fields Aug 14, 2020

kaiyan-sheng merged commit 7bb31e6 into elastic:master Aug 14, 2020

kaiyan-sheng deleted the host_name branch August 14, 2020 18:19

kaiyan-sheng mentioned this pull request Aug 14, 2020

Cherry-pick #20490 to 7.x: Add replace_fields config option in add_host_metadata for replacing host fields #20617

Merged

6 tasks

kaiyan-sheng added v7.10.0 and removed needs_backport PR is waiting to be backported to other branches. labels Aug 14, 2020

kaiyan-sheng pushed a commit that referenced this pull request Aug 17, 2020

Add replace_fields config option in add_host_metadata for replacing h…

44103e9

…ost fields (#20490) (#20617) * add replace_host_fields config param (cherry picked from commit 7bb31e6)

jsoriano mentioned this pull request Aug 20, 2020

Add host inventory metrics to ec2 metricset #20171

Merged

6 tasks

andrewkroh reviewed Aug 25, 2020

View reviewed changes

andresrc added the test-plan-added This PR has been added to the test plan label Oct 3, 2020

kaiyan-sheng mentioned this pull request Oct 13, 2020

Change default value replace_fields to false in add_host_metadata processor #21774

Closed

melchiormoulin pushed a commit to melchiormoulin/beats that referenced this pull request Oct 14, 2020

Add replace_fields config option in add_host_metadata for replacing h…

31bc851

…ost fields (elastic#20490) * add replace_host_fields config param

	func tryToMapStr(v interface{}) (common.MapStr, bool) {
	switch m := v.(type) {
	case common.MapStr:
	return m, true
	case map[string]interface{}:
	return common.MapStr(m), true
	default:
	return nil, false
	}
	}

Conversation

kaiyan-sheng commented Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Why is it important?

Checklist

How to test this PR locally

Related issues

Uh oh!

elasticmachine commented Aug 7, 2020 • 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 🧪

Uh oh!

elasticmachine commented Aug 10, 2020

Uh oh!

Uh oh!

kaiyan-sheng commented Aug 12, 2020

Uh oh!

jsoriano commented Aug 12, 2020

Uh oh!

kaiyan-sheng commented Aug 13, 2020

Uh oh!

Uh oh!

jsoriano commented Aug 13, 2020

Uh oh!

Uh oh!

kaiyan-sheng commented Aug 13, 2020

Uh oh!

jsoriano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsoriano left a comment

Choose a reason for hiding this comment

Uh oh!

kaiyan-sheng commented Aug 14, 2020

Uh oh!

andrewkroh Aug 25, 2020

Choose a reason for hiding this comment

Uh oh!

kaiyan-sheng Aug 26, 2020

Choose a reason for hiding this comment

Uh oh!

kaiyan-sheng Aug 26, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kaiyan-sheng commented Aug 7, 2020 •

edited

Loading

elasticmachine commented Aug 7, 2020 •

edited by jenkins-beats-ci bot

Loading