implement selective enabling of additional collectors, closes #216

[cherti]

This implements a proposal for the final decision of the new API for collector-enabling as discussed in #216.

Feedback is very welcome!

[discordianfish]

Not saying it's wrong but why using a global map? I imagined just having a (global) flag per collector to enable it, which would toggle adding the collector instance via the Factories map.

[cherti]

I did that at first as well, but then we have the issue that those flags are not compiled in based on architecture, but always.
Because the map lives in the collector-package, not in the main package, we can just add the flags into the collector-files and have the nice property that collectors that are not compiled in (e.g. mdadm_linux.go on BSDs) don't show up in -help.

[discordianfish]

If you define the flags in the arch files (or files with arch build tags) they should only get compiled there.

[cherti]

yes, but as far as I see, we don't have them yet for the main-package, therefore I'd tried to avoid introducing them and additionally, this would be additional bookkeeping, because we have to keep track of those flags in addition to the collectors, when adding or removing collectors.
This way, the collectors and their "state" is confined to their files, that's the intention behind that and with the map we can easily iterate over the available collectors.
That was the intention behind that.

[discordianfish]

If I understand this correctly, it will only allow adding collectors via the per-collector flag, not removing them.

[cherti]

Yes, currently this is the case for maximum compatibility with the old way. We could also go for being able to disable, but when I wrote this, I didn't come up with a satisfying idea how to marry both versions without possibly behaving weird, i.e. you set collectors via the old way and combine that with the new flag, what should happen then? We would probably just need to define the behavior, but I didn't want to come up here with something without having discussed this first.

[discordianfish]

We discussed a different behavior in the issue. See #216 (comment) and my follow up comment.
If that is something you want to work on, go for it. Otherwise I'd do the change.

[cherti]

Indeed, I missed that when implementing. I'll look into that as soon as I get to it.

But first we need to decide if I should change the map-implementation for the flags above, because that would affect the implementation here, I suspect.

[discordianfish]

Going to close this for now. Feel free to reopen if you like to continue with this.

[chandrasekarkr]

Is there any reason for not using config files for node exporter (similar to Prometheus/Alert Manager) instead of command-line options ? Using command-line options in node exporter becomes too clumsy when enabling systemd (with whitelist/blacklist options) for monitoring services.

Config files also gives an additional feature of re-loading without process/service restart, unlike command-line option.

[SuperQ]

The exporter is stateless, which means a reload and a restart are nearly the same thing. None of the collectors have any kind of reload plumbing, so change it would be a large amount of work.

You can easily simulate a configs that don't require a daemon-reload via EnvironmentFile.

For example on Debian it's common to do something like /etc/default/foo where foo contains

ARGS="-a -bunch -of -flags"

And then in the unit

EnvironmentFile=/etc/default/foo
ExecStart=/foo $ARGS

You can even get fancy and use a HEREDOC style ARGS env.