mp: extend cobj API to expose group metadata and attributes #11043
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (2)tests/internal/mp_chunk_cobj.c (7)
src/flb_mp.c (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (30)
🔇 Additional comments (13)
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.
ℹ️ 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".
src/flb_mp.c
Outdated
| if (record->event.group_metadata && | ||
| (record->event.group_metadata->type == MSGPACK_OBJECT_MAP || | ||
| record->event.group_metadata->type == MSGPACK_OBJECT_ARRAY)) { | ||
| record->cobj_group_metadata = flb_mp_object_to_cfl(record->event.group_metadata); | ||
| if (!record->cobj_group_metadata) { | ||
| cfl_object_destroy(record->cobj_record); | ||
| cfl_object_destroy(record->cobj_metadata); | ||
| flb_free(record); | ||
| return FLB_MP_CHUNK_RECORD_ERROR; | ||
| } | ||
| } | ||
|
|
||
| if (record->event.group_attributes && | ||
| (record->event.group_attributes->type == MSGPACK_OBJECT_MAP || | ||
| record->event.group_attributes->type == MSGPACK_OBJECT_ARRAY)) { | ||
| record->cobj_group_attributes = flb_mp_object_to_cfl(record->event.group_attributes); | ||
| if (!record->cobj_group_attributes) { |
There was a problem hiding this comment.
flb_mp_chunk_cobj_record_next now builds cobj_group_metadata and cobj_group_attributes so processors can mutate group context, but flb_mp_chunk_cobj_encode still serializes only cobj_metadata and cobj_record. If a processor updates the new group objects on a record, those changes are never copied back to the group header when the chunk is re-encoded, so modifications are silently lost and the feature described in the commit message (“read and modify … using the CFL object interface”) does not work. The encoder should either use these new pointers when emitting group start records or share a single cobj across all records in a group.
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/flb_mp.c (2)
807-837: Bug: ARRAY branch uses map fields and counts (breaks array conversion).In mp_object_to_cfl(), the MSGPACK_OBJECT_ARRAY case allocates and iterates using o->via.map.size instead of o->via.array.size. This can cause out-of-bounds/incorrect conversion and now affects group_attributes/metadata when arrays are present.
Apply:
- case MSGPACK_OBJECT_ARRAY: - array = cfl_array_create(o->via.map.size); + case MSGPACK_OBJECT_ARRAY: + array = cfl_array_create(o->via.array.size); if (!array) { return -1; } ret = 0; - - for (i = 0; i < o->via.map.size; i++) { + for (i = 0; i < o->via.array.size; i++) { ret = mp_object_to_cfl((void *) &var, &o->via.array.ptr[i]); if (ret == CFL_OBJECT_KVLIST) { ret = cfl_array_append_kvlist(array, var); } else if (ret == CFL_OBJECT_VARIANT) { ret = cfl_array_append(array, var); } else if (ret == CFL_OBJECT_ARRAY) { ret = cfl_array_append_array(array, var); } else { ret = -1; break; } }
1036-1045: Early-return leak in flb_mp_cfl_to_msgpack on error.If mp_cfl_to_msgpack() fails, mp_sbuf.data isn’t freed. Destroy the sbuffer before returning.
ret = mp_cfl_to_msgpack(obj->variant, &mp_sbuf, &mp_pck); if (ret == -1) { - return -1; + msgpack_sbuffer_destroy(&mp_sbuf); + return -1; }
🧹 Nitpick comments (8)
tests/internal/log_event_decoder.c (1)
24-24: Optional: drop unused include.flb_log_event_encoder.h isn’t used in this file; consider removing to reduce compile surface.
include/fluent-bit/flb_mp_chunk.h (1)
39-41: New group fields added — please document lifecycle.cobj_group_metadata and cobj_group_attributes extend per-record context. Add a brief comment noting:
- They mirror event.group_metadata/attributes when present.
- Initialized to NULL in flb_mp_chunk_record_create and destroyed in both flb_mp_chunk_cobj_destroy and flb_mp_chunk_cobj_record_destroy.
Confirm no external users rely on flb_mp_chunk_record binary layout across releases.
tests/internal/mp_chunk_cobj.c (3)
56-66: Avoid hardcoded key lengths; use sizeof to prevent off-by-one mistakes.Use sizeof("literal") - 1 for key lengths.
- ret = flb_log_event_encoder_append_metadata_values(builder, - FLB_LOG_EVENT_STRING_VALUE("group_id", 8), + ret = flb_log_event_encoder_append_metadata_values(builder, + FLB_LOG_EVENT_STRING_VALUE("group_id", sizeof("group_id") - 1), FLB_LOG_EVENT_INT64_VALUE(42)); @@ - ret = flb_log_event_encoder_append_body_values(builder, - FLB_LOG_EVENT_STRING_VALUE("resource_type", 13), + ret = flb_log_event_encoder_append_body_values(builder, + FLB_LOG_EVENT_STRING_VALUE("resource_type", sizeof("resource_type") - 1), FLB_LOG_EVENT_CSTRING_VALUE("demo"));
142-169: Test directly accesses cfl_variant internals; prefer public helpers when possible.Accessing variant->type/data couples the test to internal layout. If available, use public getters (e.g., type and string accessors) to reduce brittleness. Current checks are fine for now.
If CFL lacks such getters, consider adding minimal helpers and switching tests to them.
132-140: Optional: also assert the trailing GROUP_END record.You build a start, one normal record, and a group end. Verifying the GROUP_END strengthens coverage and ensures decoder iteration aligns with expectations.
Also applies to: 162-169
src/flb_mp.c (3)
849-853: Nit: typo in comment.“strin” → “string”.
- /* force key type to be strin, otherwise just abort */ + /* force key type to be string, otherwise just abort */
1118-1172: Encoding path ignores group metadata/attributes — confirm intent.flb_mp_chunk_cobj_encode() emits timestamp, metadata, body only. Group context (cobj_group_metadata/attributes) isn’t serialized, so processors’ changes to group objects won’t round-trip.
- If intentional: add a short comment clarifying encode omits groups by design.
- If not: we’ll need to emit a GROUP_START header via encoder (group_init + set group metadata/attributes) before the first record, and a GROUP_END at the end.
I can draft the patch if you want this preserved.
1286-1296: Replace recursion with iteration to avoid deep call chains on large skips.Recursive calls to flb_mp_chunk_cobj_record_next when a record doesn’t match a condition can stack up. Convert to a loop that advances until a match/EOF to avoid potential stack growth on large streams.
Also applies to: 1312-1321, 1336-1345
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
include/fluent-bit/flb_mp_chunk.h(1 hunks)src/flb_mp.c(4 hunks)tests/internal/CMakeLists.txt(1 hunks)tests/internal/log_event_decoder.c(1 hunks)tests/internal/mp_chunk_cobj.c(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/flb_mp.c (2)
lib/cfl/src/cfl_object.c (1)
cfl_object_destroy(95-106)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
tests/internal/mp_chunk_cobj.c (6)
src/flb_log_event_encoder.c (8)
flb_log_event_encoder_create(76-97)flb_log_event_encoder_group_init(390-407)flb_log_event_encoder_group_header_end(409-419)flb_log_event_encoder_begin_record(246-254)flb_log_event_encoder_set_timestamp(276-287)flb_log_event_encoder_commit_record(256-274)flb_log_event_encoder_group_end(421-443)flb_log_event_encoder_destroy(99-116)include/fluent-bit/flb_time.h (1)
flb_time_set(75-79)src/flb_log_event_decoder.c (4)
flb_log_event_decoder_init(99-116)flb_log_event_decoder_read_groups(85-97)flb_log_event_decoder_get_record_type(408-428)flb_log_event_decoder_destroy(147-179)src/flb_mp.c (3)
flb_mp_chunk_cobj_create(1063-1083)flb_mp_chunk_cobj_record_next(1214-1356)flb_mp_chunk_cobj_destroy(1182-1212)lib/cfl/src/cfl_kvlist.c (1)
cfl_kvlist_fetch(418-421)lib/cfl/src/cfl_variant.c (1)
cfl_variant_size_get(292-295)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (30)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: PR - fuzzing test
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: pr-compile-without-cxx (3.31.6)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: pr-compile-centos-7
🔇 Additional comments (5)
tests/internal/CMakeLists.txt (1)
27-27: Wiring new internal test looks good.mp_chunk_cobj.c is correctly added to UNIT_TESTS_FILES and will be linked against fluent-bit-static/cfl-static.
src/flb_mp.c (4)
1257-1282: Group CFL conversion and cleanup look correct.Good type checks, creation, and partial-failure cleanup for cobj_group_metadata/attributes.
1057-1059: Init of new group pointers to NULL is correct.Prevents accidental free/double-free on error paths.
1200-1205: Destroy group objects in container destructor — good.Properly frees cobj_group_metadata/attributes.
1383-1388: Destroy group objects in record destructor — good.Consistent cleanup in per-record destroy path.
| #include <cfl/cfl_kvlist.h> | ||
|
|
There was a problem hiding this comment.
Include CFL umbrella header for variant APIs and struct layout.
The test uses struct cfl_variant fields and cfl_variant_size_get; include cfl/cfl.h to ensure prototypes and full type definitions are available across toolchains.
#include <fluent-bit/flb_mp_chunk.h>
-#include <cfl/cfl_kvlist.h>
+#include <cfl/cfl.h>
+#include <cfl/cfl_kvlist.h>📝 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.
| #include <cfl/cfl_kvlist.h> | |
| #include <cfl/cfl.h> | |
| #include <cfl/cfl_kvlist.h> | |
🤖 Prompt for AI Agents
In tests/internal/mp_chunk_cobj.c around lines 26 to 27, the test uses struct
cfl_variant fields and calls cfl_variant_size_get but only includes
cfl/cfl_kvlist.h; add the umbrella header cfl/cfl.h to the includes so variant
APIs and full type definitions/prototypes are available across toolchains,
placing the new include alongside existing includes (before or after
cfl_kvlist.h) and remove no other includes.
Add cobj_group_metadata and cobj_group_attributes to flb_mp_chunk_record to give processors access to shared group context alongside individual record data. This enables processors to read and modify OpenTelemetry resource attributes and scope metadata using the CFL object interface. Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
1 participant
Add cobj_group_metadata and cobj_group_attributes to flb_mp_chunk_record to give processors access to shared group context alongside individual record data. This enables processors to read and modify OpenTelemetry
resource attributes and scope metadata using the CFL object interface.
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
New Features
Tests