Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Metricbeat] Fix: mixed modules fail loading standard and light metricsets #15011

Merged
merged 6 commits into from
Dec 11, 2019
Merged

[Metricbeat] Fix: mixed modules fail loading standard and light metricsets #15011

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c18ea8e
Prepare test case for mixed module
Dec 9, 2019
08b3629
Ignore registered modules while loading light metricsets
Dec 9, 2019
b1f5b7b
Test case: fail on unregistered module
Dec 9, 2019
746a9df
Fix: mage fmt
Dec 9, 2019
ca533a7
Fix: replace String with ...Info
Dec 9, 2019
84c4636
Rename method to ModulesInfo()
Dec 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 17 additions & 11 deletions metricbeat/mb/lightmodules.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ func (s *LightModulesSource) HasModule(moduleName string) bool {
}

// DefaultMetricSets list the default metricsets for a given module
func (s *LightModulesSource) DefaultMetricSets(moduleName string) ([]string, error) {
module, err := s.loadModule(moduleName)
func (s *LightModulesSource) DefaultMetricSets(r *Register, moduleName string) ([]string, error) {
module, err := s.loadModule(r, moduleName)
if err != nil {
return nil, errors.Wrapf(err, "failed to get default metricsets for module '%s'", moduleName)
}
Expand All @@ -83,8 +83,8 @@ func (s *LightModulesSource) DefaultMetricSets(moduleName string) ([]string, err
}

// MetricSets list the available metricsets for a given module
func (s *LightModulesSource) MetricSets(moduleName string) ([]string, error) {
module, err := s.loadModule(moduleName)
func (s *LightModulesSource) MetricSets(r *Register, moduleName string) ([]string, error) {
module, err := s.loadModule(r, moduleName)
if err != nil {
return nil, errors.Wrapf(err, "failed to get metricsets for module '%s'", moduleName)
}
Expand Down Expand Up @@ -118,7 +118,7 @@ func (s *LightModulesSource) HasMetricSet(moduleName, metricSetName string) bool

// MetricSetRegistration obtains a registration for a light metric set
func (s *LightModulesSource) MetricSetRegistration(register *Register, moduleName, metricSetName string) (MetricSetRegistration, error) {
lightModule, err := s.loadModule(moduleName)
lightModule, err := s.loadModule(register, moduleName)
if err != nil {
return MetricSetRegistration{}, errors.Wrapf(err, "failed to load module '%s'", moduleName)
}
Expand All @@ -131,12 +131,12 @@ func (s *LightModulesSource) MetricSetRegistration(register *Register, moduleNam
return ms.Registration(register)
}

// String returns a string representation of this source, with a list of known metricsets
func (s *LightModulesSource) String() string {
// LightModulesInfo returns a string representation of this source, with a list of known metricsets
func (s *LightModulesSource) LightModulesInfo(r *Register) string {
var metricSets []string
modules, _ := s.Modules()
for _, module := range modules {
moduleMetricSets, _ := s.MetricSets(module)
moduleMetricSets, _ := s.MetricSets(r, module)
for _, name := range moduleMetricSets {
metricSets = append(metricSets, fmt.Sprintf("%s/%s", module, name))
}
Expand All @@ -156,7 +156,7 @@ type LightModule struct {
MetricSets map[string]LightMetricSet
}

func (s *LightModulesSource) loadModule(moduleName string) (*LightModule, error) {
func (s *LightModulesSource) loadModule(register *Register, moduleName string) (*LightModule, error) {
modulePath, found := s.findModulePath(moduleName)
if !found {
return nil, fmt.Errorf("module '%s' not found", moduleName)
Expand All @@ -167,7 +167,7 @@ func (s *LightModulesSource) loadModule(moduleName string) (*LightModule, error)
return nil, errors.Wrapf(err, "failed to load light module '%s' definition", moduleName)
}

metricSets, err := s.loadMetricSets(filepath.Dir(modulePath), moduleConfig.Name, moduleConfig.MetricSets)
metricSets, err := s.loadMetricSets(register, filepath.Dir(modulePath), moduleConfig.Name, moduleConfig.MetricSets)
if err != nil {
return nil, errors.Wrapf(err, "failed to load metric sets for light module '%s'", moduleName)
}
Expand Down Expand Up @@ -198,9 +198,15 @@ func (s *LightModulesSource) loadModuleConfig(modulePath string) (*lightModuleCo
return &moduleConfig, nil
}

func (s *LightModulesSource) loadMetricSets(moduleDirPath, moduleName string, metricSetNames []string) (map[string]LightMetricSet, error) {
func (s *LightModulesSource) loadMetricSets(register *Register, moduleDirPath, moduleName string, metricSetNames []string) (map[string]LightMetricSet, error) {
metricSets := make(map[string]LightMetricSet)
for _, metricSet := range metricSetNames {
if moduleMetricSets, exists := register.metricSets[moduleName]; exists {
if _, exists := moduleMetricSets[metricSet]; exists {
continue
}
}

manifestPath := filepath.Join(moduleDirPath, metricSet, manifestYML)

metricSetConfig, err := s.loadMetricSetConfig(manifestPath)
Expand Down
13 changes: 12 additions & 1 deletion metricbeat/mb/lightmodules_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,9 +168,10 @@ func TestLoadModule(t *testing.T) {
}

for _, c := range cases {
register := NewRegister()
r := NewLightModulesSource("testdata/lightmodules")
t.Run(c.name, func(t *testing.T) {
_, err := r.loadModule(c.name)
_, err := r.loadModule(register, c.name)
if c.err {
assert.Error(t, err)
}
Expand Down Expand Up @@ -227,10 +228,20 @@ func TestNewModuleFromConfig(t *testing.T) {
config: common.MapStr{"module": "service", "enabled": false},
err: true,
},
"mixed module with standard and light metricsets": {
config: common.MapStr{"module": "mixed", "metricsets": []string{"standard", "light"}},
expectedOption: "default",
},
"mixed module with unregistered and light metricsets": {
config: common.MapStr{"module": "mixedbroken", "metricsets": []string{"unregistered", "light"}},
err: true,
},
}

r := NewRegister()
r.MustAddMetricSet("foo", "bar", newMetricSetWithOption)
r.MustAddMetricSet("foo", "light", newMetricSetWithOption)
r.MustAddMetricSet("mixed", "standard", newMetricSetWithOption)
r.SetSecondarySource(NewLightModulesSource("testdata/lightmodules"))

for title, c := range cases {
Expand Down
12 changes: 6 additions & 6 deletions metricbeat/mb/registry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,11 +116,11 @@ type Register struct {
type ModulesSource interface {
Modules() ([]string, error)
HasModule(module string) bool
MetricSets(module string) ([]string, error)
DefaultMetricSets(module string) ([]string, error)
MetricSets(r *Register, module string) ([]string, error)
DefaultMetricSets(r *Register, module string) ([]string, error)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many methods here start to need the main register, we may need to rethink this interface and the relation between module sources. But no need to address this in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is valid concern. I admit it took me a while to find the best solution for the moment (don't refactor half of the project). I'm afraid that it will be related with heavier refactoring.

HasMetricSet(module, name string) bool
MetricSetRegistration(r *Register, module, name string) (MetricSetRegistration, error)
String() string
LightModulesInfo(r *Register) string
mtojek marked this conversation as resolved.
Show resolved Hide resolved
}

// NewRegister creates and returns a new Register.
Expand Down Expand Up @@ -280,7 +280,7 @@ func (r *Register) DefaultMetricSets(module string) ([]string, error) {
// List also default metrics from secondary sources
if source := r.secondarySource; source != nil && source.HasModule(module) {
exists = true
sourceDefaults, err := source.DefaultMetricSets(module)
sourceDefaults, err := source.DefaultMetricSets(r, module)
if err != nil {
r.log.Errorf("Failed to get default metric sets for module '%s' from secondary source: %s", module, err)
} else if len(sourceDefaults) > 0 {
Expand Down Expand Up @@ -352,7 +352,7 @@ func (r *Register) MetricSets(module string) []string {

// List also metric sets from secondary sources
if source := r.secondarySource; source != nil && source.HasModule(module) {
sourceMetricSets, err := source.MetricSets(module)
sourceMetricSets, err := source.MetricSets(r, module)
if err != nil {
r.log.Errorf("Failed to get metricsets from secondary source: %s", err)
}
Expand Down Expand Up @@ -390,7 +390,7 @@ func (r *Register) String() string {

var secondarySource string
if source := r.secondarySource; source != nil {
secondarySource = ", " + source.String()
secondarySource = fmt.Sprintf(", LightModules:[%s]", source.LightModulesInfo(r))
}

sort.Strings(modules)
Expand Down
4 changes: 4 additions & 0 deletions metricbeat/mb/testdata/lightmodules/mixed/light/manifest.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
default: true
input:
module: foo
metricset: light
4 changes: 4 additions & 0 deletions metricbeat/mb/testdata/lightmodules/mixed/module.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
name: mixed
metricsets:
- light
- standard
Empty file added metricbeat/mb/testdata/lightmodules/mixed/standard/.placeholder
View file
Open in desktop
Empty file.
4 changes: 4 additions & 0 deletions metricbeat/mb/testdata/lightmodules/mixedbroken/light/manifest.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
default: true
input:
module: foo
metricset: light
4 changes: 4 additions & 0 deletions metricbeat/mb/testdata/lightmodules/mixedbroken/module.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
name: mixedbroken
metricsets:
- light
- unregistered
Empty file added metricbeat/mb/testdata/lightmodules/mixedbroken/unregistered/.placeholder
View file
Open in desktop
Empty file.