Refactor plugin configurations #322
Conversation
| } | ||
| // Make Plugins empty because we should not use this later. | ||
| // Use MetricPlugins and CheckPlugins. | ||
| conf.Plugin = nil |
There was a problem hiding this comment.
I think similar comment should be noted in type Config struct to let developers know they should not use it.
| @@ -190,21 +191,31 @@ func TestLoadConfigFile(t *testing.T) { | |||
| t.Error("PostMetricsRetryMax should be 5") | |||
| } | |||
|
|
|||
| if config.Plugin["metrics"] == nil { | |||
There was a problem hiding this comment.
maybe, you can add a test to check config.Plugin == nil?
There was a problem hiding this comment.
I'll add a simple test.
config/config.go
Outdated
| } | ||
| pluginSaved[kind][key] = conf | ||
| } | ||
| // Overwrite plugin configurations. |
There was a problem hiding this comment.
Hmm... I think this comment is misreading, I guess?
It doesn't delete old keys.
There was a problem hiding this comment.
It overwrites the configuration of the same plugin name...
There was a problem hiding this comment.
Yes, I understand that. but old code explicitly saves plugins and restores them.
So, I'm confused at first. maybe just adding "same plugin name of configurations" is fine.
config/config_test.go
Outdated
| if pluginConf.Command != "ruby /path/to/your/plugin/mysql.rb" { | ||
| t.Errorf("plugin conf command should be 'ruby /path/to/your/plugin/mysql.rb' but %v", pluginConf.Command) | ||
| } | ||
| if pluginConf.User != "mysql" { | ||
| t.Errorf("plugin user_name should be 'mysql'") | ||
| } | ||
| if *pluginConf.CustomIdentifier != "app1.example.com" { | ||
| t.Errorf("plugin custom_identifier should be 'app1.example.com'") |
There was a problem hiding this comment.
it should be t.Error or show *pluginConf.CustomIdentifier as well.
Including following error messages.
Maybe we can do it by implementing |
config/config.go
Outdated
| @@ -208,25 +289,49 @@ func LoadConfig(conffile string) (*Config, error) { | |||
| return config, err | |||
| } | |||
|
|
|||
| func (conf *Config) buildPlugins() error { | |||
There was a problem hiding this comment.
I think this name is ambiguous or misleading.
From this name, people may misunderstand it to build plugin-related configs only from config.Plugin. But actually this method doesn't erase already existing conf.MetricPlugins and conf.CheckPlugins, and should not erase them because it's called on includeConfigFile.
There was a problem hiding this comment.
Okay, I renamed to setMetricPluginsAndCheckPlugins to avoid ambiguity.
config/config.go
Outdated
| checks = append(checks, name) | ||
| } | ||
| return checks | ||
| } | ||
|
|
||
| // LoadConfig XXX | ||
| // CustomIdentifiers returns a list of customIdentifiers. |
There was a problem hiding this comment.
This comment, or method name should be more verbose that this method aggregates information inside config.MetricPlugins. We cannot read it from current method nameCustomIdentifiers.
There was a problem hiding this comment.
OK, I renamed to ListCustomIdentifiers.
Yes, probably we have to implement unmarshalizer by hand using reflect. I think it does too much... func createTagMap(rv reflect.Type) map[string]int {
num := rv.NumField()
ret := make(map[string]int)
for i := 0; i < num; i += 1 {
field := rv.Field(i)
name := field.Tag.Get("toml")
if name == "" {
name = strings.ToLower(field.Name)
}
ret[name] = i
}
return ret
}
// UnmarshalTOML defines how to decode toml data to Config
func (conf *Config) UnmarshalTOML(data interface{}) error {
dataMap, ok := data.(map[string]interface{})
if !ok != nil {
return errors.New("type mismatch for Config: expected a map but found %T", data)
}
rv := reflect.ValueOf(v)
if rv.IsNil() {
return errors.New("Decode of nil %s", reflect.TypeOf(v))
}
tm := createTagMap(rv.Type())
for key, datum := range dataMap {
if key == "plugin" {
continue
}
if i, ok := tm[key]; ok {
switch rv.Field(i).Kind() {
case reflect.String:
// Apibase, Apikey, etc
rv.Field(i).Set(reflect.ValueOf...
case reflect.Struct:
// ConnectionConfig, HostStatus, FileSystems
Unmarshalize again and again...
}
}
} |
|
LGTM! |
|
Thank you! |
The configurations of metric plugins and check plugins are contained in Config.Plugin. It makes it easy to decode from the configuration file. But basically we should differentiate the struct of metric plugin and check plugin in my opinion. A check plugin config has
NotificationIntervalfield but a metric plugin config does not.This pull request introduces
MetricPluginandCheckPluginand now aConfighasMetricPluginsandCheckPlugins. The method*Config.buildPluginsbuilds these plugin configurations and clears the Plugin field to stop others use this field later. I think we should remove this field if possible and move the building phase on toml unmarshalization but I'm not sure how to do.