Require unique names for modules and metricsets. (#1426)

- AddModuler and AddMetricSeter now validate their inputs and return an error if there is any problem with the registration parameters.
- GetModule and GetMetricSet retrieve data from method receiver instead of the singleton Register instance.
- Adding a String() method to Register. Removed include.ListAll().
- Add all modules and metricsets to include/list.go.

metricbeat/beater/metricbeat.go

@@ -30,8 +30,8 @@ import (
 	"fmt"
 
 	"github.com/elastic/beats/libbeat/beat"
+	"github.com/elastic/beats/libbeat/logp"
 	"github.com/elastic/beats/metricbeat/helper"
-	"github.com/elastic/beats/metricbeat/include"
 )
 
 type Metricbeat struct {
@@ -45,15 +45,15 @@ func New() *Metricbeat {
 }
 
 func (mb *Metricbeat) Config(b *beat.Beat) error {
+	// List all registered modules and metricsets.
+	logp.Info("%s", helper.Registry.String())
+
 	mb.config = &Config{}
 	err := b.RawConfig.Unpack(mb.config)
 	if err != nil {
 		return fmt.Errorf("error reading configuration file. %v", err)
 	}
 
-	// List all registered modules and metricsets
-	include.ListAll()
-
 	return nil
 }
 

metricbeat/docs/developer-guide/create-metricset.asciidoc

@@ -43,7 +43,9 @@ import (
 )
 
 func init() {
-	helper.Registry.AddMetricSeter("{module-name}", "{metricset-name}", New)
+	if err := helper.Registry.AddMetricSeter("{module-name}", "{metricset-name}", New); err != nil {
+        panic(err)
+	}
 }
 
 // New creates new instance of MetricSeter
@@ -90,7 +92,9 @@ registry with the given module and makes sure it is run later as expected.
 [source,go]
 ----
 func init() {
-	helper.Registry.AddMetricSeter("{module-name}", "{metricset-name}", New)
+	if err := helper.Registry.AddMetricSeter("{module-name}", "{metricset-name}", New); err != nil {
+        panic(err)
+	}
 }
 ----
 

metricbeat/docs/developer-guide/index.asciidoc

@@ -137,7 +137,9 @@ import (
 )
 
 func init() {
-	helper.Registry.AddModuler("{module-name}", New)
+	if err := helper.Registry.AddModuler("{module-name}", New); err != nil {
+        panic(err)
+	}
 }
 
 // New creates new instance of Moduler
@@ -167,7 +169,9 @@ import (
 )
 
 func init() {
-	helper.Registry.AddMetricSeter("{module-name}", "{metricset-name}", New)
+	if err := helper.Registry.AddMetricSeter("{module-name}", "{metricset-name}", New); err != nil {
+        panic(err)
+	}
 }
 
 // New creates new instance of MetricSeter
@@ -299,7 +303,9 @@ import (
 )
 
 func init() {
-	helper.Registry.AddModuler("{module-name}", New)
+	if err := helper.Registry.AddModuler("{module-name}", New); err != nil {
+        panic(err)
+	}
 }
 
 // New creates new instance of Moduler

metricbeat/helper/register.go

@@ -2,84 +2,133 @@ package helper
 
 import (
 	"fmt"
+	"sort"
+	"strings"
 
 	"github.com/elastic/beats/libbeat/common"
 	"github.com/elastic/beats/libbeat/logp"
 )
 
-// Global register for moduler and metricseter
-// The Register keeps a global list of moduler and metricseter
-// A copy of the moduler or metricset instance can be used to create a module or metricset
-
-// TODO: Global variables should be prevent.
-// 	This should be moved into the metricbeat object but can't because the init()
-//	functions in each metricset are called before the beater object exists.
+// Registry is the singleton Register instance where all modules and metricsets
+// should register their factories.
 var Registry = Register{}
 
+// Register contains the factory functions for creating new Modulers and new
+// MetricSeters.
 type Register struct {
-	Modulers     map[string]func() Moduler
-	MetricSeters map[string]map[string]func() MetricSeter
+	Modulers     map[string]func() Moduler                // A map of module name to Moduler factory function.
+	MetricSeters map[string]map[string]func() MetricSeter // A map of module name to nested map of metricset name to MetricSeter factory function.
 }
 
-// AddModule registers the given module with the registry
-func (r *Register) AddModuler(name string, m func() Moduler) {
-
+// AddModuler registers a new Moduler factory. An error is returned if the
+// name is empty, factory is nil, or if a factory has already been registered
+// under the name.
+func (r *Register) AddModuler(name string, factory func() Moduler) error {
 	if r.Modulers == nil {
 		r.Modulers = map[string]func() Moduler{}
 	}
 
-	logp.Info("Register module: %s", name)
+	if name == "" {
+		return fmt.Errorf("module name is required")
+	}
 
-	r.Modulers[name] = m
-}
+	_, exists := r.Modulers[name]
+	if exists {
+		return fmt.Errorf("module '%s' is already registered", name)
+	}
 
-func (r *Register) AddMetricSeter(module string, name string, new func() MetricSeter) {
+	if factory == nil {
+		return fmt.Errorf("module '%s' cannot be registered with a nil factory", name)
+	}
+
+	r.Modulers[name] = factory
+	logp.Info("Module registered: %s", name)
+	return nil
+}
 
+// AddMetricSeter registers a new MetricSeter factory. An error is returned if
+// any parameter is empty or nil or if a factory has already been registered
+// under the name.
+func (r *Register) AddMetricSeter(module string, name string, factory func() MetricSeter) error {
 	if r.MetricSeters == nil {
 		r.MetricSeters = map[string]map[string]func() MetricSeter{}
 	}
 
-	if _, ok := r.MetricSeters[module]; !ok {
+	if module == "" {
+		return fmt.Errorf("module name is required")
+	}
+
+	if name == "" {
+		return fmt.Errorf("metricset name is required")
+	}
+
+	if metricsets, ok := r.MetricSeters[module]; !ok {
 		r.MetricSeters[module] = map[string]func() MetricSeter{}
+	} else if _, exists := metricsets[name]; exists {
+		return fmt.Errorf("metricset '%s/%s' is already registered", module, name)
 	}
 
-	logp.Info("Register metricset %s for module %s", name, module)
+	if factory == nil {
+		return fmt.Errorf("metricset '%s/%s' cannot be registered with a nil factory", module, name)
+	}
 
-	r.MetricSeters[module][name] = new
+	r.MetricSeters[module][name] = factory
+	logp.Info("metricset registered: %s/%s", module, name)
+	return nil
 }
 
-// GetModule returns a new module instance for the given moduler name
+// GetModule returns a new Module instance for the given moduler name. An
+// error is returned if the module does not exist.
 func (r *Register) GetModule(cfg *common.Config) (*Module, error) {
-
-	// Unpack config to load module name
+	// Unpack config to get the module name.
 	config := struct {
 		Module string `config:"module"`
 	}{}
 	if err := cfg.Unpack(&config); err != nil {
 		return nil, err
 	}
 
-	moduler, ok := Registry.Modulers[config.Module]
+	moduler, ok := r.Modulers[config.Module]
 	if !ok {
-		return nil, fmt.Errorf("Module %s does not exist", config.Module)
+		return nil, fmt.Errorf("module '%s' does not exist", config.Module)
 	}
 
 	return NewModule(cfg, moduler)
 }
 
-// GetMetricSet returns a new metricset instance for the given metricset name combined with the module name
-func (r *Register) GetMetricSet(module *Module, metricsetName string) (*MetricSet, error) {
-
-	if _, ok := Registry.MetricSeters[module.name]; !ok {
-		return nil, fmt.Errorf("Module %s does not exist", module.name)
+// GetMetricSet returns a new MetricSet instance given the module and metricset
+// name. An error is returned if the module or metricset do not exist.
+func (r *Register) GetMetricSet(module *Module, name string) (*MetricSet, error) {
+	metricSets, found := r.MetricSeters[module.name]
+	if !found {
+		return nil, fmt.Errorf("module '%s' does not exist", module.name)
 	}
 
-	if _, ok := Registry.MetricSeters[module.name][metricsetName]; !ok {
-		return nil, fmt.Errorf("Metricset %s in module %s does not exist", metricsetName, module.name)
+	factory, found := metricSets[name]
+	if !found {
+		return nil, fmt.Errorf("metricset '%s/%s' does not exist", module.name, name)
 	}
 
-	newMetricSeter := Registry.MetricSeters[module.name][metricsetName]
+	return NewMetricSet(name, factory, module)
+}
+
+// String return a string representation of the registered modules and
+// metricsets.
+func (r Register) String() string {
+	var modules []string
+	for module := range r.Modulers {
+		modules = append(modules, module)
+	}
+	sort.Strings(modules)
 
-	return NewMetricSet(metricsetName, newMetricSeter, module)
+	var metricSets []string
+	for module, m := range r.MetricSeters {
+		for name := range m {
+			metricSets = append(metricSets, fmt.Sprintf("%s/%s", module, name))
+		}
+	}
+	sort.Strings(metricSets)
 
+	return fmt.Sprintf("Register [Modules:[%s], MetricSets:[%s]]",
+		strings.Join(modules, ", "), strings.Join(metricSets, ", "))
 }

metricbeat/helper/register_test.go

@@ -9,16 +9,138 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func TestGetModuleInvalid(t *testing.T) {
+const (
+	moduleName    = "mymodule"
+	metricSetName = "mymetricset"
+)
+
+func TestAddModulerEmptyName(t *testing.T) {
+	registry := &Register{}
+	err := registry.AddModuler("", func() Moduler { return nil })
+	if assert.Error(t, err) {
+		assert.Equal(t, "module name is required", err.Error())
+	}
+}
+
+func TestAddModulerNilFactory(t *testing.T) {
+	registry := &Register{}
+	err := registry.AddModuler(moduleName, nil)
+	if assert.Error(t, err) {
+		assert.Equal(t, "module 'mymodule' cannot be registered with a nil factory", err.Error())
+	}
+}
+
+func TestAddModulerDuplicateName(t *testing.T) {
+	registry := &Register{}
+	err := registry.AddModuler(moduleName, func() Moduler { return nil })
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	err = registry.AddModuler(moduleName, func() Moduler { return nil })
+	if assert.Error(t, err) {
+		assert.Equal(t, "module 'mymodule' is already registered", err.Error())
+	}
+}
+
+func TestAddModuler(t *testing.T) {
+	registry := &Register{}
+	err := registry.AddModuler(moduleName, func() Moduler { return nil })
+	if err != nil {
+		t.Fatal(err)
+	}
+	factory, found := registry.Modulers[moduleName]
+	assert.True(t, found, "module not found")
+	assert.NotNil(t, factory, "factory fuction is nil")
+}
+
+func TestAddMetricSeterEmptyModuleName(t *testing.T) {
+	registry := &Register{}
+	err := registry.AddMetricSeter("", metricSetName, func() MetricSeter { return nil })
+	if assert.Error(t, err) {
+		assert.Equal(t, "module name is required", err.Error())
+	}
+}
+
+func TestAddMetricSeterEmptyName(t *testing.T) {
+	registry := &Register{}
+	err := registry.AddMetricSeter(moduleName, "", func() MetricSeter { return nil })
+	if assert.Error(t, err) {
+		assert.Equal(t, "metricset name is required", err.Error())
+	}
+}
+
+func TestAddMetricSeterNilFactory(t *testing.T) {
+	registry := &Register{}
+	err := registry.AddMetricSeter(moduleName, metricSetName, nil)
+	if assert.Error(t, err) {
+		assert.Equal(t, "metricset 'mymodule/mymetricset' cannot be registered with a nil factory", err.Error())
+	}
+}
+
+func TestAddMetricSeterDuplicateName(t *testing.T) {
+	registry := &Register{}
+	factory := func() MetricSeter { return nil }
+	err := registry.AddMetricSeter(moduleName, metricSetName, factory)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	err = registry.AddMetricSeter(moduleName, metricSetName, factory)
+	if assert.Error(t, err) {
+		assert.Equal(t, "metricset 'mymodule/mymetricset' is already registered", err.Error())
+	}
+}
+
+func TestAddMetricSeter(t *testing.T) {
+	registry := &Register{}
+	factory := func() MetricSeter { return nil }
+	err := registry.AddMetricSeter(moduleName, metricSetName, factory)
+	if err != nil {
+		t.Fatal(err)
+	}
+	f, found := registry.MetricSeters[moduleName][metricSetName]
+	assert.True(t, found, "metricset not found")
+	assert.NotNil(t, f, "factory fuction is nil")
+}
+
+func TestGetModule(t *testing.T) {
+	registry := Register{
+		Modulers: make(map[string]func() Moduler),
+	}
+	registry.Modulers[moduleName] = func() Moduler { return nil }
+
+	config, _ := common.NewConfigFrom(ModuleConfig{
+		Module: moduleName,
+	})
+	module, err := registry.GetModule(config)
+	if assert.NoError(t, err) {
+		assert.NotNil(t, module)
+	}
+}
 
+func TestGetModuleInvalid(t *testing.T) {
 	config, _ := common.NewConfigFrom(ModuleConfig{
-		Module: "test",
+		Module: moduleName,
 	})
 
 	registry := Register{}
-
 	module, err := registry.GetModule(config)
+	if assert.Error(t, err) {
+		assert.Nil(t, module)
+	}
+}
+
+func TestGetMetricSet(t *testing.T) {
+	registry := &Register{}
+	factory := func() MetricSeter { return nil }
+	err := registry.AddMetricSeter(moduleName, metricSetName, factory)
+	if err != nil {
+		t.Fatal(err)
+	}
 
-	assert.Nil(t, module)
-	assert.Error(t, err)
+	ms, err := registry.GetMetricSet(&Module{name: moduleName}, metricSetName)
+	if assert.NoError(t, err) {
+		assert.NotNil(t, ms)
+	}
 }