-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor collector package configuration #2755
base: master
Are you sure you want to change the base?
Conversation
456401e
to
922ea97
Compare
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. Signed-off-by: Ben Kochie <superq@gmail.com>
922ea97
to
9c6ff78
Compare
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: |
@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 |
@discordianfish We need one top-level config struct so that we can pass it all to We will need to create a number of |
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. |
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.