Refactor: Introduce a Global Registry #7392

ph · 2018-06-21T16:07:52Z

Motivation:

In the current libbeat implementation and also inside the actual beat we are defining registries for each
types of feature that we want to expose. This add duplication to the project, the global registry
is a way to keep the flexibility of multiple features and reduce the duplication code only to take
care of the type satefy.

Also all features now use an init function to make the plugin register with their specific registry.
This PR is a step forward to remove that pattern and use a global variable in the package to identify
the feature. This change will allow a beat author to build a beat with only a specific set of feature.

Example: Build with only ES out and not Logstash and kafka, this could reduce the size of some beats.

This PR is written in a backward compatible way, to make the init and the new feature works both at
the same time.

Instead of using an init function you will the following to expose the feature.

// Feature exposes a spooling to disk queue.
var Feature = queue.Feature("spool", create, feature.Beta)

Each new type of feature require to implement two things for type satefy:

A factory method to assert the correct type at runtime.
A sugar method like the queue.Feature, for type satefy at compile time.

Example:

// Feature creates a new type of queue.
func Feature(name string, factory Factory, stability feature.Stability) *feature.Feature {
	return feature.New(Namespace, name, factory, stability)
}

// FindFactory retrieves a queue types constructor. Returns nil if queue type is unknown
func FindFactory(name string) Factory {
	f, err := feature.Registry.Find(Namespace, name)
	if err != nil {
		return nil
	}
	factory, ok := f.Factory().(Factory)
	if !ok {
		return nil
	}

	return factory
}

How it will look like for building beats with a minimal set of plugins:

b := MustBundle(
  MustBundle(docker.Feature),
  MustBundle(dissect.Feature),
  MustBundle(elasticsearch.Feature, logstash.Feature),
)

feature.RegisterBundle(b)

Caveats:

we still expose the methods and the registry as global, but this is a step to to isolate a registry
per beat.

TODO:

Ref: #7093

*Motivation:* In the current libbeat implementation and also inside the actual beat we are defining registries for each types of feature that we want to expose. This add duplication to the project, the global registry is a way to keep the flexibility of multiple features and reduce the duplication code only to take care of the type satefy. Also all features now use an init function to make the plugin register with their specific registry. This PR is a step forward to remove that pattern and use a global variable in the package to identify the feature. This change will allow a beat author to build a beat with only a specific set of feature. Example: Build with only ES out and not Logstash and kafka, this could reduce the size of some beats. This PR is written in a backward compatible way, to make the init and the new feature works both at the same time. Instead of using an init function you will the following to expose the feature. ```golang // Feature exposes a spooling to disk queue. var Feature = queue.Feature("spool", create, feature.Beta) ``` Each new type of feature require to implement two things for type satefy: - A factory method to assert the correct type at runtime. - A sugar method like the `queue.Feature`, for type satefy at compile time. *Example:* ```golang // Feature creates a new type of queue. func Feature(name string, factory Factory, stability feature.Stability) *feature.Feature { return feature.New(Namespace, name, factory, stability) } // FindFactory retrieves a queue types constructor. Returns nil if queue type is unknown func FindFactory(name string) Factory { f, err := feature.Registry.Find(Namespace, name) if err != nil { return nil } factory, ok := f.Factory().(Factory) if !ok { return nil } return factory } ``` How it will look like for building beats with a minimal set of plugins: ``` b := MustBundle( MustBundle(docker.Feature), MustBundle(dissect.Feature), MustBundle(elasticsearch.Feature, logstash.Feature), ) feature.RegisterBundle(b) ``` *Caveats:* we still expose the methods and the registry as global, but this is a step to to isolate a registry per beat.

ph · 2018-06-21T18:43:12Z

As a side effect this gives us the opportunity to make sure a beats doesn't load anything experimental or beta.

b := MustBundle(
  MustBundle(docker.Feature),
  MustBundle(dissect.Feature),
  MustBundle(elasticsearch.Feature, logstash.Feature),
)

