opentelemetry: reduce protobuf log allocation churn - #12087
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe CFL arena gains configurable allocator callbacks, bounded geometric growth, and public raw allocation helpers. CFL consumers and benchmarks adopt the APIs. OTLP protobuf conversion moves object construction and cleanup to a dedicated arena, with added empty-binary coverage. ChangesArena allocation and OTLP conversion
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Msgpack
participant OTLPConverter
participant otlp_proto_arena
participant ExportLogs
Msgpack->>OTLPConverter: provide log records
OTLPConverter->>otlp_proto_arena: allocate converted protobuf objects
otlp_proto_arena-->>OTLPConverter: return arena-backed objects
OTLPConverter->>ExportLogs: assemble converted logs
OTLPConverter->>otlp_proto_arena: destroy arena after completion or failure
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
37cac04 to
4b03b34
Compare
4b03b34 to
7f66e86
Compare
![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: 4b03b3466f
ℹ️ 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 (object->via.bin.size > 0) { | ||
| value->bytes_value.data = cfl_arena_memdup( |
There was a problem hiding this comment.
When a log attribute is an empty msgpack binary, this branch sets the OTLP bytes_value oneof with len == 0 but leaves data == NULL. The bundled protobuf-c packer still calls memcpy(out + rv, bd->data, len) for bytes fields (lib/fluent-otel-proto/proto_c/protobuf-c/protobuf-c.c:1020), so empty binaries now pass a NULL pointer to memcpy; that is undefined behavior under the C library contract and can be flagged by sanitizers even with a zero length. Use a one-byte arena allocation or a static empty byte for the zero-length case before packing.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@lib/cfl/include/cfl/cfl_arena.h`:
- Line 48: Remove the duplicate consecutive declaration of cfl_arena_malloc from
the public header, leaving exactly one declaration with the existing signature.
🪄 Autofix (Beta)
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
Run ID: 8106b291-8d5a-4808-83a0-eae5079d98a7
📒 Files selected for processing (17)
lib/cfl/ARENA.mdlib/cfl/CHANGELOG.mdlib/cfl/CMakeLists.txtlib/cfl/README.mdlib/cfl/benchmarks/README.mdlib/cfl/benchmarks/arena.clib/cfl/benchmarks/variant_mutable.clib/cfl/include/cfl/cfl_arena.hlib/cfl/src/cfl_arena.clib/cfl/src/cfl_arena_internal.hlib/cfl/src/cfl_array.clib/cfl/src/cfl_kvlist.clib/cfl/src/cfl_sds.clib/cfl/tests/arena.clib/cfl/tests/installed_consumer/main.csrc/opentelemetry/flb_opentelemetry_otlp_proto.ctests/internal/opentelemetry.c
💤 Files with no reviewable changes (1)
- lib/cfl/src/cfl_arena_internal.h
| * A zero-sized or overflowing request returns NULL. Allocation failure leaves | ||
| * the arena valid and does not define errno. | ||
| */ | ||
| void *cfl_arena_malloc(struct cfl_arena *arena, size_t size); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove duplicate cfl_arena_malloc declaration.
cfl_arena_malloc is declared twice on consecutive lines with an identical signature. While valid C, this is clearly unintentional and should be removed from the public header.
📝 Proposed fix
void *cfl_arena_malloc(struct cfl_arena *arena, size_t size);
-void *cfl_arena_malloc(struct cfl_arena *arena, size_t size);
void *cfl_arena_calloc(struct cfl_arena *arena,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void *cfl_arena_malloc(struct cfl_arena *arena, size_t size); | |
| void *cfl_arena_malloc(struct cfl_arena *arena, size_t size); |
🤖 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 `@lib/cfl/include/cfl/cfl_arena.h` at line 48, Remove the duplicate consecutive
declaration of cfl_arena_malloc from the public header, leaving exactly one
declaration with the existing signature.
Build temporary OTLP protobuf log trees with the public CFL arena and release all allocations after packing. Projected task-clock remains 7.94% lower for small requests, 11.54% lower for 2,048 records, and 10.78% lower for 256 resources. Peak heap is unchanged. Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
7f66e86 to
df20b7a
Compare
1 participant
Summary
Reduce allocator overhead while converting Fluent Bit log events into an OTLP protobuf
ExportLogsServiceRequest.The protobuf resource, scope, record, attribute,
AnyValue, string, and binary-ID objects all have the same lifetime: they remain live until the request is packed. This change allocates those objects from a request-local geometric arena and releases all arena chunks together after packing. Pointer arrays that still requirereallocremain heap-backed and are released separately.This is intentionally limited to OTLP protobuf log output construction. It does not change protobuf ingestion, OTLP JSON encoding, metrics encoding, or trace encoding.
Beneficial use cases
This benefits workloads that serialize Fluent Bit log-event chunks to OTLP protobuf, including:
out_opentelemetryexporting logs with protobuf;in_opentelemetryevents are re-encoded to protobuf;The benefit grows with record and attribute count. It is not expected to affect JSON output or non-log signals.
Performance
Paired measurements used GCC 13.3.0, a Release build, CPU 2 affinity, and
perf stat -r 5. Baseline and arena binaries used identical benchmark code and fixtures.Valgrind Massif peak heap was unchanged:
The improvement is CPU and allocator-operation reduction, not a peak-memory reduction.
Unit and integration verification
The existing
flb-it-opentelemetryunit binary directly exercises protobuf log output, including plain-log conversion and resource/schema grouping. No new unit test was added because the change replaces allocation ownership without changing output behavior.ctest --test-dir build \ -R '^(flb-it-opentelemetry|flb-rt-in_opentelemetry)$' \ --output-on-failurePassed 2/2.
The complete internal OpenTelemetry unit binary passed under valgrind:
All unit tests passed. Valgrind reported 28,005 allocations and 28,005 frees, zero bytes in use at exit, zero leaks, and zero errors.
Focused OTLP protobuf log integration tests passed normally and with strict valgrind:
Both runs passed 2/2. A direct memcheck of the 40,960-record render also completed with zero errors, zero leaks, and zero bytes in use at exit.
Commit-prefix validation passed both for HEAD and the full PR range against
master.Summary by CodeRabbit