Merge pull request #322 from mackerelio/refactor-plugin-configs

Refactor plugin configurations

checks/checker.go

@@ -37,12 +37,8 @@ var exitCodeToStatus = map[int]Status{
 // It invokes its given command and transforms the result to a Report
 // to be sent to Mackerel periodically.
 type Checker struct {
-	Name string
-	// NOTE(motemen): We make use of config.PluginConfig as it happens
-	// to have the Command field which was used by metrics.pluginGenerator.
-	// If the configuration of *checks.Checker and/or metrics.pluginGenerator changes,
-	// we should reconsider using config.PluginConfig.
-	Config *config.PluginConfig
+	Name   string
+	Config *config.CheckPlugin
 }
 
 // Report is what Checker produces by invoking its command.

checks/checker_test.go

@@ -8,13 +8,13 @@ import (
 
 func TestChecker_Check(t *testing.T) {
 	checkerOK := Checker{
-		Config: &config.PluginConfig{
+		Config: &config.CheckPlugin{
 			Command: "go run testdata/exit.go -code 0 -message OK",
 		},
 	}
 
 	checkerWarning := Checker{
-		Config: &config.PluginConfig{
+		Config: &config.CheckPlugin{
 			Command: "go run testdata/exit.go -code 1 -message something_is_going_wrong",
 		},
 	}

checks/checker_unix_test.go

@@ -18,7 +18,7 @@ func TestChecker_CheckTimeout(t *testing.T) {
 	util.TimeoutDuration = 1 * time.Second
 
 	checkerTimeout := Checker{
-		Config: &config.PluginConfig{
+		Config: &config.CheckPlugin{
 			Command: "sleep 2",
 		},
 	}

command/command.go

@@ -123,15 +123,7 @@ func prepareHost(conf *config.Config, api *mackerel.API) (*mackerel.Host, error)
 // configuration of the custom_identifier fields.
 func prepareCustomIdentiferHosts(conf *config.Config, api *mackerel.API) map[string]*mackerel.Host {
 	customIdentifierHosts := make(map[string]*mackerel.Host)
-	customIdentifiers := make(map[string]bool) // use a map to make them unique
-	for _, pluginConfigs := range conf.Plugin {
-		for _, pluginConfig := range pluginConfigs {
-			if pluginConfig.CustomIdentifier != nil {
-				customIdentifiers[*pluginConfig.CustomIdentifier] = true
-			}
-		}
-	}
-	for customIdentifier := range customIdentifiers {
+	for _, customIdentifier := range conf.ListCustomIdentifiers() {
 		host, err := api.FindHostByCustomIdentifier(customIdentifier)
 		if err != nil {
 			logger.Warningf("Failed to retrieve the host of custom_identifier: %s, %s", customIdentifier, err)
@@ -618,7 +610,7 @@ func Run(c *Context, termCh chan struct{}) error {
 func createCheckers(conf *config.Config) []*checks.Checker {
 	checkers := []*checks.Checker{}
 
-	for name, pluginConfig := range conf.Plugin["checks"] {
+	for name, pluginConfig := range conf.CheckPlugins {
 		checker := &checks.Checker{
 			Name:   name,
 			Config: pluginConfig,
@@ -642,7 +634,7 @@ func prepareGenerators(conf *config.Config) []metrics.Generator {
 func pluginGenerators(conf *config.Config) []metrics.PluginGenerator {
 	generators := []metrics.PluginGenerator{}
 
-	for _, pluginConfig := range conf.Plugin["metrics"] {
+	for _, pluginConfig := range conf.MetricPlugins {
 		generators = append(generators, metrics.NewPluginGenerator(pluginConfig))
 	}
 

command/command_unix_test.go

@@ -23,16 +23,14 @@ func TestRunOnce(t *testing.T) {
 	}
 
 	conf := &config.Config{
-		Plugin: map[string]config.PluginConfigs{
-			"metrics": map[string]*config.PluginConfig{
-				"metric1": {
-					Command: diceCommand,
-				},
+		MetricPlugins: map[string]*config.MetricPlugin{
+			"metric1": {
+				Command: diceCommand,
 			},
-			"checks": map[string]*config.PluginConfig{
-				"check1": {
-					Command: "echo 1",
-				},
+		},
+		CheckPlugins: map[string]*config.CheckPlugin{
+			"check1": {
+				Command: "echo 1",
 			},
 		},
 	}
@@ -52,16 +50,14 @@ func TestRunOncePayload(t *testing.T) {
 	}
 
 	conf := &config.Config{
-		Plugin: map[string]config.PluginConfigs{
-			"metrics": map[string]*config.PluginConfig{
-				"metric1": {
-					Command: diceCommand,
-				},
+		MetricPlugins: map[string]*config.MetricPlugin{
+			"metric1": {
+				Command: diceCommand,
 			},
-			"checks": map[string]*config.PluginConfig{
-				"check1": {
-					Command: "echo 1",
-				},
+		},
+		CheckPlugins: map[string]*config.CheckPlugin{
+			"check1": {
+				Command: "echo 1",
 			},
 		},
 	}

config/config.go

@@ -52,23 +52,20 @@ type Config struct {
 	Filesystems Filesystems `toml:"filesystems"`
 	HTTPProxy   string      `toml:"http_proxy"`
 
-	// Corresponds to the set of [plugin.<kind>.<name>] sections
-	// the key of the map is <kind>, which should be one of "metrics" or "checks".
-	Plugin map[string]PluginConfigs
+	// This Plugin field is used to decode the toml file. After reading the
+	// configuration from file, this field is set to nil.
+	// Please consider using MetricPlugins and CheckPlugins.
+	Plugin map[string]map[string]*PluginConfig
 
 	Include string
 
 	// Cannot exist in configuration files
 	HostIDStorage HostIDStorage
+	MetricPlugins map[string]*MetricPlugin
+	CheckPlugins  map[string]*CheckPlugin
 }
 
-// PluginConfigs represents a set of [plugin.<kind>.<name>] sections in the configuration file
-// under a specific <kind>. The key of the map is <name>, for example "mysql" of "plugin.metrics.mysql".
-type PluginConfigs map[string]*PluginConfig
-
-// PluginConfig represents a section of [plugin.*].
-// `MaxCheckAttempts`, `NotificationInterval` and `CheckInterval` options are used with check monitoring plugins. Custom metrics plugins ignore these options.
-// `User` option is ignore in windows
+// PluginConfig represents a plugin configuration.
 type PluginConfig struct {
 	CommandRaw           interface{} `toml:"command"`
 	Command              string
@@ -80,6 +77,78 @@ type PluginConfig struct {
 	CustomIdentifier     *string `toml:"custom_identifier"`
 }
 
+// MetricPlugin represents the configuration of a metric plugin
+// The User option is ignored on Windows
+type MetricPlugin struct {
+	Command          string
+	CommandArgs      []string
+	User             string
+	CustomIdentifier *string
+}
+
+func (pconf *PluginConfig) buildMetricPlugin() (*MetricPlugin, error) {
+	err := pconf.prepareCommand()
+	if err != nil {
+		return nil, err
+	}
+	return &MetricPlugin{
+		Command:          pconf.Command,
+		CommandArgs:      pconf.CommandArgs,
+		User:             pconf.User,
+		CustomIdentifier: pconf.CustomIdentifier,
+	}, nil
+}
+
+// Run the metric plugin.
+func (pconf *MetricPlugin) Run() (string, string, int, error) {
+	if len(pconf.CommandArgs) > 0 {
+		return util.RunCommandArgs(pconf.CommandArgs, pconf.User)
+	}
+	return util.RunCommand(pconf.Command, pconf.User)
+}
+
+// CommandString returns the command string for log messages
+func (pconf *MetricPlugin) CommandString() string {
+	if len(pconf.CommandArgs) > 0 {
+		return strings.Join(pconf.CommandArgs, " ")
+	}
+	return pconf.Command
+}
+
+// CheckPlugin represents the configuration of a check plugin
+// The User option is ignored on Windows
+type CheckPlugin struct {
+	Command              string
+	CommandArgs          []string
+	User                 string
+	NotificationInterval *int32
+	CheckInterval        *int32
+	MaxCheckAttempts     *int32
+}
+
+func (pconf *PluginConfig) buildCheckPlugin() (*CheckPlugin, error) {
+	err := pconf.prepareCommand()
+	if err != nil {
+		return nil, err
+	}
+	return &CheckPlugin{
+		Command:              pconf.Command,
+		CommandArgs:          pconf.CommandArgs,
+		User:                 pconf.User,
+		NotificationInterval: pconf.NotificationInterval,
+		CheckInterval:        pconf.CheckInterval,
+		MaxCheckAttempts:     pconf.MaxCheckAttempts,
+	}, nil
+}
+
+// Run the check plugin.
+func (pconf *CheckPlugin) Run() (string, string, int, error) {
+	if len(pconf.CommandArgs) > 0 {
+		return util.RunCommandArgs(pconf.CommandArgs, pconf.User)
+	}
+	return util.RunCommand(pconf.Command, pconf.User)
+}
+
 func (pconf *PluginConfig) prepareCommand() error {
 	const errFmt = "failed to prepare plugin command. A configuration value of `command` should be string or string slice, but %T"
 	v := pconf.CommandRaw
@@ -106,14 +175,6 @@ func (pconf *PluginConfig) prepareCommand() error {
 	return nil
 }
 
-// Run the plugin
-func (pconf *PluginConfig) Run() (string, string, int, error) {
-	if len(pconf.CommandArgs) > 0 {
-		return util.RunCommandArgs(pconf.CommandArgs, pconf.User)
-	}
-	return util.RunCommand(pconf.Command, pconf.User)
-}
-
 const postMetricsDequeueDelaySecondsMax = 59   // max delay seconds for dequeuing from buffer queue
 const postMetricsRetryDelaySecondsMax = 3 * 60 // max delay seconds for retrying a request that caused errors
 
@@ -152,16 +213,36 @@ func (r *Regexpwrapper) UnmarshalText(text []byte) error {
 	return err
 }
 
-// CheckNames return list of plugin.checks._name_
+// CheckNames returns a list of name of the check plugins
 func (conf *Config) CheckNames() []string {
 	checks := []string{}
-	for name := range conf.Plugin["checks"] {
+	for name := range conf.CheckPlugins {
 		checks = append(checks, name)
 	}
 	return checks
 }
 
-// LoadConfig XXX
+// ListCustomIdentifiers returns a list of customIdentifiers.
+func (conf *Config) ListCustomIdentifiers() []string {
+	var customIdentifiers []string
+	for _, pconf := range conf.MetricPlugins {
+		if pconf.CustomIdentifier != nil && index(customIdentifiers, *pconf.CustomIdentifier) == -1 {
+			customIdentifiers = append(customIdentifiers, *pconf.CustomIdentifier)
+		}
+	}
+	return customIdentifiers
+}
+
+func index(xs []string, y string) int {
+	for i, x := range xs {
+		if x == y {
+			return i
+		}
+	}
+	return -1
+}
+
+// LoadConfig loads a Config from a file.
 func LoadConfig(conffile string) (*Config, error) {
 	config, err := loadConfigFile(conffile)
 	if err != nil {
@@ -208,25 +289,49 @@ func LoadConfig(conffile string) (*Config, error) {
 	return config, err
 }
 
+func (conf *Config) setMetricPluginsAndCheckPlugins() error {
+	if pconfs, ok := conf.Plugin["metrics"]; ok {
+		var err error
+		for name, pconf := range pconfs {
+			conf.MetricPlugins[name], err = pconf.buildMetricPlugin()
+			if err != nil {
+				return err
+			}
+		}
+	}
+	if pconfs, ok := conf.Plugin["checks"]; ok {
+		var err error
+		for name, pconf := range pconfs {
+			conf.CheckPlugins[name], err = pconf.buildCheckPlugin()
+			if err != nil {
+				return err
+			}
+		}
+	}
+	// Make Plugins empty because we should not use this later.
+	// Use MetricPlugins and CheckPlugins.
+	conf.Plugin = nil
+	return nil
+}
+
 func loadConfigFile(file string) (*Config, error) {
 	config := &Config{}
 	if _, err := toml.DecodeFile(file, config); err != nil {
 		return config, err
 	}
 
+	config.MetricPlugins = make(map[string]*MetricPlugin)
+	config.CheckPlugins = make(map[string]*CheckPlugin)
+	if err := config.setMetricPluginsAndCheckPlugins(); err != nil {
+		return nil, err
+	}
+
 	if config.Include != "" {
 		if err := includeConfigFile(config, config.Include); err != nil {
 			return config, err
 		}
 	}
-	for _, pconfs := range config.Plugin {
-		for _, pconf := range pconfs {
-			err := pconf.prepareCommand()
-			if err != nil {
-				return nil, err
-			}
-		}
-	}
+
 	return config, nil
 }
 
@@ -243,12 +348,6 @@ func includeConfigFile(config *Config, include string) error {
 		rolesSaved := config.Roles
 		config.Roles = nil
 
-		// Also, save plugin values for later merging
-		pluginSaved := map[string]PluginConfigs{}
-		for kind, plugins := range config.Plugin {
-			pluginSaved[kind] = plugins
-		}
-
 		meta, err := toml.DecodeFile(file, &config)
 		if err != nil {
 			return fmt.Errorf("while loading included config file %s: %s", file, err)
@@ -260,16 +359,10 @@ func includeConfigFile(config *Config, include string) error {
 			config.Roles = rolesSaved
 		}
 
-		for kind, plugins := range config.Plugin {
-			for key, conf := range plugins {
-				if pluginSaved[kind] == nil {
-					pluginSaved[kind] = PluginConfigs{}
-				}
-				pluginSaved[kind][key] = conf
-			}
+		// Add new plugin or overwrite a plugin with the same plugin name.
+		if err := config.setMetricPluginsAndCheckPlugins(); err != nil {
+			return err
 		}
-
-		config.Plugin = pluginSaved
 	}
 
 	return nil