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

improvement: Expose hwmon collector config options in prometheus.exporter.unix component. #1369

Merged
merged 6 commits into from
Aug 1, 2024

Conversation

dtrejod
Copy link
Contributor

@dtrejod dtrejod commented Jul 26, 2024

PR Description

Exposes hwmon collector config options in the prometheus.exporter.unix component. The hwmon collector currently in the last v1.8.2 release of node_exporter supports hwmon configuration for which hwmon chips to filter to. This is useful for those users that just want a subset of hwmon metrics, and don't want to list a collection of relabel_rules to drop irrelevant hwmon metrics.

https://github.com/prometheus/node_exporter/blob/f1e0e8360aa60b6cb5e5cc1560bed348fc2c1895/collector/hwmon_linux.go#L35-L36

Which issue(s) this PR fixes

n/a

Notes to the Reviewer

A recent PR to node_exporter also added support for filtering on sensors. However these is not a release of node_exporter that has this change yet therefore I didn't include it in this PR
prometheus/node_exporter@e11a4f0

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

@dtrejod dtrejod marked this pull request as ready for review July 26, 2024 21:12
@dtrejod dtrejod requested a review from clayton-cornell as a code owner July 26, 2024 21:12
Comment on lines 172 to 173
| `chip_include` | `string` | Regexp of hwmon chip to (include mutually exclusive to chip-exclude). | | no |
| `chip_exclude` | `string` | Regexp of hwmon chip to (include mutually exclusive to chip-include). | | no |
Copy link
Contributor

@clayton-cornell clayton-cornell Jul 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These descriptions are.... hard to understand.

  1. We try to avoid using brackets in the text descriptions.
  2. If you do use brackets like this, it's for additional information and the text should make sense without the bracketed additional info. If I take out the bracketed info I'm left with Regexp of hwmon chip to which is incomplete.

Sometimes people use brackets for info that would fit just fine within the sentence. Even taking out the just brackets and leaving the text, I'm unable to really understand the description.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc here is not correct, the correct version should be:

  • Regexp of hwmon chip to include (mutually exclusive to chip-exclude).
  • Regexp of hwmon chip to exclude (mutually exclusive to chip-include).

If we want to avoid the brackets, I would suggest the following:

  • Regexp of hwmon chip to include
  • Regexp of hwmon chip to exclude

And below we can add a text saying "chip_include and chip_exclude are mutually exclusive".

@clayton-cornell feel free to change the wording

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it makes a LOT more sense. Thanks.

Copy link
Contributor Author

@dtrejod dtrejod Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I poorly copied the upstream help docs. Updated to use the proposed new wording.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now.

@clayton-cornell clayton-cornell added the type/docs Docs Squad label across all Grafana Labs repos label Jul 30, 2024
Copy link
Contributor

@wildum wildum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for adding this!

@wildum wildum merged commit c711385 into grafana:main Aug 1, 2024
15 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age type/docs Docs Squad label across all Grafana Labs repos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants