in_tail: drop multiline records whose reassembled content is empty (fixes #6703) - #12262
Open
allxm4 wants to merge 3 commits into
Open
in_tail: drop multiline records whose reassembled content is empty (fixes #6703)#12262allxm4 wants to merge 3 commits into
allxm4 wants to merge 3 commits into
Conversation
Add a drop_empty_content flag to struct flb_ml_parser_ins. When set, flb_ml_flush_stream_group() drops a flush whose key_content value is an empty string instead of re-emitting the stored first-line map. Defaults to FLB_FALSE (flb_calloc zero-inits the struct), so existing behavior is unchanged unless a caller opts an instance in. Signed-off-by: allxm4 <77125344+allxm4@users.noreply.github.com>
Skip_Empty_Lines only inspects the raw line as read from disk, before multiline reassembly runs. A CRI-framed line with an empty payload still has a non-zero raw length (timestamp/stream/tag prefix), so it never trips that check, and the reassembled record is emitted with an empty content field regardless. Set drop_empty_content on each multiline parser instance from the existing Skip_Empty_Lines option so the same user-facing intent also covers post-reassembly content. Signed-off-by: allxm4 <77125344+allxm4@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe multiline parser now tracks the tail input’s ChangesEmpty multiline content handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Author
|
Example configuration (as used for all runs below; the fix activates via the existing Input file ( Before/after (same patched binary;
Debug log output (Ubuntu 24.04, gcc, Valgrind ( Unit tests: full internal multiline suite ( |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/multiline/flb_ml.c`:
- Around line 1687-1689: Apply the project’s C brace style by moving each
control-block opening brace onto its own following line: the if block in
src/multiline/flb_ml.c (1687-1689), and the for/if blocks in
tests/internal/multiline.c (650-655, 659-661, 697-702, 706-708, 744-749, and
753-755). Preserve all existing control-flow behavior.
In `@tests/internal/multiline.c`:
- Around line 85-114: Update the initializer entries in the new issue_6703
fixtures to use four-space indentation instead of two spaces, including
issue_6703_input, issue_6703_output, issue_6703_disabled_output,
issue_6703_all_empty_input, and issue_6703_all_empty_output.
- Around line 649-655: In tests/internal/multiline.c at lines 649-655, 696-702,
and 743-749, update the append loops in test_issue_6703 and
test_issue_6703_all_empty to assign each flb_ml_append_text() result to ret and
assert ret >= 0, covering both success and failure paths.
🪄 Autofix
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 Plus
Run ID: 788433fc-f547-4a84-9f9a-cbb2673c7acd
📒 Files selected for processing (4)
include/fluent-bit/multiline/flb_ml.hplugins/in_tail/tail_config.csrc/multiline/flb_ml.ctests/internal/multiline.c
Comment on lines
+1687
to
+1689
| if (parser_i->drop_empty_content) { | ||
| key_id = get_key_id(&map, parser_i->key_content); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the required control-block brace style.
Move each control-block opening brace to the next line.
src/multiline/flb_ml.c#L1687-L1689: move theifopening brace to the next line.tests/internal/multiline.c#L650-L655: move theforopening brace to the next line.tests/internal/multiline.c#L659-L661: move theifopening brace to the next line.tests/internal/multiline.c#L697-L702: move theforopening brace to the next line.tests/internal/multiline.c#L706-L708: move theifopening brace to the next line.tests/internal/multiline.c#L744-L749: move theforopening brace to the next line.tests/internal/multiline.c#L753-L755: move theifopening brace to the next line.
As per coding guidelines, C control blocks must place the opening brace on the next line.
📍 Affects 2 files
src/multiline/flb_ml.c#L1687-L1689(this comment)tests/internal/multiline.c#L650-L655tests/internal/multiline.c#L659-L661tests/internal/multiline.c#L697-L702tests/internal/multiline.c#L706-L708tests/internal/multiline.c#L744-L749tests/internal/multiline.c#L753-L755
🤖 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 `@src/multiline/flb_ml.c` around lines 1687 - 1689, Apply the project’s C brace
style by moving each control-block opening brace onto its own following line:
the if block in src/multiline/flb_ml.c (1687-1689), and the for/if blocks in
tests/internal/multiline.c (650-655, 659-661, 697-702, 706-708, 744-749, and
753-755). Preserve all existing control-flow behavior.
Source: Coding guidelines
Comment on lines
+85
to
+114
| /* CRI, empty payload dropped via drop_empty_content */ | ||
| struct record_check issue_6703_input[] = { | ||
| {"2025-01-01T00:00:00.000000000Z stdout F hello world"}, | ||
| {"2025-01-01T00:00:00.000000001Z stdout F "}, | ||
| {"2025-01-01T00:00:00.000000002Z stdout F goodbye world"} | ||
| }; | ||
|
|
||
| struct record_check issue_6703_output[] = { | ||
| {"hello world"}, | ||
| {"goodbye world"} | ||
| }; | ||
|
|
||
| /* Same input, drop_empty_content left off (default): behavior is unchanged */ | ||
| struct record_check issue_6703_disabled_output[] = { | ||
| {"hello world"}, | ||
| {""}, | ||
| {"goodbye world"} | ||
| }; | ||
|
|
||
| /* CRI, every line in isolation is an empty payload */ | ||
| struct record_check issue_6703_all_empty_input[] = { | ||
| {"2025-01-01T00:00:00.000000000Z stdout F "}, | ||
| {"2025-01-01T00:00:00.000000001Z stdout F "}, | ||
| {"2025-01-01T00:00:00.000000002Z stdout F "} | ||
| }; | ||
|
|
||
| /* Sink for issue_6703_all_empty: only read if the fix regresses */ | ||
| struct record_check issue_6703_all_empty_output[] = { | ||
| {""}, {""}, {""} | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use four-space indentation in the new fixtures.
Lines 87-113 use two-space indentation for initializer entries. Use four spaces.
As per coding guidelines, **/*.{c,h,cc,cpp,cxx} requires four-space indentation.
🤖 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/internal/multiline.c` around lines 85 - 114, Update the initializer
entries in the new issue_6703 fixtures to use four-space indentation instead of
two spaces, including issue_6703_input, issue_6703_output,
issue_6703_disabled_output, issue_6703_all_empty_input, and
issue_6703_all_empty_output.
Source: Coding guidelines
Comment on lines
+649
to
+655
| entries = sizeof(issue_6703_input) / sizeof(struct record_check); | ||
| for (i = 0; i < entries; i++) { | ||
| r = &issue_6703_input[i]; | ||
| len = strlen(r->buf); | ||
| flb_time_get(&tm); | ||
| flb_ml_append_text(ml, stream_id, &tm, r->buf, len); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check every flb_ml_append_text() result.
The tests discard the append result. In test_issue_6703 and test_issue_6703_all_empty, failure to process empty input can still satisfy the expected flush count. Assign the result to ret and check ret >= 0 in each loop.
tests/internal/multiline.c#L649-L655: check each append result in the enabled test.tests/internal/multiline.c#L696-L702: check each append result in the disabled test.tests/internal/multiline.c#L743-L749: check each append result in the all-empty test.
As per coding guidelines, validate both success and failure paths.
📍 Affects 1 file
tests/internal/multiline.c#L649-L655(this comment)tests/internal/multiline.c#L696-L702tests/internal/multiline.c#L743-L749
🤖 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/internal/multiline.c` around lines 649 - 655, In
tests/internal/multiline.c at lines 649-655, 696-702, and 743-749, update the
append loops in test_issue_6703 and test_issue_6703_all_empty to assign each
flb_ml_append_text() result to ret and assert ret >= 0, covering both success
and failure paths.
Source: Coding guidelines
Add three cases: a standalone CRI line with an empty payload is dropped when drop_empty_content is set, is still emitted when the option is not set (default, backward-compat), and a group where every line is empty does not crash or flush anything. Signed-off-by: allxm4 <77125344+allxm4@users.noreply.github.com>
allxm4
force-pushed
the
fix/6703-drop-empty-multiline-content
branch
from
August 11, 2026 18:38
6a82496 to
daa157c
Compare
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.
Fixes #6703.
Skip_Empty_Lines(tail_file.c) only inspects the raw line as read fromdisk, before multiline reassembly. A CRI-framed empty-payload line
(
<ts> stdout Fwith nothing after it) still has a non-zero raw length(timestamp/stream/tag prefix), so it never trips that check, and the
reassembled record is emitted with an empty content field regardless.
This adds an opt-in
drop_empty_contentfield tostruct flb_ml_parser_ins,wired from
in_tail's existingSkip_Empty_Linesoption (default off, nobehavior change unless already set), checked in
flb_ml_flush_stream_group()at the point that has visibility into the final reassembled content.
Verified with a real CRI-format log file on disk. Before: emits a record
with
"log":""; after: dropped, surrounding valid lines untouched (notruncation or merge). Full existing internal multiline test suite passes
(gcc and clang), plus 3 new tests covering the fix, backward compatibility
(option off = unchanged behavior), and an all-empty group. Valgrind clean.
Known limitation: only verified for the CRI parser (the one reported in
#6703). The built-in
docker/JSON parser has its own, separateskip_empty=TRUEbehavior at the JSON-decode level that drops the key beforethis code runs; not addressed here, and not the same failure mode.
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
Bug Fixes