out_forward: fix retain_metadata_in_forward_mode default value (true)#11756
Merged
out_forward: fix retain_metadata_in_forward_mode default value (true)#11756
Conversation
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe default for ChangesForward plugin default + tests
Sequence Diagram(s)sequenceDiagram
participant Test as Integration Test
participant Sender as Fluent Bit (sender)
participant Receiver as Fluent Bit (receiver)
participant Capture as Forward Capture Server
Test->>Sender: Start (config: default/true/false)
Test->>Receiver: Start
Test->>Capture: Start capture server
Note over Test: Build Forward protocol message\nincluding TEST_METADATA
Test->>Sender: Send message to sender port
Sender->>Receiver: Forward message\n(mode=forward, may include metadata)
Receiver->>Capture: Send to capture endpoint
Note over Capture: Store message in data_storage
Test->>Capture: Poll for captured messages
Capture-->>Test: Return captured messages
alt Metadata preserved
Test->>Test: Assert record["metadata"] contains TEST_METADATA
else Metadata dropped
Test->>Test: Assert record["metadata"] is empty/absent
end
Test->>Sender: Stop
Test->>Receiver: Stop
Test->>Capture: Stop & cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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: cf58f73f7b
ℹ️ 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.
🧹 Nitpick comments (2)
tests/integration/scenarios/out_forward/tests/test_out_forward_metadata_001.py (2)
42-52: Reset capture storage at test-run start to enforce isolation.
forward_data_storage["messages"]is shared/global; clearing it instart()avoids stale messages from prior runs affecting assertions.♻️ Suggested patch
def start(self): + forward_data_storage["messages"].clear() self.sender_port = find_available_port() self.receiver_port = find_available_port() self.capture_port = find_available_port()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/scenarios/out_forward/tests/test_out_forward_metadata_001.py` around lines 42 - 52, The test setup's start() does not clear shared forward_data_storage["messages"], risking test pollution; modify start() to reset forward_data_storage["messages"] (e.g., set to empty list or appropriate empty structure) before calling forward_server_run(self.capture_port) so each test run starts with an isolated storage state; locate this in the start method alongside the existing _set_env calls and ensure the reset happens prior to starting the server.
56-56: Replace fixed sleep with readiness-based synchronization.A hardcoded
time.sleep(1)can make this test flaky under slower environments; prefer waiting for receiver readiness instead of elapsed time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/scenarios/out_forward/tests/test_out_forward_metadata_001.py` at line 56, Replace the hardcoded time.sleep(1) in test_out_forward_metadata_001 with readiness-based synchronization: remove time.sleep(1) and instead poll a readiness predicate (for example call receiver.is_ready() or use a helper like wait_for_port(host, port, timeout=5) or receiver.wait_until_ready(timeout=5)) in a short loop until success or timeout; this keeps the test stable across slow environments and uses explicit readiness semantics rather than a fixed sleep.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@tests/integration/scenarios/out_forward/tests/test_out_forward_metadata_001.py`:
- Around line 42-52: The test setup's start() does not clear shared
forward_data_storage["messages"], risking test pollution; modify start() to
reset forward_data_storage["messages"] (e.g., set to empty list or appropriate
empty structure) before calling forward_server_run(self.capture_port) so each
test run starts with an isolated storage state; locate this in the start method
alongside the existing _set_env calls and ensure the reset happens prior to
starting the server.
- Line 56: Replace the hardcoded time.sleep(1) in test_out_forward_metadata_001
with readiness-based synchronization: remove time.sleep(1) and instead poll a
readiness predicate (for example call receiver.is_ready() or use a helper like
wait_for_port(host, port, timeout=5) or receiver.wait_until_ready(timeout=5)) in
a short loop until success or timeout; this keeps the test stable across slow
environments and uses explicit readiness semantics rather than a fixed sleep.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d9b7592b-64ce-4de7-94c0-0bffe5d13bb9
📒 Files selected for processing (6)
plugins/out_forward/forward.ctests/integration/scenarios/out_forward/config/out_forward_metadata_receiver.yamltests/integration/scenarios/out_forward/config/out_forward_metadata_sender_default.yamltests/integration/scenarios/out_forward/config/out_forward_metadata_sender_false.yamltests/integration/scenarios/out_forward/config/out_forward_metadata_sender_true.yamltests/integration/scenarios/out_forward/tests/test_out_forward_metadata_001.py
cosmo0920
approved these changes
Apr 30, 2026
cosmo0920
requested changes
Apr 30, 2026
Contributor
cosmo0920
left a comment
cosmo0920
left a comment
There was a problem hiding this comment.
Ah, out_forward tests are good but group_semantics is failing. So, we need to fix it:
[731](https://github.com/fluent/fluent-bit/actions/runs/25064403993/job/73427376837?pr=11756#step:8:4732)
Start 82: flb-rt-group_counter_semantics
79/185 Test #82: flb-rt-group_counter_semantics ...................***Failed 12.00 sec
Test forward_group_size_default... [ FAILED ]
group_counter_semantics.c:661: Check get_forward_size() == 1... failed
Test forward_group_size_retain_metadata... [ OK ]
Test forward_group_size_retain_metadata_upstream_node... [ OK ]
Test loki_group_values_count... [ OK ]
Test stackdriver_group_entries_count... [ OK ]
Test forward_output_processor_mixed_payload_smoke... [ OK ]
FAILED: 1 of 6 unit tests has failed.
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Member
Author
|
@cosmo0920 thanks for the review, fixed:
|
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
3 participants
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.
This fixes a gap in Fluent Bit-to-Fluent Bit forwarding where log event metadata could be silently dropped by default in out_forward forward mode.
Fluent Bit’s current log event model includes metadata, so forwarding between Fluent Bit instances should preserve that metadata unless the user explicitly opts out for strict Forward-compatible records. This change updates
retain_metadata_in_forward_modeto default totruewhile preserving the existing opt-out behavior withretain_metadata_in_forward_modefalse.The integration coverage adds a Fluent Bit sender and receiver chain and verifies that metadata is preserved by default, preserved when explicitly enabled, and dropped when explicitly disabled.
Tests:
test_out_forward_metadata_001.py -q
scenarios/out_forward/tests/test_out_forward_metadata_001.py -q
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
Configuration Changes
Tests