in_podman_metrics: add per-container disk I/O metrics#11986
Open
stondo wants to merge 1 commit into
Open
Conversation
Author
Local checks
Device validationBuilt into a Yocto firmware (Fluent Bit 5.0.2) and run on an aarch64 Linux Cross-checked against each container's cgroup A companion |
📝 WalkthroughWalkthroughThe podman metrics input plugin gains four disk I/O counters ( ChangesDisk I/O metrics for podman metrics plugin
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugins/in_podman_metrics/podman_metrics_data.c (1)
414-417: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid reading
io.statfour times per container scrape.These four calls open and parse the same file repeatedly. A single-pass parser that extracts all four fields at once would cut sysfs I/O and reduce repeated failure warnings on missing files.
🤖 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_podman_metrics/podman_metrics_data.c` around lines 414 - 417, The four consecutive calls to sum_io_stat_field are each opening and parsing the same V2_SYSFS_FILE_IO_STAT file separately, causing unnecessary sysfs I/O and repeated error handling. Refactor this code to create a single-pass parser function that reads the io.stat file once and extracts all four fields (IO_STAT_KEY_READ_BYTES, IO_STAT_KEY_WRITE_BYTES, IO_STAT_KEY_READS, IO_STAT_KEY_WRITES) in a single operation, then assign the extracted values to cnt->disk_read_bytes, cnt->disk_write_bytes, cnt->disk_reads, and cnt->disk_writes respectively.
🤖 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_podman_metrics/podman_metrics_data.c`:
- Line 175: The debug logging statements are using the `%lu` format specifier to
print `uint64_t` values (the variables `data` and `total`), which is not
portable across different ABIs and systems. Replace the `%lu` format specifiers
with `PRIu64` macro in both the flb_plg_debug call containing the `data`
variable and the flb_plg_debug call containing the `total` variable to ensure
portable formatting of `uint64_t` values across different platforms.
---
Nitpick comments:
In `@plugins/in_podman_metrics/podman_metrics_data.c`:
- Around line 414-417: The four consecutive calls to sum_io_stat_field are each
opening and parsing the same V2_SYSFS_FILE_IO_STAT file separately, causing
unnecessary sysfs I/O and repeated error handling. Refactor this code to create
a single-pass parser function that reads the io.stat file once and extracts all
four fields (IO_STAT_KEY_READ_BYTES, IO_STAT_KEY_WRITE_BYTES, IO_STAT_KEY_READS,
IO_STAT_KEY_WRITES) in a single operation, then assign the extracted values to
cnt->disk_read_bytes, cnt->disk_write_bytes, cnt->disk_reads, and
cnt->disk_writes respectively.
🪄 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: 672718c9-ae70-4caa-b76f-bce98160e465
📒 Files selected for processing (4)
plugins/in_podman_metrics/podman_metrics.cplugins/in_podman_metrics/podman_metrics_config.hplugins/in_podman_metrics/podman_metrics_data.cplugins/in_podman_metrics/podman_metrics_data.h
| flb_free(line); | ||
| fclose(fp); | ||
|
|
||
| flb_plg_debug(ctx->ins, "%s [%s]: %lu", path, key, total); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify format-specifier usage around uint64_t logging in this plugin file.
sed -n '1,40p' plugins/in_podman_metrics/podman_metrics_data.c
rg -n --type=c 'flb_plg_debug\(.*%l[udx]' plugins/in_podman_metrics/podman_metrics_data.c
rg -n --type=c '\buint64_t\b' plugins/in_podman_metrics/podman_metrics_data.cRepository: fluent/fluent-bit
Length of output: 2543
🏁 Script executed:
# Check if inttypes.h is included anywhere in the file
rg -n 'inttypes\.h' plugins/in_podman_metrics/podman_metrics_data.c
# Read the context around line 175 to see the function and variable declarations
sed -n '140,180p' plugins/in_podman_metrics/podman_metrics_data.c
# Also check line 131 context
sed -n '120,135p' plugins/in_podman_metrics/podman_metrics_data.cRepository: fluent/fluent-bit
Length of output: 1486
Use uint64_t-safe format specifiers in debug logging.
Lines 131 and 175 format uint64_t values (data and total) with %lu, which is not portable across ABIs. Use PRIu64 (or an explicit cast with a matching specifier).
Proposed fix
+#include <inttypes.h>
...
- flb_plg_debug(ctx->ins, "%s: %lu", path, data);
+ flb_plg_debug(ctx->ins, "%s: %" PRIu64, path, data);
...
- flb_plg_debug(ctx->ins, "%s [%s]: %lu", path, key, total);
+ flb_plg_debug(ctx->ins, "%s [%s]: %" PRIu64, path, key, total);🤖 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_podman_metrics/podman_metrics_data.c` at line 175, The debug
logging statements are using the `%lu` format specifier to print `uint64_t`
values (the variables `data` and `total`), which is not portable across
different ABIs and systems. Replace the `%lu` format specifiers with `PRIu64`
macro in both the flb_plg_debug call containing the `data` variable and the
flb_plg_debug call containing the `total` variable to ensure portable formatting
of `uint64_t` values across different platforms.
Author
|
Companion documentation PR: fluent/fluent-bit-docs#2608 |
Add four counters exposing per-container block I/O, read from the cgroups v2 io.stat file and summed across block devices: - container_disk_read_bytes_total - container_disk_write_bytes_total - container_disk_reads_total - container_disk_writes_total This complements the existing CPU, memory and network metrics. The values are collected in the cgroups v2 path; on cgroups v1 hosts the counters are reported as invalid and skipped. Signed-off-by: Stefano Tondo <stondo@gmail.com>
stondo
force-pushed
the
in_podman_metrics-disk-io
branch
from
5958c56 to
32fc804
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
plugins/in_podman_metrics/podman_metrics_data.c (1)
197-199: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse portable
uint64_tformatting in the debug log.
%luis not portable foruint64_t; usePRIu64to avoid ABI-dependent output.Proposed fix
+#include <inttypes.h> ... - flb_plg_debug(ctx->ins, "%s: rbytes=%lu wbytes=%lu rios=%lu wios=%lu", path, + flb_plg_debug(ctx->ins, "%s: rbytes=%" PRIu64 " wbytes=%" PRIu64 " rios=%" PRIu64 " wios=%" PRIu64, path, cnt->disk_read_bytes, cnt->disk_write_bytes, cnt->disk_reads, cnt->disk_writes);#!/bin/bash # Verify non-portable uint64_t printf usage and whether inttypes.h is present. rg -n --type=c 'flb_plg_debug\(.*%l[udx]' plugins/in_podman_metrics/podman_metrics_data.c rg -n --type=c '`#include` <inttypes.h>' plugins/in_podman_metrics/podman_metrics_data.c🤖 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_podman_metrics/podman_metrics_data.c` around lines 197 - 199, The debug log in podman_metrics_data.c uses non-portable %lu specifiers for uint64_t counters, which can break on different ABIs. Update the flb_plg_debug call in the disk metrics logging path to use PRIu64 for cnt->disk_read_bytes, cnt->disk_write_bytes, cnt->disk_reads, and cnt->disk_writes, and make sure the file includes inttypes.h so the format macros are available.
🤖 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.
Duplicate comments:
In `@plugins/in_podman_metrics/podman_metrics_data.c`:
- Around line 197-199: The debug log in podman_metrics_data.c uses non-portable
%lu specifiers for uint64_t counters, which can break on different ABIs. Update
the flb_plg_debug call in the disk metrics logging path to use PRIu64 for
cnt->disk_read_bytes, cnt->disk_write_bytes, cnt->disk_reads, and
cnt->disk_writes, and make sure the file includes inttypes.h so the format
macros are available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36ad8236-cddb-4f2f-9350-530cbef9fdfb
📒 Files selected for processing (4)
plugins/in_podman_metrics/podman_metrics.cplugins/in_podman_metrics/podman_metrics_config.hplugins/in_podman_metrics/podman_metrics_data.cplugins/in_podman_metrics/podman_metrics_data.h
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/in_podman_metrics/podman_metrics.c
- plugins/in_podman_metrics/podman_metrics_config.h
Author
|
Addressed in
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add four per-container block-I/O counters to
in_podman_metrics, read from thecgroups v2
io.statfile and summed across block devices:container_disk_read_bytes_totalcontainer_disk_write_bytes_totalcontainer_disk_reads_totalcontainer_disk_writes_totalThis complements the plugin's existing CPU, memory and network metrics so the
input reports a full CPU / memory / disk / network picture per container.
Implementation
sum_io_stat_field()helper parsesio.stat(lines like8:0 rbytes=.. wbytes=.. rios=.. wios=..) and sums one field across alldevices. It returns
UINT64_MAX(skipped bycreate_counter) when the filecannot be read, and a valid
0for an existing-but-empty file.fill_counters_with_sysfs_data_v2). Oncgroups v1 the fields stay
UINT64_MAX, so the counters are simply skipped.create_countercalls follow the existingCPU / memory / network pattern.
Example
Testing
cmake --build build --target flb-plugin-in_podman_metricscompletes with no warnings on the changed files.
correct per-container values (cross-checked against the containers'
io.stat);unset on cgroups v1; existing CPU/memory/network output unchanged.
Documentation
fluent-bit-docsupdate will list the new metrics.Signed-off-by: Stefano Tondo stondo@gmail.com
Summary by CodeRabbit