out_logrotate: add new out plugin #11110
Conversation
SagiROosto
requested review from
celalettin1286,
cosmo0920,
edsiper,
fujimotos,
niedbalski and
patrick-stephens
as code owners
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new out_logrotate output plugin (sources, header, CMake), an example configuration, runtime tests, and build-option wiring (FLB_OUT_LOGROTATE enabled under FLB_ALL and Windows defaults). Changes
Sequence Diagram(s)sequenceDiagram
participant Input as Input Plugin
participant Out as out_logrotate
participant FS as File System
participant GZ as Gzip (optional)
Input->>Out: flush event chunk
Out->>FS: ensure directory exists / open current file
Out->>Out: check/track current file size
alt size >= Max_Size
Out->>FS: rename current -> rotated with timestamp
alt Gzip enabled
Out->>GZ: stream-compress rotated file
GZ-->>FS: write rotated .gz file
FS->>Out: remove uncompressed rotated file
end
Out->>FS: cleanup old rotated files (Max_Files)
Out->>FS: create new current file
end
Out->>Out: format records (JSON/CSV/LTSV/PLAIN/TEMPLATE/MSGPACK)
Out->>FS: append formatted records to current file
Out-->>Input: acknowledge flush
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CMakeLists.txt(1 hunks)cmake/plugins_options.cmake(1 hunks)cmake/windows-setup.cmake(1 hunks)conf/fluent-bit-logrotate.conf(1 hunks)plugins/CMakeLists.txt(1 hunks)plugins/out_logrotate/CMakeLists.txt(1 hunks)plugins/out_logrotate/logrotate.c(1 hunks)plugins/out_logrotate/logrotate.h(1 hunks)tests/runtime/CMakeLists.txt(1 hunks)tests/runtime/out_logrotate.c(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
tests/runtime/CMakeLists.txtcmake/windows-setup.cmakeplugins/out_logrotate/CMakeLists.txtCMakeLists.txt
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit, the correct CMake flag for using system librdkafka is `FLB_PREFER_SYSTEM_LIB_KAFKA=ON`.
Applied to files:
CMakeLists.txt
🧬 Code graph analysis (2)
tests/runtime/out_logrotate.c (1)
src/flb_lib.c (9)
flb_create(143-225)flb_input(266-276)flb_input_set(305-335)flb_output(279-289)flb_output_set(520-551)flb_start(983-994)flb_lib_push(843-870)flb_stop(1011-1055)flb_destroy(228-263)
plugins/out_logrotate/logrotate.c (6)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1298-1321)src/flb_output.c (2)
flb_output_get_property(1096-1099)flb_output_set_context(1524-1527)src/flb_pack.c (1)
flb_msgpack_to_json_str(1459-1500)lib/cmetrics/src/cmt_decode_msgpack.c (1)
cmt_decode_msgpack_create(1450-1496)src/flb_log_event_decoder.c (3)
flb_log_event_decoder_init(103-120)flb_log_event_decoder_next(309-456)flb_log_event_decoder_destroy(151-178)
🔇 Additional comments (8)
conf/fluent-bit-logrotate.conf (1)
1-21: LGTM! Configuration demonstrates plugin features well.The example configuration effectively showcases the logrotate plugin's key features: size-based rotation (Max_Size 10M), file retention (Max_Files 5), gzip compression, and automatic directory creation.
Note: The path
/tmp/logsis Linux-specific. While this is acceptable for an example, consider documenting that users on Windows would need to adjust the path (e.g.,C:\logs).plugins/out_logrotate/CMakeLists.txt (1)
1-5: LGTM! Build configuration is correct.The CMake configuration properly declares the ZLIB dependency (required for gzip compression) and follows the standard plugin registration pattern used throughout Fluent Bit.
plugins/CMakeLists.txt (1)
366-367: LGTM! Plugin registration is correct.The
out_logrotateplugin is properly registered following the same pattern as other output plugins.cmake/plugins_options.cmake (1)
120-120: LGTM! Build option properly defined.The plugin option is correctly defined with an appropriate description and default value (ON), consistent with other output plugins in the codebase.
CMakeLists.txt (1)
301-301: LGTM! Plugin enabled in all-features build.The
FLB_OUT_LOGROTATEflag is correctly set within theFLB_ALLblock, ensuring the plugin is included when building with all features enabled.cmake/windows-setup.cmake (1)
103-103: LGTM! Windows support enabled.The plugin is enabled for Windows builds following the standard pattern used for other output plugins in this file.
Note: Runtime tests for this plugin are currently only registered for non-Windows systems (see
tests/runtime/CMakeLists.txtline 237). If Windows support is intended, consider adding Windows-specific tests or verifying the existing tests work on Windows.plugins/out_logrotate/logrotate.h (1)
1-32: LGTM! Header structure is clean and correct.The header properly defines output format constants for the logrotate plugin with appropriate include guards and licensing. The enum provides clear format options (JSON, CSV, LTSV, PLAIN, MSGPACK, TEMPLATE) that align with the PR objectives for multi-format support.
tests/runtime/CMakeLists.txt (1)
235-238: Discrepancy confirmed: out_logrotate is enabled on Windows in cmake/windows-setup.cmake but excluded from runtime tests.Verification shows the review comment's concern is valid. The out_logrotate plugin:
- Is set to enabled (
Yes) on Windows in cmake/windows-setup.cmake line 103- Contains extensive Windows-specific code: Windows headers (Shlobj.h, Shlwapi.h), Windows API calls (strncpy_s, localtime_s, PathRemoveFileSpecA), and platform-specific macros
- Has no Windows-specific build exclusions in its own CMakeLists.txt
Yet the runtime test is excluded on Windows (tests/runtime/CMakeLists.txt line 236-238) under a comment stating "These plugins work only on Linux"—contradicting the Windows code present in the source.
The same pattern applies to out_file, which also has Windows-specific code but excluded tests.
Action needed: Manually verify whether Windows support is production-ready and either extend tests to cover Windows or disable the plugin on Windows if support is incomplete.
|
Once the coderabbit comments are addressed we should trigger a full package build but @SagiROosto can you confirm it builds for all targets? |
|
@patrick-stephens UT failed for typo issue I Fixed it along with coderabbit suggestions. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/runtime/out_logrotate.c (1)
294-354: Address the past review concern and clarify expected file count.The addition of
TEST_ASSERT(file_count >= 0)at lines 340 and 349 correctly addresses the past review concern about directory listing failures being silently ignored. Good fix!However, there's an inconsistency in the comment at line 350: it states "Current file + 2 rotated files" but checks
file_count <= 4. Withmax_files=3configured at line 324, the expected count should be 1 current file + 3 rotated files = 4 total, not "2 rotated files" as the comment suggests.Consider updating the comment to match the actual logic:
- TEST_CHECK(file_count <= 4); /* Current file + 2 rotated files */ + TEST_CHECK(file_count <= 4); /* Current file + 3 rotated files (max_files=3) */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/CMakeLists.txt(1 hunks)plugins/out_logrotate/logrotate.c(1 hunks)tests/runtime/out_logrotate.c(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
plugins/out_logrotate/logrotate.c
🔇 Additional comments (11)
plugins/CMakeLists.txt (1)
366-366: LGTM!The plugin registration follows the established pattern and is correctly positioned alphabetically between
out_fileandout_forward.tests/runtime/out_logrotate.c (3)
36-154: LGTM!The helper functions are well-structured with appropriate error handling and resource cleanup. The recursive directory deletion, file counting, and pattern matching logic are all correct.
156-229: LGTM!The basic rotation test correctly configures the plugin, triggers rotation by writing sufficient data, and validates that both the original file and rotated files exist.
231-292: LGTM!The gzip compression test properly validates that rotated files are compressed by waiting for
.gzfiles to appear, accounting for potential async compression.plugins/out_logrotate/logrotate.c (7)
106-206: LGTM!The initialization function correctly sets up the plugin context, creates the hash table for per-file size tracking, parses configuration options, and includes proper error handling with resource cleanup.
531-576: LGTM!The file size tracking correctly uses a hash table to maintain per-file size state, addressing the previous concern about a shared global counter. The
should_rotate_filefunction retrieves size by filename, andupdate_file_size_counterusesfstatto read actual file size before updating the hash table.
625-745: LGTM!The streaming gzip compression implementation is memory-efficient and correct:
- Processes files in 64KB chunks to avoid loading entire files into memory
- Properly initializes deflate with raw deflate mode (negative window bits)
- Writes RFC 1952 compliant gzip header and footer with CRC32
- Verifies all input is consumed and compression completes with
Z_STREAM_END- Includes proper error handling and resource cleanup
860-944: LGTM!The cleanup logic correctly:
- Counts rotated files matching the timestamp pattern
- Returns early if within the
max_fileslimit- Sorts files by modification time (oldest first) using a simple bubble sort
- Removes excess files to maintain the
max_fileslimitThe approach ensures that rotated files are cleaned up while preserving the current active file.
1067-1081: LGTM!The metrics handling path now correctly calls
update_file_size_counter(ctx, out_file, fp)at line 1078 before closing the file, which addresses the previous review concern about stale file size counters preventing rotation for metrics-only streams.
946-1182: LGTM!The flush callback implements the complete workflow correctly:
- Checks rotation condition using per-file size from hash table
- Performs rotation and cleanup when needed
- Removes rotated file's entry from hash table to reset tracking
- Initializes file size from disk for new files using
fstat- Updates file size counter after all write operations (logs, metrics, msgpack)
The per-file size tracking ensures correct behavior when multiple files are managed by a single plugin instance.
1184-1274: LGTM!The exit function properly cleans up resources by destroying the hash table and freeing the context. The configuration map is comprehensive with sensible defaults: 100MB max size, 7 max files retained, and gzip compression enabled by default.
|
You'll have to fix the commits that don't match the guidelines btw, i.e. you need a plugin/module prefix like |
SagiROosto
force-pushed
the
out_logrotate
branch
2 times, most recently
from
0dd42f9 to
99917b8
Compare
* add logrotate out plugin * fix gzip * fix test * update max_size * update gzip * add logs Signed-off-by: SagiROosto <[email protected]>
Signed-off-by: SagiROosto <[email protected]>
Signed-off-by: SagiROosto <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
plugins/out_logrotate/logrotate.c (8)
129-134: Consider a larger hash table size for multi-file scenarios.The hash table is created with only 64 buckets. If the plugin writes to many different files (when
fileis not set and tags are used as filenames), this could lead to excessive hash collisions and degraded performance. Consider using a larger size (e.g., 256 or 512) or making it configurable for high-throughput, multi-file deployments.
605-623: Document the 4GB limitation for gzip compression.The gzip footer stores the original file size in 32 bits (4 bytes), which limits support to files under 4GB. While this is a standard gzip limitation, files larger than 4GB will have their size truncated modulo 2^32, which could cause decompression tools to report incorrect sizes or fail validation. Consider logging a warning when compressing files approaching or exceeding this limit, or documenting this limitation in the plugin configuration.
783-795: Check unlink() return values for error logging.Both
unlink()calls (lines 789 and 793) don't check return values. If deletion fails (e.g., permission denied), the files remain on disk but the function returns success or failure based solely on compression status. While the gzip failure path returns -1, the success path (line 790) doesn't verify that cleanup succeeded. Consider logging warnings ifunlink()fails.if (ret == 0) { /* Remove the uncompressed file */ - unlink(rotated_filename); + if (unlink(rotated_filename) != 0) { + flb_plg_warn(ctx->ins, "failed to remove uncompressed file %s", rotated_filename); + } flb_plg_debug(ctx->ins, "rotated and compressed file: %s", gzip_filename); } else { /* Remove the failed gzip file */ - unlink(gzip_filename); + if (unlink(gzip_filename) != 0) { + flb_plg_warn(ctx->ins, "failed to remove incomplete gzip file %s", gzip_filename); + } return -1; }
911-923: Bubble sort may be inefficient for many rotated files.The bubble sort has O(n²) complexity. If
max_filesis high and many files accumulate (e.g., hundreds of rotated logs), this could introduce noticeable latency during cleanup. For production environments with large retention counts, consider usingqsort()with a comparison function for O(n log n) performance.
216-242: No validation for non-map objects in CSV output.The function checks
obj->type == MSGPACK_OBJECT_MAP && obj->via.map.size > 0(line 216) but doesn't handle the case where the object is not a map or is empty. If a non-map object is passed, the function returns 0 (success) without writing anything. While this may be intentional, it could lead to silent data loss. Consider logging a warning when non-map objects are encountered.static int csv_output(FILE *fp, int column_names, struct flb_time *tm, msgpack_object *obj, struct flb_logrotate_conf *ctx) { int i; int map_size; msgpack_object_kv *kv = NULL; - if (obj->type == MSGPACK_OBJECT_MAP && obj->via.map.size > 0) { + if (obj->type != MSGPACK_OBJECT_MAP) { + flb_plg_warn(ctx->ins, "csv_output: expected map, got type %d", obj->type); + return -1; + } + + if (obj->via.map.size > 0) { kv = obj->via.map.ptr; map_size = obj->via.map.size; ... + } else { + flb_plg_debug(ctx->ins, "csv_output: empty map, skipping"); } return 0; }
252-272: No validation for non-map objects in LTSV output.Similar to
csv_output, this function silently returns success if the object is not a map or is empty (line 252). Consider adding validation and logging for consistency with error handling elsewhere in the plugin.
370-381: Silent failure in plain_output when JSON conversion fails.If
flb_msgpack_to_json_str()returns NULL, the function silently skips writing and returns 0 (success). This could lead to data loss without any indication to the caller. Consider returning -1 or logging a warning when conversion fails.static int plain_output(FILE *fp, msgpack_object *obj, size_t alloc_size, int escape_unicode) { char *buf; buf = flb_msgpack_to_json_str(alloc_size, obj, escape_unicode); if (buf) { fprintf(fp, "%s" NEWLINE, buf); flb_free(buf); + } else { + return -1; } return 0; }
383-407: print_metrics_text doesn't return error status.The function is
voidand doesn't return error status to the caller. Ifcmt_decode_msgpack_create()fails (line 394), an error is logged but the caller (line 1075) has no way to know the write failed. Consider changing the return type tointand propagating the error so the flush operation can retry or handle the failure appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/CMakeLists.txt(1 hunks)plugins/out_logrotate/logrotate.c(1 hunks)tests/runtime/out_logrotate.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/runtime/out_logrotate.c
- plugins/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
plugins/out_logrotate/logrotate.c
🧬 Code graph analysis (1)
plugins/out_logrotate/logrotate.c (7)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)src/flb_hash_table.c (5)
flb_hash_table_create(99-137)flb_hash_table_destroy(197-215)flb_hash_table_get(496-522)flb_hash_table_add(401-494)flb_hash_table_del(605-674)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1298-1321)src/flb_output.c (2)
flb_output_get_property(1096-1099)flb_output_set_context(1524-1527)src/flb_pack.c (1)
flb_msgpack_to_json_str(1459-1500)lib/cmetrics/src/cmt_decode_msgpack.c (1)
cmt_decode_msgpack_create(1450-1496)src/flb_log_event_decoder.c (3)
flb_log_event_decoder_init(103-120)flb_log_event_decoder_next(309-456)flb_log_event_decoder_destroy(151-178)
🔇 Additional comments (1)
plugins/out_logrotate/logrotate.c (1)
1073-1081: Metrics file size update now included - excellent!The past review noted that
update_file_size_counter()was missing after writing metrics, causing rotation to never trigger for metrics-only streams. This has been correctly addressed at line 1078.
SagiROosto
force-pushed
the
out_logrotate
branch
from
99917b8 to
2a77707
Compare
Signed-off-by: SagiROosto <[email protected]>
SagiROosto
force-pushed
the
out_logrotate
branch
from
2a77707 to
329cba4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CMakeLists.txt(1 hunks)cmake/plugins_options.cmake(1 hunks)cmake/windows-setup.cmake(1 hunks)conf/fluent-bit-logrotate.conf(1 hunks)plugins/CMakeLists.txt(1 hunks)plugins/out_logrotate/CMakeLists.txt(1 hunks)plugins/out_logrotate/logrotate.c(1 hunks)plugins/out_logrotate/logrotate.h(1 hunks)tests/runtime/CMakeLists.txt(1 hunks)tests/runtime/out_logrotate.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- cmake/plugins_options.cmake
- tests/runtime/out_logrotate.c
- conf/fluent-bit-logrotate.conf
- plugins/out_logrotate/CMakeLists.txt
- plugins/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
cmake/windows-setup.cmakeCMakeLists.txttests/runtime/CMakeLists.txt
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit, the correct CMake flag for using system librdkafka is `FLB_PREFER_SYSTEM_LIB_KAFKA=ON`.
Applied to files:
cmake/windows-setup.cmakeCMakeLists.txt
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
plugins/out_logrotate/logrotate.c
Signed-off-by: SagiROosto <[email protected]>
Signed-off-by: SagiROosto <[email protected]>
Signed-off-by: SagiROosto <[email protected]>
…d fix UT Signed-off-by: SagiROosto <[email protected]>
SagiROosto
force-pushed
the
out_logrotate
branch
from
329cba4 to
9d1e142
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
plugins/out_logrotate/logrotate.c (2)
578-582: Optional: Remove redundant hash table delete.Lines 579–581 delete the existing key before re-adding it, but
flb_hash_table_add()already handles replacement internally (seesrc/flb_hash_table.clines 430–442). The delete-then-add pattern adds unnecessary overhead—though it's safe, it doubles the hash lookups.Apply this diff to streamline the update:
- /* Delete existing entry if it exists to ensure we can update the value */ - flb_hash_table_del(ctx->file_sizes, filename); /* Store or update file size in hash table */ ret = flb_hash_table_add(ctx->file_sizes, filename, strlen(filename), &file_size, sizeof(size_t));The
flb_hash_table_addfunction already performs an in-place update for existing keys, so the explicit delete is not required.
783-795: Optional: Add path-length validation before constructing rotated filenames.Lines 784 and 795 use
snprintfwithPATH_MAX - 1, which will silently truncate iffilenameis too long. WhilePATH_MAXis typically large (4096 on Linux), a truncated filename would cause rotated files to be unreachable byis_valid_rotation_filename()andcleanup_old_files(), breaking rotation cleanup.Add a length check before constructing the rotated filename:
+ /* Validate that we have room for timestamp suffix */ + if (strlen(filename) + 16 >= PATH_MAX) { /* ".YYYYMMDD_HHMMSS" = 16 chars */ + flb_plg_error(ctx->ins, "filename too long for rotation: %s", filename); + return -1; + } + /* Create rotated filename with timestamp */ snprintf(rotated_filename, PATH_MAX - 1, "%s.%s", filename, timestamp);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
plugins/CMakeLists.txt(1 hunks)plugins/out_logrotate/CMakeLists.txt(1 hunks)plugins/out_logrotate/logrotate.c(1 hunks)tests/runtime/out_logrotate.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/out_logrotate/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
plugins/out_logrotate/logrotate.c
🧬 Code graph analysis (2)
plugins/out_logrotate/logrotate.c (7)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)src/flb_hash_table.c (5)
flb_hash_table_create(99-137)flb_hash_table_destroy(197-215)flb_hash_table_get(496-522)flb_hash_table_del(605-674)flb_hash_table_add(401-494)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1298-1321)src/flb_output.c (2)
flb_output_get_property(1096-1099)flb_output_set_context(1524-1527)src/flb_pack.c (1)
flb_msgpack_to_json_str(1459-1500)lib/cmetrics/src/cmt_decode_msgpack.c (1)
cmt_decode_msgpack_create(1450-1496)src/flb_log_event_decoder.c (3)
flb_log_event_decoder_init(103-120)flb_log_event_decoder_next(309-456)flb_log_event_decoder_destroy(151-178)
tests/runtime/out_logrotate.c (1)
src/flb_lib.c (10)
flb_create(143-225)flb_service_set(652-678)flb_input(266-276)flb_input_set(305-335)flb_output(279-289)flb_output_set(520-551)flb_start(983-994)flb_lib_push(843-870)flb_stop(1011-1055)flb_destroy(228-263)
🔇 Additional comments (4)
plugins/CMakeLists.txt (1)
366-366: LGTM: Plugin registration is correct.The
out_logrotateplugin is registered following the same pattern as other output plugins, and the placement maintains alphabetical order.tests/runtime/out_logrotate.c (3)
157-235: LGTM: Basic rotation test is well-structured.The test properly:
- Sets up and tears down the test directory
- Configures rotation with
max_size=5Kandgzip=false- Writes enough data (~16KB) to trigger rotation
- Verifies both the current file and at least one rotated file exist
The sleep intervals (lines 210, 218) are necessary to allow async flush operations to complete.
237-300: LGTM: Gzip compression test validates the compression feature.The test correctly:
- Enables gzip compression with
gzip=true- Triggers rotation by writing sufficient data
- Uses
wait_for_file_pattern()to poll for rotated files with.gzsuffix- Verifies that gzip compression was applied to rotated files
302-366: LGTM: Max files cleanup test validates the retention policy.The test properly:
- Configures
max_files=3to limit retained rotated files- Triggers multiple rotations by writing data in 5 iterations
- Validates that file count never exceeds 4 (current file + 3 rotated files)
- Uses
TEST_ASSERT(file_count >= 0)to catch directory listing failures (addressing the past review comment)- Verifies the final count after all rotations complete
The iterative checking (lines 349–351) ensures cleanup happens progressively during rotation, not just at the end.
2 participants
This PR present a new out plugin
out_logrotate. This Will answer #7572 question and solve #7541 suggestion.This feature is highly required in bare-metal or physical machines where you have finite and limited storage.
The plugin is basically a copy of
out_filewith a rotation feature. The reason to do that in additional plugin and not on theout_fileitself is to avoid accidentally turning this feature on and having redundant info/allocs/processing where it is not needed (so normal users can still use the regular file plugin without issue).(this replaces closed #10824 PR)
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
New Features
Tests