feature.RegisterBundle(b.Filter(feature.Stable))
feature.RegisterBundle(b.Filter(feature.Stable, feature.Beta))

ph · 2018-06-22T11:37:03Z

In my previous comment, I meant execute not load, the features will still get compiled in.

ph · 2018-06-22T18:24:22Z

libbeat/feature/bundle.go

+		}
+	}
+	return NewBundle(merged), nil
+}


I will change this, I will remove the switch case.

The following might be better.

diff --git a/libbeat/feature/bundle.go b/libbeat/feature/bundle.go index 174c2f973..9886b68b5 100644 --- a/libbeat/feature/bundle.go +++ b/libbeat/feature/bundle.go @@ -1,22 +1,26 @@ package feature -import "fmt" +// Groupable allows features and Bundle to be merged together. +type Groupable interface { + // Features returns a slice of features. + Features() []Featurable +} // Bundle defines a list of features available in the current beat. type Bundle struct { - Features []Featurable + features []Featurable } // NewBundle creates a new Bundle of feature to be registered. func NewBundle(features []Featurable) *Bundle { - return &Bundle{Features: features} + return &Bundle{features: features} } // Filter creates a new bundle with only the feature matching the requested stability. func (b *Bundle) Filter(stabilities ...Stability) *Bundle { var filtered []Featurable - for _, feature := range b.Features { + for _, feature := range b.features { for _, stability := range stabilities { if feature.Stability() == stability { filtered = append(filtered, feature) @@ -27,29 +31,16 @@ func (b *Bundle) Filter(stabilities ...Stability) *Bundle { return NewBundle(filtered) } -// MustBundle takes existing bundle or features and create a new Bundle with all the merged Features, -// will panic on errors. -func MustBundle(features ...interface{}) *Bundle { - b, err := BundleFeature(features...) - if err != nil { - panic(err) - } - return b +// Features returns a slice of features. +func (b *Bundle) Features() []Featurable { + return b.features } -// BundleFeature takes existing bundle or features and create a new Bundle with all the merged -// Features, -func BundleFeature(features ...interface{}) (*Bundle, error) { +// MustBundle takes existing bundle or features and create a new Bundle with all the merged Features. +func MustBundle(features ...Groupable) *Bundle { var merged []Featurable for _, feature := range features { - switch v := feature.(type) { - case Featurable: - merged = append(merged, v) - case *Bundle: - merged = append(merged, v.Features...) - default: - return nil, fmt.Errorf("unknown type, expecting 'Featurable' or 'Bundle' and received '%T'", v) - } + merged = append(merged, feature.Features()...) } - return NewBundle(merged), nil + return NewBundle(merged) } diff --git a/libbeat/feature/bundle_test.go b/libbeat/feature/bundle_test.go index df293cc4b..e675a7f09 100644 --- a/libbeat/feature/bundle_test.go +++ b/libbeat/feature/bundle_test.go @@ -16,25 +16,25 @@ func TestBundle(t *testing.T) { t.Run("Creates a new Bundle", func(t *testing.T) { b := NewBundle(features) - assert.Equal(t, 3, len(b.Features)) + assert.Equal(t, 3, len(b.Features())) }) t.Run("Filters feature based on stability", func(t *testing.T) { b := NewBundle(features) new := b.Filter(Experimental) - assert.Equal(t, 1, len(new.Features)) + assert.Equal(t, 1, len(new.Features())) }) t.Run("Filters feature based on multiple different stability", func(t *testing.T) { b := NewBundle(features) new := b.Filter(Experimental, Stable) - assert.Equal(t, 2, len(new.Features)) + assert.Equal(t, 2, len(new.Features())) }) t.Run("Creates a new Bundle from specified feature", func(t *testing.T) { f1 := New("libbeat.outputs", "elasticsearch", factory, Stable) b := MustBundle(f1) - assert.Equal(t, 1, len(b.Features)) + assert.Equal(t, 1, len(b.Features())) }) t.Run("Creates a new Bundle with grouped features", func(t *testing.T) { @@ -49,6 +49,6 @@ func TestBundle(t *testing.T) { MustBundle(f3, f4), ) - assert.Equal(t, 4, len(b.Features)) + assert.Equal(t, 4, len(b.Features())) }) } diff --git a/libbeat/feature/feature.go b/libbeat/feature/feature.go index 0931bbe53..1d2621982 100644 --- a/libbeat/feature/feature.go +++ b/libbeat/feature/feature.go @@ -59,6 +59,11 @@ func (f *Feature) Stability() Stability { return f.stability } +// Features allows to Bundle a Feature with a bundle of features. +func (f *Feature) Features() []Featurable { + return []Featurable{f} +} + // Equal return true if both object are equals. func (f *Feature) Equal(other Featurable) bool { // There is no safe way to compare function in go,

