Replace --collectors.enabled with per-collector flags #640

carlpett · 2017-08-13T11:22:43Z

@SuperQ mentioned that after the switch to kingpin, now would be a good time to also replace --collectors.enabled with individual flags for each collector, since there is a breaking change anyway.

This PR does that. The flags are now --collector.<name>.enabled (or --no-collector.<name>.enabled to disable a default collector), but if there are shorter alternatives that might be good to consider.

This PR also moves the NodeExporter type into the collector package, as suggested by @SuperQ to follow mysqld_exporter #209.

It is possible to remove a few more exported symbols from the collector package, but I'd like some opinions on if that would be positive or not. Making Factories internal seems like a reasonable thing, for instance, but then we'd need to make --collectors.print call into some new function.

carlpett · 2017-08-13T11:41:04Z

Working on the bash-fu to fix the e2e test script...

SuperQ · 2017-08-13T16:44:26Z

end-to-end-test.sh

@@ -76,7 +83,8 @@ fi
 ./node_exporter \
  --collector.procfs="collector/fixtures/proc" \
  --collector.sysfs="collector/fixtures/sys" \
-  --collectors.enabled="$(echo ${collectors} | tr ' ' ',')" \
+  $(for c in $enabled_collectors; do echo --collector.$c.enabled  ; done) \


Minor bash style thing here, for consistency I prefer to use ${var_name}.

Sure, fixed!

SuperQ · 2017-08-13T16:45:45Z

This is great, thanks for taking care of this.

discordianfish · 2017-08-14T09:34:18Z

Overall looking good, thanks!
One concern though: If a user wants a specific set of collectors enable, they would need to keep track of the currently enabled defaults and disabled the ones they don't need selectively, right?
I think we should have a way to only explicitly enable a list of collectors and I don't think this is possible with this right now. Maybe we could have a flag to disable the defaults? E.g --no-collector.defaults.enabled would disable all default-enabled collectors so you can specify explicitly which ones you want to use.

SuperQ · 2017-08-14T09:42:31Z

@discordianfish Either way, the user has to track collectors between releases. I'd rather keep this simple if possible.

I suggest we file an issue for "disable everything flag" and see if anyone really needs it.

grobie · 2017-08-14T09:47:18Z

I have similar concerns to @discordianfish and I'm not sure the current change is a clear win in usability. Given PromCon and related preparations, I won't have time to research for ideas in similar setups and comment before next week.

SuperQ · 2017-08-14T10:07:35Z

For what it's worth, this is the same style as we use on the mysqld_exporter, and I don't recall any requests for "disable everything by default".

To, me, there are some clear usability wins. The reasoning is detailed in #216.

discordianfish · 2017-08-14T10:45:25Z

@SuperQ But as right now, without this PR you can explicitly set the collectors you want to use. With this change this isn't possible anymore which is why I believe we need to change this before merging this PR. Otherwise I would agree that such improvements could be made later.

The mysql-exporter is IMO a different story where the need to explicitly enable a specific set of option isn't as important.

carlpett · 2017-08-14T18:10:20Z

One concern though: If a user wants a specific set of collectors enable, they would need to keep track of the currently enabled defaults and disabled the ones they don't need selectively, right?

@discordianfish Yes, pretty much. Which is the inverse of the current issue - if I just want to run with the defaults and add a few more, I will have to pull out the defaults, add the optional ones I need, and then keep that list up to date as I update the exporter.

Empirically, this need is more common requirement (the defaults are fairly uncontroversial, but I want to add one or two of the non-defaults), but maybe the groups I've worked with are outliers in this?

Anyway, I don't think it is unreasonable to have a "disable all" flag which then lets the user enable the specific ones with --collector.<name>.enabled for those cases where someone needs to be very precise about what collectors are enabled (although if one of the default collectors are troublesome in some cases, should it really be default?).

grobie · 2017-08-15T07:58:03Z

although if one of the default collectors are troublesome in some cases, should it really be default?

They shouldn't if they really cause trouble. The most common use case to disable default collectors is to reduce the amount of metrics produced in space limited setups.

matthiasr · 2017-08-15T08:22:00Z

+1 for having a way to enable only specific collectors. We enable almost all new collectors by default these days. Multiple people have spoken up here with a need to contain this, so I don't see the need for a separate issue.

…

On Tue, Aug 15, 2017, 09:58 Tobias Schmidt ***@***.***> wrote: although if one of the default collectors are troublesome in some cases, should it really be default? They shouldn't if they really cause trouble. The most common use case to disable default collectors is to reduce the amount of metrics produced in space limited setups. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#640 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAICBrPq8nr3b4BpQzWiX-AwJbmemAm8ks5sYU-LgaJpZM4O1qSn> .

discordianfish · 2017-08-15T09:26:44Z

Yes, also thinking about limit-number-of-metrics usecase. So we agree about a flag like --no-collector.defaults.enabled? Or other preferences?

SuperQ · 2017-08-15T09:28:39Z

How about --disable-collector-defaults, with the default of false.

matthiasr · 2017-08-15T09:29:42Z

I like this one better. no-collector.defaults.enabled is really hard to parse.

…

On Tue, Aug 15, 2017 at 9:28 AM Ben Kochie ***@***.***> wrote: How about --disable-collector-defaults, with the default of false. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#640 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAICBvDgteHFklgvxIdeWORfzOfj9Rs7ks5sYWTHgaJpZM4O1qSn> .

discordianfish · 2017-08-16T14:35:48Z

I liked the consistency but sure that name work

SuperQ · 2017-08-20T14:49:50Z

@carlpett Would you add a flag --disable-collector-defaults to this PR that sets all collectors to false unless explicitly enabled?

carlpett · 2017-08-20T17:28:16Z

@SuperQ Sorry for not responding! I've been travelling without internet access since submitting the PR, but I'll fix that flag as soon as I have access to laptop and wifi again :)

SuperQ · 2017-08-21T07:08:02Z

No worries, I was just wanting to ping. 😃

carlpett · 2017-08-25T18:41:01Z

Hm, so actually this isn't as easy as I would have first thought. Currently the "enabledness" of collectors is tracked in a map[string]*bool, pointed to by the flag, and the default state is set while setting up the flag. This means that changing the default state via a flag is hard - the setting of default state is done before parsing flags.
One potential solution would be to add another map for default state and then copy that over to the enabled-map if the defaults are not disabled. Any better suggestions?

discordianfish · 2017-08-28T08:12:56Z

I realized that too when looking at this. Don't really have a better answer. Maybe we should check out how others are doing this.

carlpett · 2017-08-31T19:39:13Z

Finally got my laptop back... I split the bool into two, so it is possible to track if the user has chosen to enable it and the default state separately.
I named the flag --collectors.disable-defaults to keep it more in line with the general namespacing. What do you think? If you prefer --disable-collector-defaults I'll change it to that.

carlpett · 2017-08-31T19:47:33Z

Huh, and apparently I broke some of the previous functionality when implementing it. Whoops. I even added tests, but forgot this case.
Fixing it.
EDIT: ... but apparently not tonight. I'll look at it again tomorrow.

carlpett · 2017-09-27T14:58:14Z

Rebased now.
As an aside, I cannot run the end-to-end tests locally since my company-issued laptop is a Windows machine, which does not allow :s in file names (created in the sysfs fixtures). Are those paths crucial to the tests, or could they be dropped?

SuperQ · 2017-09-27T14:59:44Z

We should have fixed the windows filename issues with the ttar tool.

SuperQ

LGTM

SuperQ · 2017-09-27T15:17:42Z

Oh, sorry, I was confused. We fixed the code so you can check it out, but tests will still not run on windows because it needs to unpack the ttar fixtures which contain windows-incompatible file names. There is currently no way around this.

discordianfish · 2017-09-27T15:44:08Z

I also note that there are --collector.procfs and --collector.sysfs that are a bit confusing now

Agreed, we should find a better name for these. I'd say either --collector.global.procfs or --global.procfs or something?

Beside that, LGTM!

carlpett · 2017-09-27T19:18:46Z

That naming seems fine to me!
@SuperQ @grobie Any feedback on the procfs/sysfs naming from @discordianfish?

SuperQ · 2017-09-27T20:48:33Z

No objection to the naming.

grobie · 2017-09-27T21:40:38Z

What about just --procfs and --sysfs? Global is pretty generic, what does that namespace give us in the end?

SuperQ · 2017-09-28T06:41:05Z

@grobie Good idea, I like that one best.

carlpett · 2017-09-28T06:49:11Z

If we still want some namespace, how about path? So --path.procfs for example.
Or we could do without

discordianfish · 2017-09-28T09:38:21Z

@carlpett I like that, let's go with --path..

discordianfish · 2017-09-28T09:38:56Z

@SuperQ Guess you're fine with that too? IMO better than no namespace. We might have more path options in the future so IMO this makes sense.

grobie · 2017-09-28T10:00:39Z

SG.

…

On Thu, Sep 28, 2017 at 11:38 AM discordianfish ***@***.***> wrote: @SuperQ <https://github.com/superq> Guess you're fine with that too? IMO better than no namespace. We might have more path options in the future so IMO this makes sense. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#640 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAANaG0j0ufrKymz2TFJaYw1nIz6TObvks5sm2kxgaJpZM4O1qSn> .

SuperQ · 2017-09-28T10:35:30Z

Yes, --path. is approved.

grobie

Thanks! I'd drop the print command, but looks great otherwise!

grobie · 2017-09-28T10:37:25Z

README.md

