Refactor: Renaming prospector to input #6078

ph · 2018-01-15T18:34:31Z

This is the first PR to refactor the usage of the prospector type to the input type, the code uses type aliasing to allow the input and the prospector to continue to work in parallel, providing an easier update path to the community beats. We have build pragma option to make sure we cannot compile this project if you are using an older version of go.

The prospector code didn't change in this PR, they were just moved into the input directory and still use the old prospector type.

Changes that will come after:

Refactor each prospector to use the new Input type.
Rename prospector reference in the yaml to input.
Update the test to use prospector instead of input.
Update log statement to use the new wording.

Note: for clarity I didn't merge the commits in this PR before the review.

This is a followup of #5944

ph · 2018-01-15T18:36:22Z

More stuff need to be extracted from #5944, but lets get this reviewed merged. I think at 500 lines is much easier.

@ruflin Also for the input, I will migrate them one by one to reduce conflicts :)

ph · 2018-01-15T21:51:55Z

Missed the docker compose commit. . .

ruflin

Change looks good to me. The only thing missing from my perspective is some system tests that validate that the old options are still working as expected. As this is a pretty major change without expecting to break BC we should make sure the old options still work as intended and deprecated messages are logged as expected to make sure users knows about it.

ruflin · 2018-01-16T05:48:08Z

filebeat/beater/filebeat.go

@@ -54,6 +54,14 @@ func New(b *beat.Beat, rawConfig *common.Config) (beat.Beater, error) {
 		return nil, err
 	}

+	if len(config.Prospectors) > 0 {


Can you add some system tests with the old config options to make sure this works as expected? We have done this for previous renamings and I think it's pretty useful to make sure it actually works and be remembered that we break something, when we remove it.

Of course, I was planning to do it in a dedicated PR, because right now all the tests use the old config options :)

SGTM. Didn't realise it's all on the old config. Good to know.

ruflin · 2018-01-16T05:49:40Z

filebeat/channel/factory.go

 	// event processing
 	common.EventMetadata `config:",inline"`      // Fields and tags to add to events.
 	Processors           processors.PluginConfig `config:"processors"`

 	// implicit event fields
-	Type string `config:"type"` // prospector.type
+	Type string `config:"type"` // input.type


In case the comment is correct, this is a breaking change of the data format I think.

Why its a breaking changes? The name of the private struct changed and the only the comment changed?

Only indirectly breaking if the comment is correct. I assume when looking at it that you removed prospector.type from the event, but you copied it to input.type.

I did not remove the prospector.type in the event. https://github.com/elastic/beats/pull/6078/files#diff-b6244e3a701e1fad9969756499d1e8fcR99

ruflin · 2018-01-16T05:50:23Z

filebeat/channel/factory.go

