From 10da237d9b60a246662c512d91f241d2551d956e Mon Sep 17 00:00:00 2001 From: Jaime Soriano Pastor Date: Tue, 3 Mar 2020 21:34:48 +0100 Subject: [PATCH] Fix and extend logging of light modules load errors (#14706) * Fix and extend logging of light modules load errors * Add BEAT_STRICT_PERMS to beat container in metricbeat docker compose * Add changelog * Use local logger, and fix error messages * Ignore errors if module directory doesn't exist * Fix issue when modules directory contains regular files * Remove empty line * Add test for issue with files listed as modules --- CHANGELOG.next.asciidoc | 1 + metricbeat/mb/lightmodules.go | 47 ++++++++++++------- metricbeat/mb/lightmodules_test.go | 20 ++++++++ .../mb/testdata/lightmodules/regular_file | 0 metricbeat/module/ceph/test_ceph.py | 2 +- 5 files changed, 53 insertions(+), 17 deletions(-) create mode 100644 metricbeat/mb/testdata/lightmodules/regular_file diff --git a/CHANGELOG.next.asciidoc b/CHANGELOG.next.asciidoc index 7b5766aa0f6..ce57fb2d845 100644 --- a/CHANGELOG.next.asciidoc +++ b/CHANGELOG.next.asciidoc @@ -105,6 +105,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d - Made `logstash-xpack` module once again have parity with internally-collected Logstash monitoring data. {pull}16198[16198] - Change sqs metricset to use average as statistic method. {pull}16438[16438] - Revert changes in `docker` module: add size flag to docker.container. {pull}16600[16600] +- Fix detection and logging of some error cases with light modules. {pull}14706[14706] - Fix imports after PR was merged before rebase. {pull}16756[16756] - Add dashboard for `redisenterprise` module. {pull}16752[16752] diff --git a/metricbeat/mb/lightmodules.go b/metricbeat/mb/lightmodules.go index bffbfbe026e..a7c141bd834 100644 --- a/metricbeat/mb/lightmodules.go +++ b/metricbeat/mb/lightmodules.go @@ -39,12 +39,14 @@ const ( // LightModulesSource loads module definitions from files in the provided paths type LightModulesSource struct { paths []string + log *logp.Logger } // NewLightModulesSource creates a new LightModulesSource func NewLightModulesSource(paths ...string) *LightModulesSource { return &LightModulesSource{ paths: paths, + log: logp.NewLogger("registry.lightmodules"), } } @@ -57,7 +59,7 @@ func (s *LightModulesSource) Modules() ([]string, error) { func (s *LightModulesSource) HasModule(moduleName string) bool { names, err := s.moduleNames() if err != nil { - logp.Error(errors.Wrap(err, "failed to get list of light module names")) + s.log.Errorf("Failed to get list of light module names: %v", err) return false } for _, name := range names { @@ -72,7 +74,7 @@ func (s *LightModulesSource) HasModule(moduleName string) bool { 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) + return nil, errors.Wrapf(err, "getting default metricsets for module '%s'", moduleName) } var metricsets []string for _, ms := range module.MetricSets { @@ -87,7 +89,7 @@ func (s *LightModulesSource) DefaultMetricSets(r *Register, moduleName string) ( 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) + return nil, errors.Wrapf(err, "getting metricsets for module '%s'", moduleName) } metricsets := make([]string, 0, len(module.MetricSets)) for _, ms := range module.MetricSets { @@ -105,7 +107,7 @@ func (s *LightModulesSource) HasMetricSet(moduleName, metricSetName string) bool moduleConfig, err := s.loadModuleConfig(modulePath) if err != nil { - logp.Error(errors.Wrapf(err, "failed to load module config for module '%s'", moduleName)) + s.log.Errorf("Failed to load module config for module '%s': %v", moduleName, err) return false } @@ -121,7 +123,7 @@ func (s *LightModulesSource) HasMetricSet(moduleName, metricSetName string) bool func (s *LightModulesSource) MetricSetRegistration(register *Register, moduleName, metricSetName string) (MetricSetRegistration, error) { lightModule, err := s.loadModule(register, moduleName) if err != nil { - return MetricSetRegistration{}, errors.Wrapf(err, "failed to load module '%s'", moduleName) + return MetricSetRegistration{}, errors.Wrapf(err, "loading module '%s'", moduleName) } ms, found := lightModule.MetricSets[metricSetName] @@ -135,9 +137,15 @@ func (s *LightModulesSource) MetricSetRegistration(register *Register, moduleNam // ModulesInfo returns a string representation of this source, with a list of known metricsets func (s *LightModulesSource) ModulesInfo(r *Register) string { var metricSets []string - modules, _ := s.Modules() + modules, err := s.Modules() + if err != nil { + s.log.Errorf("Failed to list modules: %s", err) + } for _, module := range modules { - moduleMetricSets, _ := s.MetricSets(r, module) + moduleMetricSets, err := s.MetricSets(r, module) + if err != nil { + s.log.Errorf("Failed to list light metricsets for module %s: %v", module, err) + } for _, name := range moduleMetricSets { metricSets = append(metricSets, fmt.Sprintf("%s/%s", module, name)) } @@ -155,7 +163,7 @@ type lightModuleConfig struct { func (s *LightModulesSource) ProcessorsForMetricSet(r *Register, moduleName string, metricSetName string) (*processors.Processors, error) { module, err := s.loadModule(r, moduleName) if err != nil { - return nil, errors.Wrapf(err, "reading processors for metricset '%s' in module '%s' failed", metricSetName, moduleName) + return nil, errors.Wrapf(err, "reading processors for metricset '%s' in module '%s'", metricSetName, moduleName) } metricSet, ok := module.MetricSets[metricSetName] if !ok { @@ -178,12 +186,12 @@ func (s *LightModulesSource) loadModule(register *Register, moduleName string) ( moduleConfig, err := s.loadModuleConfig(modulePath) if err != nil { - return nil, errors.Wrapf(err, "failed to load light module '%s' definition", moduleName) + return nil, errors.Wrapf(err, "loading light module '%s' definition", moduleName) } 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) + return nil, errors.Wrapf(err, "loading metric sets for light module '%s'", moduleName) } return &LightModule{Name: moduleName, MetricSets: metricSets}, nil @@ -202,12 +210,12 @@ func (s *LightModulesSource) findModulePath(moduleName string) (string, bool) { func (s *LightModulesSource) loadModuleConfig(modulePath string) (*lightModuleConfig, error) { config, err := common.LoadFile(modulePath) if err != nil { - return nil, errors.Wrapf(err, "failed to load module configuration from '%s'", modulePath) + return nil, errors.Wrapf(err, "loading module configuration from '%s'", modulePath) } var moduleConfig lightModuleConfig if err = config.Unpack(&moduleConfig); err != nil { - return nil, errors.Wrapf(err, "failed to parse light module definition from '%s'", modulePath) + return nil, errors.Wrapf(err, "parsing light module definition from '%s'", modulePath) } return &moduleConfig, nil } @@ -225,7 +233,7 @@ func (s *LightModulesSource) loadMetricSets(register *Register, moduleDirPath, m metricSetConfig, err := s.loadMetricSetConfig(manifestPath) if err != nil { - return nil, errors.Wrapf(err, "failed to load light metricset '%s'", metricSet) + return nil, errors.Wrapf(err, "loading light metricset '%s'", metricSet) } metricSetConfig.Name = metricSet metricSetConfig.Module = moduleName @@ -238,11 +246,11 @@ func (s *LightModulesSource) loadMetricSets(register *Register, moduleDirPath, m func (s *LightModulesSource) loadMetricSetConfig(manifestPath string) (ms LightMetricSet, err error) { config, err := common.LoadFile(manifestPath) if err != nil { - return ms, errors.Wrapf(err, "failed to load metricset manifest from '%s'", manifestPath) + return ms, errors.Wrapf(err, "loading metricset manifest from '%s'", manifestPath) } if err := config.Unpack(&ms); err != nil { - return ms, errors.Wrapf(err, "failed to parse metricset manifest from '%s'", manifestPath) + return ms, errors.Wrapf(err, "parsing metricset manifest from '%s'", manifestPath) } return } @@ -250,11 +258,18 @@ func (s *LightModulesSource) loadMetricSetConfig(manifestPath string) (ms LightM func (s *LightModulesSource) moduleNames() ([]string, error) { modules := make(map[string]bool) for _, dir := range s.paths { + if _, err := os.Stat(dir); os.IsNotExist(err) { + s.log.Debugf("Light modules directory '%d' doesn't exist", dir) + continue + } files, err := ioutil.ReadDir(dir) if err != nil { - return nil, errors.Wrapf(err, "failed to list modules on path '%s'", dir) + return nil, errors.Wrapf(err, "listing modules on path '%s'", dir) } for _, f := range files { + if !f.IsDir() { + continue + } modulePath := filepath.Join(dir, f.Name(), moduleYML) if _, err := os.Stat(modulePath); os.IsNotExist(err) { continue diff --git a/metricbeat/mb/lightmodules_test.go b/metricbeat/mb/lightmodules_test.go index 07a4ceeb54f..389cc2433c6 100644 --- a/metricbeat/mb/lightmodules_test.go +++ b/metricbeat/mb/lightmodules_test.go @@ -425,6 +425,26 @@ func TestProcessorsForMetricSet_ProcessorsRead(t *testing.T) { require.Len(t, procs.List, 1) } +func TestProcessorsForMetricSet_ListModules(t *testing.T) { + source := NewLightModulesSource("testdata/lightmodules") + modules, err := source.Modules() + require.NoError(t, err) + + // Check that regular file in directory is not listed as module + require.FileExists(t, "testdata/lightmodules/regular_file") + assert.NotContains(t, modules, "regular_file") + + expectedModules := []string{ + "broken", + "httpextended", + "mixed", + "mixedbroken", + "service", + "unpack", + } + assert.ElementsMatch(t, expectedModules, modules, "Modules found: %v", modules) +} + type metricSetWithOption struct { BaseMetricSet Option string diff --git a/metricbeat/mb/testdata/lightmodules/regular_file b/metricbeat/mb/testdata/lightmodules/regular_file new file mode 100644 index 00000000000..e69de29bb2d diff --git a/metricbeat/module/ceph/test_ceph.py b/metricbeat/module/ceph/test_ceph.py index 9fe207e6d65..9f9c70561f5 100644 --- a/metricbeat/module/ceph/test_ceph.py +++ b/metricbeat/module/ceph/test_ceph.py @@ -51,7 +51,7 @@ def test_ceph_mgr(self, metricset): return self.render_config_template(modules=[self.get_ceph_mgr_module_config(metricset)]) - proc = self.start_beat(home=self.beat_path) + proc = self.start_beat() self.wait_until(lambda: self.output_lines() > 0) proc.check_kill_and_wait() self.assert_no_logged_warnings(replace=['SSL/TLS verifications disabled.'])