kv: add flb_kv_get_all_key_values function - #11788
Conversation
The goal of this enhancement is to extend the key-value store API to support retrieving all elements from the linked list in a single call. Currently, the only way to retrieve all elements is to guess the keys and repeatedly call `flb_kv_get_key_value`, which is especially problematic for plugin writers. Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request adds a bulk retrieval API for key-value entries and output properties. It introduces ChangesProperty Bulk Retrieval API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 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.
Actionable comments posted: 2
🤖 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 `@include/fluent-bit/flb_kv.h`:
- Line 44: flb_kv_get_all_key_values currently returns a pointer array without
length metadata, making callers unsafe to iterate; modify its signature to
include an output count (e.g., add a size_t *out_count parameter) or change the
return to a small struct { struct flb_kv **items; size_t count; } so callers can
know the array length; update the function implementation and all call sites
that use flb_kv_get_all_key_values to set/read the count and to free/iterate
correctly (search for references to flb_kv_get_all_key_values to update usage).
In `@tests/internal/kv.c`:
- Around line 49-61: The test accesses pairs[0..2] without ensuring the
underlying list contains three items; change the check so after verifying pairs
is non-NULL you also assert mk_list_size(&list) == 3 (or equivalent size check
against the list used to build pairs) before indexing pairs[0], pairs[1], and
pairs[2]; reference the list used when calling flb_kv_item_set() and use that
size guard so the subsequent TEST_CHECK(strcmp(...)) validations only run when
the list actually has three entries.
🪄 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: 96ef314f-ede6-4b61-9703-2e217b4ad117
📒 Files selected for processing (3)
include/fluent-bit/flb_kv.hsrc/flb_kv.ctests/internal/kv.c
This out_count parameter is needed so the callers can safely iterate without external assumptions. Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
…t parameter Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/internal/kv.c (1)
44-46: ⚡ Quick winCheck each
flb_kv_item_setresult explicitlyPlease assert each insertion succeeds, so failures are reported at the source rather than only via downstream
count/content checks.Suggested patch
- flb_kv_item_set(&list, "host", "localhost"); - flb_kv_item_set(&list, "port", "8080"); - flb_kv_item_set(&list, "path", "/api"); + TEST_CHECK(flb_kv_item_set(&list, "host", "localhost") != NULL); + TEST_CHECK(flb_kv_item_set(&list, "port", "8080") != NULL); + TEST_CHECK(flb_kv_item_set(&list, "path", "/api") != NULL);🤖 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 `@tests/internal/kv.c` around lines 44 - 46, Each flb_kv_item_set call should have its return value checked and asserted immediately so insertion failures are reported at the source; update the three calls to flb_kv_item_set(&list, "host", "localhost"), flb_kv_item_set(&list, "port", "8080"), and flb_kv_item_set(&list, "path", "/api") to capture their returns into distinct variables (e.g., ret_host/ret_port/ret_path) and add assertions that each return indicates success (match the test framework's success condition) before proceeding to later count/content checks.
🤖 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.
Nitpick comments:
In `@tests/internal/kv.c`:
- Around line 44-46: Each flb_kv_item_set call should have its return value
checked and asserted immediately so insertion failures are reported at the
source; update the three calls to flb_kv_item_set(&list, "host", "localhost"),
flb_kv_item_set(&list, "port", "8080"), and flb_kv_item_set(&list, "path",
"/api") to capture their returns into distinct variables (e.g.,
ret_host/ret_port/ret_path) and add assertions that each return indicates
success (match the test framework's success condition) before proceeding to
later count/content checks.
|
hey @edsiper, do you think this one is ready to be merged? |
Why do we need this PR? |
The main issue is that as a Fluent Bit Go plugin creator currently I have no straightforward way to retrieve all available plugin configuration fields through a single API. Instead, I must manually query individual fields using
More context is available here. |
|
@edsiper wdyt? |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/flb_config.c`:
- Around line 679-682: Add a documentation comment block above the function
`flb_config_prop_get_all` that clearly specifies the memory ownership model and
cleanup responsibility. The comment should document that the returned array is
heap-allocated by `flb_kv_get_all_key_values()`, explain that the caller is
responsible for freeing it using `flb_free()`, describe the return value as a
heap-allocated array of `flb_kv` pointers that may be NULL if the list is empty
or NULL, and clarify that the `out_count` parameter receives the element count
and is set to 0 if a NULL array is returned. This documentation is critical for
API consumers, particularly Go plugin authors, to prevent memory leaks.
In `@src/flb_output.c`:
- Around line 1182-1186: The function flb_output_get_all_properties lacks
documentation about memory ownership and cleanup requirements, which is critical
since it is exposed via the public API in flb_api.h for external plugin
developers. Add a documentation comment above the function definition that
clearly specifies: the returned heap-allocated array must be freed by the caller
using flb_free(), the function returns NULL if the output instance has no
properties, and the out_count parameter receives the element count of the
returned array. Follow the same documentation pattern as
flb_config_prop_get_all() since flb_output_get_all_properties delegates to that
function.
🪄 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: eaa4c41e-ad6b-435a-9af0-553fee8017a7
📒 Files selected for processing (6)
include/fluent-bit/flb_api.hinclude/fluent-bit/flb_config.hinclude/fluent-bit/flb_output.hsrc/flb_api.csrc/flb_config.csrc/flb_output.c
|
@cosmo0920 @edsiper I included the |
|
@cosmo0920 @edsiper wdyt? |
should we implement instead a way for Golang plugins to register a config map ? I know this is a simple workaround but how do we handle config options which are mistyped ? (actually thats why we implemented config maps) cc: @cosmo0920 |
207d2ae to
fa5e6ed
Compare
Introduce flb_config_prop_get_all() as the property-list counterpart of flb_config_prop_get(). It delegates to flb_kv_get_all_key_values() so callers get the full list of properties in one call instead of looking them up one key at a time. Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Expose flb_output_get_all_properties() so plugin code with only an output instance in hand can obtain every configured property in a single call. Delegates to flb_config_prop_get_all() over the instance's properties list. Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Publish flb_output_get_all_properties() through struct flb_api so Go proxy plugins can retrieve every configured property on an output instance from a single call, matching the individual-getter pattern already provided for output_get_property. Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Add a docstring describing ownership: the returned array is owned by the caller and released with flb_free(), while the flb_kv entries it points to remain owned by the source list. Also spell out the return semantics for empty/NULL input and allocation failure. Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Add a docstring pointing at flb_kv_get_all_key_values() for the full ownership contract, and summarise the caller responsibilities inline. Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Add a docstring covering ownership of the returned array, the fate of the underlying flb_kv entries, and the NULL-return conditions. Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Add the input-side counterpart of flb_output_get_all_properties() so Go proxy plugins on the input path can retrieve every configured property in a single call, matching the parity already established for flb_input_get_property/flb_output_get_property. Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Move output_get_all_properties to the end of struct flb_api and add input_get_all_properties beside it, so existing Go plugins compiled against the previous flb_api layout keep pointing at the correct slots. Function-pointer assignment in flb_api_create() is reordered to match. Reported-by: Hiroshi Hatake <cosmo0920.wp@gmail.com> Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
fa5e6ed to
925c83f
Compare
If I understand it correctly, the idea is to extend |
yes, a way for the golang developer to register a config map from Go and get exposed to fluent bit, so fluent bit will not allow to run it if an unknown property is passed by mistake |
@cosmo0920 @edsiper I created this PR #12058 as follow-up to this discussion, when you have time can give it a look |
Add
flb_kv_get_all_key_valuesfunction formk_list.Addresses #11776 (first and second sub-tasks mentioned in the issue)
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
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.