Improve fields.yml generator of modules #7533

kvch · 2018-07-06T14:26:12Z

From now on when a user provides a type hint in an Ingest pipeline, it's added to the generated fields.yml instead of guessing.

Closes #7472

ruflin · 2018-07-09T11:48:06Z

filebeat/scripts/generator/fields/main.go

-			Type:     ee[0],
-			Elements: nil,
-		}
+	if 2 != len(ee) && len(ee) != 3 {


Could you add a comment here on why we check for these 2 values? Should make reading the code easier.

ruflin · 2018-07-09T11:49:43Z

filebeat/scripts/generator/fields/main.go

@@ -95,17 +96,18 @@ func newFieldYml(name, typeName string, noDoc bool) *fieldYml {
 func newField(lp string) field {
 	lp = lp[1 : len(lp)-1]


This code is fine as long as we use it just as generator on the dev side. But as recently we started to move some code also in the Beats binary we should probably be more careful with validation checks. What happens here in case an empty string is passed to it?

This function never gets an empty string as a param, because it parses the strings returned by FindAllString which does not match for empty lines. But just to be sure, if some tries to use it incorrectly in the future, I added additional error checking.

I think the key here is we can't make assumption about future usage of this method as you stated so checks should be in place to make sure the method never panics.

Seems the checks didn't make it in yet? If I run newField("") the code would panic?

kvch · 2018-07-13T11:56:08Z

I added additional comments and telling names to functions. Furthermore, I renamed the struct attributes to reflect the notions of the Grok processor.

ruflin · 2018-07-16T07:53:33Z

filebeat/scripts/generator/fields/main.go

@@ -95,20 +100,34 @@ func newFieldYml(name, typeName string, noDoc bool) *fieldYml {
 func newField(lp string) field {
 	lp = lp[1 : len(lp)-1]
 	ee := strings.Split(lp, ":")


What stands ee for?

Well, it's "elements". I don't really have a good name for elements of SYNTAX:SEMANTIC:TYPE split at :s. I can rename it to elements, but I don't think it would help. :(

I quite like elements, sounds better to me then ee

…es dict Closes elastic#7472

ruflin

The reason I initially struggle with this PR is the mix of the following names:

type vs syntax
hint vs type

Reading up on https://www.elastic.co/guide/en/elasticsearch/reference/current/grok-processor.html I understand now where the mix is coming from. For the readability of the code I think it's important we clean it up and not that at some point hint is assigned to a type variable or type to a syntax.

It seems based on the code we never treat it as a hint but for us it's directly the type. Do we need the reference to hint?

As this code will potentially move into the Filebeat binary, we should also make sure we have good error handling from the start to tell the user what went wrong instead of creating just an empty / no field for him.

This is a valuable change and we should mention it in the developer changelog.

Would be nice if this "feature" will also show up in our developer guide somewhere. Before the issue was opened I didn't even know grok patterns support hints.

ruflin · 2018-07-17T13:15:14Z

filebeat/scripts/generator/fields/main.go

-		}
+func newField(pattern string) field {
+	if len(pattern) <= 2 {
+		return field{}


Should we log errors instead of return empty fields? This would make debugging much easier in the future.

ruflin · 2018-07-17T13:15:21Z

filebeat/scripts/generator/fields/main.go

+
+	elements := strings.Split(pattern, ":")
+	if !isValidFormat(elements) {
+		return field{}


Return error?

ruflin · 2018-07-17T13:23:58Z

filebeat/scripts/generator/fields/main.go

+	if len(pattern) <= 2 {
+		return field{}
+	}
+	pattern = pattern[1 : len(pattern)-1]


Alternative here would strings.Trim("pattern", "{}")

ruflin · 2018-07-17T13:25:27Z

filebeat/scripts/generator/fields/main.go

+}
+
+// the last element is the type hint
+func containsType(ee []string) bool {


Should this be containsHint? I understand the the hint is for the type but this checks if it contains hint?

ruflin · 2018-07-17T13:26:15Z

filebeat/scripts/generator/fields/main.go

+		return field{}
+	}
+
+	hint := ""


What about having a function getType(elements) that does the below? Would also make it easier to test I think.

ruflin · 2018-07-17T13:27:16Z

filebeat/scripts/generator/fields/main.go

-		Elements: e,
+		Syntax:           elements[typeIdx],
+		SemanticElements: strings.Split(elements[elementsIdx], "."),
+		Type:             hint,


The variable should be called type probably, see the above.

ruflin · 2018-07-17T13:28:07Z

filebeat/scripts/generator/fields/main.go

@@ -33,6 +33,10 @@ import (
 const (
 	pipelinePath  = "%s/module/%s/%s/ingest/pipeline.json"
 	fieldsYmlPath = "%s/module/%s/%s/_meta/fields.yml"
+
+	typeIdx     = 0


Isn't that the syntax index?

kvch · 2018-07-18T08:59:44Z

I would rather not address all your review comments in this PR as I already have another one specifically for refactoring and making this script ready to be used as a subcommand. I intended this PR to be a quick fix orthogonal to refactoring, so I can rebase my other PR on top of it with minimal conflicts.

So what do you think if we merge this PR and I do the refactoring you requested in #7506?

ruflin · 2018-07-18T11:56:04Z

Merged as we discussed that refactoring of the comments will happen in #7506 or a separate PR.

* Fix breaking change in monitoring data (#7563) The prefix for the stats metrics was metrics but renamed to `stats` by accident as the name is now auto generated. This reverts this change. Closes #7562 * Add http.request.mehod to Kibana log filset (#7607) Take `http.request.method` from ECS and apply it to the Kibana fileset. Additional logs are added to the example log files. * Fix rename log message (#7614) Instead of the from field the to field was logged. * Add tests to verify template content (#7606) We recently started to move fields.yml into the Golang binary to be used internally. To make sure the loading important and loading of all the data into the binary works as expected for Metricbeat, this adds some basic tests. Related to #7605. * Basic support of ES GC metrics for jvm9 (#7628) GC log format for JVM9 is more detailed than for JVM8. Differences and possible improvements: * To get cpu_times.* a corellation between log lines is required. * Some GC metrics are available in jvm8 are not in jvm9 (class_unload_time_sec, weak_refs_processing_time_sec, ...) * heap.used_kb is empty, but it can be calculated as young_gen.used_kb + old_gen.size_kb * GC phase times are logged in miliseconds vs seconds in jvm8 * Improve fields.yml generator of modules (#7533) From now on when a user provides a type hint in an Ingest pipeline, it's added to the generated `fields.yml` instead of guessing. Closes #7472 * Fix filebeat registry meta being nil vs empty (#7632) Filebeat introduces a meta field to registry entries in 6.3.1. The meta field is used to distuingish different log streams in docker files. For other input types the meta field must be null. Unfortunately the input loader did initialize the meta field with an empty dictionary. This leads to failing matches of old and new registry entries. Due to the match failing, old entries will not be removed, and filebeat will handle all files as new files on startup (old logs are send again). Users will observe duplicate entries in the reigstry file. One entry with "meta": null and one entry with "meta": {}. The entry with "meta": {} will be used by filebeat. The null-entry will not be used by filebeat, but is kept in the registry file, cause it has now active owner (yet). Improvements provided by this PR: * when matching states consider an empty map and a null-map to be equivalent * update input loader to create a null map for old state -> registry entries will be compatible on upgrade * Add checks in critical places replacing an empty map with a null-map * Add support to fix registry entries on load. states from corrupted 6.3.1 files will be merged into one single state on load * introduce unit tests for loading different registry formats * introduce system tests validating output and registry when upgrading filebeat from an older version Closes: #7634 * Heartbeat Job Validation + addition of libbeat/mapval (#7587) This commit seeks to establish a pattern for testing heartbeat jobs. It currently tests the HTTP and TCP jobs. It also required some minor refactors of those tasks for HTTP/TCP. To do this, it made sense to validate event maps with a sort of schema library. I couldn't find one that did exactly what I wanted here, so I wrote one called mapval. That turned out to be a large undertaking, and is now the majority of this commit. Further tests need to be written, but this commit is large enough as is. One of the nicest things about the heartbeat architecture is the dialer chain behavior. It should be the case that any validated protocol using TCP (e.g. HTTP, TCP, Redis, etc.) has the exact same tcp metadata. To help make testing these properties easy mapval lets users compose portions of a schema into a bigger one. In other words, you can say "An HTTP response should be a TCP response, with the standard monitor data added in, and also the special HTTP fields". Even having only written a handful of tests this has uncovered some inconsistencies there, where TCP jobs have a hostname, but HTTP ones do not. * Only fetch shard metrics from master node (#7635) This PR makes it so that the `elasticsearch/shard` metricset only fetches information from the Elasticsearch node if that node is the master node. * Create (X-Pack Monitoring) stats metricset for Kibana module (#7525) This PR takes the `stats` metricset of the `kibana` Metricbeat module and makes it ship documents to `.monitoring-kibana-6-mb-%{YYYY.MM.DD}` indices, while preserving the current format/mapping expected by docs in these indices. This will ensure that current consumers of the data in these indices, viz. the X-Pack Monitoring UI and the Telemetry shipping module in Kibana, will continue to work as-is. * Add kubernetes specs for auditbeat file integrity monitoring (#7642) * Release the rename processor as GA * Fix log message for Kibana beta state (#7631) From copy paste Kafka was in the log message instead of Kibana. * Clean up experimental and beta messages (#7659) Sometimes the old logging mechanism was used. If all use the new one it is easier to find all the entries. In addition some messages were inconsistent. * Release raid and socket metricset from system module as GA (#7658) * Release raid and socket metricset from system module as GA * remove raid metricset title * Update geoip config docs (#7640) * Document breaking change in monitoring shcema Situation: * Edit breaking changes statement about monitoring schema changes (#7666) * Marking Elasticsearch module and its metricsets as beta (#7662) This PR marks the `elasticsearch` module and all its 8 existing metricsets all as `beta`. Previously only 2 metricsets were marked as `beta` with the remaining 6 marked as `experimental`. * Increase kafka version in tests to 1.1.1 (#7655) * Add missing mongodb status fields (#7613) Add `locks`, `global_locks`, `oplatencies` and `process` fields to `status` metricset of MongoDB module. * Remove outdated vendor information. (#7676) * Fix Filebeat tests with new region_iso_code field (#7678) In elastic/elasticsearch#31669 the field `region_iso_code` was added to the geoip processor. Because of this test broke with the most recent release of Elasticsearch as the events contain an undocumented field. * Fix duplicated module headers (#7650) * Fix duplicated module headers Closes #7643 * fix metricset titles for munin and kvm * fix imssing kubernetes apiserver metricset doc * remove headers from modules / metricset generator and clean up traefik title * Release munin and traefik module as beta. (#7660) * Release munin and treafik module as beta. * fixes to munin module * Report k8s pct metrics from enrichment process (#7677) Instead of doing it from the `state_container`. Problem with the previous approach is that `state_container` metricset is not run in all nodes, but from a single point. Making performance metrics not available in all cases. With this new approach, the enriching process will also collect performance metrics, so they should be available everywhere where the module is run. * Fix misspell in Beats repo (#7679) Running `make misspell`. * Update sarama (kafka client) to 1.17 (#7665) - Update Sarama to 1.17. The Sarama testsuite tests kafka versions between 0.11 and 1.1.0. - Update compatible versions in output docs - Add compression_level setting for gzip compression * Update github.com/OneOfOne/xxhash to fix mips * Update boltdb to use github.com/coreos/bbolt fork Closes #6052 * Generate fields.yml using Mage (#7670) Make will now delegate to mage for generating fields.yml. Make will check if the mage command exists and go install it if not. The FIELDS_FILE_PATH make variable is not longer used because the path(s) are specified in magefile.go. This allows fields.yml to be generated on Windows using Mage. The CI scripts for Windows have been updated so that fields.yml is generated for all Beats during testing. This also adds a make.bat in each directory where building occurs to give Windows users a starting point. Some fixes were made to the generators because: - rsync was excluding important source files contained in a directory named "build" - the generated project needed to be `git init` before running certain magefile targets that detect project's root dir and import path. * Update go-ucfg to 0.6.1 (#7599) Update fixes config unpacking if users overwrite settings from CLI, with missing values. When using `-E key=` (e.g. in scripts defining potential empty defaults via env variables like `-E key=${MYVALUE}`), an untyped `nil`-values was inserted into the config. This untyped value will make Unpack fail for most typed settings. * Docs: Add deprecation check for dashboard loading. (#7675) For APM Server the recommended way of loading dashboards and Kibana index pattern will be through the Kibana UI from 6.4 on. Since the docs are based on the libbeat docs we need to add a deprecation flag for dashboard and index pattern related documentation. relates to elastic/apm-server#1142 * Update expected filebeat module files for geoip change

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

kvch requested a review from ruflin July 6, 2018 14:26

ruflin reviewed Jul 9, 2018

View reviewed changes

kvch force-pushed the feature/filebeat/improve-module-fields-yml-generator branch from e2abf32 to c7d7268 Compare July 13, 2018 11:47

ruflin reviewed Jul 16, 2018

View reviewed changes

kvch added 6 commits July 17, 2018 10:27

add hint to fields.yml when present instead of guessing using the typ…

29b013d

…es dict Closes elastic#7472

fix condition && remove unncessary print

e97c177

refactor and add comments

19660e3

add unit test

afb2d5f

refactor struct && add error checking

2bd678e

more renaming

07b59df

kvch force-pushed the feature/filebeat/improve-module-fields-yml-generator branch from e6beb85 to 07b59df Compare July 17, 2018 08:27

ruflin requested changes Jul 17, 2018

View reviewed changes

ruflin mentioned this pull request Jul 18, 2018

Move existing Filebeat generator logic to package generator #7506

Closed

ruflin merged commit 5eaafff into elastic:master Jul 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve fields.yml generator of modules #7533

Improve fields.yml generator of modules #7533

kvch commented Jul 6, 2018

ruflin Jul 9, 2018

kvch Jul 13, 2018

ruflin Jul 9, 2018

kvch Jul 13, 2018

ruflin Jul 16, 2018

kvch commented Jul 13, 2018

ruflin Jul 16, 2018

kvch Jul 16, 2018

ruflin Jul 17, 2018

ruflin left a comment

ruflin Jul 17, 2018

ruflin Jul 17, 2018

ruflin Jul 17, 2018

ruflin Jul 17, 2018

ruflin Jul 17, 2018

ruflin Jul 17, 2018

ruflin Jul 17, 2018

kvch commented Jul 18, 2018

ruflin commented Jul 18, 2018

		@@ -95,17 +96,18 @@ func newFieldYml(name, typeName string, noDoc bool) *fieldYml {
		func newField(lp string) field {
		lp = lp[1 : len(lp)-1]

Improve fields.yml generator of modules #7533

Improve fields.yml generator of modules #7533

Conversation

kvch commented Jul 6, 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

kvch commented Jul 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

kvch commented Jul 18, 2018

ruflin commented Jul 18, 2018