@@ -99,6 +99,9 @@ func (f *OutletFactory) Create(cfg *common.Config, dynFields *common.MapStrPoint
 		fields["prospector"] = common.MapStr{
 			"type": config.Type,
 		}
+		fields["input"] = common.MapStr{


Could we call this event.type instead if we already duplicate the field? Thinking of ECS here.

I can change it to event.type, I would prefer to not mix concerns of ECS, but since we already add this fields it should be OK.

ruflin · 2018-01-16T05:51:13Z

filebeat/config/config.go

@@ -82,7 +83,15 @@ func mergeConfigFiles(configFiles []string, config *Config) error {
 			return fmt.Errorf("Failed to read %s: %s", file, err)
 		}

-		config.Prospectors = append(config.Prospectors, tmpConfig.Filebeat.Prospectors...)
+		if len(tmpConfig.Filebeat.Prospectors) > 0 {


Some system tests for this would also be nice.

ruflin · 2018-01-16T05:53:00Z

filebeat/config/config.go

+			if len(tmpConfig.Filebeat.Inputs) > 0 {
+				return fmt.Errorf("prospectors and inputs used in the configuration file, define only inputs not both")
+			}
+			config.Inputs = append(config.Inputs, tmpConfig.Filebeat.Prospectors...)


Nit but code wise I would prefer if you would do here what you did above which is copying tmpConfig.Filebeat.Prospectors to tmpConfig.Filebeat.Inputs and then not do an else. Like this when the code is removed later, nothing changes in the "correct" code.

I am not sure, my argument was to prevent people defining both prospector and inputs, but I understand your point I will just remove that error and just copy it. Once we hit 7.0 we can "die hard"

I have the same logic there https://github.com/elastic/beats/pull/6078/files/c2b149f7d67b082b20f8201db719ba1c01755683#diff-7a00b9fb331934c8b3bb7cf5eac57576R37, So I will remove both.

nevermind, my previous comments, I understand your proposal.

ruflin · 2018-01-16T05:55:11Z

filebeat/docker-compose.yml

@@ -28,13 +28,13 @@ services:

  elasticsearch:
    extends:
-      file: ${ES_BEATS}/testing/environments/${TESTING_ENVIRONMENT}.yml
+      file: ../testing/environments/${TESTING_ENVIRONMENT}.yml


What is the reason you changed this back?

I shouldn't have changed this.

bad rebase on my side.

ruflin · 2018-01-16T05:55:52Z

filebeat/fileset/config.go

+		return nil, fmt.Errorf("error unpacking configuration")
+	}
+
+	if len(fcfg.Prospector) > 0 {


I have added unit test for this path, which are a lot simplier than integration test.

There is also test there https://github.com/elastic/beats/pull/6078/files/c2b149f7d67b082b20f8201db719ba1c01755683#diff-68d15c248f81d1c5e67c59c6dab45574R59

The part which I think unit tests here can't cover is to check if it's logged correctly. +1 on the unit test.

ruflin · 2018-01-16T05:58:14Z

filebeat/input/input.go

@@ -0,0 +1,150 @@
+package input


I assume that code is mostly copy/paste/rename

Exact, this should be as is.

ruflin · 2018-01-16T05:59:52Z

filebeat/prospector/prospector.go

-		Once:     false,
-		beatDone: beatDone,
-	}
+//+build go1.9


Do you really want to add this build flag as we already require Golang 1.9 for beats. Not having it would make us detect if by accident something is built previous to 1.9 and have an error.

We do require golang 1.9, but after looking at it I am not sure we enforce it the correct way.

 go version go version go1.8.3 darwin/amd64

This is with the pragma build go1.9

 make go build -i -ldflags "-X github.com/elastic/beats/libbeat/version.buildTime=2018-01-16T14:18:24Z -X github.com/elastic/beats/libbeat/version.commit=c2b149f7d67b082b20f8201db719ba1c01755683" fileset/factory.go:7:2: no buildable Go source files in /Users/ph/go/src/github.com/elastic/beats/filebeat/prospector make: *** [filebeat] Error 1

This is without the pragma build go1.9

go build -i -ldflags "-X github.com/elastic/beats/libbeat/version.buildTime=2018-01-16T14:18:42Z -X github.com/elastic/beats/libbeat/version.commit=c2b149f7d67b082b20f8201db719ba1c01755683" # github.com/elastic/beats/libbeat/logp ../libbeat/logp/logger.go:8: syntax error: unexpected = in type declaration make: *** [filebeat] Error 2

ph · 2018-01-16T17:08:40Z

@ruflin I have updated and fixed a few of your concerns.

Also I didn't update the json expected file, they will come in specific PR for each of the prospector. (re: this concern the event.type)

ph · 2018-01-17T22:12:09Z

@ruflin minus the rebase, anything else you need on this PR?

ruflin

LGTM. I added the label in progress to make sure it does not get merged before we do the next bulk backport to 6.x.

Can you add a CHANGELOG entry and squash the commits?

ruflin · 2018-01-18T22:56:48Z

filebeat/fileset/config.go

+		return nil, fmt.Errorf("error unpacking configuration")
+	}
+
+	if len(fcfg.Prospector) > 0 {


The part which I think unit tests here can't cover is to check if it's logged correctly. +1 on the unit test.

ruflin · 2018-01-18T22:58:19Z

filebeat/prospector/prospector.go

-	}
+// GetFactory wrapper for backward compatibility
+// Deprecated: See input.GetFactory
+var GetFactory = input.GetFactory


Good timing that Golang 1.9 added this :-)

ruflin · 2018-01-18T22:58:52Z

filebeat/tests/system/config/filebeat.yml.j2

@@ -213,7 +213,3 @@ path:
  data: {{path_data}}
 {% endif %}

-{% if keystore_path %}


Was this a duplicate?

ph · 2018-01-19T15:37:37Z

@ruflin Rebased and changelog added.

ruflin · 2018-01-21T21:27:04Z

@ph There seem to be some issues on CI (I looked up the commit id).

