out_forward: fix retain_metadata_in_forward_mode default value (true)#11756
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>
📝 WalkthroughWalkthroughThe default value of the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Integration Test
participant Sender as FluentBit Sender
participant Receiver as FluentBit Receiver
participant Capture as Forward Server
Test->>Sender: Start (default/true/false config)
Test->>Receiver: Start
Test->>Capture: Start capture server
Note over Test: Construct Forward message<br/>with TEST_METADATA
Test->>Sender: Send message to sender port
Sender->>Receiver: Forward message<br/>(metadata retained or dropped)
Receiver->>Capture: Send to capture endpoint
Note over Capture: Store message in data_storage
Test->>Capture: Poll for forwarded messages
Capture-->>Test: Return captured messages
alt Metadata Preserved Config
Test->>Test: Assert record[metadata]<br/>contains TEST_METADATA
else Metadata Dropped Config
Test->>Test: Assert metadata<br/>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 labels
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 |
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".
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
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.
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