ruflin · 2018-06-25T08:51:52Z

@ph As we currently generate the imports we guarantee that we don't forget about including an output for example or a new Metricbeat module. Would be nice if we could find a method to still verify this with this new method.

urso · 2018-06-25T09:02:29Z

libbeat/feature/feature.go

+	"reflect"
+)
+
+//go:generate stringer -type=Stability


How about the 'generate stringer' command next to the type declaration or in a separate 0gen.go file. The generated file already contains the type name. Having the generate command next to the type declaration makes it easier to find when navigating the code. Like:

// Comment... // go:generate stringer -type=Stability type Stability int

This was added automatically when you call generate stringer -type=Stability in the feature directory :)

urso · 2018-06-25T09:04:02Z

libbeat/feature/feature.go

+
+//go:generate stringer -type=Stability
+
+// Registry is the global plugin registry, this variable is mean to be temporary to move all the


s/mean/meant/

urso · 2018-06-25T09:07:42Z

libbeat/feature/feature.go

+	Equal(other Featurable) bool
+
+	String() string
+}


'X returns x' is kind of redundant. Plus godoc will not generate any docs for the methods. Instead the type will be shown in the docs as is in code. Rather then stating the obvious, describing how Featurable interacts with the Bundle and Registry would be more helpful. What's the purpose of Namespace. Why do I need to implement 'equal'?

urso · 2018-06-25T09:09:01Z

libbeat/feature/feature.go

+// Equal return true if both object are equals.
+func (f *Feature) Equal(other Featurable) bool {
+	// There is no safe way to compare function in go,
+	// but since the method are global it should be stable.


s/method/function pointers/

urso · 2018-06-25T09:09:54Z

libbeat/feature/feature.go

+	Beta
+	Experimental
+	Undefined
+)


Nice trick:

const ( Undefined Stability = iota Stable Beta Experimental )

This way the 'zero value' of Stability becomes Undefiend -> force developer to use a constant.

Good point, I have added undefined at the end, will make the change.

urso · 2018-06-25T09:16:34Z

libbeat/feature/registry.go

+		feature.Name(),
+	)
+
+	r.namespaces[feature.Namespace()][feature.Name()] = feature


