Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SuperQ
Copy link
Member

@SuperQ SuperQ commented Jul 21, 2023

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.

@SuperQ SuperQ force-pushed the superq/refactor_kingpin branch 2 times, most recently from 456401e to 922ea97 Compare July 21, 2023 14:06
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>
@discordianfish
Copy link
Member

What is the use case here? I think keeping the flag definitions in the collector where they are used is better.

@marctc
Copy link

marctc commented Jul 31, 2023

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:

@SuperQ
Copy link
Member Author

SuperQ commented Aug 1, 2023

@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.

@discordianfish
Copy link
Member

Ok but what about having collector specific config structs instead of one big node collector config?

@marctc
Copy link

marctc commented Aug 8, 2023

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.

@SuperQ
Copy link
Member Author

SuperQ commented Aug 15, 2023

@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)

@discordianfish
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants