Repository navigation
Conversation
A group whose start and end arrive in separate appends, such as forward messages from a sender that splits a group, was split across chunks when the chunk was flushed or closed for size between the appends. Outputs then received the end of the group without its start and lost its context. Track whether a log chunk ends inside a group, from the group markers of each append, and keep such a chunk open until the group ends: it is not closed when it grows past its size limit, up to twice that limit, and flb_input_chunk_holds_open_group() tells the dispatcher to wait for it. The wait is bounded to five seconds and ends on shutdown. A cheap byte scan skips buffers that cannot contain markers. Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Skip a chunk that still ends inside a log group, so the rest of the group is appended to it instead of a new chunk. Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
When an append left a group open, the route payload repeated its start marker before every matching record, and the end of a group started by a previous append was only kept by accident when no other group followed. Emit the start of a group once, and emit the end of a continued group after its records. Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
A Forward mode frame larger than the ingress byte limit was split into batches by size. The decoder skipped group markers without counting their bytes, so a batch could cut the end of its last record and drop markers, and a split could fall inside a group. Read the markers as events, and when a group does not fit, ingest the events before it and start the next batch with the group. Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 46.15% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 26 functions across 6 files. (3 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the primary change: preserving log groups across input chunk appends.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 46.15% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 26 functions across 6 files. (3 skipped: 3 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 2
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks: 💡
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
![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: a4f6105a9e
ℹ️ 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".
| if (p[1] != 0x92) { | ||
| p++; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
When an input appends a valid FLB_LOG_EVENT_FORMAT_FORWARD record, the accepted layout is [timestamp, body], so an EventTime group marker starts with 0x92 0xd7..., not 0x92 0x92.... This check therefore makes log_buffer_may_have_groups() return false before the parser's existing legacy-header path can run; group_open remains false and a flush between appends can still split that group. Accept the scalar timestamp layout here as well as the Fluent Bit v2 nested header.
Useful? React with 👍 / 👎.
This branch was successfully deployed
1 participant
Problem
A log group (a group start marker, its records and the group end marker, for
example one OpenTelemetry resource and scope) must reach outputs whole. If a
group is split across chunks, the output that flushes the second chunk gets
records and an end marker without the start. OTLP outputs then export those
records without their resource and scope.
Groups were split in these cases:
messages, the start and the end arrive in separate appends. A flush in
between turned the chunk into a task, and the rest of the group went into a
new chunk. The same happened with in_forward Message mode, where every
event is its own append.
2 MB, even in the middle of a group, so the next append of the group went
into a new chunk.
limit (in practice
workers > 1withmem_buf_limitset, and a framelarger than
mem_buf_limit) was split into batches by size. Two problems:so a batch could cut off the end of its last record and write invalid
MessagePack into the chunk, and markers were dropped;
repeated the group start before every matching record. The end of a group
started by a previous append was only kept by accident, when no other
group followed it in the append.
The in-tree inputs that create groups (opentelemetry, and rewrite_tag through
emitter) always append complete groups. I found no other input that leaves a
group open.
Changes
markers of each append, and keep such a chunk open until the group ends:
flb_input_chunk_holds_open_group()tells the dispatcher to wait;does not hold its records, and it ends on shutdown;
so inputs that do not create groups pay a
memchrpass and nothingelse.
so every batch is exact, and split only between groups: when a group does
not fit, the events before it are ingested and the group starts the next
batch. A single group larger than the limit is rejected, as a single record
larger than the limit already is.
continued group after its records.
Compatibility
before, and only until the group ends. Chunks without open groups are
unaffected.
Testing
New integration tests in
tests/integration/scenarios/in_forward. All failon 5.1.0 and pass with this change.
test_in_forward_group_split_across_messages_stays_in_one_chunk: a flushbetween the start and the end of a group;
test_in_forward_group_crossing_the_chunk_size_stays_in_one_chunk: a groupcrossing the 2 MB chunk limit;
test_in_forward_open_group_is_flushed_after_the_hold_limit: a group thatnever ends is released after the hold limit;
test_in_forward_workers_split_large_frames_between_groups: a 600 KB framewith six groups against a 128 KB ingress limit is delivered intact, with
every group whole;
test_in_forward_conditional_route_keeps_a_split_group_whole: a routedgroup split across appends, followed by a complete group.
Linux x86_64 debug build:
ctest: 213 of 215 pass.flb-rt-out_httpis the flaky test that alsofails on
master.flb-rt-in_forwardbinds port 24224, which anotherprocess holds on the test machine.
happen on
master: the Kafka schema registry tests need the protobufencoder,
unix_socket_decode_statusneeds the unix_socket input, and thesystemd collector tests.
VALGRIND_STRICT=1) on the scenarios listed above: 369 Fluent Bitruns, no errors and no definite leaks.
across two messages, restarts every 60 seconds, 2% of exports rejected with
503. 1,181 split groups were all delivered with their resource, with no
missing and no duplicated records. Without this change, between 2% and 3% of
the forward records were exported without their resource.
Summary by CodeRabbit