Instead of calling a function that might potentially generate a string on every call (it's all interfaces), let's introduce some local vars: ns, name := feature.Namespace(), feature.Name().

urso · 2018-06-25T09:18:09Z

libbeat/feature/registry.go

+
+	f, found := r.namespaces[feature.Namespace()][feature.Name()]
+	if found {
+		if feature.Equal(f) {


We can remove Equal from the public interface by providing a helper function checking for equal:

func featuresEq(f1, f2 Featurable) bool { type comparable interface { Equal(other Featurable) bool } if cmp, ok := f1.(comparable); ok { return cmp.Equal(f2) } return false }

urso · 2018-06-25T09:21:10Z

libbeat/feature/registry.go

+	return nil
+}
+
+// Find returns a specific Find from a namespace or an error if not found.


Find searches for a Feature by the namespace-name pair.

Alternative name: Lookup

I'm OK for using Lookup instead.

urso · 2018-06-25T09:50:29Z

libbeat/publisher/queue/queue_reg.go

+		return nil
+	}
+
+	return factory


Using reflection, this pattern can be simplified to:

func FindFactory(name string) (f Factory) { if err := feature.Registry.Lookup(Namespace, name, &f); err != nil { // log error? } return }

Using reflaction based function calling support, we could even implement an 'Instantiate/Invoke' method in the registry (maybe too much magic, though).

I think I would prefer to stay away from reflexion as much as possible in the context of plugin, this force user to make the choice.

I've created a private bundleable interface implemented by both Feature and Bundle.

I've kept the Feature and Bundle type public, since we will interact with it for creating new Features and filtering / merging bundle.

ph · 2018-06-26T12:30:58Z

@ruflin Nothing prevents us to use code generation to generate an include with the new format, there is a point in the description for Update the auto generation tools

ph · 2018-06-26T17:18:10Z

I've moved the TODO list to a meta issue at #7423

ph added 2 commits June 21, 2018 12:04

Example of backward compatibility for the mem/spool queues

ad6adc0

ph added review libbeat labels Jun 21, 2018

ph changed the title ~~Introduce a Global Registry~~ Refactor: Introduce a Global Registry Jun 21, 2018

allow to filter on multiple stabilities

2f46cf3

use a reference

2dfac4f

ph mentioned this pull request Jun 21, 2018

Migrate processor to the new global registry #7396

Closed

ph commented Jun 22, 2018

View reviewed changes

urso reviewed Jun 25, 2018

View reviewed changes

ph added 4 commits June 26, 2018 09:34

Stability extracts into his own file.

fdb5243

Review first round.

a565de2

uses lookup and lookupAll instead

d3bea98

adding developer changelog

10c0f86

urso approved these changes Jun 26, 2018

View reviewed changes

use a variadic function

0665439

urso merged commit e380bf9 into elastic:master Jun 26, 2018

ph mentioned this pull request Jun 26, 2018

[Meta] Migrate Libbeat plugins to the Global registry #7423

Closed

13 tasks

This was referenced Aug 1, 2018

[WIP] Migrate to the new registry #7803

Closed

[Meta] Add FunctionBeat to run beat on serverless infrastructure #7880

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: Introduce a Global Registry #7392

Refactor: Introduce a Global Registry #7392

ph commented Jun 21, 2018 •

edited

Loading

ph commented Jun 21, 2018 •

edited

Loading

ph commented Jun 22, 2018

ph Jun 22, 2018

ph Jun 22, 2018

ruflin commented Jun 25, 2018

urso Jun 25, 2018

ph Jun 26, 2018

urso Jun 25, 2018

urso Jun 25, 2018

urso Jun 25, 2018

urso Jun 25, 2018 •

edited

Loading

ph Jun 26, 2018

urso Jun 25, 2018

urso Jun 25, 2018 •

edited

Loading

urso Jun 25, 2018

ph Jun 26, 2018

urso Jun 25, 2018

ph Jun 26, 2018

ph Jun 26, 2018 •

edited

Loading

ph commented Jun 26, 2018

ph commented Jun 26, 2018


		//go:generate stringer -type=Stability

		// Registry is the global plugin registry, this variable is mean to be temporary to move all the

Refactor: Introduce a Global Registry #7392

Refactor: Introduce a Global Registry #7392

Conversation

ph commented Jun 21, 2018 • edited Loading

ph commented Jun 21, 2018 • edited Loading

ph commented Jun 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin commented Jun 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

urso Jun 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

urso Jun 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ph Jun 26, 2018 • edited Loading

Choose a reason for hiding this comment

ph commented Jun 26, 2018

ph commented Jun 26, 2018

ph commented Jun 21, 2018 •

edited

Loading

ph commented Jun 21, 2018 •

edited

Loading

urso Jun 25, 2018 •

edited

Loading

urso Jun 25, 2018 •

edited

Loading

ph Jun 26, 2018 •

edited

Loading