When a plugin exposes config with defaults those defaults need to be available to framework

[jcooklin]

For instance, the call to GetMetricTypes is called with config from the pluginConfig which does not include the defaults that were exposed by the plugin.

[geauxvirtual]

Can this be expanded? Is the intent for the plugin to be able to pass defaults to snapd? Wouldn't the plugin already know of it's defaults? Maybe I'm missing something here.

[jcooklin]

If a plugin defines a rule for something, say the path to a binary, and provides a default the plugin author could reasonably expect that when it's called it is provided a config that contains an entry for 'path' that contains the default. Today the config is empty when we call GetMetricTypes.

Before calling GetMetricTypes we should something similar to what is being done in ValidateDeps.

[geauxvirtual]

So a plugin defines a rule for in it's config policy that has a default, and that default is not being added to the config for the plugin. I would say the default should only be added to the config if the rule is not required. If the plugin sets the rule as required, then the config item should need to be passed in via a task manifest or snapd config for the plugin.

[jcooklin]

👍
On Thu, Mar 24, 2016 at 8:56 AM Justin Guidroz notifications@github.com
wrote:

So a plugin defines a rule for in it's config policy that has a default,
and that default is not being added to the config for the plugin. I would
say the default should only be added to the config if the rule is not
required. If the plugin sets the rule as required, then the config item
should need to be passed in via a task manifest or snapd config for the
plugin.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#799 (comment)

[tjmcs]

The specific case here was passing the path to an executable where the plugin defined a default path and that path could be overridden in the global configuration.

In that example we need a way to set the path from the configuration if it was passed in but use the default from the plugin if it is not. I don't think it's reasonable to expect that this path always be set; it should only need to be set if the user wants to override the default location for that executable. Thoughts on that example (which is currently failing since the default isn't being passed through)?

[geauxvirtual]

If the plugin sets the config policy rule as required, then (as in this example) the path must be in the task manifest config or the global plugin config for the task to the be created.

If the plugin sets he config policy rule as not required but sets a default, then path is optional in the task manifest config or the global plugin config. While I agree that if we allow a default to be set on a rule in the config policy, then that default should be used in the config provided the task manifest or global plugin config does not set the config item, but the plugin author could easily check to see if path exists in the config, and if it doesn't, just use the default the plugin already knows about.

[tjmcs]

It's currently set as not required with a default value (if I understand the code correctly), but the default value is not being used to set a value in the configuration when the plugin is loaded. Does that make sense?

[candysmurf]

I haven't read all your comments yet. This may be a different issue but config related. I found that config in task manifest is only good for overriding the global config in my use cases. I have to specify params in the global config. We need to document this clearly.

[geauxvirtual]

I haven't read all your comments yet. This may be a different issue but config related. I found that config in task manifest is only good for overriding the global config in my use cases. I have to specify params in the global config. We need to document this clearly.

This has nothing to do with this issue.

then this is another issue. I think that we need to address it if you agree.

[candysmurf]

Also, I found that plugin is tightly coupled with snap now by using snap's global config. For example, in the plugin client code, I have to pass in global config if we don't prefer to use environmental variables. Task manifest does not help at all in this case. It's just a thought to share while you design configs.

[geauxvirtual]

Also, I found that plugin is tightly coupled with snap now by using snap's global config. For example, in the plugin client code, I have to pass in global config if we don't prefer to use environmental variables. Task manifest does not help at all in this case. It's just a thought to share while you design configs.

This is not relevant to this issue. If you feel documentation needs to be clearer with regards to configurations specified in task manifests or global configs, please create an issue for that documentation request.

[tiffanyfay]

@jcooklin @geauxvirtual @tjmcs

Right now policy info received from GetConfigPolicy is not sent back to the plugin on the GetMetricTypes call. What is being sent is the global config info (if any) from snap startup for that plugin. A few ways we could address this based on conversation with @IRCody are:

1. Add the plugin wide (ones at cp.Add([]string{""}, config)) level config policy default to the cfg data node with AddItem and then merge it with the plugin config. The config file passed into snap doesn't go into metric-level detail right now and some plugins are written with plugin wide config namespaced e.g. https://github.com/intelsdi-x/snap-plugin-collector-apache/blob/master/apache/apache.go#L155
   Pros: We now get the defaults that are on the same level as ones we pass in through the config file
   Cons: GetMetricsTypes still doesn't know anything about metric level config. Also as a result of this, some plugins will need to be changed so plugin level config gets moved up to the root.

2. Add the config policy to GetMetricsTypes since the config policy goes down to metric level which further than the plugin config goes
   Pros: Now we get all of the defaults
   Cons: Lots of refactoring

3. (my vote) Make the plugin author make sure to handle plugin level defaults
   e.g. something along the lines of https://github.com/intelsdi-x/snap-plugin-collector-apache/blob/master/apache/apache.go#L141
   Pros: No changes to the framework
   Cons: Not every author will make sure they do this

I'm not sure how we want to handle this.

[jcooklin]

Collector plugins should avoid requiring configuration on load.

This is related to the work recently done to add metric schema support. With dynamic metrics there is very little need to dynamically create the results for GetMetricTypes as a result we will begin to provide stronger guidance to plugin authors that they should avoid requiring config when implementing GetMetricTypes. We will also address this in plugins we have written that currently require config on load and probably don't yet support the metric schema (dynamic metrics). While there are cases when a collector plugin will want/need configuration on load the exception should not be the rule. I would go with option 3 above.