This refactor rename the prospector type to the input type, this allow this project to be more aligned with Logstash naming convention and also remove some of the last naming legacy of the `logstash fowarder`. The input name is also more appropriate for the UDP and the Redis code. The prospectors are now moved to the `input` folder. Backward compatibility is keep by using type aliasing over the older `prospector` types. Logs statements were also changed to reflect this refactor. Currently all the code and YAML are still using the *prospector(s)* keys, but other PRs will move the usage to the inputs. If the `input(s)` are present we will use them instead of the *prospectors* key.

ph · 2018-01-22T18:36:48Z

@ruflin I have fixed the keystore issue, added the event.type fields and updated the asciidoc and pushed a clean rebase.

ruflin · 2018-01-22T22:43:51Z

Small PR for @ph, big step for Filebeat 🎉

This commit revert the decision done in elastic#6078 and will use `input.type` to replace the `prospector.type`

This commit revert the decision done in #6078 and will use `input.type` to replace the `prospector.type`

require elastic#6078

require #6078

This got renamed in Beats 6.3: elastic/beats#6078

* Rename beat prospectors to inputs This got renamed in Beats 6.3: elastic/beats#6078 * Add graylog fields to beats default templates The collector "Show messages" button links to a search for a matching `gl2_source_collector`. Thus we need to define these fields in every beats configuration. Furthermore, we need to set `fields_under_root` because the new Beats input does not strip away the "fields_" prefix for us. Only "Beats Legacy" does that. * Add filebeat and winlogbeat default template Provide users with a sensible default template to get them started. * Fix template variable name It's called sidecarVersion now.

ph requested a review from ruflin January 15, 2018 18:35

ph added the review label Jan 15, 2018

ruflin added the Filebeat Filebeat label Jan 16, 2018

ruflin reviewed Jan 16, 2018

View reviewed changes

ruflin approved these changes Jan 18, 2018

View reviewed changes

ph force-pushed the refactor/type-aliasing-input branch from 5e66928 to d027f45 Compare January 19, 2018 15:34

ph added 2 commits January 22, 2018 13:34

update changelog with mention of prospector changes

0e31dc7

ph force-pushed the refactor/type-aliasing-input branch from d027f45 to 0e31dc7 Compare January 22, 2018 18:35

wrong assert

e571419

ruflin merged commit bc1a68e into elastic:master Jan 22, 2018

ph added a commit to ph/beats that referenced this pull request Jan 30, 2018

revert event.type and use input.type instead

80082dd

This commit revert the decision done in elastic#6078 and will use `input.type` to replace the `prospector.type`

ph mentioned this pull request Jan 30, 2018

revert event.type and use input.type instead #6227

Merged

ruflin pushed a commit that referenced this pull request Jan 31, 2018

revert event.type and use input.type instead (#6227)

6de1b26

This commit revert the decision done in #6078 and will use `input.type` to replace the `prospector.type`

ph added a commit to ph/beats that referenced this pull request Feb 5, 2018

Migrate: Docker prospector to the the input interface

a9e12e7

require elastic#6078

ph mentioned this pull request Feb 5, 2018

Migrate: Docker prospector to the the input interface #6278

Merged

ruflin pushed a commit that referenced this pull request Feb 6, 2018

Migrate: Docker prospector to the the input interface (#6278)

e39b441

require #6078

mpfz0r added a commit to Graylog2/graylog2-server that referenced this pull request Oct 11, 2018

Rename beat prospectors to inputs

3acf6f7

This got renamed in Beats 6.3: elastic/beats#6078

secretworry mentioned this pull request Jan 3, 2019

Adapt new pipeline configuration for filebeats 6.3 geerlingguy/ansible-role-filebeat#32

Closed

Refactor: Renaming prospector to input #6078

Refactor: Renaming prospector to input #6078

Conversation

ph commented Jan 15, 2018 • edited Loading

ph commented Jan 15, 2018

ph commented Jan 15, 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

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

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

This is with the pragma build go1.9

This is without the pragma build go1.9

ph commented Jan 16, 2018

ph commented Jan 17, 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

ph commented Jan 19, 2018

ruflin commented Jan 21, 2018

ph commented Jan 22, 2018

ruflin commented Jan 22, 2018

ph commented Jan 15, 2018 •

edited

Loading

This is with the pragma `build go1.9`

This is without the pragma `build go1.9`