Modify Collector structure #810
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
|
Can one of the admins verify this patch? |
| } | ||
|
|
||
| //regexprs[ind] is set to nil if metricConfig.Regex cannot be parsed | ||
| regexprs[ind], _ = regexp.Compile(metricConfig.Regex) |
There was a problem hiding this comment.
Why not fail collector creation if regex can't be compiled?
There was a problem hiding this comment.
The idea was to allow collection of other valid metrics by creating the constructor even if a few regexps errored. Can we can remove the entire metric whose regex is invalid and proceed with creation of a collector if there is atleast one valid metric?
There was a problem hiding this comment.
My thinking is that if regex compile fails, then we should just error out and not create the collector. If we do partial collections, it might be hard for the user to notice that something was actually wrong with supplied regex. Failing to collect a metric is okay and we can continue processing, but invalid regex is probably something user should be reported back to user with a failure.
There was a problem hiding this comment.
My thought (and I think Rohit's too), is that here we're able to tell the user that their config is wrong and allow them to fix it.
You bring up a good point about trying to fail with reduced functionality (but in that case, we should at least return the error). Only concern is that if some metrics show up and some don't the user would think that something was broken in cAdvisor (or that the metric is empty), whereas a clear yes no to the metric would possibly make them think the config was wrong.
We can go either way :) but I think this makes it clear that we should be doing very good reporting of errors on metric collection so users know what's happening. We should export some stats and possibly a page.
There was a problem hiding this comment.
And it looks like we were on the same page @rjnagal :) lol
|
ok to test |
|
small nit on not keeping errored collectors around. looks okay otherwise. |
| @@ -45,8 +57,20 @@ func NewCollector(collectorName string, configfile string) (*GenericCollector, e | |||
|
|
|||
| //TODO : Add checks for validity of config file (eg : Accurate JSON fields) | |||
|
|
|||
| minPollFrequency := configInJSON.MetricsConfig[0].PollingFrequency | |||
There was a problem hiding this comment.
We should check that there is at least one (we have the TODO above, but we should at least check that so we don't crash)
|
LGTM |
@rjnagal , @vmarmol This is a rough draft of how we could change the Collector structure
The newly added fields 'minPollingFrequency' and 'regexps' in collectorInfo field in Collector structure are necessary for collection of metrics.
We can avoid calculation of these during actual metric collection to prevent repeated calculation during every metric calculation.