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

Bump node_exporter version to v1.5.0 #2629

Merged
merged 9 commits into from
Jan 30, 2023
Merged

Conversation

Thor77
Copy link
Contributor

@Thor77 Thor77 commented Dec 18, 2022

PR Description

Update node_exporter integration to v1.5.0
Specific reason for the update: in v1.3.1 the zfs_zpool_state metric is not available with my ZFS version, prometheus/node_exporter#2451 solves that issue and is included in v1.4.0.

PR Checklist

  • CHANGELOG updated

@CLAassistant
Copy link

CLAassistant commented Dec 18, 2022

CLA assistant check
All committers have signed the CLA.

@tpaschalis
Copy link
Member

tpaschalis commented Dec 19, 2022

Hey there, thanks for the PR! 👋

Just a small note, the version listed in the go.mod file github.com/prometheus/node_exporter v1.3.1-0.20220127103407-4f27a4fd8efa is a little misleading; the first part v1.3.1 is a pseudo-version. The actual commit that we depend on is 4f27a4fd8efa.

Could you also take a look at the failing test and mention the new collectors that were introduced in node_exporter's 1.4.0 in our documentation?

@Thor77
Copy link
Contributor Author

Thor77 commented Dec 20, 2022

@tpaschalis Thanks for taking a look!
I see, thanks for the explanation, that pseudo version still doesn't "contain" the PR fixing the ZFS issue, though.

The test is failing because the flag is available for Darwin now (collector was refactored in prometheus/node_exporter#2417).
I didn't realize it would need this many changes but I added documentation for the new collectors, a migration for the deprecated diskstats ignored devices flag and configuration options for the new sysctl collector.
Please let me know if I missed something or should implement it differently.
I'm particularly unsure about the changes in component/prometheus/integration/node_exporter as there doesn't seem to be a migration path for those? Should I just leave the config names as is then?

Copy link
Contributor

@karengermond karengermond left a comment

Choose a reason for hiding this comment

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

LGTM

@tpaschalis
Copy link
Member

Hey Jonas! This looks reasonable at a first glance, but would like to sanity check it. Just a heads up, reviews might be a little slower due to most of the reviewers being away on PTO, but it shouldn't be too long!

Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! This needs a look from the CODEOWNERS for final review, but I left a quick comment :)

Comment on lines -105 to +106
DiskStatsIgnoredDevices string `yaml:"diskstats_ignored_devices,omitempty"`
DiskStatsDeviceExclude string `yaml:"diskstats_device_exclude,omitempty"`
DiskStatsDeviceInclude string `yaml:"diskstats_device_include,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

This would be considered a breaking change, so we'd need to document it as such in the upgrade guide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This shouldn't be breaking because there's a migration from the old to the new value. But I added a note to the changelog and upgrade-guide and also rebased on main to resolve the merge conflict.

@@ -127,6 +128,8 @@ type Config struct {
PowersupplyIgnoredSupplies string `yaml:"powersupply_ignored_supplies,omitempty"`
RunitServiceDir string `yaml:"runit_service_dir,omitempty"`
SupervisordURL string `yaml:"supervisord_url,omitempty"`
SysctlInclude flagext.StringSlice `yaml:"sysctl_include,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we want to expose this values here too: component/prometheus/integration/node_exporter/config.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added and also rebased again because of the merge conflicts in go.mod

CHANGELOG.md Outdated
@@ -25,6 +26,10 @@ v0.31.0-rc.0 (2023-01-26)
- `agentctl` is now `grafana-agentctl`.
- `agent-operator` is now `grafana-agent-operator`.

- Node Exporter configuration options changed to align with new upstream version (@Thor77):
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be in Main (unreleased) section

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I messed this up while rebasing, fixed.

@marctc marctc merged commit 3725047 into grafana:main Jan 30, 2023
rfratto added a commit to rfratto/agent that referenced this pull request Jan 31, 2023
rfratto added a commit that referenced this pull request Jan 31, 2023
* Flow: support reloading when receiving a SIGHUP

This matches the behavior of the static mode.

* deb, rpm: respond to `systemctl reload grafana-agent`

Send a SIGHUP signal to Grafana Agent when `systemctl reload
grafana-agent` is invoked.

* update CHANGELOG

* fix broken river stanza in prometheus.integration.node_exporter

Fixes issue introduced by #2629
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Mar 11, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants