Add registry for prospectors #4980

kvch · 2017-08-23T16:32:46Z

From now on it is possible to register prospectors in init functions, just like modules in Metricbeat.

Example:

func init() {
	err := prospectors.Register("log", NewProspector)
	if err != nil {
		panic(err)
	}
}

General prospector functions were moved to prospectors package (for a lack of a better name). In prospector.go new prospectors should be imported to register.

urso · 2017-08-23T19:20:11Z

filebeat/crawler/crawler.go

 	"github.com/elastic/beats/filebeat/registrar"
 	"github.com/elastic/beats/libbeat/cfgfile"
 	"github.com/elastic/beats/libbeat/common"
 	"github.com/elastic/beats/libbeat/common/cfgwarn"
 	"github.com/elastic/beats/libbeat/logp"
+
+	_ "github.com/elastic/beats/filebeat/prospector"


in other places we use something like github.om/elastic/beats/<beatname>/include/list.go, to build/provide an import list of 'plugins'.

It would be nice not to have a package named prospector and another one named prospectors. Rather have on prospector package only if possible.

Here is for example the script that generates the imports for metricbeat: https://github.com/elastic/beats/blob/master/metricbeat/scripts/generate_imports.py

urso · 2017-08-23T19:25:42Z

filebeat/prospector/prospectors/registry.go

+	"github.com/elastic/beats/libbeat/logp"
+)
+
+type ProspectorFactoryData struct {


how about naming it 'Context' or 'Environment'?

Keep in mind, the package names is part of a type its full. Try to reduce stuttering if possible. e.g.

prospectors.ProspectorFactory can be minimized to prospectors.Factory. And prospectors.ProspectorFactoryData to prospectors.Context.

urso · 2017-08-23T19:26:23Z

filebeat/prospector/prospectors/registry.go

+	BeatDone chan struct{}
+}
+
+type ProspectorFactory func(config *common.Config, outletFactory channel.OutleterFactory, data ProspectorFactoryData) (Prospectorer, error)


appropos naming, I will not complain if you rename channel.OutleterFactory to channel.Factory ;)

urso · 2017-08-23T19:31:00Z

Just a few minor comments. All in all LGTM

ruflin · 2017-08-23T19:39:33Z

filebeat/prospector/redis/prospector.go

@@ -49,6 +50,13 @@ func NewProspector(cfg *common.Config, outletFactory channel.OutleterFactory) (*
 	return p, nil
 }

+func init() {


Could we always put the init functions on the top after the imports? I kind of like them at the top as it becomes also obvious they are executed first.

ruflin

It's really great to have this. This will give us much more flexibility. I think we need to improve / fix the naming over time and it will probably become more obvious on what it should be when we start working with it.

kvch · 2017-08-23T21:10:17Z

I addressed the notes. However, I am not satisfied with the package structure and names, but so far I don't have better idea.

I also added renaming channel.OutleterFactory to channel.Factory.

ruflin · 2017-08-24T07:38:49Z

filebeat/prospector/include/list.go

@@ -0,0 +1,8 @@
+package include


I would suggest to move this directly under filebeat instead of prospectr as we ave it in other projects.

ruflin

LGTM. I think we can keep iterating on the naming to get it right. I suggest to merge this feature now as it will give us new flexibility in adding prospector types.

ruflin · 2017-08-24T07:43:27Z

@kvch There seem to be some issues with the tests. Ping me when you got the build green.

urso · 2017-08-24T12:31:39Z

We still need the prospector and prospectors packages each? Given the imports moved to filebeat/include/list.go, there should be no more circular dependencies.

kvch · 2017-08-24T12:38:46Z

I will remove it \o/

kvch · 2017-08-24T13:18:43Z

I had to rename Factory in factory.go which is a struct that contains data of the Registrar. Its new name is RegistrarContext. So outside the package its name is prospector.RegistrarContext and the concrete factories of prospectors type can be prospector.Factory.

kvch · 2017-08-24T16:45:09Z

jenkins, test it

ruflin · 2017-08-25T11:30:30Z

@urso Never saw this Jenkins error before:

.....F........S.SS..SS........
======================================================================
FAIL: Checks CLI overwrite actually overwrites some config variable by
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Users\jenkins\workspace\elastic+beats+pull-request+multijob-windows\beat\libbeat\label\windows\src\github.com\elastic\beats\libbeat\tests\system\test_base.py", line 57, in test_invalid_config_cli_param
    proc.check_kill_and_wait()
  File "C:\Users\jenkins\workspace\elastic+beats+pull-request+multijob-windows\beat\libbeat\label\windows\src\github.com\elastic\beats\libbeat\tests\system\beat\beat.py", line 89, in check_kill_and_wait
    return self.check_wait(exit_code=exit_code)
  File "C:\Users\jenkins\workspace\elastic+beats+pull-request+multijob-windows\beat\libbeat\label\windows\src\github.com\elastic\beats\libbeat\tests\system\beat\beat.py", line 78, in check_wait
    exit_code, actual_exit_code)
