Bug 1471949 - [RFE] refactor the collectd configuration files 20-builtins-conf-for-ovirt-engine.conf and 20-builtins-conf-for-ovirt.conf so they will be more readable
[RFE] refactor the collectd configuration files 20-builtins-conf-for-ovirt-en...
Status: VERIFIED
Product: ovirt-engine-metrics
Classification: oVirt
Component: RFEs (Show other bugs)
1.0.5
Unspecified Unspecified
low Severity unspecified
: ovirt-4.2.0
: ---
Assigned To: Shirly Radco
Lukas Svaty
: FutureFeature
Depends On:
Blocks: 1475135 1513010 oVirt-Metrics-and-Logs
  Show dependency treegraph
 
Reported: 2017-07-17 14:24 EDT by Shirly Radco
Modified: 2017-12-01 08:12 EST (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: No Doc Update
Doc Text:
undefined
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: Metrics
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
lsvaty: needinfo+
rule-engine: ovirt‑4.2+
pstehlik: testing_plan_complete-
mgoldboi: planning_ack+
rule-engine: devel_ack+
pstehlik: testing_ack+


Attachments (Terms of Use)


External Trackers
Tracker ID Priority Status Summary Last Updated
oVirt gerrit 79509 master MERGED refactor collectd conf file 2017-07-20 09:21 EDT
oVirt gerrit 83875 master MERGED collectd: update collectd conf structure 2017-11-09 15:24 EST

  None (edit)
Description Shirly Radco 2017-07-17 14:24:14 EDT
Description of problem:
Move all "Load plugin" to the top of the collectd configuration file so it will be easier for the user to understand what plugins are loaded.
Comment 1 Lukas Svaty 2017-09-21 11:58:25 EDT
Correct me if I'm wrong but I would imagine all LoadPLugin would be in single file.

Engine:
[root@ls-engine1 ~]# grep -R "^LoadPlugin" /etc/collectd.*
/etc/collectd.conf:LoadPlugin syslog
/etc/collectd.conf:LoadPlugin cpu
/etc/collectd.conf:LoadPlugin interface
/etc/collectd.conf:LoadPlugin load
/etc/collectd.conf:LoadPlugin memory
/etc/collectd.d/postgresql.conf:LoadPlugin postgresql
/etc/collectd.d/30-write_http.conf:LoadPlugin write_http
/etc/collectd.d/20-builtins-conf-for-ovirt-engine.conf:LoadPlugin disk
/etc/collectd.d/20-builtins-conf-for-ovirt-engine.conf:LoadPlugin cpu
/etc/collectd.d/20-builtins-conf-for-ovirt-engine.conf:LoadPlugin memory
/etc/collectd.d/20-builtins-conf-for-ovirt-engine.conf:LoadPlugin load
/etc/collectd.d/20-builtins-conf-for-ovirt-engine.conf:LoadPlugin nfs
/etc/collectd.d/20-builtins-conf-for-ovirt-engine.conf:LoadPlugin entropy
/etc/collectd.d/20-builtins-conf-for-ovirt-engine.conf:LoadPlugin swap
/etc/collectd.d/20-builtins-conf-for-ovirt-engine.conf:LoadPlugin df
/etc/collectd.d/20-builtins-conf-for-ovirt-engine.conf:LoadPlugin interface
/etc/collectd.d/20-builtins-conf-for-ovirt-engine.conf:LoadPlugin aggregation
/etc/collectd.d/20-builtins-conf-for-ovirt-engine.conf:LoadPlugin processes
/etc/collectd.d/20-builtins-conf-for-ovirt-engine.conf:LoadPlugin uptime
/etc/collectd.d/20-collectd_postgresql.conf:LoadPlugin postgresql


Host:
[root@localhost ~]# grep -R "^LoadPlugin" /etc/collectd.*
/etc/collectd.conf:LoadPlugin syslog
/etc/collectd.conf:LoadPlugin cpu
/etc/collectd.conf:LoadPlugin interface
/etc/collectd.conf:LoadPlugin load
/etc/collectd.conf:LoadPlugin memory
/etc/collectd.d/30-write_http.conf:LoadPlugin write_http
/etc/collectd.d/20-builtins-conf-for-ovirt.conf:LoadPlugin disk
/etc/collectd.d/20-builtins-conf-for-ovirt.conf:LoadPlugin cpu
/etc/collectd.d/20-builtins-conf-for-ovirt.conf:LoadPlugin memory
/etc/collectd.d/20-builtins-conf-for-ovirt.conf:LoadPlugin load
/etc/collectd.d/20-builtins-conf-for-ovirt.conf:LoadPlugin nfs
/etc/collectd.d/20-builtins-conf-for-ovirt.conf:LoadPlugin entropy
/etc/collectd.d/20-builtins-conf-for-ovirt.conf:LoadPlugin swap
/etc/collectd.d/20-builtins-conf-for-ovirt.conf:LoadPlugin df
/etc/collectd.d/20-builtins-conf-for-ovirt.conf:LoadPlugin interface
/etc/collectd.d/20-builtins-conf-for-ovirt.conf:LoadPlugin aggregation
/etc/collectd.d/20-builtins-conf-for-ovirt.conf:LoadPlugin processes
/etc/collectd.d/20-builtins-conf-for-ovirt.conf:LoadPlugin uptime
/etc/collectd.d/10-read-statsd.conf:LoadPlugin statsd

tested in ovirt-engine-metrics-1.1.1-0.0.master.20170919065728.el7.centos.noarch
Comment 2 Shirly Radco 2017-09-24 05:02:56 EDT
No. Sorry.

This fix only refers to the main files:
20-builtins-conf-for-ovirt-engine.conf and 20-builtins-conf-for-ovirt.conf.

There are additional files that load additional files before this change that we did not touch.
Comment 3 Lukas Svaty 2017-09-25 04:06:02 EDT
Don't think this helps in any way on readability, admin still needs to look through different files to search where which plugins is enabled. I would even say it might confuse the admin to not to see the configuration of the collectd plugin right after where it's enabled, and have to search for it somewhere else. I would go with the common collectd structure:

Load Plugin A
Configuration A
Load PLugin B
Configuration B 


Plugins like memory, cpu, interface, load... are loaded in multiple config files:

/etc/collectd.conf:LoadPlugin memory
/etc/collectd.d/20-builtins-conf-for-ovirt.conf:LoadPlugin memory

We should have single point of entry, thus if admin wants to edit this he should be able to in single config files.

Please add feature page where each configuration of collectd plugins are located and what are the predefined values. For now we have only this one[1], which is not sufficient.

[1] https://www.ovirt.org/develop/release-management/features/metrics/metrics-store-collected-metrics/
Comment 4 Shirly Radco 2017-09-25 05:44:54 EDT
(In reply to Lukas Svaty from comment #3)
> Don't think this helps in any way on readability, admin still needs to look
> through different files to search where which plugins is enabled. I would
> even say it might confuse the admin to not to see the configuration of the
> collectd plugin right after where it's enabled, and have to search for it
> somewhere else. I would go with the common collectd structure:
> 
> Load Plugin A
> Configuration A
> Load PLugin B
> Configuration B 

There is no "commom" collectd structure.

This was discussed also with PM and this gives a one look to see which plugins are loaded at the top on the config file.

> 
> 
> Plugins like memory, cpu, interface, load... are loaded in multiple config
> files:


This is due to packaging that add these files as part of the plugin.
It was already discussed with Didi, Sandro and PM and was decided to keep as is.
If we decide to stop using a plugin it will have to be uninstalled and the file will be removed with it. In this case the "/etc/collectd.conf:LoadPlugin memory" is the file that was deployed with the plugin package.

> 
> /etc/collectd.conf:LoadPlugin memory
> /etc/collectd.d/20-builtins-conf-for-ovirt.conf:LoadPlugin memory
> 
> We should have single point of entry, thus if admin wants to edit this he
> should be able to in single config files.
> 

Having a single file is not a requirement.

Admin is not supposed to edit any of these files. We do not support that.

> Please add feature page where each configuration of collectd plugins are
> located and what are the predefined values. For now we have only this
> one[1], which is not sufficient.
> 
> [1]
> https://www.ovirt.org/develop/release-management/features/metrics/metrics-
> store-collected-metrics/

wip. Not part of this RFE.

Please move back to ON_QA.
Comment 5 Lukas Svaty 2017-10-02 02:46:12 EDT
(In reply to Shirly Radco from comment #4)
> (In reply to Lukas Svaty from comment #3)
> > Don't think this helps in any way on readability, admin still needs to look
> > through different files to search where which plugins is enabled. I would
> > even say it might confuse the admin to not to see the configuration of the
> > collectd plugin right after where it's enabled, and have to search for it
> > somewhere else. I would go with the common collectd structure:
> > 
> > Load Plugin A
> > Configuration A
> > Load PLugin B
> > Configuration B 
> 
> There is no "commom" collectd structure.

With common, I meant similar to /etc/collectd.conf

> 
> This was discussed also with PM and this gives a one look to see which
> plugins are loaded at the top on the config file.
> 

This does not seem to me as user-friendly, as the configuration is an important part of each collectd plugin and admin does not want to run around the file after he enabled the collectd plugin. Yaniv can we revisit this decision?

> > 
> > 
> > Plugins like memory, cpu, interface, load... are loaded in multiple config
> > files:
> 
> 
> This is due to packaging that add these files as part of the plugin.
> It was already discussed with Didi, Sandro and PM and was decided to keep as
> is.
> If we decide to stop using a plugin it will have to be uninstalled and the
> file will be removed with it. In this case the
> "/etc/collectd.conf:LoadPlugin memory" is the file that was deployed with
> the plugin package.
> 

Still seems like a mess in the configuration file, Yaniv what do you think?

> > 
> > /etc/collectd.conf:LoadPlugin memory
> > /etc/collectd.d/20-builtins-conf-for-ovirt.conf:LoadPlugin memory
> > 
> > We should have single point of entry, thus if admin wants to edit this he
> > should be able to in single config files.
> > 
> 
> Having a single file is not a requirement.
> 

I agree, however, at the moment we have most of the plugins in this file, while they can be spread among multiple files for more visibility, or cleaned up in 1 file where it would make more sense for admins etc..

20-ovirt-postgres.conf
20-ovirt-resources.conf
...

> Admin is not supposed to edit any of these files. We do not support that.
> 

Fact that we do not support is not always the deciding factor for admins if they can get few more statistics from collectd or configure the collectd plugins to the format they prefer we might want to do rather do a QuickFix. 
Thoughts Yaniv?

> > Please add feature page where each configuration of collectd plugins are
> > located and what are the predefined values. For now we have only this
> > one[1], which is not sufficient.
> > 
> > [1]
> > https://www.ovirt.org/develop/release-management/features/metrics/metrics-
> > store-collected-metrics/
> 
> wip. Not part of this RFE.
> 

Do we have any other BZ where this feature page is considered as necessary. This one seems to be like the best RFE where we can document these collectors.

> Please move back to ON_QA.
Comment 6 Shirly Radco 2017-10-03 04:23:09 EDT
There is an option of having per plugin configuration file.

Pros:
It can perhaps give a better visualization to which plugins are loaded.

Cons:
Handling of this file in case we decide to stop reporting a specific plugin.

We will have to remove this file, since just commenting out the "loadPlugin" text will confuse the user since the file still exists.

We decided to avoid cleaning up files, so having per plugin configuration file will have to reconsider this decition.

Can you please open an FRE with requirements and I'll discuss this with Yaniv L?
Comment 7 Lukas Svaty 2017-10-04 12:37:11 EDT
Bug requested - https://bugzilla.redhat.com/show_bug.cgi?id=1498579

I don't see a problem in removing a conf file when deprecating a plugin.

I still disagree all LoadPlugin should be in the top of the file as it would be harder to find a configuration for the relevant plugin. And you can easily search for the Loaded plugins with 1 command of basic Admin skills - grep -R "^LoadPlugin" /etc/collectd.*

I disagree this feature should go through and suggest for a revert to the old version. However, the patch included is correct even if the design is questionable.

I'll leave the decision to PM. Yaniv thoughts?
Comment 8 Yaniv Lavi 2017-11-05 06:06:05 EST
(In reply to Lukas Svaty from comment #7)
> Bug requested - https://bugzilla.redhat.com/show_bug.cgi?id=1498579
> 
> I don't see a problem in removing a conf file when deprecating a plugin.
> 
> I still disagree all LoadPlugin should be in the top of the file as it would
> be harder to find a configuration for the relevant plugin. And you can
> easily search for the Loaded plugins with 1 command of basic Admin skills -
> grep -R "^LoadPlugin" /etc/collectd.*
> 
> I disagree this feature should go through and suggest for a revert to the
> old version. However, the patch included is correct even if the design is
> questionable.
> 
> I'll leave the decision to PM. Yaniv thoughts?

I also agree that LoadPlugin is more readable with sections over putting everything on top and suggest a revert.
Shirly, please discuss with integration.
Comment 9 Yedidyah Bar David 2017-11-08 04:40:24 EST
I tend to agree with Lukas here, I find having the conf of a plugin right after loading it easier to read. No strong opinion, though, and since these files are not meant to be manually edited by admins, but mostly by the ovirt-engine-metrics maintainer, perhaps the maintainer should decide...
Comment 10 Lukas Svaty 2017-12-01 08:12:28 EST
verified in ovirt-engine-metrics-1.1.1-0.3.beta2.20171114114644.el7ev.noarch

Note You need to log in before you can comment on or make changes to this bug.