Center more functionality around RunnerFactory #16715

urso · 2020-02-29T14:11:33Z

Refactoring

What does this PR do?

Remove CheckableRunnerFactory and require RunnerFactory to implement
CheckConfig. CheckableRunnerFactory more and more superseded
RunnerFactory. As we want more config validation support in the future
as well I combined the two into RunnerFactory

Remove autodiscover.Adapter. The adapter did inherit from
CheckableRunnerFactory, giving us some inheritance chain between
RunnerFactory, CheckableRunnerFactory, and Adapter for autodiscovery.
By removig the Adapter and CheckableRunnerFactory we have one common
type (RunnerFactory) to integrate with config file reloading, static
input/module setup, and autodiscovery.

Add selectors for autodiscovery event selection that are used as
additional parameters when creating a new Autodiscover instance. This
gives us some more composability, yet I wonder if we can even remove those,
as every instance of NewAutodiscover did look for events with a 'config'
field of type []*common.Config. @exekias WDYT?

Why is it important?

The new input API will have a compatiblity layer creating a RunnerFactory. By having only one interface to integrate with, the integration will be more straight forward.

The change also makes it more clear how to integrate with autodiscovery and config file reloading, as it standardizes the two based on RunnerFactory only.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
~~- [ ] I have made corresponding changes to the documentation~~
~~- [ ] I have made corresponding change to the default configuration files~~
I have added tests that prove my fix is effective or that my feature works

Developer Docs

The types autodiscovery.Adapter and cfgfile.CheckableRunnerFactory have been removed. Helpers for creating autodiscovery.Adapter have been removed as well. Instead cfgfile.RunnerFactory requires CheckConfig to be implemented now.

cfgfile.CheckableRunnerFactory and cfgfile.Runner factory have been combined, whereas the Adapter is split into cfgfile.RunnerFactory and autodiscover.EventConfigurer. The autodiscover.NewAutodiscover call now needs both, the RunnerFactory and EventConfigurer parameters.

The (cfgfile.RunnerFactory).Create method now accepts a beat.PipelineConnector instead of a beat.Pipeline as first parameter.

For example:

adapter := autodiscover.NewFactoryAdapter(bt.dynamicFactory)
autodiscover.NewAutodiscover("heartbeat", b.Publisher, adapter, bt.config.Autodiscover)

has become:

autodiscover.NewAutodiscover("heartbeat", b.Publisher, bt.dynamicFactory, autodiscover.QueryConfig(), bt.config.Autodiscover)

It is assumed that CheckConfig returns an error if the configuration does not apply. In case the factory did not support CheckConfig before, the functionality needs to be implemented like this:

func (f *Factory) CheckConfig(c *common.Config) error {
	_, err := f.Create(pubpipeline.NewNilPipeline(), c, nil)
	return err
}

urso · 2020-02-29T14:13:43Z

libbeat/autodiscover/autodiscover.go

+	pipeline beat.Pipeline,
+	factory cfgfile.RunnerFactory,
+	selector EventSelector,
+	configurer EventConfigurer,


selector/configurer are the same for all Beats. How about removing these altogether? I did only introduce them to keep the overall flexiblity that the Adapter did provide. @exekias Is there a reason/use-case to not remove those?

I cannot think of any current use case so +1 to removing these. We can always add it when we need it

How would they be removed -- does this mean that queryConfigFrom is the only implementor of EventConfigurer, and we can remove the interface and simply refer to the concrete struct? Or are you saying that the concrete type is also somehow redundant and we can remove this parameter entirely? Removing the interface would certainly resolve the associated documentation concerns, so I'm inclined to be in favor of it :-)

Does "selector" mean the same thing as "configurer" in this context or are you referring to two different values?

Checking all Beats code the implementation is always the same. The Configurer always returns []string{ "config" }, and the selector always reads the field event["config"]. In this sense the configurer and selector need to be configured together to be sound. I already combined the two into one interface in this PR. But given that all call sides always use "config", I'm thinking to remove the interfaces completely.

exekias · 2020-03-03T12:14:29Z

I like how interfaces are converging into a single runner factory!

urso · 2020-03-05T22:19:07Z

Note: linux build all green, windows all failing during setup.

urso · 2020-03-05T22:19:15Z

