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

allow/merge fields.yml overrides #9188

Merged
merged 15 commits into from
Nov 27, 2018
Merged

Conversation

graphaelli
Copy link
Member

@graphaelli graphaelli commented Nov 21, 2018

With the introduction of fields.ecs.yml fields that were application specific are now defined generically by libbeat.

This change introduces the ability to customize certain attributes of fields by those downstream. For example, APM sets the count attribute (kibana "popularity") on error.id and is unable to integrate fields.ecs.yml and retain that without a change like this.

Care was taken to forbid field type changes. That is a field can't be defined as a keyword in one place and overridden to long in another.

Reviewers: Please consider whether all affected fields.yml consumers are handled by these changes.

libbeat/kibana/fields_transformer.go Outdated Show resolved Hide resolved
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.

Could we introduce perhaps a convention that fields which are defined as duplicates actually must have a flag like overwrite: true or something like this. Otherwise we can't tell the difference between accidential duplicates and duplicates which are there on purpose.

libbeat/kibana/fields_transformer.go Outdated Show resolved Hide resolved
libbeat/kibana/fields_transformer_test.go Outdated Show resolved Hide resolved
@webmat webmat added the ecs label Nov 21, 2018
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.

Left a few question.

libbeat/generator/fields/fields.go Show resolved Hide resolved
libbeat/kibana/fields_transformer.go Show resolved Hide resolved
libbeat/kibana/fields_transformer_test.go Show resolved Hide resolved
libbeat/kibana/fields_transformer_test.go Show resolved Hide resolved
 Conflicts:
	filebeat/include/fields.go
	libbeat/kibana/fields_transformer.go
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.

Overall LGTM. There seems to be one left over file.

Could you also add a note to the Developer changelog?

libbeat/kibana/try1.patch Outdated Show resolved Hide resolved
@graphaelli
Copy link
Member Author

Thanks for the reviews. All feedback has been addressed and a merge with master + make update has been run.

@graphaelli graphaelli merged commit 1c1b7d7 into elastic:master Nov 27, 2018
@graphaelli graphaelli deleted the fields-merge branch November 27, 2018 21:16
@graphaelli
Copy link
Member Author

Merging without waiting on Darwin to avoid being beaten by a merge conflict yet again. Rest of the Jenkins run is green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants