Make fields.yml part of the binary #4834

ruflin · 2017-08-07T08:33:59Z

Currently the fields.yml file is shipped as its own file and has to be place in the right directory to be used. As fields have become a central part of beats I suggest to make them part of the binary which will allow us to also split up the fields.yml inside the binary into multiple part (for example one per module) and use it for testing like fields checks.

The behavior from the user perspective is as following:

By default internal fields.yml data is used
If fields.yml path is set in the config file, it is loaded from disk

The golang files are generated when running make update. The content of fields.yml is stored as a compressed string.

Each asset is currently prefixed by the Beat name. The current fields registry is a basic implementation that likely needs to be extended when we add fields per module or processors.

tsg · 2017-08-08T12:15:46Z

I think we need to move into the direction where we no longer concatenate all the module's fields.yml into one large file, but have them per module. This allows the compiling of "partial" mapping templates and index patterns, with only the fields that are in use.

Note sure if this is conflicting or is just orthogonal with this PR, but I wanted us to keep that in mind during the discussion.

ruflin · 2017-08-08T19:20:36Z

I like the idea of having the "partial" fields.yml locally. The main thing that I see as a blocker to load partial mappings is that with ES 6.0 inheritance of templates was removed. This means if someone starts with the mysql module and then adds the redis module one large template with a new index would have to be used. One way out of this would be to have one index per module.

ruflin · 2017-10-06T08:59:45Z

@andrewkroh go-bindata causes an issue here as it does not generate with which goimports is happy. I remember you had your own implementation of loading file data. Can you point me to it again?

andrewkroh · 2017-10-06T16:15:18Z

It's over in elastic/go-libaudit.

ruflin · 2017-10-09T08:08:57Z

@andrewkroh Thanks. I took your implementation and modified it a bit to be used here.

tsg · 2017-10-09T08:48:59Z

@ruflin There's a probably related test failure here: https://travis-ci.org/elastic/beats/jobs/285439037#L1872

tsg

Generally LGTM. I think you want to remove the packaging of fields.yml in each platform (for example here), and then test the packages.

ruflin · 2017-10-09T10:33:26Z

I was thinking of removing fields.yml from the package only when the export of fields.yml is possible. I added it as a task to this issue here: #5347

ruflin · 2017-10-09T13:07:54Z

@tsg Have a look again, tests should be fixed. Travis failure is related to elastic/logstash#8456

andrewkroh · 2017-10-09T14:11:42Z

filebeat/include/fields.go

+)
+
+func init() {
+	if err := assets.AddFields("fields.yml", Asset); err != nil {


If two Beats were combined into the same process would this still work?

The AddFields would work as currently the name is not used at all internally. What would cause problems is that the fields from libbeat would be duplicated. For now I kept the fields.yml exactly as it is and a 1-1 copy is added to each file. For some long terms ideas like splitting up for libbeat or modules, see PR description.

I was thinking that you would want the fields from each Beat to be stored independently such that each template (from asset.GetFields) contains only the fields for that Beat. But maybe that is moot if all beats in one process share a single output (not sure how that would work).

It's definitively a valid point but I would ignore it for now.

andrewkroh · 2017-10-09T14:15:05Z

libbeat/cmd/instance/beat.go

@@ -147,6 +148,7 @@ func NewBeat(name, indexPrefix, v string) (*Beat, error) {
 			Hostname:    hostname,
 			UUID:        uuid.NewV4(),
 		},
+		Fields: assets.GetFields(),


Maybe the beat name could be a param? (thinking of two beats in the same process)

Are you thinking it cause some issues with the collectbeat from @vjsamuel ? Did not think about that TBH.

andrewkroh · 2017-10-09T14:21:48Z

libbeat/template/load_integration_test.go

@@ -237,7 +237,7 @@ func TestOverwrite(t *testing.T) {
 		Enabled: true,
 		Fields:  absPath + "/fields.yml",
 	})
-	loader, err := NewLoader(config, client, beatInfo)
+	loader, err := NewLoader(config, client, beatInfo, []byte{})


Can you pass nil instead of allocating an empty slice?

andrewkroh · 2017-10-09T14:25:58Z

libbeat/template/template.go

+func load(cfg *ucfg.Config) (common.Fields, error) {
+	keys := []common.Field{}
+
+	cfg.Unpack(&keys)


Missing error check?

andrewkroh · 2017-10-09T14:26:35Z

libbeat/template/template.go

+}
+
+func load(cfg *ucfg.Config) (common.Fields, error) {
+	keys := []common.Field{}


If you use var keys common.Field will Unpack allocate this for you?

tested and seems to work.

andrewkroh · 2017-10-09T21:20:55Z

Just a few notes from discussion:

You could auto-detect if {path.home}/fields.yml exists and use it.

You might want to compress the fields.yml data internally and then extract it on first use.

houndci-bot · 2018-01-04T07:18:15Z

winlogbeat/include/fields.go

+	}
+}
+
+func Asset() ([]byte, error) {


exported function Asset should have comment or be unexported

houndci-bot · 2018-01-04T07:18:15Z

packetbeat/include/fields.go

+	}
+}
+
+func Asset() ([]byte, error) {


exported function Asset should have comment or be unexported

houndci-bot · 2018-01-04T07:18:15Z

libbeat/template/template.go

+	return output, nil
+}
+
+// Load the given input and generates the input based on it