AssertionError: Expected exit code to be 0, but it was 1

As the error is not related to this change, I think we can move forward here.

ruflin · 2017-08-25T11:32:31Z

@kvch Great job on this one. Lets now do small interations on top of it to potentially improve handling and even simplify things further.

kvch added Filebeat Filebeat refactoring review labels Aug 23, 2017

kvch force-pushed the refactor/prospector-plugin branch from feb2fad to 7e00cfe Compare August 23, 2017 16:33

urso reviewed Aug 23, 2017

View reviewed changes

ruflin reviewed Aug 23, 2017

View reviewed changes

kvch force-pushed the refactor/prospector-plugin branch from 7e00cfe to f51bc23 Compare August 23, 2017 19:48

kvch added the in progress Pull request is currently in progress. label Aug 23, 2017

ruflin reviewed Aug 24, 2017

View reviewed changes

ruflin approved these changes Aug 24, 2017

View reviewed changes

kvch force-pushed the refactor/prospector-plugin branch 2 times, most recently from cf521c9 to e6eba4c Compare August 24, 2017 11:40

kvch removed the in progress Pull request is currently in progress. label Aug 24, 2017

kvch added 2 commits August 24, 2017 15:15

filebeat: add registry for prospectors

4a63327

channel: rename OutletFactory to Factory

9fae267

kvch force-pushed the refactor/prospector-plugin branch from e6eba4c to 9fae267 Compare August 24, 2017 13:15

ruflin merged commit 366a584 into elastic:master Aug 25, 2017

kvch mentioned this pull request Aug 25, 2017

Add prospector plugin && make imports #5026

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add registry for prospectors #4980

Add registry for prospectors #4980

kvch commented Aug 23, 2017 •

edited by monicasarbu

urso Aug 23, 2017 •

edited

ruflin Aug 23, 2017

urso Aug 23, 2017

urso Aug 23, 2017

urso commented Aug 23, 2017

ruflin Aug 23, 2017

ruflin left a comment

kvch commented Aug 23, 2017

ruflin Aug 24, 2017

ruflin left a comment

ruflin commented Aug 24, 2017

urso commented Aug 24, 2017 •

edited

kvch commented Aug 24, 2017

kvch commented Aug 24, 2017

kvch commented Aug 24, 2017

ruflin commented Aug 25, 2017

ruflin commented Aug 25, 2017

Add registry for prospectors #4980

Add registry for prospectors #4980

Conversation

kvch commented Aug 23, 2017 • edited by monicasarbu

urso Aug 23, 2017 • edited

Choose a reason for hiding this comment

ruflin Aug 23, 2017

Choose a reason for hiding this comment

urso Aug 23, 2017

Choose a reason for hiding this comment

urso Aug 23, 2017

Choose a reason for hiding this comment

urso commented Aug 23, 2017

ruflin Aug 23, 2017

Choose a reason for hiding this comment

ruflin left a comment

Choose a reason for hiding this comment

kvch commented Aug 23, 2017

ruflin Aug 24, 2017

Choose a reason for hiding this comment

ruflin left a comment

Choose a reason for hiding this comment

ruflin commented Aug 24, 2017

urso commented Aug 24, 2017 • edited

kvch commented Aug 24, 2017

kvch commented Aug 24, 2017

kvch commented Aug 24, 2017

ruflin commented Aug 25, 2017

ruflin commented Aug 25, 2017

kvch commented Aug 23, 2017 •

edited by monicasarbu

urso Aug 23, 2017 •

edited

urso commented Aug 24, 2017 •

edited