adding a possibility to enable collectors additional to standard

[cherti]

Hi,

over in IRC there was a discussion if it would make sense to include a commandline-flag of some sort to be able to enable collectors additional to the standard.

This would have several benefits:

• default exporters are enabled by default for a reason. If one could simply specify the collectors one wants to have enabled additionally and a new collector is added, one wouldn't have to adjust the entire list of enabled collectors (which is actually not that short and therefore not that clear to read imo) with the new one that would be enabled by default anyways. This could be countered by reading the release notes, but one might have skipped one release or actually run node_exporter from git, where we don't have any release notes. And as things are moving fast in Prometheus-land currently (and usually into very desirable directions), I actually know a lot of people who are running node_exporter from a git checkout.
• the commandline would get much shorter and therefore much clearer, because currently, for enabling one additional exporter I have to include 132 characters for this single commandline flag, which is far from readable imo. Especially if you add other flags (like -collectors.textfile.directory) that can get fairly long.

Therefore I'm proposing adding some possibility to just specify the additional collectors.

We talked about some ideas in IRC how this could be implemented, which I will just recall here:

• add an additional flag like -collectors.enabled.additional
• allow -collectors.enabled not just to take a list of collectors, but check if the first character is a +. If that's the case, append to the list of default collectors.
• allow for each collector a prefix like + or - to disable default collectors as well.
  These, are ideas emerging from the first discussion and do of course not have to be the final user interface.

Any opinions on the idea in general? Finding a good user interface would be the second step if the general idea is found to be worth investigating.

@juliusv @brian-brazil @grobie @fabxc
If I have forgotten to highlight someone, feel free to do it. ;)

[hryamzik]

Current behaviour is unusual but could be easily handled with any deployment system, for example

[cherti]

This, though, would require that you manually add each new exporter to that role, so if you miss a changelog (or build from git-HEAD, which is quite common from what I experience), you have no idea that you might have disabled an exporter you want to have.
another thing is that such a feature wouldn't hurt people who use automated instrumentation like ansible, but would benefit those who don't. Especially it might make no sense to roll out automated instrumentation if you have only one server that you monitor, but you might still want to monitor that one.

[hryamzik]

True, but you'll still have to add rules or graphs for new metrics, otherwise they are useless.

[therealbill]

There are a couple ways this could be done. IMO the ideal way would be to drop the comma separate list approach and go with a map[string]bool approach for collectors, with flags to enable or disable each - with default collectors initialized as true before application of the flags. I'd also combine it with a 'enableDefault' flag, defaulting to true.

That way if someone wants to only have a few specific ones they can use -enableDefault=false (or whatever it wound up being called) combined with eg. -enable-NAME=true for the ones they wanted.

I acknowledge this would be much easier to implement with the cli package but should be doable with the flag package as well.

[discordianfish]

I see how it's convinient, but I like the explicitness of the current approach. In general I assume we will have more collectors that are enabled by default and less that are disabled, give that we want to limit the node-exporter to system metrics for which it's usually cheap to have the enabled but not expose anything.

@SuperQ Thoughts?

[SuperQ]

I personally prefer the individual flags for each collector, the way we do it in the mysqld_exporter. No need for a global default flag, each collector can just keep their defaults.

[discordianfish]

@SuperQ The question (as I understand it) is about enabling additional collectors without having to specific all collectors as you do now.

[SuperQ]

Yes, individual collector flags allow for exactly that. Either take the default on a collector-by-collector basis, or override them.

[discordianfish]

Ah, got it. What about we keep the comma-separated enabled flag and use that as default for per collector flags like --<collector>.enabled? That way we don't break existing usage and support the use case @therealbill described (... --collectors.enabled="" --foo.enabled). The only downside is that collectors.enabled only enables a collector if the collector specific flag doesn't disable it again.. But I'd say that's fine if we mention is in the usage.

[SuperQ]

What I'm thinking is this:

• Add -collector.<collector>.enabled for all existing collectors, with respective defaults.
• Add a handler to make -collectors.enabled work with the new flags.
• Declare future collectors not supported by -collectors.enabled
• Declare -collectors.enabled deprecated in v0.14.

We can remove support for -collectors.enabled sometime after v0.14.

[discordianfish]

Just to be clear: -collectors.enabled would for now set the defaults for all -collector.<collector>.enabled flags as I described above? You basically just suggest that we deprecate it later? That sounds reasonable.

[SuperQ]

Yes, that's what I was thinking

[cherti]

see #390.

[discordianfish]

Since #390 isn't exactly what we want, I created https://github.com/prometheus/node_exporter/compare/master...discordianfish:rework-flags?expand=1
It's not yet what we want either but want to get feedback early in case I miss something.

In it's current form, the defaults are as before. You can enable or disable collectors explicitly. This is the desired functionality, correct?

What's not working yet is enabling/disabling the collectors via the old flag and this is a bit tricky, since we need to first parse the collectors.enabled flag, then set up a bunch of flags. Hope that's even possible with stdlib flag. Pointers welcome but otherwise I'll dig deeper tomorrow.

[SuperQ]

FYI: This work is now being done in #640

[mdlayher]

With #640 closed, it appears this can be closed as well. Feel free to re-open if I made a mistake.