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

Add Beats compatible fields.yml for ECS #108

Merged
merged 3 commits into from
Sep 4, 2018

Conversation

ruflin
Copy link
Contributor

@ruflin ruflin commented Aug 30, 2018

Add type: group to each prefix.

@ruflin ruflin added the review label Aug 30, 2018
@ruflin ruflin requested a review from andrewkroh August 30, 2018 09:02
Makefile Outdated
sed -i.bak 's/---//g' fields.tmp.yml
cat scripts/fields_header.yml > fields.yml
cat fields.tmp.yml >> fields.yml
rm fields.tmp.yml.bak
Copy link
Member

Choose a reason for hiding this comment

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

make check needs to updated to call fields.

It looks like it could be simplified by making it depend on the targets that it invokes by calling $(MAKE) (e.g. change it to check: generate fmt fields).

fields.yml Outdated
ECS fields.
fields:

- name: agent
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this have type: group?

fields.yml Outdated
ECS fields.
fields:

- name: agent
Copy link
Member

Choose a reason for hiding this comment

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

Would it be easy to move this over another 2 spaces? My goal is to have the it look like this

fields:
  - blah

rather than

fields:
- blah

which is consistent with indentations found in other areas of the file.

Makefile Outdated
sed -i.bak 's/---//g' fields.tmp.yml
cat scripts/fields_header.yml > fields.yml
cat fields.tmp.yml >> fields.yml
rm fields.tmp.yml.bak
Copy link
Member

Choose a reason for hiding this comment

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

make check needs to updated to call fields.

It looks like it could be simplified by making it depend on the targets that it invokes by calling $(MAKE) (e.g. change it to check: generate fmt fields).

@andrewkroh
Copy link
Member

@ruflin I pushed an update.

And I tried out the fields.yml file with a Beat and it worked 👍 . I had to remove the duplicates for things that were already defined in libbeat.

@ruflin
Copy link
Contributor Author

ruflin commented Aug 31, 2018

Thanks @andrewkroh I rebased on master and pushed again. Somehow build had a diff, not sure why.

@andrewkroh
Copy link
Member

With the rebase it looks like it pulled in the recent .original changes so ‘make fields’ needs to run again.

@ruflin
Copy link
Contributor Author

ruflin commented Aug 31, 2018

The problem was that the fields target was missing for generate, so only check failed but it was not updated when running just make. I assume updating it always is the behaviour we want?

Copy link
Contributor

@webmat webmat left a comment

Choose a reason for hiding this comment

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

LGTM

Love the small cleanups in Makefile and whitespace 👍🏼

title: ECS
description: >
ECS fields.
fields:
Copy link
Contributor

Choose a reason for hiding this comment

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

Not familiar with the automation we have in Beats that will consume this. I won't nest everything under ecs.*, will it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will not. There is quite a bit of history on why this additional thingy is here but in short it has to do with docs and how the fields.yml was organised in the past.

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.

The blank lines in fields.yml contain some trailing whitespace, but it's not a big deal and we can consider cleaning that up later.

@andrewkroh andrewkroh merged commit 3ca1f4b into elastic:master Sep 4, 2018
adriansr added a commit to elastic/beats that referenced this pull request Oct 2, 2018
This adds a new fields file, libbeat/_meta/fields.ecs.yml, that is a copy of fields.yml from elastic/ecs#108, except: 
- the document separator --- has been removed.
- keyword subfields have been renamed to raw (to maintain backwards compatibility).
- source group has been removed.
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.

3 participants