feat(inputs.system): Add option to specify collected information#18533
Conversation
|
Thanks so much for the pull request! |
|
!signed-cla |
|
@skartikey @srebhan @mstrandboge Hi, could you take a look at this PR when you have a moment? |
|
First of all: Thank you for your nice contribution @bilkoua ! I like the basic idea and see this being helpful, however I feel like merging this into the system input plugin would make the features more discoverable. What do you think @bilkoua? |
|
@srebhan I actually had the same idea at first. But then I figured a smaller, separate plugin would be easier to get merged. |
|
Thanks for rebasing @bilkoua! As this PR is fairly large I suggest to cut it into more digestible pieces. My understanding is that this PR does
So my idea would be to split this PR into the three steps above. I'm not really sure if we need step 1 as the metrics exist right now and they can easily be filtered using metric filtering options. For 2: I suggest you take a look at the gopsutil library as this library can extract the required information in an OS independent way instead of "hand-coding" the stuff. For 3: How about using the smbios library to read DMI information in an platform independent way (at least for most platforms we support)? This reduces the amount of code we need to maintain and we gain support of at least Linux, Windows and BSD variants... |
|
@srebhan Thanks for the feedback - splitting the PR into smaller pieces makes sense, and I can restructure it accordingly. Regarding point 1: my reasoning for guarding existing metrics with user settings is that metric filtering does not fully replace the ability to disable entire metric groups. As I understand it, when filters are used the metrics are still collected and resources are still spent gathering them, even if they are later filtered out and not exposed. With the proposed approach, the metrics would not be collected at all, which avoids unnecessary resource usage. This approach is also already used by several existing metric plugins such as For points 2 and 3, I agree that using existing libraries instead of implementing everything manually would make sense. However, the go-smbios library appears to be largely unmaintained. From a quick look, the alternatives seem to be either relatively unknown forks, platform-specific implementations, or larger libraries such as ghw. The latter might be somewhat heavier than what we need, although it could make sense if we plan to expose additional hardware-related metrics in the future. What are your thoughts on this? |
|
@bilkoua regarding filtering. Yes some plugins allow to enable feature sets but you seem to go down to a per-field level. From a resource-usage standpoint your approach might make sense, even though here the "cost" for the extra data is really low, but from a maintenance standpoint it is horrible to allow users to specify collection on a per-field basis. So how about having Regarding usage of libraries, I'm not bound to a particular library, my only criterion is that is should be maintained if necessary (some "old" libraries are maintained but only rarely have issues...). However, using a library is highly encouraged! |
|
@srebhan Apologies for the delay - didn't have much time to get back to this sooner. I've reworked the PR based on your suggestions: What changed in this revision:
Plan for follow-up PRs:
Each will add a new value to the same |
Hipska
left a comment
There was a problem hiding this comment.
I like the idea, however, I feel like there is too much added in the load group.
|
@Hipska Initially, the existing metrics were split into groups: It’s possible I didn’t fully understand the original idea, but I’m happy to adjust the implementation however we agree. The main thing is to reach a clear consensus on the structure. |
|
As I read the comment you're pointing at, it looks like the suggestion is to have these 4 groups: Edit: Looking at the existing code, it doesn't look like you are adding any new fields, so I would not change metric name for existing ones for backward compatibility. |
929d7b2 to
b161aed
Compare
|
@srebhan Thanks for the review! |
Hipska
left a comment
There was a problem hiding this comment.
Looks very good! One small remark regarding the tests.
| s := &System{Include: include, Log: &testutil.Logger{}} | ||
| require.NoError(t, s.Init()) |
There was a problem hiding this comment.
Use t.Run when looping over tests
There was a problem hiding this comment.
I agree, that would be better to avoid side effects between the tests.
| s := &System{Include: include, Log: &testutil.Logger{}} | ||
| require.NoError(t, s.Init()) |
There was a problem hiding this comment.
I agree, that would be better to avoid side effects between the tests.
Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
|
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
Summary
Add a
collectoption to the existingsysteminput plugin, allowing users to selectively enable or disable metric groups.Motivation
The
systemplugin currently always collects all metrics. Adding acollectoption gives users control over which groups are gathered. Whencollectis omitted, the behaviour is identical to the previous version of the plugin.The
collectoption is designed to be extensible - future metric groups can be added without changing the configuration interface.By default both groups are enabled.
Checklist