in_opentelemetry: do not use temp buffer for protobuf handling#11421
in_opentelemetry: do not use temp buffer for protobuf handling#11421
Conversation
📝 WalkthroughWalkthroughThe change refactors buffer management in OpenTelemetry log processing, replacing per-call local MsgPack buffers with direct use of encoder-provided packers. It updates error handling, dynamic field reset/flush management, nested map construction, and log body marshalling while preserving overall processing structure. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
…handling
Reuse the log event encoder's body/metadata packers for the protobuf
log path instead of temporary msgpack_sbuffer instances.
- Point mp_pck/mp_pck_meta at encoder->body.packer and encoder->metadata.packer.
Remove mp_sbuf and mp_sbuf_meta.
- Pack resource/scope group content into encoder->body via dynamic_field_reset,
pack, dynamic_field_flush (drop set_body_from_raw_msgpack and the temp
buffer copy).
- Pack per-record metadata into encoder->metadata the same way
(reset, otel_pack_v1_metadata, flush); drop set_metadata_from_raw_msgpack.
- Pack per-record body into encoder->body (reset, then either otlp_pack_any_value
for KVLIST or inline single-key map + value, then flush); drop per-record
set_body_from_raw_msgpack and append_body_values(MSGPACK_RAW_VALUE).
Removes per-record and per-group temporary packing, buffer clears,
and raw msgpack copies, reducing CPU and allocator pressure on the
protobuf log conversion path.
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
0c7e6fd to
3a07f0b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/in_opentelemetry/opentelemetry_logs.c (1)
370-384:⚠️ Potential issue | 🟠 MajorDon’t overwrite a prior failure from metadata append.
retfromflb_log_event_encoder_append_metadata_valuesis never checked before being overwritten, so a failure can be silently ignored and the group continues. Add a guard before the body reset.🛠️ Proposed fix
ret = flb_log_event_encoder_append_metadata_values(encoder, FLB_LOG_EVENT_STRING_VALUE("schema", 6), FLB_LOG_EVENT_STRING_VALUE("otlp", 4), FLB_LOG_EVENT_STRING_VALUE("resource_id", 11), FLB_LOG_EVENT_INT64_VALUE(resource_logs_index), FLB_LOG_EVENT_STRING_VALUE("scope_id", 8), FLB_LOG_EVENT_INT64_VALUE(scope_log_index)); +if (ret != FLB_EVENT_ENCODER_SUCCESS) { + flb_plg_error(ctx->ins, "failed to append group metadata: %s", + flb_log_event_encoder_get_error_description(ret)); + goto binary_payload_to_msgpack_end; +} ret = flb_log_event_encoder_dynamic_field_reset(&encoder->body); if (ret != FLB_EVENT_ENCODER_SUCCESS) {
🧹 Nitpick comments (1)
plugins/in_opentelemetry/opentelemetry_logs.c (1)
549-590: Cachelogs_body_keylength to avoid repeatedstrlen.This reduces repeated scans and makes the packing block a bit clearer.
♻️ Suggested tweak
logs_body_key = ctx->logs_body_key; if (logs_body_key == NULL) { logs_body_key = "log"; } +size_t logs_body_key_len = strlen(logs_body_key); ret = msgpack_pack_map(mp_pck, 1); if (ret == 0) { - ret = msgpack_pack_str(mp_pck, strlen(logs_body_key)); + ret = msgpack_pack_str(mp_pck, logs_body_key_len); } if (ret == 0) { ret = msgpack_pack_str_body(mp_pck, logs_body_key, - strlen(logs_body_key)); + logs_body_key_len); }
Reuse the log event encoder's body/metadata packers for the protobuf log path instead of temporary msgpack_sbuffer instances.
Point mp_pck/mp_pck_meta at encoder->body.packer and encoder->metadata.packer. Remove mp_sbuf and mp_sbuf_meta.
Pack resource/scope group content into encoder->body via dynamic_field_reset, pack, dynamic_field_flush (drop set_body_from_raw_msgpack and the temp buffer copy).
Pack per-record metadata into encoder->metadata the same way (reset, otel_pack_v1_metadata, flush); drop set_metadata_from_raw_msgpack.
Pack per-record body into encoder->body (reset, then either otlp_pack_any_value for KVLIST or inline single-key map + value, then flush); drop per-record set_body_from_raw_msgpack and append_body_values(MSGPACK_RAW_VALUE).
Removes per-record and per-group temporary packing, buffer clears, and raw msgpack copies, reducing CPU and allocator pressure on the protobuf log conversion path.
Perf changes
Tests with OTel gen tool, analyze overall time spent in
binary_payload_to_msgpack()Before

After

Gain: +1.69% , this is just CPU time (by voiding a temporary buffer we reduce potential memory fragmentation)
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