add logrotate out plugin (#2) #10824
Conversation
SagiROosto
requested review from
fujimotos,
niedbalski,
patrick-stephens,
celalettin1286,
edsiper and
koleini
as code owners
WalkthroughAdds a new out_logrotate output plugin: build and CMake integration, public header, full C implementation with rotation/compression/cleanup/formats, example config, and runtime tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin as Config/Admin
participant FB as Fluent Bit
participant PL as out_logrotate
participant FS as Filesystem
Admin->>FB: Load config (input + out_logrotate)
FB->>PL: cb_init(config)
Note over PL: configure path,file,format,max_size,max_files,gzip,mkdir
loop On flush
FB->>PL: cb_flush(events)
PL->>PL: Determine target filename (path/file/tag)
alt size >= max_size
PL->>FS: Close & rename current -> file.timestamp
opt gzip enabled
PL->>FS: Stream-compress -> file.timestamp.gz
end
PL->>FS: Remove oldest rotated files if count > max_files
PL->>PL: Reset current_file_size
end
PL->>FS: Ensure directory (mkdir) and open append
alt msgpack payload
PL->>FS: Write raw msgpack
else metrics payload
PL->>FS: Write rendered metrics text
else
PL->>PL: Render records (json/csv/ltsv/plain/template)
PL->>FS: Append rendered lines
end
PL->>PL: Update current_file_size
PL-->>FB: cb_flush return status
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-08-31T12:46:11.940ZApplied to files:
🔇 Additional comments (2)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 15
🧹 Nitpick comments (7)
plugins/out_logrotate/logrotate.h (1)
23-30: Optional: C++ friendliness and API commentIf tests or embedders include this from C++, consider extern "C" guards; also add a brief comment mapping enum values to the config's Format strings.
Example:
+#ifdef __cplusplus +extern "C" { +#endif enum { @@ }; +#ifdef __cplusplus +} +#endifconf/fluent-bit-logrotate.conf (1)
12-21: Clarify size units and option names to match plugin config map“Max_Size 10” is ambiguous (bytes? KB?). Prefer explicit units if supported (e.g., 10M), and ensure option keys exactly match the plugin’s config map.
- Max_Size 10 + Max_Size 10M # 10 megabytesPlease confirm the accepted keys/values (Format, Max_Size, Max_Files, Gzip, Mkdir) align with logrotate.c’s flb_config_map; adjust example accordingly.
tests/runtime/out_logrotate.c (2)
322-325: Assertion doesn’t prove rotation happened.Comparing sizes without ensuring a rotation occurred can pass spuriously. Instead, verify a rotated file exists and the active file size reset.
I can rewrite this test to:
- set max_size to a small value (e.g., 64K),
- push until rotation triggers,
- assert count_files_in_directory(".", "flb_test_logrotate.log.") >= 1,
- and the current file size < 64K.
498-500: Time-based performance assertions are brittle in CI.CPU and I/O variance makes this flaky. Either increase the threshold generously, skip under CI, or convert to a basic smoke test.
plugins/out_logrotate/logrotate.c (3)
511-516: Comment mismatch: size is in bytes, not MB.Avoid confusion with FLB_CONFIG_MAP_SIZE which accepts human-readable units but stores bytes.
-/* Function to check if file size exceeds max size in MB */ +/* Function to check if file size exceeds max size (bytes) */ static int should_rotate_file(struct flb_logrotate_conf *ctx)
165-176: Delimiter handling is restrictive vs. docs.Docs say “Set a custom delimiter,” but check_delimiter only accepts a few keywords/escapes. Consider passing through arbitrary delimiters when provided.
Minimal change inside init:
tmp = flb_output_get_property("delimiter", ins); ret_str = check_delimiter(tmp); - if (ret_str != NULL) { - ctx->delimiter = ret_str; - } + if (ret_str != NULL) { + ctx->delimiter = ret_str; + } else if (tmp && *tmp) { + ctx->delimiter = tmp; /* accept arbitrary delimiter string */ + }Same for label_delimiter if desired.
180-185: Log human-friendly sizes to improve operability.Printing raw bytes is harder to read. Optional: format as KiB/MiB when logging.
I can provide a small helper to pretty-print sizes if you want it included.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
cmake/plugins_options.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
🧬 Code graph analysis (2)
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(1250-1291)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(99-116)flb_log_event_decoder_next(310-406)flb_log_event_decoder_destroy(147-179)
tests/runtime/out_logrotate.c (2)
src/flb_lib.c (10)
flb_create(138-220)flb_service_set(647-673)flb_input(261-271)flb_input_set(300-330)flb_output(274-284)flb_output_set(515-546)flb_start(914-925)flb_lib_push(774-801)flb_stop(942-985)flb_destroy(223-258)src/flb_sds.c (4)
flb_sds_create(78-90)flb_sds_create_size(92-95)flb_sds_printf(336-387)flb_sds_destroy(389-399)
🔇 Additional comments (4)
plugins/CMakeLists.txt (1)
367-369: LGTM: conditional registration is correct and placed logically after out_fileRegistration behind FLB_OUT_LOGROTATE matches the new option; ordering is consistent with related outputs.
cmake/plugins_options.cmake (1)
120-120: Option wiring looks correct; defaults align with existing pattern.DEFINE_OPTION for FLB_OUT_LOGROTATE mirrors other outputs and respects FLB_MINIMAL. No blockers.
tests/runtime/out_logrotate.c (2)
263-266: File count assertion may be racy; wait until rotation settles.After heavy writes, let the engine flush again before counting files, or poll until count stabilizes within TEST_TIMEOUT.
I can add a small polling loop to wait until count_files_in_directory(".", "flb_test_logrotate.log.") <= 3 or timeout. Want me to patch it?
380-382: Header check aligns once csv_column_names=true.After enabling column names, "timestamp" should be present. No further action.
* add logrotate out plugin * fix gzip * fix test * update max_size * update gzip * add logs Signed-off-by: SagiROosto <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
plugins/out_logrotate/logrotate.c (4)
31-39: Add missing headers for PATH_MAX, PRIu64, and dirname().
Compilation will fail or be non-portable without these includes.Apply:
#include <miniz/miniz.h> +#include <limits.h> /* PATH_MAX */ +#include <inttypes.h> /* PRIu64 */ +#ifndef FLB_SYSTEM_WINDOWS +#include <libgen.h> /* dirname */ +#endif
869-901: Use project allocators and guard against uninitialized directory on strdup failure.
Replace strdup/free with flb_strdup/flb_free and skip cleanup if directory couldn’t be resolved.Apply:
- out_file_copy = strdup(out_file); - if (out_file_copy) { + out_file_copy = flb_strdup(out_file); + if (out_file_copy != NULL) { #ifdef FLB_SYSTEM_WINDOWS PathRemoveFileSpecA(out_file_copy); strncpy(directory, out_file_copy, PATH_MAX - 1); directory[PATH_MAX - 1] = '\0'; #else strncpy(directory, dirname(out_file_copy), PATH_MAX - 1); directory[PATH_MAX - 1] = '\0'; #endif - free(out_file_copy); + flb_free(out_file_copy); } @@ - /* Rotate the file */ + /* Rotate the file */ if (rotate_file(ctx, out_file) == 0) { /* Reset file size counter after rotation */ ctx->current_file_size = 0; - /* Clean up old rotated files */ - cleanup_old_files(ctx, directory, base_filename); + /* Clean up old rotated files (only if directory was resolved) */ + if (directory[0] != '\0') { + cleanup_old_files(ctx, directory, base_filename); + } }Also initialize directory to an empty string before use:
- char directory[PATH_MAX]; + char directory[PATH_MAX] = {0};
903-919: Use project allocators in mkdir branch and avoid passing possibly static buffer from dirname directly.Apply:
- out_file_copy = strdup(out_file); - if (out_file_copy) { + out_file_copy = flb_strdup(out_file); + if (out_file_copy != NULL) { #ifdef FLB_SYSTEM_WINDOWS PathRemoveFileSpecA(out_file_copy); ret = mkpath(ctx->ins, out_file_copy); #else - ret = mkpath(ctx->ins, dirname(out_file_copy)); + { + char tmpdir[PATH_MAX]; + const char *dd = dirname(out_file_copy); + strncpy(tmpdir, dd, sizeof(tmpdir) - 1); + tmpdir[sizeof(tmpdir) - 1] = '\0'; + ret = mkpath(ctx->ins, tmpdir); + } #endif - free(out_file_copy); + flb_free(out_file_copy); if (ret == 0) { fp = fopen(out_file, "ab+"); } }
937-944: Update size counter after writing metrics; otherwise rotation may never trigger.Apply:
if (event_chunk->type == FLB_INPUT_METRICS) { print_metrics_text(ctx->ins, fp, event_chunk->data, event_chunk->size); + update_file_size_counter(ctx, fp); fclose(fp); FLB_OUTPUT_RETURN(FLB_OK); }
🧹 Nitpick comments (4)
plugins/out_logrotate/logrotate.c (4)
81-99: Make check_delimiter const-correct and align call sites.
Avoid discarding const and storing string literals in non-const pointers.Apply:
-static char *check_delimiter(const char *str) +static const char *check_delimiter(const char *str) @@ - char *ret_str; + const char *ret_str; @@ - ret_str = check_delimiter(tmp); + ret_str = check_delimiter(tmp);(No functional change; improves type safety.)
Also applies to: 165-176
511-516: Comment says “in MB” but value is bytes.
Update comment to avoid confusion.Apply:
-/* Function to check if file size exceeds max size in MB */ +/* Check if current file size in bytes exceeds max_size (bytes) */
1110-1114: Align default value with documentation and allow size suffix parsing.
Use “100M” so FLB_CONFIG_MAP_SIZE parses the suffix and matches the doc string.Apply:
- FLB_CONFIG_MAP_SIZE, "max_size", "100000000", + FLB_CONFIG_MAP_SIZE, "max_size", "100M",
715-731: Optional: reuse flb_gzip helpers instead of hand-rolled gzip wrapper.
Reduces maintenance and ensures parity with other plugins. If feasible, replace write_gzip_header/write_gzip_footer/deflate logic with flb_gzip APIs.I can draft a patch that streams file->gzip via flb_gzip_* and handles CRC/ISIZE internally; let me know.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
cmake/plugins_options.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 (6)
- cmake/plugins_options.cmake
- plugins/out_logrotate/CMakeLists.txt
- conf/fluent-bit-logrotate.conf
- tests/runtime/out_logrotate.c
- plugins/out_logrotate/logrotate.h
- tests/runtime/CMakeLists.txt
🧰 Additional context used
🧬 Code graph analysis (1)
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(1250-1291)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(99-116)flb_log_event_decoder_next(310-406)flb_log_event_decoder_destroy(147-179)
🔇 Additional comments (1)
plugins/CMakeLists.txt (1)
367-369: LGTM: conditional registration for out_logrotate is correct and in the right section.
Build-time gating via FLB_OUT_LOGROTATE matches the project’s plugin pattern.
Signed-off-by: SagiROosto <[email protected]>
conf/fluent-bit-logrotate.conf
Outdated
There was a problem hiding this comment.
We should provide a YAML version too.
There was a problem hiding this comment.
Sure, I didn't see other configs. let me know where can I put it. anyway in fluent-bit-docs I'll list both options
| @@ -117,6 +117,7 @@ DEFINE_OPTION(FLB_OUT_DATADOG "Enable DataDog output plugin" | |||
| DEFINE_OPTION(FLB_OUT_ES "Enable Elasticsearch output plugin" ON) | |||
| DEFINE_OPTION(FLB_OUT_EXIT "Enable Exit output plugin" ON) | |||
| DEFINE_OPTION(FLB_OUT_FILE "Enable file output plugin" ON) | |||
| DEFINE_OPTION(FLB_OUT_LOGROTATE "Enable logrotate output plugin" ON) | |||
There was a problem hiding this comment.
Will this build for all targets including macOS and Windows? They may need configuring via their specific cmake/ config files.
There was a problem hiding this comment.
the logrotate plugin is actually a fork of the out_file with gzip and couple of move/copy files.
I've matched all cmakes to be the same as the out_file
Signed-off-by: SagiROosto <[email protected]>
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).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
Configuration
Build
Tests