comment on exported method Template.LoadBytes should be of the form "LoadBytes ..."

houndci-bot · 2018-01-04T07:18:15Z

libbeat/template/load.go

 }

-func NewLoader(cfg *common.Config, client ESClient, beatInfo beat.Info) (*Loader, error) {
+func NewLoader(cfg *common.Config, client ESClient, beatInfo beat.Info, fields []byte) (*Loader, error) {


exported function NewLoader should have comment or be unexported

houndci-bot · 2018-01-04T07:18:16Z

libbeat/assets/registry.go

+	return nil
+}
+
+func GetFields() []byte {


exported function GetFields should have comment or be unexported

houndci-bot · 2018-01-04T07:18:16Z

libbeat/assets/registry.go

+
+var FieldsRegistry [][]byte
+
+func AddFields(name string, asset func() ([]byte, error)) error {


exported function AddFields should have comment or be unexported

houndci-bot · 2018-01-04T07:18:16Z

libbeat/assets/registry.go

@@ -0,0 +1,23 @@
+package assets
+
+var FieldsRegistry [][]byte


exported var FieldsRegistry should have comment or be unexported

houndci-bot · 2018-01-04T07:18:16Z

heartbeat/include/fields.go

+	}
+}
+
+func Asset() ([]byte, error) {


exported function Asset should have comment or be unexported

houndci-bot · 2018-01-04T07:18:16Z

filebeat/include/fields.go

+	}
+}
+
+func Asset() ([]byte, error) {


exported function Asset should have comment or be unexported

houndci-bot · 2018-01-04T07:18:16Z

auditbeat/include/fields.go

+	}
+}
+
+func Asset() ([]byte, error) {


exported function Asset should have comment or be unexported

ruflin · 2018-04-25T16:08:41Z

This PR is ready for an other round of reviews.

ruflin · 2018-05-01T06:49:58Z

jenkins, test this

ruflin · 2018-05-08T08:52:29Z

One temporary issue of this change is that it will increase the PR conflicts every time fields are changed. But as soon as each module loads the fields locally, this should disappear again.

Currently the fields.yml file is shipped as its own file and has to be place in the right directory to be used. As fields have become a central part of beats I suggest to make them part of the binary which will allow us to also split up the fields.yml inside the binary into multiple part (for example one per module) and use it for testing like fields checks. The behavior from the user perspective is as following: * By default internal `fields.yml` data is used * If `fields.yml` path is set in the config file, it is loaded from disk The golang files are generated when running `make update`. The content of fields.yml is stored as a compressed string. Each asset is currently prefixed by the Beat name. The current fields registry is a basic implementation that likely needs to be extended when we add fields per module or processors.

exekias

LGTM, there are some valid comments by hound, apart from that: 👍

andrewkroh · 2018-05-15T13:04:55Z

dev-tools/cmd/asset/asset.go

+
+var tpl = template.Must(template.New("normalizations").Parse(`
+// ${ES_BEATS}/dev-tools/cmd/asset/asset.go
+// MACHINE GENERATED BY THE ABOVE COMMAND; DO NOT EDIT


This should be changed to Code generated by beats/dev-tools/cmd/asset/asset.go - DO NOT EDIT. Or something that matches this pattern golang/go#13560 (comment). Then tools like IntelliJ and golint will recognize it as being generated.

andrewkroh · 2018-05-15T13:11:54Z

generator/beat/{beat}/main.go.tmpl

@@ -4,6 +4,7 @@ import (
 	"os"

 	"{beat_path}/cmd"
+	_ "{beat_path}/include"


I think that unused imports should be separated into their own group and have a comment explaining it's existence. Similar to the golint warning, but golint exempts main packages.

ruflin · 2018-05-15T13:36:29Z

@andrewkroh Will do a follow up PR with your requested changes.

Follow up from review comments in elastic#4834

Follow up from review comments in #4834

Make fields.yml part of the binary Currently the fields.yml file is shipped as its own file and has to be place in the right directory to be used. As fields have become a central part of beats I suggest to make them part of the binary which will allow us to also split up the fields.yml inside the binary into multiple part (for example one per module) and use it for testing like fields checks.

Follow up from review comments in elastic#4834

Make fields.yml part of the binary Currently the fields.yml file is shipped as its own file and has to be place in the right directory to be used. As fields have become a central part of beats I suggest to make them part of the binary which will allow us to also split up the fields.yml inside the binary into multiple part (for example one per module) and use it for testing like fields checks.

Follow up from review comments in elastic#4834

ruflin added discuss Issue needs further discussion. in progress Pull request is currently in progress. labels Aug 7, 2017

ruflin mentioned this pull request Sep 1, 2017

Fix ceph cluster_status field mapping #5070

Merged

ruflin force-pushed the add-fields-to-package branch from 6d253db to c1608d7 Compare October 3, 2017 12:13

ruflin mentioned this pull request Oct 6, 2017

Improve handling of fields.yml and templates #5347

Closed

6 tasks

ruflin force-pushed the add-fields-to-package branch 2 times, most recently from cdb2f60 to 9d391cb Compare October 6, 2017 08:49

ruflin force-pushed the add-fields-to-package branch from c99b111 to 2c4978f Compare October 9, 2017 08:06

ruflin added libbeat review and removed discuss Issue needs further discussion. in progress Pull request is currently in progress. labels Oct 9, 2017

tsg reviewed Oct 9, 2017

View reviewed changes

ruflin force-pushed the add-fields-to-package branch from 2c4978f to bdc0d8b Compare October 9, 2017 11:56

andrewkroh reviewed Oct 9, 2017

View reviewed changes

andrewkroh approved these changes Oct 9, 2017

View reviewed changes

ruflin added in progress Pull request is currently in progress. and removed review labels Oct 9, 2017

ruflin force-pushed the add-fields-to-package branch from b75405c to e8de18d Compare October 9, 2017 21:41

ruflin force-pushed the add-fields-to-package branch from e8de18d to 11b63a2 Compare January 4, 2018 07:18

houndci-bot reviewed Jan 4, 2018

View reviewed changes

ruflin mentioned this pull request Apr 26, 2018

Update to Golang 1.10.1 #6948

Merged

ruflin force-pushed the add-fields-to-package branch 4 times, most recently from 4f7db2d to cd27521 Compare April 27, 2018 15:41

ruflin force-pushed the add-fields-to-package branch 5 times, most recently from f18e304 to 7c80fe0 Compare May 8, 2018 08:52

ruflin force-pushed the add-fields-to-package branch from 7c80fe0 to 0861acf Compare May 11, 2018 11:45

exekias approved these changes May 14, 2018

View reviewed changes

make houndci happy

e79eb6e

exekias merged commit 726fb00 into elastic:master May 15, 2018

andrewkroh reviewed May 15, 2018

View reviewed changes

ruflin deleted the add-fields-to-package branch May 15, 2018 13:36

ruflin mentioned this pull request May 17, 2018

Cleanup fields generated code #7125

Merged

ruflin added a commit to ruflin/beats that referenced this pull request May 18, 2018

Cleanup fields generated code

f486102

Follow up from review comments in elastic#4834

andrewkroh pushed a commit that referenced this pull request May 18, 2018

Cleanup fields generated code (#7125)

6930d27

Follow up from review comments in #4834

stevea78 pushed a commit to stevea78/beats that referenced this pull request May 20, 2018

Cleanup fields generated code (elastic#7125)

cdab3df

Follow up from review comments in elastic#4834

stevea78 pushed a commit to stevea78/beats that referenced this pull request May 20, 2018

Cleanup fields generated code (elastic#7125)

b739565

Follow up from review comments in elastic#4834

ruflin mentioned this pull request Oct 13, 2018

fields.yml is ignored but still shipped with packages #8607

Closed


		var FieldsRegistry [][]byte

		func AddFields(name string, asset func() ([]byte, error)) error {

Make fields.yml part of the binary #4834

Make fields.yml part of the binary #4834

Conversation

ruflin commented Aug 7, 2017 • edited Loading

tsg commented Aug 8, 2017

ruflin commented Aug 8, 2017

ruflin commented Oct 6, 2017

andrewkroh commented Oct 6, 2017

ruflin commented Oct 9, 2017

tsg commented Oct 9, 2017

tsg left a comment

Choose a reason for hiding this comment

ruflin commented Oct 9, 2017

ruflin commented Oct 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkroh commented Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin commented Apr 25, 2018

ruflin commented May 1, 2018

ruflin commented May 8, 2018

exekias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin commented May 15, 2018

ruflin commented Aug 7, 2017 •

edited

Loading

andrewkroh commented Oct 9, 2017 •

edited

Loading