Jenkins, test this.

faec

Various comments, mostly documentation-related :-)

faec · 2020-03-06T20:07:06Z

filebeat/beater/crawler.go

 		}

+	}
+
+	if c.inputReloader != nil {


will c.inputReloader ever be non-nil when crawler.Start is called, or is it always initialized from this function?

It will be nil if input reloading has not been enabled. I just moved the check down here, so we do not leak the go-routine in case of startup going wrong when the module loader is faulty.

faec · 2020-03-06T20:17:05Z

libbeat/cfgfile/factories.go

+type multiplexedFactory []FactoryMatcher
+
+// FactoryMatcher returns a RunnerFactory that should be used to
+// handle the given configuration.


Clarify that it's intended that this will return nil in normal operation? E.g. A FactoryMatcher returns a RunnerFactory that can handle the given configuration if it supports it, otherwise it returns nil.

faec · 2020-03-06T20:21:31Z

libbeat/cfgfile/factories.go

+
+var errConfigDoesNotMatch = errors.New("config does not match accepted configurations")
+
+// MultiplexedRunnerFactory is a RunnerFactory that uses a list of


Uses the list how? It looks like it chooses the first match in the ordered list, if one exists, but please document the intent for callers.

faec · 2020-03-06T20:25:01Z

libbeat/cfgfile/factories.go

+	return multiplexedFactory(matchers)
+}
+
+// MatchHasField returns the configured RunnerFactory if the configation contains the configured field.


Can the language here be clarified? I think I understand the intent, but MatchHasField returns a FactoryMatcher not a RunnerFactory. E.g. (if I understand the intent correctly): MatchHasField returns a FactoryMatcher that returns the given RunnerFactory when the input config contains the given field name.

faec · 2020-03-06T20:26:07Z

libbeat/cfgfile/factories.go

+	}
+}
+
+// MatchDefault always returns the configured runner factory.


as above, "...returns a FactoryMatcher that always..."

faec · 2020-03-06T20:46:44Z

libbeat/autodiscover/autodiscover.go

-
-	// EventFilter returns the bus filter to retrieve runner start/stop triggering events
+// EventConfigurer generates a valid list of configs from the given event, the
+// received event will have all keys defined by `StartFilter`.


I can't find any other instance of StartFilter in the repo, does this mean to say EventFilter?

Also, can you clarify what is actually done to the event? Does a call to CreateConfig modify its input, or just create new common.Configs based on it? Are the raw strings returned by EventFilter added as top-level keys of the event's MapStr structure, or to the output configs, or something else?

faec · 2020-03-06T20:49:26Z

libbeat/autodiscover/autodiscover.go

