Simplify fields.yml generator #7713

kvch · 2018-07-24T19:26:27Z

This PR simplifies the process of generating a new fields.yml file for a Beat.

libbeat/fields.yml is not a dependency anymore
only fields.yml is written to disk

Changes in the asset files are due to different number of newlines in generated fields.yml files.

Interface changes of generator tools

Both dev-tools/cmd/asset/asset.go and libbeat/scripts/cmd/global_fields/main.go is changed. The first one has two new flags: -in, and -out. By default it reads from stdin and writes to stdout. Using the flags it's possible to read and write from other files. The second one has one new flag -out which specifies the output file. If it's not set, the output is written to stdout.

This makes it possible to generate asset files for Metricbeat without writing libbeat/fields.yml to disk. It will be useful in the future when more Beats have modular assets.

Closes #7671

andrewkroh

Thanks for the quick turn-around.

andrewkroh · 2018-07-24T20:00:33Z

libbeat/Makefile

@@ -6,4 +6,4 @@ include scripts/Makefile

 # Collects all dependencies and then calls update
 .PHONY: collect
-collect: libbeat_fields
+collect: fields


Is there any reason not to give update a direct dependency on fields?

andrewkroh · 2018-07-24T20:01:50Z

libbeat/scripts/Makefile

@@ -309,13 +309,9 @@ coverage-report:
 fields: mage
 	mage -v fields


Can you set this line to @mage fields. I was debugging when changed it to its current state and didn't undo my changes.

andrewkroh · 2018-07-24T20:22:12Z

I notice a missing newline from one of the commands. Maybe you can track it down.

ERROR: Field <beat.name> is duplicated. Please update and try again.exit status 1
                                                                   ^

andrewkroh · 2018-07-24T20:57:32Z

libbeat/scripts/cmd/global_fields/main.go

