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

feat(prom/exp/snmp): add support to pass multiple SNMP config files to prometheus.exporter.snmp #967

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

hainenber
Copy link
Contributor

PR Description

Which issue(s) this PR fixes

Closes #916

Notes to the Reviewer

I decided not to add another test case into snmp_test.go as it doesn't test whole component with argument but rather just a unit test for LoadSNMP function. Incoming integration test in #954 will address this.

PR Checklist

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

…o `prometheus.exporter.snmp`

Signed-off-by: hainenber <dotronghai96@gmail.com>
Signed-off-by: hainenber <dotronghai96@gmail.com>
…ument

Signed-off-by: hainenber <dotronghai96@gmail.com>
@clayton-cornell clayton-cornell requested a review from a team May 30, 2024 19:13
@clayton-cornell clayton-cornell added the type/docs Docs Squad label across all Grafana Labs repos label May 30, 2024
Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
@bastischubert
Copy link
Contributor

Hi,
does this change support globbing, like the mentioned upstream PR (prometheus/snmp_exporter#970)? This will be needed to make the cloud integration have a better out of the box experience for users running with their own credentials (instead of using "public" community string)

@hainenber
Copy link
Contributor Author

I think it does! Not yet testing it though (actually im waiting for the integration test for snmp got merged first)


The `config_file` argument points to a YAML file defining which snmp_exporter modules to use.
Refer to [snmp_exporter](https://github.com/prometheus/snmp_exporter#generating-configuration) for details on how to generate a configuration file.

Each file listed in in the `config_files` argument must conform to the `config_file` requirements.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Each file listed in in the `config_files` argument must conform to the `config_file` requirements.
Each file listed in in the `config_files` argument must conform to the `config_file` requirements.
Only one of `config_file` or `config_files` can be provided.

Comment on lines +145 to +146
SnmpConfigFile: a.ConfigFile,
SnmpConfigFiles: a.ConfigFiles,
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be simpler if we return an error if both config_files and config_file are specified. In this function we could create a config_files out of a config_file, and only pass config_files to snmp_exporter.Config. The static mode code doesn't need to support config_file because it's not used in Alloy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/docs Docs Squad label across all Grafana Labs repos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

prometheus.exporter.snmp to support multiple config files
4 participants