@@ -16,7 +16,8 @@ The [WMI exporter](https://github.com/martinlindhe/wmi_exporter) is recommended
 There is varying support for collectors on each operating system. The tables
 below list all existing collectors and the supported systems.

-Which collectors are used is controlled by the `--collectors.enabled` flag.
+Collectors are enabled by providing a `--collector.<name>` flag.
+Collectors that are enabled by default can be disabled by providing a `--no-collector.name` flag.


For consistency, this should be <name>

grobie · 2017-09-28T10:40:03Z

collector/collector.go

+type NodeCollector struct {
+	Collectors map[string]Collector
+}
+


NewNodeCollector should have a docstring as it's exported.

Hm, perhaps it shouldn't be exported? Not much point to it?

Eh, I meant that the NodeCollector struct possibly shouldn't be exported. I'll add a doc string to NewNodeCollector.

grobie · 2017-09-28T10:42:11Z

node_exporter.go

-		printCollectors   = kingpin.Flag("collectors.print", "If true, print available collectors and exit.").Bool()
+		listenAddress   = kingpin.Flag("web.listen-address", "Address on which to expose metrics and web interface.").Default(":9100").String()
+		metricsPath     = kingpin.Flag("web.telemetry-path", "Path under which to expose metrics.").Default("/metrics").String()
+		printCollectors = kingpin.Flag("collectors.print", "If true, print available collectors and exit.").Bool()


Do we still need this? All available collectors are already visible when showing --help.

Agree, that should be dropped.

Yes, I thought we had talked about dropping this previously.

discordianfish · 2017-09-28T10:46:31Z

LGTM beside @grobie's comments.

discordianfish · 2017-09-28T13:07:01Z

Awesome, thanks @carlpett for the contribution!

* Move NodeCollector into package collector * Refactor collector enabling * Update README with new collector enabled flags * Fix out-of-date inline flag reference syntax * Use new flags in end-to-end tests * Add flag to disable all default collectors * Track if a flag has been set explicitly * Add --collectors.disable-defaults to README * Revert disable-defaults flag * Shorten flags * Fixup timex collector registration * Fix end-to-end tests * Change procfs and sysfs path flags * Fix review comments

carlpett force-pushed the collector-flags branch 2 times, most recently from 5c18ebc to 061842c Compare August 13, 2017 12:10

SuperQ reviewed Aug 13, 2017

View reviewed changes

SuperQ requested review from grobie and discordianfish August 13, 2017 16:45

SuperQ mentioned this pull request Aug 13, 2017

adding a possibility to enable collectors additional to standard #216

Closed

carlpett force-pushed the collector-flags branch from 061842c to 936c184 Compare August 13, 2017 17:24

discordianfish closed this Aug 16, 2017

discordianfish reopened this Aug 16, 2017

carlpett added 2 commits September 27, 2017 16:06

Shorten flags

dea9a7b

Fixup timex collector registration

f8dc13a

carlpett force-pushed the collector-flags branch from 82e7a0e to f8dc13a Compare September 27, 2017 14:42

Fix end-to-end tests

7147b30

SuperQ approved these changes Sep 27, 2017

View reviewed changes

Change procfs and sysfs path flags

a55f065

grobie reviewed Sep 28, 2017

View reviewed changes

Fix review comments

ec50ff5

grobie approved these changes Sep 28, 2017

View reviewed changes

discordianfish merged commit 859a825 into prometheus:master Sep 28, 2017

TheMeier mentioned this pull request Oct 18, 2017

node_expoerter v0.15.0 voxpupuli/puppet-prometheus#70

Closed

tereschenkov mentioned this pull request Oct 20, 2017

Support node_exporter >=0.15.0 UnderGreen/ansible-prometheus-node-exporter#18

Closed

discordianfish mentioned this pull request Mar 26, 2018

Question: Disable all collectors? #735

Closed

Replace --collectors.enabled with per-collector flags #640

Replace --collectors.enabled with per-collector flags #640

Conversation

carlpett commented Aug 13, 2017

carlpett commented Aug 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperQ commented Aug 13, 2017

discordianfish commented Aug 14, 2017

SuperQ commented Aug 14, 2017

grobie commented Aug 14, 2017

SuperQ commented Aug 14, 2017 • edited Loading

discordianfish commented Aug 14, 2017

carlpett commented Aug 14, 2017

grobie commented Aug 15, 2017

matthiasr commented Aug 15, 2017 via email

discordianfish commented Aug 15, 2017

SuperQ commented Aug 15, 2017

matthiasr commented Aug 15, 2017 via email

discordianfish commented Aug 16, 2017

SuperQ commented Aug 20, 2017

carlpett commented Aug 20, 2017 via email

SuperQ commented Aug 21, 2017

carlpett commented Aug 25, 2017

discordianfish commented Aug 28, 2017

carlpett commented Aug 31, 2017

carlpett commented Aug 31, 2017 • edited Loading

carlpett commented Sep 27, 2017

SuperQ commented Sep 27, 2017

SuperQ left a comment

Choose a reason for hiding this comment

SuperQ commented Sep 27, 2017

discordianfish commented Sep 27, 2017

carlpett commented Sep 27, 2017

SuperQ commented Sep 27, 2017

grobie commented Sep 27, 2017

SuperQ commented Sep 28, 2017

carlpett commented Sep 28, 2017

discordianfish commented Sep 28, 2017

discordianfish commented Sep 28, 2017

grobie commented Sep 28, 2017 via email

SuperQ commented Sep 28, 2017

grobie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

discordianfish commented Sep 28, 2017

discordianfish commented Sep 28, 2017

SuperQ commented Aug 14, 2017 •

edited

Loading

carlpett commented Aug 31, 2017 •

edited

Loading