-	}
-
-	if len(beatFieldsPaths) == 0 {
+	if len(beatFieldsPaths) == 0 && *esBeatsPath == *beatPath {


What condition is *esBeatsPath == *beatPath checking for?

Is this an assertion that the command is not being run from the elastic/beats dir? If so, comparing the two path strings without normalization isn't advisable; at a minimum I would filepath.Clean them. Using os.SameFile would be the most reliable why to check that condition, but why event check it at all.

andrewkroh · 2018-07-24T21:01:37Z

libbeat/scripts/cmd/global_fields/main.go

 	}

-	err = fields.Generate(*esBeatsPath, *beatPath, fieldsFiles)
+	err := fields.Generate(*esBeatsPath, *beatPath, fieldsFiles)


How about internalizing the collection of module files to the fields.Generate(). Make it accept a list of module directories like Generate(elasticBeatsDir, beatDir string, moduleDirs ...string) ([]byte, error).

Note that I added a []byte return which is the actual content. Then the caller becomes responsible for writing the content which makes it a bit more reusable. I think the YAML content is small enough to be rendered to memory without any issues.

andrewkroh · 2018-07-24T21:04:18Z

libbeat/generator/fields/fields.go

+		commonFields = append(
+			[]string{
+				filepath.Join(esBeatsPath, "libbeat", "_meta", "fields.common.yml"),
+				filepath.Join(beatPath, "_meta", "fields.yml"),


I think _meta/fields.yml is not used anymore.

I found a place that has some old references to fields files that needs updated to point to plain old fields.yml.

beats/libbeat/tests/system/beat/beat.py

Lines 462 to 496 in 5443c8a

if fields_doc is None:

fields_doc = self.beat_path + "/_meta/fields.generated.yml"

def extract_fields(doc_list, name):

fields = []

dictfields = []

if doc_list is None:

return fields, dictfields

for field in doc_list:

# Skip fields without name entry

if "name" not in field:

continue

# Chain together names

if name != "":

newName = name + "." + field["name"]

else:

newName = field["name"]

if field.get("type") == "group":

subfields, subdictfields = extract_fields(field["fields"], newName)

fields.extend(subfields)

dictfields.extend(subdictfields)

else:

fields.append(newName)

if field.get("type") in ["object", "geo_point"]:

dictfields.append(newName)

return fields, dictfields

# Not all beats have a fields.generated.yml. Fall back to fields.yml

if not os.path.isfile(fields_doc):

fields_doc = self.beat_path + "/_meta/fields.yml"

andrewkroh · 2018-07-24T21:36:43Z

These lines should be removed now:

beats/dev-tools/jenkins_ci.ps1

Lines 27 to 30 in 86dc973

    
           echo "Building libbeat fields.yml" 
        
           cd libbeat 
        
           exec { mage fields } 
        
           cd ..

andrewkroh · 2018-07-24T21:51:12Z

metricbeat/Makefile

@@ -73,7 +73,7 @@ test-module: python-env update metricbeat.test
 	. ${PYTHON_ENV}/bin/activate && INTEGRATION_TESTS=1 nosetests tests/system/test_${MODULE}.py

 .PHONY: assets
-assets: libbeat_fields


Unfortunately I don't think Metricbeat is free from this dependency yet due to the way that the fields.go files are composed.

One solution would be to include both metricbeat common fields and libbeat fields in metricbeat/include/fields.go. This could be generated by running global_fields without passing in the module dir as an argument.

It's be nice if we could pipe the output of one command into another like:

go run ${ES_BEATS}/libbeat/scripts/cmd/global_fields/main.go -es_beats_path ${ES_BEATS} -beat_path ${BEAT_NAME} | go run ${ES_BEATS}/dev-tools/cmd/asset/asset.go -pkg include -in - -out include/fields.go

Let's hope this becomes obsolete again in the near future :-)

ruflin · 2018-07-25T05:46:32Z

@elastic/apm-server FYI, this change is happening in master.

andrewkroh · 2018-07-25T15:04:29Z

libbeat/scripts/cmd/global_fields/main.go

-			os.Exit(2)
+	if len(beatFieldsPaths) == 0 && os.SameFile(esBeatsInfo, beatInfo) {
+		if !*toStdout {
+			fmt.Println("No field files to collect")


You might consider using the stdlib log package in the file and the util packages. Just initialize it with log.SetFlags() to remove the timestamps. All messages will go to stderr all the time.

And all the places where it logs and exits can use a log.Fatal*.

I would rather keep it as it is right now, because this is the "idiomatic" Beats way when printing to console during scripts or commands. I see that you started to use log in mage files, but apart from that fmt package is used all over the codebase.

@andrewkroh Wasn't aware of this feature. We definitively should start using this in the future.

andrewkroh · 2018-07-25T15:05:17Z

libbeat/scripts/cmd/global_fields/main.go

+	}
+	esBeatsInfo, err := esBeats.Stat()
+	if err != nil {
+		fmt.Fprintf(os.Stderr, "Error getting file info of elastic/beat: %+v\n", err)


elastic/beats (It's missing the s.)

andrewkroh · 2018-07-25T15:15:55Z

libbeat/scripts/cmd/global_fields/main.go

@@ -29,6 +29,7 @@ import (
 func main() {
 	esBeatsPath := flag.String("es_beats_path", "..", "Path to elastic/beats")
 	beatPath := flag.String("beat_path", ".", "Path to your Beat")
+	toStdout := flag.Bool("stdout", false, "Write output to stdout")


Personally I'd prefer to standardize on using -out to specify the output file in our CLI tools. And make the default be - mean output to os.Stdout like most unix tools. Then you get the same behavior across all tools. The downside being that you have to write -out fields.yml in the common case, but the common case is almost always done via make so it's not actually a problem.

andrewkroh · 2018-07-25T15:21:06Z

libbeat/generator/fields/fields.go

+	var err error
+	if !isLibbeat(beatPath) {
+		commonFields = append(commonFields,
+			filepath.Join(esBeatsPath, "libbeat", "_meta", "fields.common.yml"),


BTW filepath.Join always calls filepath.Clean on the result. https://golang.org/pkg/path/filepath/#Join

This means you can write filepath.Join(esBeatsPath, "libbeat/_meta/fields.common.yml") which is more clear IMO.

andrewkroh

LGTM. Can you rebase this on master to get my changes to fix go test on Windows.

andrewkroh · 2018-07-25T20:55:45Z

libbeat/scripts/cmd/global_fields/main.go

-			fmt.Fprintf(os.Stderr, "Cannot generate global fields.yml for %s: %+v\n", name, err)
-			os.Exit(2)
+	if len(beatFieldsPaths) == 0 && os.SameFile(esBeatsInfo, beatInfo) {
+		if *output != "-" {


Should this be *input?

andrewkroh · 2018-07-25T20:59:00Z

dev-tools/cmd/asset/asset.go


 func init() {
 	pkg = flag.String("pkg", "", "Package name")
+	input = flag.String("in", "-", "Source of input. \"-\" means reading from stdin")


It would be nice to switch this file over to flag.StringVar so we don't have to dereference string pointers.

@kvch Want to do this in this PR or follow up?

Done. I forgot to push this commit.

* `libbeat/fields.yml` is not a dependency anymore * intermediary files are not written Closes elastic#7671

kvch · 2018-07-26T07:30:21Z

Rebased and refactored.

ruflin

Change LGTM, only a few minor questions.

ruflin · 2018-07-26T08:32:47Z

libbeat/generator/fields/module_fields_collector.go

@@ -71,7 +71,7 @@ func CollectModuleFiles(modulesDir string) ([]*YmlFile, error) {
 func CollectFiles(module string, modulesPath string) ([]*YmlFile, error) {

 	var files []*YmlFile
-	fieldsYmlPath := filepath.Join(modulesPath, module, "_meta", "fields.yml")
+	fieldsYmlPath := filepath.Join(modulesPath, module, "_meta/fields.yml")


What is the reason you introduced the / here? Could break on Windows. Any issues with the previous option?

Andrew pointed out that it is working everywhere in #7713 (comment)

ruflin · 2018-07-26T08:32:59Z

libbeat/generator/fields/module_fields_collector.go

@@ -91,7 +91,7 @@ func CollectFiles(module string, modulesPath string) ([]*YmlFile, error) {
 		if !s.IsDir() {
 			continue
 		}
-		fieldsYmlPath = filepath.Join(modulesPath, module, s.Name(), "_meta", "fields.yml")
+		fieldsYmlPath = filepath.Join(modulesPath, module, s.Name(), "_meta/fields.yml")


Same question as above.

ruflin · 2018-07-26T08:35:49Z

libbeat/scripts/cmd/global_fields/main.go

-		if err != nil {
-			fmt.Fprintf(os.Stderr, "Cannot generate global fields.yml for %s: %+v\n", name, err)
-			os.Exit(2)
+	if len(beatFieldsPaths) == 0 && os.SameFile(esBeatsInfo, beatInfo) {


Trying to follow what this does. Could you quickly elaborate?

If a community Beat does not have its ownfields.yml file, it still requires the fields coming from libbeat to generate e.g assets. In case of Elastic Beats, it's not a problem because all of them has unique fields.yml files somewhere.

Thanks for the details. Could you perhaps add a comment to the line to make sure if we refactor this in the future we don't get rid of it?

ruflin · 2018-07-26T08:37:43Z

metricbeat/Makefile

@@ -73,7 +73,7 @@ test-module: python-env update metricbeat.test
 	. ${PYTHON_ENV}/bin/activate && INTEGRATION_TESTS=1 nosetests tests/system/test_${MODULE}.py

 .PHONY: assets
-assets: libbeat_fields


Let's hope this becomes obsolete again in the near future :-)

ruflin · 2018-07-26T08:38:16Z

packetbeat/_meta/fields.yml

@@ -1,2083 +0,0 @@
- key: common


Seems like this was a left over file that should already have been deleted before?

Yes. It is a duplication of packetbeat/_meta/fields.common.yml with fields which coming from protos. Nothing used this file at all.

ruflin · 2018-07-26T14:16:22Z

@kvch Thanks, could you follow up with the @elastic/apm-server team?

jalvz · 2018-07-26T14:23:08Z

already aware, thnks ;)

This PR simplifies the process of generating a new `fields.yml` file for a Beat. * `libbeat/fields.yml` is not a dependency anymore * only `fields.yml` is written to disk Changes in the asset files are due to different number of newlines in generated `fields.yml` files. ### Interface changes of generator tools Both `dev-tools/cmd/asset/asset.go` and `libbeat/scripts/cmd/global_fields/main.go` is changed. The first one has two new flags: `-in`, and `-out`. By default it reads from stdin and writes to stdout. Using the flags it's possible to read and write from other files. The second one has one new flag `-out` which specifies the output file. If it's not set, the output is written to stdout. This makes it possible to generate asset files for Metricbeat without writing `libbeat/fields.yml` to disk. It will be useful in the future when more Beats have modular assets. Closes elastic#7671 (cherry picked from commit 5797005)

This PR simplifies the process of generating a new `fields.yml` file for a Beat. * `libbeat/fields.yml` is not a dependency anymore * only `fields.yml` is written to disk Changes in the asset files are due to different number of newlines in generated `fields.yml` files. ### Interface changes of generator tools Both `dev-tools/cmd/asset/asset.go` and `libbeat/scripts/cmd/global_fields/main.go` is changed. The first one has two new flags: `-in`, and `-out`. By default it reads from stdin and writes to stdout. Using the flags it's possible to read and write from other files. The second one has one new flag `-out` which specifies the output file. If it's not set, the output is written to stdout. This makes it possible to generate asset files for Metricbeat without writing `libbeat/fields.yml` to disk. It will be useful in the future when more Beats have modular assets. Closes #7671 (cherry picked from commit 5797005)

This PR simplifies the process of generating a new `fields.yml` file for a Beat. * `libbeat/fields.yml` is not a dependency anymore * only `fields.yml` is written to disk Changes in the asset files are due to different number of newlines in generated `fields.yml` files. ### Interface changes of generator tools Both `dev-tools/cmd/asset/asset.go` and `libbeat/scripts/cmd/global_fields/main.go` is changed. The first one has two new flags: `-in`, and `-out`. By default it reads from stdin and writes to stdout. Using the flags it's possible to read and write from other files. The second one has one new flag `-out` which specifies the output file. If it's not set, the output is written to stdout. This makes it possible to generate asset files for Metricbeat without writing `libbeat/fields.yml` to disk. It will be useful in the future when more Beats have modular assets. Closes elastic#7671 (cherry picked from commit 5797005)

* Simplify fields.yml generator (#7713) This PR simplifies the process of generating a new `fields.yml` file for a Beat. * `libbeat/fields.yml` is not a dependency anymore * only `fields.yml` is written to disk Changes in the asset files are due to different number of newlines in generated `fields.yml` files. ### Interface changes of generator tools Both `dev-tools/cmd/asset/asset.go` and `libbeat/scripts/cmd/global_fields/main.go` is changed. The first one has two new flags: `-in`, and `-out`. By default it reads from stdin and writes to stdout. Using the flags it's possible to read and write from other files. The second one has one new flag `-out` which specifies the output file. If it's not set, the output is written to stdout. This makes it possible to generate asset files for Metricbeat without writing `libbeat/fields.yml` to disk. It will be useful in the future when more Beats have modular assets. Closes #7671 (cherry picked from commit 5797005) * update fields of Packetbeat

…ic#7777) * Simplify fields.yml generator (elastic#7713) This PR simplifies the process of generating a new `fields.yml` file for a Beat. * `libbeat/fields.yml` is not a dependency anymore * only `fields.yml` is written to disk Changes in the asset files are due to different number of newlines in generated `fields.yml` files. ### Interface changes of generator tools Both `dev-tools/cmd/asset/asset.go` and `libbeat/scripts/cmd/global_fields/main.go` is changed. The first one has two new flags: `-in`, and `-out`. By default it reads from stdin and writes to stdout. Using the flags it's possible to read and write from other files. The second one has one new flag `-out` which specifies the output file. If it's not set, the output is written to stdout. This makes it possible to generate asset files for Metricbeat without writing `libbeat/fields.yml` to disk. It will be useful in the future when more Beats have modular assets. Closes elastic#7671 (cherry picked from commit ec2e022) * update fields of Packetbeat

kvch added review libbeat :Generator Related to code generators for building custom Beats or modules. labels Jul 24, 2018

kvch requested a review from ruflin July 24, 2018 19:26

andrewkroh reviewed Jul 24, 2018

View reviewed changes

kvch added the needs_backport PR is waiting to be backported to other branches. label Jul 25, 2018

andrewkroh reviewed Jul 25, 2018

View reviewed changes

andrewkroh approved these changes Jul 25, 2018

View reviewed changes

kvch added 17 commits July 26, 2018 09:28

simplify global fields.yml generator script

db65390

* `libbeat/fields.yml` is not a dependency anymore * intermediary files are not written Closes elastic#7671

fix format of error messages of asset generator

f390bf8

changed asset file

5c25ec8

fix makefile of libbeat

f370eb3

fix libbeat generator

f01ec89

remove unnecessary meta/fields.yml file of packetbeat

3f50160

mage cleanup

1709b34

add missing newline

4771483

use os.SameFile instead of path cmp

1e367ab

do not try to open fields.generated.yml during testing

aeb6394

rename winlogbeat/_meta/fields.yml to winlogbeat/_meta/fields.common.yml

04758ee

remove _meta/fields.yml from the list && change order

41bd36b

follow up asset changes

6b4fc8e

fields asset generator reads from stdin if -stdin is set

837530e

global fields.yml generator writes to stdout if -stdout is passed

1d5822e

add missing dir

e680f94

follow up changes in makefile

17d1c1d

kvch added 7 commits July 26, 2018 09:28

add missing dir

0a2e813

make fields target direct dependency of update

b296932

do not pollute output

22bde9b

pass less params to filepath.Join

76b0547

fix tyop

d3fca54

-out as output and -in as input

c3c6aba

use stringvar

6ff6069

kvch force-pushed the refactor/libbeat/one-fields-yml branch from 9cac467 to 6ff6069 Compare July 26, 2018 07:29

ruflin reviewed Jul 26, 2018

View reviewed changes

kvch added 2 commits July 26, 2018 11:04

add missing StringVar to asset

2d64d75

add comment

6349712

ruflin merged commit 5797005 into elastic:master Jul 26, 2018

kvch mentioned this pull request Jul 27, 2018

Cherry-pick #7713 to 6.4: Simplify fields.yml generator #7777

Merged

kvch added v6.4.0 and removed needs_backport PR is waiting to be backported to other branches. labels Jul 27, 2018

kvch mentioned this pull request Jul 27, 2018

Cherry-pick #7713 to 6.x: Simplify fields.yml generator #7778

Merged

kvch added the v6.5.0 label Jul 27, 2018

		@@ -309,13 +309,9 @@ coverage-report:
		fields: mage
		mage -v fields

	if fields_doc is None:
	fields_doc = self.beat_path + "/_meta/fields.generated.yml"

	def extract_fields(doc_list, name):
	fields = []
	dictfields = []

	if doc_list is None:
	return fields, dictfields

	for field in doc_list:

	# Skip fields without name entry
	if "name" not in field:
	continue

	# Chain together names
	if name != "":
	newName = name + "." + field["name"]
	else:
	newName = field["name"]

	if field.get("type") == "group":
	subfields, subdictfields = extract_fields(field["fields"], newName)
	fields.extend(subfields)
	dictfields.extend(subdictfields)
	else:
	fields.append(newName)
	if field.get("type") in ["object", "geo_point"]:
	dictfields.append(newName)
	return fields, dictfields

	# Not all beats have a fields.generated.yml. Fall back to fields.yml
	if not os.path.isfile(fields_doc):
	fields_doc = self.beat_path + "/_meta/fields.yml"

Simplify fields.yml generator #7713

Simplify fields.yml generator #7713

Conversation

kvch commented Jul 24, 2018 • edited Loading

Interface changes of generator tools

andrewkroh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkroh commented Jul 24, 2018

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 Jul 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin commented Jul 25, 2018

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkroh Jul 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvch commented Jul 26, 2018

ruflin left a 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

ruflin commented Jul 26, 2018

jalvz commented Jul 26, 2018

kvch commented Jul 24, 2018 •

edited

Loading

andrewkroh Jul 25, 2018 •

edited

Loading