in_opentelemetry: optimize batched protobuf ingestion - #12171
Conversation
|
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 (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughOpenTelemetry protobuf log decoding now counts committed records and propagates that count through worker ingress queues, which select batched append handling. A new integration test validates received records and Prometheus metrics for batched worker requests. ChangesOpenTelemetry batched log ingestion
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant OTLPClient
participant OpenTelemetryInput
participant IngressQueue
participant LogStorage
OTLPClient->>OpenTelemetryInput: Send batched protobuf logs
OpenTelemetryInput->>IngressQueue: Queue encoded logs and record count
IngressQueue->>LogStorage: Append batched records
LogStorage-->>OTLPClient: Return HTTP 201
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/flb_input_ingest.c (1)
54-54: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not overload
records == 0as “unknown”.The event stores only a
size_tcount, while the collector uses the counted path only forrecords > 0; the compatibility wrapper passes0for callers without a count. A legitimate known zero-count payload is therefore indistinguishable from an unknown count and is reparsed throughflb_input_log_append(), making input-record accounting dependent on fallback parsing.Use an explicit known flag or documented sentinel, update the compatibility wrapper and OpenTelemetry helper consistently, and branch on knownness rather than positivity.
As per coding guidelines, explicit zero values must be preserved and unknown values must use a clear sentinel.
Also applies to: 385-401, 577-593
🤖 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/flb_input_ingest.c` at line 54, Separate “count is known” from the `records` value in the event and ingestion flow: preserve a legitimate known zero, while representing unknown counts with an explicit flag or documented sentinel. Update the compatibility wrapper and OpenTelemetry helper consistently, and make the counting branch test knownness rather than `records > 0`, including the related paths around the event structure and helper calls.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/flb_input_ingest.c`:
- Line 54: Separate “count is known” from the `records` value in the event and
ingestion flow: preserve a legitimate known zero, while representing unknown
counts with an explicit flag or documented sentinel. Update the compatibility
wrapper and OpenTelemetry helper consistently, and make the counting branch test
knownness rather than `records > 0`, including the related paths around the
event structure and helper calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d67ac3d-b3be-4940-9c71-bdd87054dadb
📒 Files selected for processing (3)
include/fluent-bit/flb_input.hplugins/in_opentelemetry/opentelemetry.hsrc/flb_input_ingest.c
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/in_opentelemetry/opentelemetry.h
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
9e769d2 to
744cf0c
Compare
1 participant
Problem
Batched OTLP/HTTP Protobuf logs performed avoidable allocation and record-accounting work. The Protobuf arena was only enabled for payloads at least 64 KiB, excluding realistic medium batches, and the worker decoded every logical record only for the central ingress thread to scan the resulting MessagePack again to count records.
Changes
flb_input_log_append_records()in the central input thread.flb_input_ingress_queue_log_take()API for out-of-tree callers and add a separate counted variant for OpenTelemetry.Caller and compatibility audit
The generic log queue is used by Forward, TCP, UDP, HTTP, Splunk, Elasticsearch, ETW, and OpenTelemetry. Ingress events are zero-initialized with
flb_calloc(), so existing callers retainrecords == 0and the established MessagePack recount path. The ownership, busy-queue, queue-drain, and destruction paths are unchanged.The initial patch changed the positional signature of
flb_input_ingress_queue_log_take(). Although OpenTelemetry was its only in-tree caller, this could break out-of-tree plugins. The original function signature and symbol are now preserved;flb_input_ingress_queue_log_take_records()is the opt-in counted interface.Impact
In the controlled four-worker benchmark, these changes improved Fluent Bit Protobuf ingest-only end-to-end throughput from 1.25M to 1.67M records/s (+33.4%) and records per CPU-second by 74.6%. With 256 persistent connections balancing all four listeners, Fluent Bit accepted 3.03M records/s at 376.9% aggregate CPU.
No configuration or wire-protocol behavior changes. JSON, profiles-as-logs, and unknown-count ingestion continue through the existing counting path.
Validation
Build and internal/runtime CTest
cmake -S . -B build -DFLB_TESTS_RUNTIME=On -DFLB_TESTS_INTERNAL=Oncmake --build build -j8: passed; all in-tree input callers compiled.ctest --test-dir build --output-on-failure -j8: 194/197 passed directly.flb-rt-in_forwardandflb-rt-out_httpwere blocked by the hostfluentd.serviceowning ports 24224 and 8888. Both passed in isolation after rebuilding with temporary unused test ports; those test-only edits were removed.flb-rt-out_tdremains blocked by its external/tmp/td.confcredential-file requirement.Complete Python integration suite
cd tests/integration && ./run_tests.pyFluentBitManagerharness lackswait_for_hot_reload_count().ignore_active_older_filescase consistently accepts a second line after aging; it also failed when rerun alone.Focused memory safety and lint
VALGRIND=1 VALGRIND_STRICT=1 tests/integration/.venv/bin/python -m pytest tests/integration/scenarios/in_opentelemetry/tests/test_in_opentelemetry_001.py::test_in_opentelemetry_batched_protobuf_logs_with_workers -q: passed.master: passed.Summary by CodeRabbit
New Features
Bug Fixes
Tests