node_exporter_metrics: Add file path to error messages#11839
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR refactors three core filesystem-reading utility functions to accept a context pointer for improved error reporting. All 15 collector modules in the node exporter plugin are updated to pass the context at call sites, enabling better error diagnostics through plugin logging. ChangesUtility functions context propagation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44bf2d2665
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/in_node_exporter_metrics/ne_utils.c`:
- Around line 135-141: The code frees the SDS string p via flb_sds_destroy(p)
before calling read(fd, ...) and then uses p in the error log
flb_plg_error(ctx->ins, "could not read from '%s'", p), leading to a
use-after-free; to fix, delay calling flb_sds_destroy(p) until after the read()
result is handled (i.e., after the if (bytes == -1) block and any flb_plg_error
that references p), and apply the same change to the second identical block
around the other read() (the one at the 280-286 region) so both read/error paths
log p while it remains valid.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 780dcfd7-8ada-4a79-b714-4c89712daf30
📒 Files selected for processing (17)
plugins/in_node_exporter_metrics/ne_cpu_linux.cplugins/in_node_exporter_metrics/ne_cpufreq_linux.cplugins/in_node_exporter_metrics/ne_diskstats_linux.cplugins/in_node_exporter_metrics/ne_filefd_linux.cplugins/in_node_exporter_metrics/ne_hwmon_linux.cplugins/in_node_exporter_metrics/ne_loadavg_linux.cplugins/in_node_exporter_metrics/ne_meminfo_linux.cplugins/in_node_exporter_metrics/ne_netdev_linux.cplugins/in_node_exporter_metrics/ne_netstat_linux.cplugins/in_node_exporter_metrics/ne_nvme_linux.cplugins/in_node_exporter_metrics/ne_processes_linux.cplugins/in_node_exporter_metrics/ne_sockstat_linux.cplugins/in_node_exporter_metrics/ne_stat_linux.cplugins/in_node_exporter_metrics/ne_thermalzone_linux.cplugins/in_node_exporter_metrics/ne_utils.cplugins/in_node_exporter_metrics/ne_utils.hplugins/in_node_exporter_metrics/ne_vmstat_linux.c
Improve error logs to indicate which file path failed when reading metrics, making it easier to debug missing hardware sensors or permissions issues. Signed-off-by: Ross Golder <ross@golder.org>
44bf2d2 to
790aeda
Compare
Signed-off-by: Ross Golder <ross@golder.org>
790aeda to
507ee88
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/in_node_exporter_metrics/ne_utils.h (1)
44-44: ⚡ Quick winWrap this prototype to match the 100-char line-length target.
This declaration exceeds the repository style target; split arguments across lines like the adjacent prototypes for consistency.
As per coding guidelines: "Use 4-space tabs/indentation and target 100 chars per line; 120 chars max."
Suggested diff
-int ne_utils_file_read_lines(struct flb_ne *ctx, const char *mount, const char *path, struct mk_list *list); +int ne_utils_file_read_lines(struct flb_ne *ctx, + const char *mount, + const char *path, + struct mk_list *list);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/in_node_exporter_metrics/ne_utils.h` at line 44, The function prototype ne_utils_file_read_lines exceeds the repository's 100-character line-length target. Refactor this prototype by splitting the function arguments across multiple lines, using 4-space indentation, similar to the style of adjacent prototypes in the file. Ensure the reformatted declaration respects the 100-character target with a maximum of 120 characters per line.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@plugins/in_node_exporter_metrics/ne_utils.h`:
- Line 44: The function prototype ne_utils_file_read_lines exceeds the
repository's 100-character line-length target. Refactor this prototype by
splitting the function arguments across multiple lines, using 4-space
indentation, similar to the style of adjacent prototypes in the file. Ensure the
reformatted declaration respects the 100-character target with a maximum of 120
characters per line.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f1f2db50-6a07-4f02-a3ab-60e84042ef26
📒 Files selected for processing (17)
plugins/in_node_exporter_metrics/ne_cpu_linux.cplugins/in_node_exporter_metrics/ne_cpufreq_linux.cplugins/in_node_exporter_metrics/ne_diskstats_linux.cplugins/in_node_exporter_metrics/ne_filefd_linux.cplugins/in_node_exporter_metrics/ne_hwmon_linux.cplugins/in_node_exporter_metrics/ne_loadavg_linux.cplugins/in_node_exporter_metrics/ne_meminfo_linux.cplugins/in_node_exporter_metrics/ne_netdev_linux.cplugins/in_node_exporter_metrics/ne_netstat_linux.cplugins/in_node_exporter_metrics/ne_nvme_linux.cplugins/in_node_exporter_metrics/ne_processes_linux.cplugins/in_node_exporter_metrics/ne_sockstat_linux.cplugins/in_node_exporter_metrics/ne_stat_linux.cplugins/in_node_exporter_metrics/ne_thermalzone_linux.cplugins/in_node_exporter_metrics/ne_utils.cplugins/in_node_exporter_metrics/ne_utils.hplugins/in_node_exporter_metrics/ne_vmstat_linux.c
✅ Files skipped from review due to trivial changes (7)
- plugins/in_node_exporter_metrics/ne_sockstat_linux.c
- plugins/in_node_exporter_metrics/ne_meminfo_linux.c
- plugins/in_node_exporter_metrics/ne_thermalzone_linux.c
- plugins/in_node_exporter_metrics/ne_filefd_linux.c
- plugins/in_node_exporter_metrics/ne_netstat_linux.c
- plugins/in_node_exporter_metrics/ne_cpufreq_linux.c
- plugins/in_node_exporter_metrics/ne_netdev_linux.c
🚧 Files skipped from review as they are similar to previous changes (6)
- plugins/in_node_exporter_metrics/ne_nvme_linux.c
- plugins/in_node_exporter_metrics/ne_vmstat_linux.c
- plugins/in_node_exporter_metrics/ne_cpu_linux.c
- plugins/in_node_exporter_metrics/ne_processes_linux.c
- plugins/in_node_exporter_metrics/ne_hwmon_linux.c
- plugins/in_node_exporter_metrics/ne_utils.c
Signed-off-by: Ross Golder <ross@golder.org>
Problem: When the node_exporter_metrics input plugin fails to read hardware sensors (hwmon, thermal zones, cpufreq), the error logs only show:
This is useless for debugging because users have no way to know which file/path is missing.
Solution: Add the file path to error messages in ne_utils_file_read_uint64, ne_utils_file_read_sds, and ne_utils_file_read_lines functions.
Example after fix:
Closes #11838
Summary by CodeRabbit
/proc/loadavgread result rather than a stale/uninitialized value.