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.

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

Add secondary source of modules to Metricbeat to read light modules #12465

Merged
merged 40 commits into from
Jun 24, 2019
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
c100145
Add light modules registry
jsoriano May 24, 2019
8ba9a38
Revert changes in current registry
jsoriano May 31, 2019
758afb2
Add light modules to registry
jsoriano May 31, 2019
0a6b190
Correctly override defaults
jsoriano Jun 1, 2019
c181e51
Add tests
jsoriano Jun 2, 2019
4ceab7b
Refactor module parsing
jsoriano Jun 3, 2019
3a1768e
Add Light modules support as a metricbeat option
jsoriano Jun 3, 2019
5f85188
Refactor metricset config reader for coherence with module config reader
jsoriano Jun 6, 2019
e23df08
Call module factories if registered
jsoriano Jun 6, 2019
c138347
Refactor base module creation for better error handling
jsoriano Jun 6, 2019
ae9c7e2
Add godocs
jsoriano Jun 6, 2019
d64ccb1
Add changelog entry
jsoriano Jun 6, 2019
b53acb3
Rename child register to secondary source and read to load
jsoriano Jun 7, 2019
1e9272c
Rename some internal variables to more meaningful names
jsoriano Jun 7, 2019
a25ec8c
Yet another refactor to simplify signatures
jsoriano Jun 7, 2019
0135353
Explictly check for module presence
jsoriano Jun 7, 2019
8b8521a
Recover test for existence of module
jsoriano Jun 7, 2019
29c55b2
Add context to some errors
jsoriano Jun 7, 2019
8652a0c
Fix beater
jsoriano Jun 7, 2019
db6d303
Remove references to child registry
jsoriano Jun 7, 2019
ae9e00f
Add PR reference to the changelog
jsoriano Jun 7, 2019
3f3d046
Fix test
jsoriano Jun 7, 2019
6af6437
Move light modules to x-pack
jsoriano Jun 12, 2019
af38b6b
Make hound CI happy
jsoriano Jun 12, 2019
6f38790
Make metricbeat modules test work with licensed metricbeat
jsoriano Jun 12, 2019
2af1d4c
Add missing license header
jsoriano Jun 12, 2019
49f5648
Merge remote-tracking branch 'origin/master' into metricbeat-light-mo…
jsoriano Jun 12, 2019
8722e1a
Add some sanity tests for the light metric set
jsoriano Jun 12, 2019
6bfb015
Add check for error in test
jsoriano Jun 12, 2019
53bbabd
Rename x-pack creator
jsoriano Jun 13, 2019
a70c415
Typo that caused failing test
jsoriano Jun 13, 2019
9a6c336
Add suggestions from review
jsoriano Jun 14, 2019
edcd41b
Merge remote-tracking branch 'origin/master' into metricbeat-light-mo…
jsoriano Jun 14, 2019
38ee770
Properly add the test options to x-pack metricbeat command
jsoriano Jun 14, 2019
724f41a
Use secondary source stringer to print light modules
jsoriano Jun 20, 2019
84cba87
Move metricbeat creator definitions to where they are used
jsoriano Jun 21, 2019
9e2cdb8
Add module names to base module override error
jsoriano Jun 21, 2019
d701bff
Move test cases to map with titles as key
jsoriano Jun 21, 2019
b3dbfda
Fix unsafe base module config override
jsoriano Jun 21, 2019
f175eb5
Add test for custom period
jsoriano Jun 21, 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
1 change: 1 addition & 0 deletions CHANGELOG.next.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d
- Always report Pod UID in the `pod` metricset. {pull}12345[12345]
- Add Vsphere Virtual Machine operating system to `os` field in Vsphere virtualmachine module. {pull}12391[12391]
- Add validation for elasticsearch and kibana modules' metricsets when xpack.enabled is set to true. {pull}12386[12386]
- Add support for metricbeat modules based on existing modules (a.k.a. light modules) {issue}12270[12270] {pull}12465[12465]
- Add a system/entropy metricset {pull}12450[12450]

*Packetbeat*
Expand Down
6 changes: 3 additions & 3 deletions metricbeat/beater/metricbeat.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,6 @@ func DefaultCreator() beat.Creator {

// newMetricbeat creates and returns a new Metricbeat instance.
func newMetricbeat(b *beat.Beat, c *common.Config, options ...Option) (*Metricbeat, error) {
// List all registered modules and metricsets.
logp.Debug("modules", "%s", mb.Registry.String())

config := defaultConfig
if err := c.Unpack(&config); err != nil {
return nil, errors.Wrap(err, "error reading configuration file")
Expand All @@ -120,6 +117,9 @@ func newMetricbeat(b *beat.Beat, c *common.Config, options ...Option) (*Metricbe
applyOption(metricbeat)
}

// List all registered modules and metricsets.
logp.Debug("modules", "%s", mb.Registry.String())
jsoriano marked this conversation as resolved.
Show resolved Hide resolved

if b.InSetupCmd {
// Return without instantiating the metricsets.
return metricbeat, nil
Expand Down
3 changes: 2 additions & 1 deletion metricbeat/cmd/modules.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ import (
"github.com/elastic/beats/libbeat/cmd"
)

func buildModulesManager(beat *beat.Beat) (cmd.ModulesManager, error) {
// BuildModulesManager adds support for modules management to a beat
func BuildModulesManager(beat *beat.Beat) (cmd.ModulesManager, error) {
jsoriano marked this conversation as resolved.
Show resolved Hide resolved
jsoriano marked this conversation as resolved.
Show resolved Hide resolved
jsoriano marked this conversation as resolved.
Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

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

nit: naming. cmd.ModulesManager is somewhat generic. Consider naming the constructor based on what is going to be constructed/initialized. For example NewLightweightModulesManager?

Copy link
Member Author

Choose a reason for hiding this comment

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

The only change made here is to make this method public so it can be used from x-pack, cmd.ModulesManager is the generic manager for modules subcommand.

Copy link

Choose a reason for hiding this comment

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

Yeah, I noticed this symbol becoming public here. But with it becoming public, the descriptive nature of the names used is even more important (a.k.a naming is hard).

config := beat.BeatConfig

glob, err := config.String("config.modules.path", -1)
Expand Down
4 changes: 2 additions & 2 deletions metricbeat/cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,6 @@ func init() {
var runFlags = pflag.NewFlagSet(Name, pflag.ExitOnError)
runFlags.AddGoFlag(flag.CommandLine.Lookup("system.hostfs"))
RootCmd = cmd.GenRootCmdWithSettings(beater.DefaultCreator(), instance.Settings{RunFlags: runFlags, Name: Name})
RootCmd.AddCommand(cmd.GenModulesCmd(Name, "", buildModulesManager))
RootCmd.TestCmd.AddCommand(test.GenTestModulesCmd(Name, ""))
RootCmd.AddCommand(cmd.GenModulesCmd(Name, "", BuildModulesManager))
RootCmd.TestCmd.AddCommand(test.GenTestModulesCmd(Name, "", test.BeatCreator()))
}
25 changes: 15 additions & 10 deletions metricbeat/cmd/test/modules.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,27 @@ import (

"github.com/spf13/cobra"

"github.com/elastic/beats/libbeat/beat"
"github.com/elastic/beats/libbeat/cmd/instance"
"github.com/elastic/beats/libbeat/testing"
"github.com/elastic/beats/metricbeat/beater"
"github.com/elastic/beats/metricbeat/mb/module"
)

func GenTestModulesCmd(name, beatVersion string) *cobra.Command {
// BeatCreator creates a customized instance of Metricbeat for the modules test subcommand
func BeatCreator() beat.Creator {
jsoriano marked this conversation as resolved.
Show resolved Hide resolved
jsoriano marked this conversation as resolved.
Show resolved Hide resolved
jsoriano marked this conversation as resolved.
Show resolved Hide resolved
// Use a customized instance of Metricbeat where startup delay has
// been disabled to workaround the fact that Modules() will return
// the static modules (not the dynamic ones) with a start delay.
return beater.Creator(
beater.WithModuleOptions(
module.WithMetricSetInfo(),
module.WithMaxStartDelay(0),
),
)
}

func GenTestModulesCmd(name, beatVersion string, create beat.Creator) *cobra.Command {
jsoriano marked this conversation as resolved.
Show resolved Hide resolved
jsoriano marked this conversation as resolved.
Show resolved Hide resolved
return &cobra.Command{
Use: "modules [module] [metricset]",
Short: "Test modules settings",
Expand All @@ -49,15 +63,6 @@ func GenTestModulesCmd(name, beatVersion string) *cobra.Command {
os.Exit(1)
}

// Use a customized instance of Metricbeat where startup delay has
// been disabled to workaround the fact that Modules() will return
// the static modules (not the dynamic ones) with a start delay.
create := beater.Creator(
beater.WithModuleOptions(
module.WithMetricSetInfo(),
module.WithMaxStartDelay(0),
),
)
mb, err := create(&b.Beat, b.Beat.BeatConfig)
if err != nil {
fmt.Fprintf(os.Stderr, "Error initializing metricbeat: %s\n", err)
Expand Down
118 changes: 118 additions & 0 deletions metricbeat/mb/lightmetricset.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
// Licensed to Elasticsearch B.V. under one or more contributor
// license agreements. See the NOTICE file distributed with
// this work for additional information regarding copyright
// ownership. Elasticsearch B.V. licenses this file to you under
// the Apache License, Version 2.0 (the "License"); you may
// not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing,
// software distributed under the License is distributed on an
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.

package mb

import (
"github.com/pkg/errors"

"github.com/elastic/beats/libbeat/common"
)

// LightMetricSet contains the definition of a non-registered metric set
type LightMetricSet struct {
Name string
Module string
Default bool `config:"default"`
Input struct {
Module string `config:"module" validate:"required"`
MetricSet string `config:"metricset" validate:"required"`
Defaults interface{} `config:"defaults"`
jsoriano marked this conversation as resolved.
Show resolved Hide resolved
} `config:"input" validate:"required"`
}

// Registration obtains a metric set registration for this light metric set, this registration
// contains a metric set factory that reprocess metric set creation taking into account the
// light metric set defaults
func (m *LightMetricSet) Registration(r *Register) (MetricSetRegistration, error) {
registration, err := r.metricSetRegistration(m.Input.Module, m.Input.MetricSet)
if err != nil {
return registration, errors.Wrapf(err,
"failed to start light metricset '%s/%s' using '%s/%s' metricset as input",
m.Module, m.Name,
m.Input.Module, m.Input.MetricSet)
}

originalFactory := registration.Factory
registration.IsDefault = m.Default

// Light modules factory has to override defaults and reproduce builder
// functionality with the resulting configuration, it does:
// - Override defaults
// - Call module factory if registered (it wouldn't have been called
// if light module is really a registered mixed module)
// - Call host parser if defined (it would have already been called
// without the light module defaults)
// - Finally, call the original factory for the registered metricset
registration.Factory = func(base BaseMetricSet) (MetricSet, error) {
// Override default config on base module and metricset
base.name = m.Name
baseModule, err := m.baseModule(base.module)
if err != nil {
return nil, errors.Wrap(err, "failed to create base module for light module")
jsoriano marked this conversation as resolved.
Show resolved Hide resolved
}
base.module = baseModule

// Run module factory if registered, it will be called once per
// metricset, but it should be idempotent
moduleFactory := r.moduleFactory(m.Input.Module)
if moduleFactory != nil {
module, err := moduleFactory(*baseModule)
if err != nil {
return nil, errors.Wrapf(err, "module factory for module '%s' failed while creating light metricset '%s/%s'", m.Input.Module, m.Module, m.Name)
}
base.module = module
}

// At this point host parser was already run, we need to run this again
// with the overriden defaults
if registration.HostParser != nil {
base.hostData, err = registration.HostParser(base.module, base.host)
if err != nil {
return nil, errors.Wrapf(err, "host parser failed on light metricset factory for '%s/%s'", m.Module, m.Name)
}
base.host = base.hostData.Host
}

return originalFactory(base)
}

return registration, nil
}

// baseModule does the configuration overrides in the base module configuration
// taking into account the light metric set default configurations
func (m *LightMetricSet) baseModule(from Module) (*BaseModule, error) {
baseModule := BaseModule{
name: m.Module,
config: from.Config(),
Copy link

Choose a reason for hiding this comment

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

I wonder if this is a save operation. Are we guaranteed to always get a copy when calling Config? In ModuleConfig the fields Hosts, MetricSets, and Query are not values, but shared references. The values can be replaced by new instances, but must not be modified (e.g. operations like config.Query["abc"] = X are forbidden).

We call 'UnpackConfigwith targetconfig` a few lines down from here. Could this call overwrite entries in Hosts, MetricSets and Query?

What is the expected result after UnpackConfig if Hosts/MetricSets/Query was not empty?

Copy link
Member Author

Choose a reason for hiding this comment

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

It worked as expected on my tests, but yes, it doesn't look safe, I will try to get a better copy of this module config.

We call 'UnpackConfigwith targetconfig` a few lines down from here. Could this call overwrite entries in Hosts, MetricSets and Query?

What is the expected result after UnpackConfig if Hosts/MetricSets/Query was not empty?

Hosts and MetricSets shouldn't be changed here, query could be, in this case the map should be merged. It may require more tests to cover these cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was not safe, and this was actually not needed as we copy from the raw config later. I have extended the tests to check that original base module is not modified, and that we apply the overrides in the case of query.

}
var err error
// Set defaults
if baseModule.rawConfig, err = common.NewConfigFrom(m.Input.Defaults); err != nil {
return nil, errors.Wrap(err, "invalid input defaults")
}
// Copy values from user configuration
if err = from.UnpackConfig(baseModule.rawConfig); err != nil {
return nil, errors.Wrap(err, "failed to copy values from user configuration")
}
// Update module configuration
if err = baseModule.UnpackConfig(&baseModule.config); err != nil {
return nil, errors.Wrap(err, "failed to set module configuration")
}
return &baseModule, nil
}
126 changes: 126 additions & 0 deletions metricbeat/mb/lightmetricset_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
// Licensed to Elasticsearch B.V. under one or more contributor
// license agreements. See the NOTICE file distributed with
// this work for additional information regarding copyright
// ownership. Elasticsearch B.V. licenses this file to you under
// the Apache License, Version 2.0 (the "License"); you may
// not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing,
// software distributed under the License is distributed on an
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.

package mb

import (
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/elastic/beats/libbeat/common"
)

func TestLightMetricSetRegistration(t *testing.T) {
cases := []struct {
title string
module string
metricSet string
isDefault bool
fail bool
}{
{
title: "metricset is registered",
module: "foo",
metricSet: "bar",
fail: false,
},
{
title: "metricset is registered and is default",
module: "foo",
metricSet: "bar",
isDefault: true,
fail: false,
},
{
title: "module is not registered",
module: "notexists",
metricSet: "notexists",
fail: true,
},
{
title: "metricset is not registered",
module: "foo",
metricSet: "notexists",
fail: true,
},
}

fakeMetricSetFactory = func(b BaseMetricSet) (MetricSet, error) { return &b, nil }

moduleName := "foo"
metricSetName := "bar"
lightMetricSetName := "metricset"
lightModuleName := "module"

r := NewRegister()
r.MustAddMetricSet(moduleName, metricSetName, fakeMetricSetFactory)

for _, c := range cases {
t.Run(c.title, func(t *testing.T) {
ms := LightMetricSet{
Name: lightMetricSetName,
Module: lightModuleName,
Default: c.isDefault,
}
ms.Input.Module = c.module
ms.Input.MetricSet = c.metricSet

registration, err := ms.Registration(r)
if c.fail {
assert.Error(t, err)
return
}
require.NoError(t, err)

// Check that registration has the light metricset settings
assert.Equal(t, c.metricSet, registration.Name)
assert.Equal(t, c.isDefault, registration.IsDefault)

// Check that calling the factory with a registered base module
// does the proper overrides in the resulting metricset
bm := baseModule(t, r, moduleName, metricSetName)
metricSet, err := registration.Factory(bm)
require.NoError(t, err)
require.NotNil(t, metricSet)

assert.Equal(t, lightModuleName, metricSet.Module().Name())
assert.Equal(t, lightMetricSetName, metricSet.Name())
})
}
}

func baseModule(t *testing.T, r *Register, module, metricSet string) BaseMetricSet {
origRegistration, err := r.metricSetRegistration(module, metricSet)
require.NoError(t, err)

c := DefaultModuleConfig()
c.Module = module
c.MetricSets = []string{metricSet}
raw, err := common.NewConfigFrom(c)
require.NoError(t, err)
baseModule, err := newBaseModuleFromConfig(raw)
require.NoError(t, err)

bm := BaseMetricSet{
name: "bar",
module: &baseModule,
registration: origRegistration,
}
return bm
}
Loading