-	// EventFilter returns the bus filter to retrieve runner start/stop triggering events
+// EventConfigurer generates a valid list of configs from the given event, the
+// received event will have all keys defined by `StartFilter`.
+type EventConfigurer interface {
 	EventFilter() []string


Please document EventFilter (it looks like you removed an existing doc comment, but even if it was restored it's not obvious to me how a list of strings represents a "bus filter").

faec · 2020-03-06T21:06:14Z

libbeat/autodiscover/autodiscover.go

+	pipeline beat.Pipeline,
+	factory cfgfile.RunnerFactory,
+	selector EventSelector,
+	configurer EventConfigurer,


How would they be removed -- does this mean that queryConfigFrom is the only implementor of EventConfigurer, and we can remove the interface and simply refer to the concrete struct? Or are you saying that the concrete type is also somehow redundant and we can remove this parameter entirely? Removing the interface would certainly resolve the associated documentation concerns, so I'm inclined to be in favor of it :-)

Does "selector" mean the same thing as "configurer" in this context or are you referring to two different values?

faec · 2020-03-06T21:22:49Z

libbeat/autodiscover/eventselect.go

+func (q queryConfigFrom) CreateConfig(e bus.Event) ([]*common.Config, error) {
+	config, ok := e[string(q)].([]*common.Config)
+	if !ok {
+		return nil, errors.New("Got a wrong value in event `config` key")


Can this error be more specific? E.g. "Event field {string(q)} does not contain a valid common.Config"

faec · 2020-03-06T21:32:00Z

libbeat/cfgfile/reload.go

 }

-// ConfigChecker is usually combined with a RunnerFactory for implementations that can check a config
-// without a pipeline and metadata.
+// ConfigChecker is usually combined with a RunnerFactory for implementations


Are there any uses of ConfigChecker without RunnerFactory? The only references I can find are casts of RunnerFactory to ConfigChecker, which should be redundant with this change. Can we move CheckConfig into RunnerFactory and remove ConfigChecker entirely, or is it still needed for something?

will have a look and remove casts.

Remove CheckableRunnerFactory and require RunnerFactory to implement CheckConfig. CheckableRunnerFactory more and more superseded RunnerFactory. As we want more config validation support in the future as well I combined the two into RunnerFactory Remove autodiscover.Adapter. The adapter did inherit from CheckableRunnerFactory, giving us some inheritance chain between RunnerFactory, CheckableRunnerFactory, and Adapter for autodiscovery. By removig the Adapter and CheckableRunnerFactory we have one common type (RunnerFactory) to integrate with config file reloading, static input/module setup, and autodiscovery. Add selectors for autodiscovery event selection that are used as additional parameters when creating a new Autodiscover instance. This gives us some more composability, yet I wonder if we can even remove those, as every instance of NewAutodiscover did look for events with a 'config' field of type []*common.Config.

- wrap errors to give some context which operation failed - do not leak inputloader background process if configuration fails

faec

Looks good, thanks!

urso · 2020-03-26T18:35:13Z

All related tests did pass.

* Center more functionality around RunnerFactory Remove CheckableRunnerFactory and require RunnerFactory to implement CheckConfig. CheckableRunnerFactory more and more superseded RunnerFactory. As we want more config validation support in the future as well I combined the two into RunnerFactory Remove autodiscover.Adapter. The adapter did inherit from CheckableRunnerFactory, giving us some inheritance chain between RunnerFactory, CheckableRunnerFactory, and Adapter for autodiscovery. By removig the Adapter and CheckableRunnerFactory we have one common type (RunnerFactory) to integrate with config file reloading, static input/module setup, and autodiscovery. Add selectors for autodiscovery event selection that are used as additional parameters when creating a new Autodiscover instance. This gives us some more composability, yet I wonder if we can even remove those, as every instance of NewAutodiscover did look for events with a 'config' field of type []*common.Config. (cherry picked from commit 7a1b524)

…tory (#18085)

Co-authored-by: Steffen Siering <steffen.siering@elastic.co>

…8853) Since metricbeat light modules support processors (#15923), module initialization requires a publisher in the beat so modules can attach their processors. `metricbeat test modules` is not initializing as normal metricbeat commands, and it is not initializing any output or publisher pipeline, so metricbeat panics when trying to initialize modules with the new method. This change adds a dummy publisher for this case, and fixes also a condition that was adding a `nil` module option, causing additional panics. A test that reproduced the issues is also added. (cherry picked from commit 25b8bf1) Add nil pipeline from #16715, required for the fix. (cherry picked from commit 7a1b524) Co-authored-by: Steffen Siering <steffen.siering@elastic.co>

…es` (elastic#18853) Since metricbeat light modules support processors (elastic#15923), module initialization requires a publisher in the beat so modules can attach their processors. `metricbeat test modules` is not initializing as normal metricbeat commands, and it is not initializing any output or publisher pipeline, so metricbeat panics when trying to initialize modules with the new method. This change adds a dummy publisher for this case, and fixes also a condition that was adding a `nil` module option, causing additional panics. A test that reproduced the issues is also added. (cherry picked from commit 316793d) Add nil pipeline from elastic#16715, required for the fix. (cherry picked from commit 257fdfc) Co-authored-by: Steffen Siering <steffen.siering@elastic.co>

urso added Filebeat Filebeat libbeat Metricbeat Metricbeat Heartbeat autodiscovery v7.7.0 Project:Filebeat-Input-v2 Team:Services (Deprecated) Label for the former Integrations-Services team labels Feb 29, 2020

urso commented Feb 29, 2020

View reviewed changes

andresrc added [zube]: Inbox [zube]: In Review and removed [zube]: Inbox labels Mar 2, 2020

urso force-pushed the runnerfactory-machinerie branch from a5ca9f2 to 6508e5b Compare March 3, 2020 18:53

urso marked this pull request as ready for review March 3, 2020 19:31

urso requested a review from a team as a code owner March 3, 2020 19:31

urso mentioned this pull request Mar 3, 2020

[DRAFT] Add filebeat input v2 compatiblity layer #16779

Closed

5 tasks

urso added the release-note:dev_docs Contains a Dev Docs section label Mar 4, 2020

urso force-pushed the runnerfactory-machinerie branch from 6508e5b to b4fd960 Compare March 4, 2020 23:44

urso force-pushed the runnerfactory-machinerie branch from b80adb4 to cf8610e Compare March 6, 2020 16:08

faec requested changes Mar 6, 2020

View reviewed changes

urso added 7 commits March 10, 2020 17:01

add missing license header

4259281

fix build

d69ad6c

remove EventSelector

d2d13af

Add godoc to cfgfile/factories.go

639b367

wrap error to show where it comes from in reloader

90577b0

More crawler improvements:

ec8cbc6

- wrap errors to give some context which operation failed - do not leak inputloader background process if configuration fails

urso added 4 commits March 10, 2020 17:01

add nilpipeline

71ee8ca

Implement CheckConfig for filebeat inputs/fileset factories

daf9964

fix some tests

5987016

godoc

83a4471

urso force-pushed the runnerfactory-machinerie branch from cf8610e to 83a4471 Compare March 10, 2020 20:41

urso requested review from faec and exekias March 10, 2020 20:41

fix eventselect

ba9f277

exekias approved these changes Mar 12, 2020

View reviewed changes

faec approved these changes Mar 26, 2020

View reviewed changes

urso merged commit 7a1b524 into elastic:master Mar 26, 2020

zube bot added [zube]: Done and removed [zube]: In Review labels Mar 26, 2020

andresrc removed the [zube]: Done label Mar 30, 2020

urso added needs_backport PR is waiting to be backported to other branches. and removed v7.7.0 labels Apr 29, 2020

urso mentioned this pull request Apr 29, 2020

Cherry-pick #16715 to 7.x: Center more functionality around RunnerFactory #18085

Merged

3 tasks

urso added v7.8.0 and removed needs_backport PR is waiting to be backported to other branches. labels Apr 29, 2020

urso pushed a commit that referenced this pull request Apr 29, 2020

Cherry-pick #16715 to 7.x: Center more functionality around RunnerFac…

ef259cf

…tory (#18085)

jsoriano added a commit to jsoriano/beats that referenced this pull request Jun 2, 2020

Backport nil pipeline from elastic#16715

0792e65

Co-authored-by: Steffen Siering <steffen.siering@elastic.co>

jsoriano mentioned this pull request Jun 2, 2020

Cherry-pick #18797 to 7.7: Fix panic on metricbeat test modules #18853

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Center more functionality around RunnerFactory #16715

Center more functionality around RunnerFactory #16715

urso commented Feb 29, 2020 •

edited

Loading

urso Feb 29, 2020

exekias Mar 3, 2020

faec Mar 6, 2020

urso Mar 10, 2020

exekias commented Mar 3, 2020

urso commented Mar 5, 2020

urso commented Mar 5, 2020

faec left a comment

faec Mar 6, 2020

urso Mar 6, 2020

faec Mar 6, 2020

faec Mar 6, 2020

faec Mar 6, 2020

faec Mar 6, 2020

faec Mar 6, 2020

faec Mar 6, 2020

faec Mar 6, 2020

faec Mar 6, 2020

faec Mar 6, 2020

urso Mar 6, 2020

faec left a comment

urso commented Mar 26, 2020


		var errConfigDoesNotMatch = errors.New("config does not match accepted configurations")

		// MultiplexedRunnerFactory is a RunnerFactory that uses a list of

Center more functionality around RunnerFactory #16715

Center more functionality around RunnerFactory #16715

Conversation

urso commented Feb 29, 2020 • edited Loading

What does this PR do?

Why is it important?

Checklist

Developer Docs

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

exekias commented Mar 3, 2020

urso commented Mar 5, 2020

urso commented Mar 5, 2020

faec 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

faec left a comment

Choose a reason for hiding this comment

urso commented Mar 26, 2020

urso commented Feb 29, 2020 •

edited

Loading