node_exporter
node_exporter copied to clipboard
prometheus
Refactor collector package configuration
Add a new pattern to allow moving the flag handling out of the collector package and into a separate package. This allows collector package users to create their own config or flag handling.
What is the use case here? I think keeping the flag definitions in the collector where they are used is better.
Hey @discordianfish, the idea is to decouple flags (which in this case is an specific way of configuring the exporter as a standalone program) from the exporter's collectors providing a config struct The use case is make node exporter embeddable in grafana agent without having to depend on global state (i.e flags) to configure the collectors. In the future, this would allow users to configure multiple instances of node exporters (with the support of Flow modules) without worrying about potential race conditions.
More info/context here:
- https://github.com/grafana/alloy/issues/548
- https://github.com/grafana/agent/issues/3155
@discordianfish Yes, the idea is to make the collector package more reusable as a library. I'm planning to do similar changes to other exporters.
Ok but what about having collector specific config structs instead of one big node collector config?
In https://github.com/prometheus/node_exporter/pull/2755/files#diff-b218d36a43d2b16cbfe05c8c78206818b70f54a802ee76ed990b5dc749ce6326R23-R26 ArpConfig struct is defined. I think it would make more sense to have those structs inside individual collector files IMHO.
@discordianfish We need one top-level config struct so that we can pass it all to NewNodeCollector().
We will need to create a number of collector_common.go files in order to contain the per-collector structs so that they always exist even if the collector itself not compiled in (BSDs, Darwin, etc)
A top level config for NewNodeCollector makes sense but not sure about passing that to each collector.. For most collectors we could also just pass the flag values to the constructor. We might also reconsider this init() based registration which I assume is also making consuming this as a module harder.
That being said, if you prefer this way here this is fine for me as well.