out_file: Support RA placeholders for dynamic creation of files#11840
Open
cosmo0920 wants to merge 2 commits into
Open
out_file: Support RA placeholders for dynamic creation of files#11840cosmo0920 wants to merge 2 commits into
cosmo0920 wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughAdds dynamic record-accessor-based path/file routing to out_file with validation, configurable actions for missing/limit cases, bounded dynamic-destination tracking, recursive directory creation support, and runtime tests covering nominal and edge behaviors. ChangesDynamic File Path Routing
sequenceDiagram
participant Input as Input
participant Accessor as RecordAccessor
participant Resolver as DestinationResolver
participant Table as DynamicTable
participant FS as FileSystem
Input->>Accessor: provide record for $path/$file
Accessor->>Resolver: translate tokens (field values)
Resolver->>Resolver: validate composed path/file
Resolver->>Table: lookup/insert destination key
Resolver->>FS: open/create file (mkdir if needed)
FS->>FS: write record
FS->>Resolver: return write result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 docstrings
🧪 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.
Reviewed commit: 9cd2cce14f
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@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
🤖 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/out_file/file.c`:
- Around line 483-489: The config validation must also reject
dynamic-destination configs that lack a metrics fallback: in the existing
initialization checks that currently validate fallback_file for
FLB_OUT_FILE_ACTION_FALLBACK (the block using ctx->missing_field_action /
ctx->limit_reached_action and calling flb_plg_error + file_conf_destroy), add a
condition to require ctx->fallback_file when ctx->dynamic_destination is true
(since cb_file_flush always uses use_fallback_destination for metrics). Apply
the same guard to the other similar validation sections (the blocks around the
other occurrences noted) so that if ctx->dynamic_destination is set and no
ctx->fallback_file is configured, initialization logs the error and returns
failure (preserving the flb_plg_error and file_conf_destroy behavior).
- Around line 240-255: validate_dynamic_file currently rejects separators and
control bytes but allows Windows-invalid characters; update the validator to
treat any of the characters ':', '*', '?', '\"', '<', '>', '|' as unsafe and
return -1 if encountered (keep using the existing const unsigned char *p loop
and comparisons), and apply the same change to the other file-name validator in
this file that mirrors validate_dynamic_file so both dynamic and the analogous
validator reject these Windows-reserved characters.
🪄 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: d3b0031c-02b2-40d1-91e2-2f4ae8145a35
📒 Files selected for processing (2)
plugins/out_file/file.ctests/runtime/out_file.c
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/runtime/out_file.c (1)
585-609: ⚡ Quick winAssert fallback is unused in the happy-path dynamic routing test.
This test proves dynamic files are created, but it does not assert that
metrics.logstays absent. A regression that writes both dynamic and fallback outputs could slip through.Suggested patch
void flb_test_file_dynamic_path_file(void) { int ret; int bytes; char *p = JSON_DYNAMIC; char path1[256]; char path2[256]; char dir1[256]; char dir2[256]; + char fallback[256]; flb_ctx_t *ctx; int in_ffd; int out_ffd; snprintf(dir1, sizeof(dir1), "%s/proxy1", TEST_LOGPATH); snprintf(dir2, sizeof(dir2), "%s/proxy2", TEST_LOGPATH); snprintf(path1, sizeof(path1), "%s/host1.log", dir1); snprintf(path2, sizeof(path2), "%s/host2.log", dir2); + snprintf(fallback, sizeof(fallback), "%s/metrics.log", TEST_LOGPATH); remove(path1); remove(path2); + remove(fallback); rmdir(dir1); rmdir(dir2); rmdir(TEST_LOGPATH); @@ flb_stop(ctx); flb_destroy(ctx); + TEST_CHECK(access(fallback, F_OK) != 0); remove(path1); remove(path2); + remove(fallback); rmdir(dir1); rmdir(dir2); rmdir(TEST_LOGPATH); }🤖 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 `@tests/runtime/out_file.c` around lines 585 - 609, The test currently verifies dynamic files path1/path2 are created but doesn't assert the fallback file (fallback_file "metrics.log" under fallback_path TEST_LOGPATH) is absent; add an assertion after flb_stop/flb_destroy (or after the wait_for_file checks) that the fallback file does not exist (use the same file-check helper or stat/access and TEST_CHECK to ensure absence) before removing files/directories so regressions that write both dynamic and fallback outputs will fail the test; reference the test function in tests/runtime/out_file.c and the variables for TEST_LOGPATH/metrics.log, path1/path2, and wait_for_file in your change.
🤖 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 `@tests/runtime/out_file.c`:
- Around line 585-609: The test currently verifies dynamic files path1/path2 are
created but doesn't assert the fallback file (fallback_file "metrics.log" under
fallback_path TEST_LOGPATH) is absent; add an assertion after
flb_stop/flb_destroy (or after the wait_for_file checks) that the fallback file
does not exist (use the same file-check helper or stat/access and TEST_CHECK to
ensure absence) before removing files/directories so regressions that write both
dynamic and fallback outputs will fail the test; reference the test function in
tests/runtime/out_file.c and the variables for TEST_LOGPATH/metrics.log,
path1/path2, and wait_for_file in your change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3940c7e1-8404-4ffd-a9ef-ab9a7fc3020f
📒 Files selected for processing (2)
plugins/out_file/file.ctests/runtime/out_file.c
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
cosmo0920
force-pushed
the
cosmo0920-out_file-for-ra-placeholders
branch
from
cceb7e7 to
466fe5e
Compare
cosmo0920
force-pushed
the
cosmo0920-out_file-for-ra-placeholders
branch
from
466fe5e to
6482a50
Compare
cosmo0920
force-pushed
the
cosmo0920-out_file-for-ra-placeholders
branch
from
6482a50 to
a544189
Compare
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
cosmo0920
force-pushed
the
cosmo0920-out_file-for-ra-placeholders
branch
from
a544189 to
740ed34
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/runtime/out_file.c (1)
641-685: ⚡ Quick winAdd a negative assertion to prevent false positives in missing-field fallback test.
This test validates fallback creation but not that unintended dynamic output is absent. A regression writing both files could still pass.
Suggested hardening
void flb_test_file_dynamic_missing_fallback(void) { int ret; int bytes; char *p = JSON_DYNAMIC_MISSING; char fallback[256]; + char unexpected_empty[256]; + char unexpected_literal[256]; flb_ctx_t *ctx; int in_ffd; int out_ffd; snprintf(fallback, sizeof(fallback), "%s/fallback.log", TEST_LOGPATH); + snprintf(unexpected_empty, sizeof(unexpected_empty), "%s/.log", TEST_LOGPATH); + snprintf(unexpected_literal, sizeof(unexpected_literal), "%s/$hostname.log", TEST_LOGPATH); remove(fallback); + remove(unexpected_empty); + remove(unexpected_literal); rmdir(TEST_LOGPATH); @@ bytes = flb_lib_push(ctx, in_ffd, p, strlen(p)); TEST_CHECK(bytes == strlen(p)); ret = wait_for_file(fallback, 1, TEST_TIMEOUT); TEST_CHECK(ret == 0); + TEST_CHECK(access(unexpected_empty, F_OK) != 0); + TEST_CHECK(access(unexpected_literal, F_OK) != 0); flb_stop(ctx); flb_destroy(ctx); remove(fallback); + remove(unexpected_empty); + remove(unexpected_literal); rmdir(TEST_LOGPATH); }🤖 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 `@tests/runtime/out_file.c` around lines 641 - 685, The test flb_test_file_dynamic_missing_fallback currently only asserts the fallback file was created and can miss a regression where both the fallback and the dynamic $hostname.log are written; add a negative assertion after wait_for_file(fallback,...) succeeds to verify the dynamic output file (constructed the same way as snprintf(fallback,...) but using "$hostname.log" name) does NOT exist—use TEST_CHECK with access()/stat or ensure wait_for_file(dynamic_path, 1, short_timeout) returns non-zero—to fail the test if the unintended dynamic file is present.
🤖 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 `@tests/runtime/out_file.c`:
- Around line 641-685: The test flb_test_file_dynamic_missing_fallback currently
only asserts the fallback file was created and can miss a regression where both
the fallback and the dynamic $hostname.log are written; add a negative assertion
after wait_for_file(fallback,...) succeeds to verify the dynamic output file
(constructed the same way as snprintf(fallback,...) but using "$hostname.log"
name) does NOT exist—use TEST_CHECK with access()/stat or ensure
wait_for_file(dynamic_path, 1, short_timeout) returns non-zero—to fail the test
if the unintended dynamic file is present.
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.
Introduce RA support on out_file plugin.
Plus, from the #11794's concern, we also needed to add a hashtable to manage the limit of the number of total used files by dynamic destinations which is handled by RA.
Closes #11794.
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
Safety & Reliability
Tests