filter_lookup: added filter for key value lookup #10620
Conversation
olegmukhin
requested review from
celalettin1286,
edsiper,
fujimotos,
koleini,
leonardo-albertovich,
niedbalski and
patrick-stephens
as code owners
|
Test configuration Fluent Bit YAML Configuration To test new filter we will load a range of log values including, strings (different cases), integer, boolean, embedded quotes and other value types. devices.log {"hostname": "server-prod-001"}
{"hostname": "Server-Prod-001"}
{"hostname": "db-test-abc"}
{"hostname": 123}
{"hostname": true}
{"hostname": " host with space "}
{"hostname": "quoted \"host\""}
{"hostname": "unknown-host"}
{}
{"hostname": [1,2,3]}
{"hostname": {"sub": "val"}}
{"hostname": " "}CSV configuration will aim to test key overwrites, different types of strings, use and escaping of quotes. device-bu.csv When executed with verbose flag the following out is produced. Test output Output shows correct matching and handling of different value types and correct output when no match is detected. Valgrind summary (after run with multiple types of lookups): |
|
Documentation for this filter has been submitted as part of #fluent/fluent-bit-docs/pull/1953. |
|
Added unit tests for lookup filter. All tests pass: Valgrind results are showing appropriate memory management. |
|
Added fix for failing checks on Cent OS 7 and Windows. Please rerun. |
|
Last check is failing due to Cent OS 7 incompatibility in unit test file - fix in last commit. Please rerun. |
|
Could this please get one more run at the checks? I didn't realise we need this to compile on Cent OS 7 - should be good now with last commit. |
|
Can you rebase and push so it reruns tests? |
|
@patrick-stephens addressed new AI review comment and rebased🤞. Let me know if there is anything else outstanding here. 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: 1
🧹 Nitpick comments (2)
plugins/filter_lookup/lookup.c (2)
348-550: Consider removing redundantgoto next_linestatements.The
next_linelabel at line 548 simply executescontinue, making thegotostatements at lines 348, 364, and 380 unnecessary. You can replace them with directcontinuestatements for clearer code flow.Apply this diff to simplify:
if (dynbuf_append_char(&key_buf, '"') != 0) { flb_plg_debug(ctx->ins, "Buffer allocation failed for line %d", line_num); dynbuf_destroy(&key_buf); dynbuf_destroy(&val_buf); flb_free(line); line_num++; - goto next_line; + continue; }Apply similar changes at lines 364 and 380, and remove the
next_line:label entirely.
1008-1008: Use precomputedresult_key_leninstead ofstrlen.Line 628 precomputes
ctx->result_key_lenspecifically for hot-path optimization, but this line callsstrlen(ctx->result_key)instead. Use the precomputed value for consistency and to avoid redundant string traversal.Apply this diff:
- ret = flb_log_event_encoder_append_body_string(&log_encoder, ctx->result_key, strlen(ctx->result_key)); + ret = flb_log_event_encoder_append_body_string(&log_encoder, ctx->result_key, ctx->result_key_len);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/filter_lookup/lookup.c(1 hunks)plugins/filter_lookup/lookup.h(1 hunks)tests/runtime/filter_lookup.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/runtime/filter_lookup.c
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/filter_lookup/lookup.c (6)
tests/runtime/filter_lookup.c (3)
dynbuf_init(691-698)dynbuf_append_char(700-711)dynbuf_destroy(713-720)include/fluent-bit/flb_mem.h (2)
flb_free(126-128)flb_calloc(84-96)src/flb_hash_table.c (4)
flb_hash_table_add(401-494)flb_hash_table_create(99-137)flb_hash_table_destroy(197-215)flb_hash_table_get(496-522)src/flb_record_accessor.c (3)
flb_ra_create(271-358)flb_ra_destroy(232-248)flb_ra_get_value_object(803-814)src/flb_log_event_encoder.c (7)
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_rollback_record(241-244)flb_log_event_encoder_init(42-74)flb_log_event_encoder_claim_internal_buffer_ownership(118-124)flb_log_event_encoder_destroy(99-116)src/flb_ra_key.c (1)
flb_ra_key_value_destroy(842-851)
🔇 Additional comments (2)
plugins/filter_lookup/lookup.h (1)
1-56: LGTM! Well-structured plugin header.The header properly defines the plugin's public API with appropriate include guards, conditional metrics compilation, and clear field documentation. The precomputed
result_key_lenfield (line 40) is a good optimization for the hot path.plugins/filter_lookup/lookup.c (1)
1056-1094: LGTM! Proper resource cleanup and plugin registration.The exit callback correctly frees all resources in the proper order: val_list nodes, record accessor, hash table, and context. The plugin registration with config_map is well-structured with appropriate field types and descriptions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugins/filter_lookup/lookup.c (1)
348-348: Optional:goto next_lineis redundant—replace withcontinue.Multiple error paths use
goto next_line(lines 348, 364, 380, 413, 429, 444), but thenext_linelabel at line 548 immediately executescontinue. Since cleanup is already complete before the goto, you can replace allgoto next_linestatements withcontinuedirectly, eliminating the unnecessary label.Also applies to: 364-364, 380-380, 413-413, 429-429, 444-444
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/filter_lookup/lookup.c(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/filter_lookup/lookup.c (6)
include/fluent-bit/flb_mem.h (2)
flb_free(126-128)flb_calloc(84-96)src/flb_hash_table.c (4)
flb_hash_table_add(401-494)flb_hash_table_create(99-137)flb_hash_table_destroy(197-215)flb_hash_table_get(496-522)src/flb_record_accessor.c (3)
flb_ra_create(271-358)flb_ra_destroy(232-248)flb_ra_get_value_object(803-814)src/flb_log_event_encoder.c (7)
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_rollback_record(241-244)flb_log_event_encoder_init(42-74)flb_log_event_encoder_claim_internal_buffer_ownership(118-124)flb_log_event_encoder_destroy(99-116)src/flb_log_event_decoder.c (3)
flb_log_event_decoder_init(103-120)flb_log_event_decoder_destroy(151-178)flb_log_event_decoder_next(309-456)src/flb_ra_key.c (1)
flb_ra_key_value_destroy(842-851)
🔇 Additional comments (2)
plugins/filter_lookup/lookup.c (2)
987-1011: LGTM: Error recovery correctly breaks out of for-loop.The
goto next_recordpattern at line 1002 is correct. Whenappend_body_valuesfails during key copying, the code:
- Rolls back the incomplete record
- Emits the original record
- Jumps to
next_recordlabel (line 1007)- Checks the error condition (line 1009) and continues the outer while-loop
This correctly skips the rest of the record construction (appending result_key and committing) and moves to the next log event. The past review comment suggesting this was incorrect was mistaken—the current implementation properly handles the error case.
1-1100: Well-implemented CSV lookup filter with robust error handling.This is a comprehensive and well-structured implementation of a CSV-based lookup filter. The code demonstrates:
✅ Solid design decisions:
- Proper separation of concerns with helper functions
- Robust CSV parsing with quote handling and dynamic buffers
- Comprehensive error handling and cleanup paths
- Efficient hash table usage for lookups
- Support for non-string type conversion with explicit array/map skipping
✅ Memory safety:
- Past review issues (unmatched quote detection, val_node allocation order) have been properly addressed
- Consistent cleanup patterns throughout
- Proper ownership tracking via val_list
✅ Good practices:
- Platform-specific code properly guarded
- Metrics integration with proper guards
- Configurable behavior via config_map
- Comprehensive logging at appropriate levels
The implementation follows Fluent Bit patterns correctly and is production-ready.
|
@patrick-stephens I think maybe the runner ran out of disk space for 1 of the jobs? I don't think this is related to this PR, but let me know if you need anything :) |
|
We fixed no left device error in the current master. |
New filter aims to address use case of simple data enrichment using static key value lookup. The filter loads first two columns of CSV file into memory as a hash table. When a specified record value matches the key in the hash table the value will be appended to the record (based on key name defined in the filter inputs).) Tested with valgrind. Signed-off-by: Oleg Mukhin <[email protected]>
- Removed unecessary FLB_FILTER_LOOKUP build flag now LookUp is enabled by default like other filters (without flag). - Fixed critical use-after-free bug in numeric value lookups. - Added processed_records_total, matched_records_total and skipped_records_total metrics to enable operational visibility - Added unit tests to cover handling of different data types, CSV loading/handling and metrics tests. Tested with valgrind - no memory leaks. All unit tests pass. Signed-off-by: Oleg Mukhin <[email protected]>
- fix variable declarations and remove C99 features - Conditional compilation for Windows vs Unix headers/functions - Replace bool with int, fix format specifiers, update comments All 15 unit tests for filter passed. Signed-off-by: Oleg Mukhin <[email protected]>
- fix variable declarations and remove C99 features for unit tests - Conditional compilation for Windows for unit test features All 15 unit tests for filter passed. Signed-off-by: Oleg Mukhin <[email protected]>
Addressed following issues: Fix potential memory leak when val_node allocation fails Wrap test metrics code with FLB_HAVE_METRICS guards Replace metric macros with enum to prevent namespace pollution Gate plugin registration on FLB_RECORD_ACCESSOR option Add unmatched quote detection after key parsing in CSV loader Replace magic numbers with semantic msgpack type checking Fix thread safety in lookup filter metrics macros Eliminated potential segfaults from null pointer dereferences Added defensive checks to the metric creation code Optimise hot path by eliminating repeated strlen calls Signed-off-by: Oleg Mukhin <[email protected]>
- Added skip_header_row config option (defaults to false for headerless CSVs) - Renamed 'file' parameter to 'data_source' for future URL/database support - Added skip_header_row test to verify both true/false behavior Signed-off-by: Oleg Mukhin <[email protected]>
Fix bug where continue inside for-loop breaks error recovery. The fix uses goto to jump past the key copying code and explicitly checks encoder state before proceeding. Signed-off-by: Oleg Mukhin <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugins/filter_lookup/lookup.c (1)
1014-1014: Minor optimization: use precomputedresult_key_len.Line 1014 calls
strlen(ctx->result_key)in the hot path, butresult_key_lenis already precomputed during initialization (line 628) for exactly this purpose.Apply this diff:
- ret = flb_log_event_encoder_append_body_string(&log_encoder, ctx->result_key, strlen(ctx->result_key)); + ret = flb_log_event_encoder_append_body_string(&log_encoder, ctx->result_key, ctx->result_key_len);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmake/plugins_options.cmake(1 hunks)plugins/CMakeLists.txt(1 hunks)plugins/filter_lookup/CMakeLists.txt(1 hunks)plugins/filter_lookup/lookup.c(1 hunks)plugins/filter_lookup/lookup.h(1 hunks)tests/runtime/CMakeLists.txt(1 hunks)tests/runtime/filter_lookup.c(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/runtime/filter_lookup.c
🚧 Files skipped from review as they are similar to previous changes (4)
- cmake/plugins_options.cmake
- plugins/filter_lookup/CMakeLists.txt
- plugins/CMakeLists.txt
- tests/runtime/CMakeLists.txt
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/filter_lookup/lookup.c (9)
tests/runtime/filter_lookup.c (3)
dynbuf_init(691-698)dynbuf_append_char(700-711)dynbuf_destroy(713-720)include/fluent-bit/flb_mem.h (2)
flb_free(126-128)flb_calloc(84-96)src/flb_hash_table.c (4)
flb_hash_table_add(401-494)flb_hash_table_create(99-137)flb_hash_table_destroy(197-215)flb_hash_table_get(496-522)lib/cmetrics/src/cmt_counter.c (1)
cmt_counter_create(26-81)src/flb_metrics.c (1)
flb_metrics_add(129-178)src/flb_record_accessor.c (3)
flb_ra_create(271-358)flb_ra_destroy(232-248)flb_ra_get_value_object(803-814)src/flb_log_event_encoder.c (6)
flb_log_event_encoder_begin_record(246-254)flb_log_event_encoder_commit_record(256-274)flb_log_event_encoder_rollback_record(241-244)flb_log_event_encoder_init(42-74)flb_log_event_encoder_claim_internal_buffer_ownership(118-124)flb_log_event_encoder_destroy(99-116)src/flb_log_event_decoder.c (3)
flb_log_event_decoder_init(103-120)flb_log_event_decoder_destroy(151-178)flb_log_event_decoder_next(309-456)src/flb_ra_key.c (1)
flb_ra_key_value_destroy(842-851)
🔇 Additional comments (8)
plugins/filter_lookup/lookup.h (1)
1-56: LGTM! Clean and well-structured header.The header file is well-organized with:
- Proper header guards
- Clear separation of concerns (metrics enum, context struct, plugin declaration)
- Good inline documentation for the hot-path optimization (
result_key_len)- Appropriate conditional compilation for metrics
plugins/filter_lookup/lookup.c (7)
45-77: LGTM! Well-structured metric macros.The metric increment macros properly handle both the CMT (CMetrics) system and the legacy metrics system, with appropriate guards for when
FLB_HAVE_METRICSis not defined.
88-260: LGTM! Robust helper functions with proper error handling.The helper functions are well-implemented:
normalize_and_trimcorrectly handles whitespace and case normalization with clear return codes- Dynamic buffer operations use standard 2× growth strategy
read_line_dynamicproperly handles arbitrary line lengths, EOF conditions, and line ending normalization
262-554: LGTM! CSV parser with comprehensive error handling.The CSV loading function correctly handles:
- Optional header row skipping based on configuration
- Quote escaping (doubled quotes within quoted fields)
- Unmatched quote detection for both keys and values
- Proper memory management with val_node tracking before hash insertion
- Dynamic buffer cleanup on all error paths
All previously flagged issues have been properly addressed.
556-679: LGTM! Comprehensive initialization with proper validation.The initialization callback is well-structured with:
- Both CMT and legacy metrics setup
- Required parameter validation
- Cross-platform file accessibility checks
- Hot-path optimization:
result_key_lenprecomputed for fast comparison- Thorough error cleanup ensuring no resource leaks
681-708: LGTM! Clean helper for emitting original records.The
emit_original_recordhelper properly encapsulates the record emission pattern with appropriate error handling and metric tracking.
710-1005: LGTM! Robust record processing with comprehensive error handling.The filter callback is well-implemented with:
- Proper decoder/encoder lifecycle management
- Thorough type handling for lookup values (primitives converted to strings, arrays/maps skipped)
- Correct error recovery using goto/label pattern (addressing previous review)
- Comprehensive error handling with rollback and original record emission
- Efficient
any_modifiedtracking to avoid unnecessary buffer copies
1062-1100: LGTM! Complete cleanup and plugin registration.The exit callback and plugin registration are properly implemented:
- Safe list traversal and cleanup of all tracked values
- Destruction of record accessor and hash table
- Comprehensive config map with clear descriptions
- Standard plugin struct definition
|
@cosmo0920 @patrick-stephens thank you both. Rebased. 🤞 I'll pick up the nitpick comment from CodeRabbit in next PR - just want to get this one over the line. |
|
Can you link the docs PR? |
5 participants
Added a new LookUp filter to address use case when enrichment of record is required based on simple static key value lookup.
The filter loads a CSV file into a hash table for performance. It consider first column of the CSV to be the key and the second column to be the value. All other columns are ignored.
Where a record value (identified by
lookup_keyinput) matches the key from the CSV, the value from the CSV row is added under a new key (defined byresult_keyinput) to the record.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.
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.