record_accessor: single-pass keymap resolution#11432
Conversation
Improve performance by removing multiple passes: - Resolve all KEYMAP lookups in one map pass and cache results - Add flb_ra_value_from_object() in flb_ra_key for value-from-object - Add optimized version of flb_ra_translate_check() Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
📝 WalkthroughWalkthroughThe record accessor refactors keymap handling to support single-pass cached resolution of KEYMAP lookups. A new helper function and cache structure pre-resolve all KEYMAP values from the input map, improving efficiency while maintaining backward compatibility with direct translation paths. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
![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: 3548a16474
ℹ️ 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".
| keymap_cache[keymap_idx] = flb_ra_value_from_object(&val, | ||
| rp->key->subkeys, | ||
| FLB_TRUE); |
There was a problem hiding this comment.
The new code calls flb_ra_value_from_object, but there is no definition or declaration for this symbol anywhere in the repository (repo-wide search only finds this call). That means the build will fail with an implicit-declaration error or undefined symbol at link time for any configuration that compiles flb_record_accessor.c. If this function is intended to be added, it needs to be implemented and declared in the appropriate header (likely alongside other flb_ra_* helpers).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@edsiper Could you take a look on this?
We didn't have a definition of flb_ra_value_from_object in our code base in master branch.
There was a problem hiding this comment.
@cosmo0920 thanks, I did a partial commit, need to repush today, thanks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/flb_record_accessor.c`:
- Around line 744-756: The warning log dereferences rp->key->name which can be
NULL; update the block in flb_record_accessor (the section checking if check ==
FLB_TRUE && found == FLB_FALSE) to guard against a NULL rp->key before accessing
name — e.g., compute a safe_key_name string from rp->key (or use a literal like
"<null>") and use that in flb_warn, or skip logging the name when rp->key is
NULL; ensure the same NULL-safe approach is applied before any use of rp->key in
this failure branch and preserve the existing cleanup of buf and keymap_cache.
- Around line 705-707: The call to flb_ra_value_from_object is unresolved
because the function is missing; implement flb_ra_value_from_object in
src/flb_ra_key.c (use the same semantics as the existing flb_ra_key_to_value_ext
— accept the object/map and subkeys and a copy flag and return struct
flb_ra_value *) and have it either perform the conversion logic or delegate to
flb_ra_key_to_value_ext, then add its prototype to
include/fluent-bit/flb_ra_key.h so the symbol is declared for callers (ensure
the implementation signature matches how it is invoked at the call site and
update any include or linkage if needed).
🧹 Nitpick comments (1)
src/flb_record_accessor.c (1)
717-724: Consider extracting cache cleanup into a helper to reduce duplication.The same cleanup pattern for
keymap_cacheappears four times. A small helper function would reduce code duplication and ensure consistent cleanup.♻️ Example helper extraction
/* Add near the top of the file or before flb_ra_translate_check */ static void ra_keymap_cache_destroy(struct flb_ra_value **cache, int count) { int i; if (!cache) { return; } for (i = 0; i < count; i++) { if (cache[i]) { flb_ra_key_value_destroy(cache[i]); } } flb_free(cache); }Then replace each cleanup block with:
ra_keymap_cache_destroy(keymap_cache, keymap_count);Also applies to: 747-754, 771-778, 786-793
| keymap_cache[keymap_idx] = flb_ra_value_from_object(&val, | ||
| rp->key->subkeys, | ||
| FLB_TRUE); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for function declaration in headers
echo "=== Checking header declaration ==="
fd -e h -e hpp | xargs rg -l 'flb_ra_value_from_object' 2>/dev/null | head -10
echo ""
rg -A3 'flb_ra_value_from_object' --type=c --type=h | head -50
echo ""
echo "=== Checking implementation in source files ==="
fd -e c | xargs grep -l 'flb_ra_value_from_object' 2>/dev/null | head -10Repository: fluent/fluent-bit
Length of output: 561
🏁 Script executed:
#!/bin/bash
# Search for the actual definition of flb_ra_value_from_object in the file
echo "=== Searching for function definition in src/flb_record_accessor.c ==="
rg -n 'flb_ra_value_from_object' src/flb_record_accessor.c
echo ""
echo "=== Looking for function signature (expanded context) ==="
rg -B5 -A10 'flb_ra_value_from_object' src/flb_record_accessor.c | head -60
echo ""
echo "=== Check if it might be in flb_ra_key header/source ==="
fd -e h -e c | xargs grep -l 'flb_ra_key' | head -10
</parameter>Repository: fluent/fluent-bit
Length of output: 1342
🏁 Script executed:
#!/bin/bash
# Search for function definition in flb_ra_key.c
echo "=== Searching for function definition in src/flb_ra_key.c ==="
rg -n 'flb_ra_value_from_object' src/flb_ra_key.c
echo ""
echo "=== Looking for flb_ra_key header files ==="
fd -e h | xargs grep -l 'flb_ra_key' | head -10
echo ""
echo "=== Search for the function in all header files ==="
fd -e h | xargs grep -n 'flb_ra_value_from_object' 2>/dev/null
echo ""
echo "=== Check if function is defined in src/flb_ra_key.c ==="
rg -B3 -A15 'flb_ra_value_from_object' src/flb_ra_key.cRepository: fluent/fluent-bit
Length of output: 466
🏁 Script executed:
#!/bin/bash
# Broader search across entire codebase
echo "=== Broad search for flb_ra_value_from_object ==="
rg 'flb_ra_value_from_object' -t c -t h
echo ""
echo "=== Check for similar function names ==="
rg 'flb_ra.*value.*object' -t c -t h
echo ""
echo "=== Check context around line 705 in detail ==="
sed -n '700,710p' src/flb_record_accessor.c
echo ""
echo "=== Look for possible macro definitions ==="
rg 'define.*flb_ra_value_from_object' -t c -t hRepository: fluent/fluent-bit
Length of output: 8823
🏁 Script executed:
#!/bin/bash
# Check what's actually in the flb_ra_key.h header
echo "=== Contents of include/fluent-bit/flb_ra_key.h ==="
cat include/fluent-bit/flb_ra_key.h
echo ""
echo "=== Check flb_record_accessor.h for related functions ==="
grep -n 'flb_ra.*value' include/fluent-bit/flb_record_accessor.h | head -20Repository: fluent/fluent-bit
Length of output: 3379
🏁 Script executed:
#!/bin/bash
# Check git log to see if this is new code
echo "=== Check git log for src/flb_record_accessor.c ==="
git log --oneline -n 10 -- src/flb_record_accessor.c
echo ""
echo "=== Check git log for src/flb_ra_key.c ==="
git log --oneline -n 10 -- src/flb_ra_key.c
echo ""
echo "=== Check if there's been a recent commit adding this function call ==="
git log -p --all -S 'flb_ra_value_from_object' -- src/flb_record_accessor.c | head -100Repository: fluent/fluent-bit
Length of output: 341
🏁 Script executed:
#!/bin/bash
# Check the recent commit that added this code
echo "=== Check the commit that introduced the function call ==="
git show --stat 3548a164
echo ""
echo "=== Look at the full diff to see if function was added ==="
git show 3548a164 | head -200Repository: fluent/fluent-bit
Length of output: 50374
🏁 Script executed:
#!/bin/bash
# Final verification - check if there's a typo or similar function name
echo "=== All functions in flb_ra_key.h header ==="
grep -n 'struct flb_ra_value\|^[a-z_]*(' include/fluent-bit/flb_ra_key.h | grep -E '^[0-9]+:.*\('
echo ""
echo "=== Functions actually defined in flb_ra_key.c ==="
rg -n '^(struct flb_ra_value|int|void) \w+\(' src/flb_ra_key.c | head -20Repository: fluent/fluent-bit
Length of output: 820
flb_ra_value_from_object() is not implemented.
The function is called at lines 705–707 but is neither declared in the header nor defined in the source. The commit message promises this function ("Add flb_ra_value_from_object() in flb_ra_key for value-from-object") but the implementation is missing. This will cause a linker error (undefined reference).
A similar function exists—flb_ra_key_to_value_ext()—which takes (ckey, map, subkeys, copy) parameters and returns struct flb_ra_value *. Verify the intended function signature and add the missing implementation in src/flb_ra_key.c, then declare it in include/fluent-bit/flb_ra_key.h.
🤖 Prompt for AI Agents
In `@src/flb_record_accessor.c` around lines 705 - 707, The call to
flb_ra_value_from_object is unresolved because the function is missing;
implement flb_ra_value_from_object in src/flb_ra_key.c (use the same semantics
as the existing flb_ra_key_to_value_ext — accept the object/map and subkeys and
a copy flag and return struct flb_ra_value *) and have it either perform the
conversion logic or delegate to flb_ra_key_to_value_ext, then add its prototype
to include/fluent-bit/flb_ra_key.h so the symbol is declared for callers (ensure
the implementation signature matches how it is invoked at the call site and
update any include or linkage if needed).
| if (check == FLB_TRUE && found == FLB_FALSE) { | ||
| flb_warn("[record accessor] translation failed, root key=%s", rp->key->name); | ||
| flb_sds_destroy(buf); | ||
| if (keymap_cache) { | ||
| for (i = 0; i < keymap_count; i++) { | ||
| if (keymap_cache[i]) { | ||
| flb_ra_key_value_destroy(keymap_cache[i]); | ||
| } | ||
| } | ||
| flb_free(keymap_cache); | ||
| } | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
Potential NULL pointer dereference when logging warning.
If rp->key is NULL (handled at lines 536-539 and 616-619 which set *found = FLB_FALSE), and check == FLB_TRUE, the code will reach line 745 and dereference rp->key->name, causing a crash.
🐛 Proposed fix to guard against NULL key
keymap_idx++;
if (check == FLB_TRUE && found == FLB_FALSE) {
- flb_warn("[record accessor] translation failed, root key=%s", rp->key->name);
+ flb_warn("[record accessor] translation failed, root key=%s",
+ rp->key ? rp->key->name : "(null)");
flb_sds_destroy(buf);
if (keymap_cache) {📝 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.
| if (check == FLB_TRUE && found == FLB_FALSE) { | |
| flb_warn("[record accessor] translation failed, root key=%s", rp->key->name); | |
| flb_sds_destroy(buf); | |
| if (keymap_cache) { | |
| for (i = 0; i < keymap_count; i++) { | |
| if (keymap_cache[i]) { | |
| flb_ra_key_value_destroy(keymap_cache[i]); | |
| } | |
| } | |
| flb_free(keymap_cache); | |
| } | |
| return NULL; | |
| } | |
| if (check == FLB_TRUE && found == FLB_FALSE) { | |
| flb_warn("[record accessor] translation failed, root key=%s", | |
| rp->key ? rp->key->name : "(null)"); | |
| flb_sds_destroy(buf); | |
| if (keymap_cache) { | |
| for (i = 0; i < keymap_count; i++) { | |
| if (keymap_cache[i]) { | |
| flb_ra_key_value_destroy(keymap_cache[i]); | |
| } | |
| } | |
| flb_free(keymap_cache); | |
| } | |
| return NULL; | |
| } |
🤖 Prompt for AI Agents
In `@src/flb_record_accessor.c` around lines 744 - 756, The warning log
dereferences rp->key->name which can be NULL; update the block in
flb_record_accessor (the section checking if check == FLB_TRUE && found ==
FLB_FALSE) to guard against a NULL rp->key before accessing name — e.g., compute
a safe_key_name string from rp->key (or use a literal like "<null>") and use
that in flb_warn, or skip logging the name when rp->key is NULL; ensure the same
NULL-safe approach is applied before any use of rp->key in this failure branch
and preserve the existing cleanup of buf and keymap_cache.
2 participants
Improve performance by removing multiple passes:
From a local perf test we go from 94.61% --> 39.16%
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
Bug Fixes
Refactor