Fix Prometheus node exporter path#986
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughRemoved the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.3)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
deployment/terraform/strings.go (1)
580-580: Consider passing arguments from the environment file.The
EnvironmentFileat line 579 loads/etc/default/prometheus-node-exporter, which definesARGS="--collector.ethtool"(lines 555-557). However, theExecStartcommand doesn't reference$ARGS, so these arguments won't be passed to the exporter.If the
--collector.ethtoolflag is intended to be active, the ExecStart should be:🔧 Proposed fix to enable environment arguments
-ExecStart=/usr/bin/prometheus-node-exporter +ExecStart=/usr/bin/prometheus-node-exporter $ARGSIf the
ethtoolcollector is not needed, consider removing theARGSdefinition fromprometheusNodeExporterConfig(lines 555-557) to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployment/terraform/strings.go` at line 580, The systemd unit loads /etc/default/prometheus-node-exporter via EnvironmentFile but ExecStart (/usr/bin/prometheus-node-exporter) ignores the ARGS variable defined there, so either update the unit's ExecStart to include the ARGS environment variable (e.g., append $ARGS to the command) so flags like --collector.ethtool are passed, or remove the ARGS definition from prometheusNodeExporterConfig if that collector is not desired; locate the unit configuration that sets EnvironmentFile and ExecStart and adjust ExecStart to reference $ARGS (or delete the ARGS lines in prometheusNodeExporterConfig) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@deployment/terraform/strings.go`:
- Line 580: The systemd unit loads /etc/default/prometheus-node-exporter via
EnvironmentFile but ExecStart (/usr/bin/prometheus-node-exporter) ignores the
ARGS variable defined there, so either update the unit's ExecStart to include
the ARGS environment variable (e.g., append $ARGS to the command) so flags like
--collector.ethtool are passed, or remove the ARGS definition from
prometheusNodeExporterConfig if that collector is not desired; locate the unit
configuration that sets EnvironmentFile and ExecStart and adjust ExecStart to
reference $ARGS (or delete the ARGS lines in prometheusNodeExporterConfig)
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 14035322-05d0-4f6d-a261-ca9121b907d3
📒 Files selected for processing (1)
deployment/terraform/strings.go
The flag was defined in an ARGS envvar in an environment file that was included in the systemd file, but the envvar itself was never used. Use the flag directly in the invocation of the binary.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
deployment/terraform/strings.go (1)
570-579: Good fix: path and flag change correctly restores node metrics.The updated path
/usr/bin/prometheus-node-exportermatches the Debian/Ubuntu package location, and embedding--collector.ethtooldirectly inExecStartensures the flag is actually applied (previously theARGSenv var was never consumed).Consider removing the orphaned
EnvironmentFileline.Since the code that uploaded
/etc/default/prometheus-node-exporterwas removed fromagent.goandcreate.go, line 575 now references a file that will never exist. The-prefix prevents failures, but keeping this line is misleading and may confuse future maintainers.♻️ Suggested cleanup
const prometheusNodeExporterServiceFile = `[Unit] Description=Node Exporter [Service] -# Fallback when environment file does not exist -EnvironmentFile=-/etc/default/prometheus-node-exporter ExecStart=/usr/bin/prometheus-node-exporter --collector.ethtool [Install] WantedBy=multi-user.target`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployment/terraform/strings.go` around lines 570 - 579, The systemd unit string constant prometheusNodeExporterServiceFile still contains an orphaned EnvironmentFile line that no longer corresponds to any uploaded file; edit the prometheusNodeExporterServiceFile constant to remove the `EnvironmentFile=-/etc/default/prometheus-node-exporter` line so the unit only contains the Description, Service (with ExecStart=/usr/bin/prometheus-node-exporter --collector.ethtool) and Install sections, ensuring the unit file reflects current behavior and avoids misleading references.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@deployment/terraform/strings.go`:
- Around line 570-579: The systemd unit string constant
prometheusNodeExporterServiceFile still contains an orphaned EnvironmentFile
line that no longer corresponds to any uploaded file; edit the
prometheusNodeExporterServiceFile constant to remove the
`EnvironmentFile=-/etc/default/prometheus-node-exporter` line so the unit only
contains the Description, Service (with
ExecStart=/usr/bin/prometheus-node-exporter --collector.ethtool) and Install
sections, ensuring the unit file reflects current behavior and avoids misleading
references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 877d52bf-f8ee-4ee3-961f-d26ed087caf6
📒 Files selected for processing (3)
deployment/terraform/agent.godeployment/terraform/create.godeployment/terraform/strings.go
💤 Files with no reviewable changes (2)
- deployment/terraform/create.go
- deployment/terraform/agent.go
deployment/terraform/strings.go
Outdated
| @@ -576,8 +572,7 @@ Description=Node Exporter | |||
|
|
|||
| [Service] | |||
| # Fallback when environment file does not exist | |||
There was a problem hiding this comment.
maybe we can modify or remove the comment
2 participants
Summary
The path for the Prometheus node exporter binary has changed without us noticing (probably due to the dependency from
sudo apt-get install -y prometheus-node-exporterbeing updated), so we were missing node metrics like CPU and memory.Ticket Link
--