out_syslog: Add nested keys support #11064
Conversation
simbou2000
requested review from
edsiper,
fujimotos and
koleini
as code owners
|
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. WalkthroughThe PR integrates record-accessor (RA) support into the syslog plugin, adding RA-based extraction for syslog fields (severity, facility, hostname, appname, procid, msgid, message) and structured-data keys, RA initialization/cleanup in config paths, and a runtime test for nested key resolution. Changes
Sequence DiagramsequenceDiagram
participant Config as Config Init
participant RA as Record Accessor
participant Proc as Message Processor
participant Norm as RA Result Normalizer
participant Output as RFC5424 Formatter
Config->>RA: Create RA handles for severity/facility/hostname/appname/procid/msgid/message and sd_keys
note right of RA #DDDDFF: Store normalized key names
Proc->>RA: Resolve field via RA if configured and field unset
RA->>Norm: Return RA result (scalar/map/array)
Norm-->>Proc: Provide normalized string (or SD entries for maps)
Proc->>Output: Build RFC5424 message with resolved fields and SD
Output-->>Proc: Formatted syslog message
Proc->>RA: Destroy RA values after use
Config->>RA: Destroy RA handles on config destroy
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
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 (1)
🧰 Additional context used🧬 Code graph analysis (1)plugins/out_syslog/syslog.c (3)
🔇 Additional comments (4)
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
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/runtime/out_syslog.c (1)
1679-1681: Incorrect TEST_LIST mappings for rfc3164 preset testsThese entries point to the rfc5424 functions instead of the rfc3164 variants. Fix the function names.
Apply:
- {"format_facility_preset_rfc3164", flb_test_facility_preset_rfc5424}, - {"format_hostname_preset_rfc3164", flb_test_hostname_preset_rfc5424}, + {"format_facility_preset_rfc3164", flb_test_facility_preset_rfc3164}, + {"format_hostname_preset_rfc3164", flb_test_hostname_preset_rfc3164},
🧹 Nitpick comments (3)
plugins/out_syslog/syslog_conf.h (1)
76-81: Prefer embedding the list head over heap-allocating the liststruct mk_list *ra_sd_keys forces extra malloc/free and error paths. Embed the list head (struct mk_list ra_sd_keys) like other mk_list usages in the codebase; initialize with mk_list_init(&ctx->ra_sd_keys). This simplifies lifetime management and reduces leak risk.
plugins/out_syslog/syslog.c (1)
550-584: snprintf size off by one (benign but unnecessary)You pass (val_size - 1) to snprintf; snprintf already accounts for the trailing NUL. Using val_size is clearer and lets it use the full buffer.
Apply:
- *val_len = snprintf(*val, val_size - 1, + *val_len = snprintf(*val, val_size, "%" PRIu64, rval->o.via.u64); @@ - *val_len = snprintf(*val, val_size - 1, + *val_len = snprintf(*val, val_size, "%" PRId64, rval->o.via.i64); @@ - *val_len = snprintf(*val, val_size - 1, + *val_len = snprintf(*val, val_size, "%f", rval->o.via.f64);tests/runtime/out_syslog.c (1)
1611-1669: Literal key with dot needs RA quotingIn flb_test_nested_keys_rfc5424, the record has a literal key "log.appname", but syslog_appname_key is set to "log.appname" (interpreted as nested path). Quote it using RA bracket syntax to address literal dots.
Apply:
- "syslog_appname_key", "log.appname", + "syslog_appname_key", "$['log.appname']",Please confirm RA grammar here; if your environment already resolves literal dots without quoting, ignore this change. Otherwise, the current test may sporadically fail to extract appname. Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
plugins/out_syslog/syslog.c(4 hunks)plugins/out_syslog/syslog_conf.c(3 hunks)plugins/out_syslog/syslog_conf.h(1 hunks)tests/runtime/out_syslog.c(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
plugins/out_syslog/syslog.c (3)
src/flb_record_accessor.c (1)
flb_ra_get_value_object(803-814)src/flb_ra_key.c (1)
flb_ra_key_value_destroy(842-851)src/flb_sds.c (1)
flb_sds_create_len(58-76)
tests/runtime/out_syslog.c (1)
src/flb_lib.c (4)
flb_output_set(515-546)flb_output_set_test(579-610)flb_start(914-925)flb_lib_push(774-801)
plugins/out_syslog/syslog_conf.c (3)
src/flb_sds.c (3)
flb_sds_cat(120-141)flb_sds_create_size(92-95)flb_sds_destroy(389-399)src/flb_record_accessor.c (2)
flb_ra_create(271-358)flb_ra_destroy(232-248)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
…ontext structure Signed-off-by: Simon Bouchard <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
plugins/out_syslog/syslog_conf.h (1)
26-26: Good fix: ensure mk_list completeness in public headerIncluding monkey/mk_core.h here guarantees struct mk_list is complete for by-value embedding.
plugins/out_syslog/syslog_conf.c (2)
319-386: Destroy path now NULL-guards first; looks goodEarly NULL check avoids UB; RA and SD-key resources are freed correctly.
222-310: Preserve legacy plain syslog_*_key behavior (avoid forced RA-only configs)Creating RAs directly from user strings breaks existing configs that specify simple keys (e.g., syslog_message_key message). flb_record_accessor patterns typically require a $… selector; plain keys will not resolve via flb_ra_get_value_object, causing fields to be empty. Add compatibility by wrapping plain keys as RA at config time.
Apply (helper + use for all *_key):
+#include <fluent-bit/flb_sds.h> + +/* Wrap plain keys into RA syntax: "key" -> "$['key']" */ +static struct flb_record_accessor *create_ra_from_property(char *key) +{ + struct flb_record_accessor *ra; + flb_sds_t pattern = NULL; + + if (key == NULL) { + return NULL; + } + /* Heuristic: if it already looks like an RA, don't wrap */ + if (strchr(key, '$') != NULL) { + return flb_ra_create(key, FLB_FALSE); + } + + pattern = flb_sds_printf(&pattern, "$['%s']", key); + if (pattern == NULL) { + return NULL; + } + ra = flb_ra_create(pattern, FLB_FALSE); + flb_sds_destroy(pattern); + return ra; +} @@ - if (ctx->severity_key) { - ctx->ra_severity_key = flb_ra_create(ctx->severity_key, FLB_FALSE); + if (ctx->severity_key) { + ctx->ra_severity_key = create_ra_from_property(ctx->severity_key); if (ctx->ra_severity_key == NULL) { @@ - if (ctx->facility_key) { - ctx->ra_facility_key = flb_ra_create(ctx->facility_key, FLB_FALSE); + if (ctx->facility_key) { + ctx->ra_facility_key = create_ra_from_property(ctx->facility_key); @@ - if (ctx->hostname_key) { - ctx->ra_hostname_key = flb_ra_create(ctx->hostname_key, FLB_FALSE); + if (ctx->hostname_key) { + ctx->ra_hostname_key = create_ra_from_property(ctx->hostname_key); @@ - if (ctx->appname_key) { - ctx->ra_appname_key = flb_ra_create(ctx->appname_key, FLB_FALSE); + if (ctx->appname_key) { + ctx->ra_appname_key = create_ra_from_property(ctx->appname_key); @@ - if (ctx->procid_key) { - ctx->ra_procid_key = flb_ra_create(ctx->procid_key, FLB_FALSE); + if (ctx->procid_key) { + ctx->ra_procid_key = create_ra_from_property(ctx->procid_key); @@ - if (ctx->msgid_key) { - ctx->ra_msgid_key = flb_ra_create(ctx->msgid_key, FLB_FALSE); + if (ctx->msgid_key) { + ctx->ra_msgid_key = create_ra_from_property(ctx->msgid_key); @@ - if (ctx->message_key) { - ctx->ra_message_key = flb_ra_create(ctx->message_key, FLB_FALSE); + if (ctx->message_key) { + ctx->ra_message_key = create_ra_from_property(ctx->message_key); @@ - sk_key_ra->ra_sd_key = flb_ra_create(mv->val.str, FLB_FALSE); + sk_key_ra->ra_sd_key = create_ra_from_property(mv->val.str);This preserves existing configs while enabling nested RA usage.
🧹 Nitpick comments (6)
plugins/out_syslog/syslog_conf.h (2)
36-41: Ownership note for flb_syslog_sd_key fieldsConfirm lifecycle: key_normalized is freed with flb_sds_destroy and ra_sd_key via flb_ra_destroy in destroy(); looks consistent. Consider a brief comment noting ownership to avoid future leaks.
50-85: Struct layout looks consistent; align paired string/RA membersPaired foo_key + ra_foo_key fields are clear. Optional: group all RA fields together or add short comments to keep maintenance easy.
Also applies to: 77-81
plugins/out_syslog/syslog.c (2)
727-739: mk_list_foreach_safe not needed when not mutating the listUse mk_list_foreach for a tiny speed/readability win since entries aren’t removed here.
901-902: Fix log message typos
- "error formating message" -> "error formatting message"
- "syslog_fromat returns NULL" -> "syslog_format returns NULL"
Apply:
- flb_plg_error(ctx->ins, "error formating message"); + flb_plg_error(ctx->ins, "error formatting message"); @@ - flb_error("syslog_fromat returns NULL"); + flb_error("syslog_format returns NULL");Also applies to: 1055-1056
plugins/out_syslog/syslog_conf.c (2)
68-106: Propagate flb_sds_cat_safe errors in normalize helperssyslog_normalize_cat ignores flb_sds_cat_safe return codes; on OOM it silently continues with a partial name. Consider returning int from syslog_normalize_cat and bubbling up failures to the caller.
Example:
-static inline void syslog_normalize_cat(struct flb_ra_parser *rp, flb_sds_t *name) +static inline int syslog_normalize_cat(struct flb_ra_parser *rp, flb_sds_t *name) { @@ - flb_sds_cat_safe(name, key->name, flb_sds_len(key->name)); + if (flb_sds_cat_safe(name, key->name, flb_sds_len(key->name)) != 0) { return -1; } @@ - flb_sds_cat_safe(name, ".", 1); + if (flb_sds_cat_safe(name, ".", 1) != 0) { return -1; } @@ - flb_sds_cat_safe(name, entry->str, flb_sds_len(entry->str)); + if (flb_sds_cat_safe(name, entry->str, flb_sds_len(entry->str)) != 0) { return -1; } @@ - len = snprintf(tmp, sizeof(tmp) -1, "%d", entry->array_id); - flb_sds_cat_safe(name, tmp, len); + len = snprintf(tmp, sizeof(tmp) - 1, "%d", entry->array_id); + if (len >= (int)sizeof(tmp)) { len = sizeof(tmp) - 1; } + if (flb_sds_cat_safe(name, tmp, len) != 0) { return -1; } @@ -} + return 0; +}Then check and propagate in syslog_normalize_ra_key_name.
108-130: Handle cat errors in syslog_normalize_ra_key_nameIf syslog_normalize_cat returns -1, free name and return NULL to keep error path consistent with the caller’s cleanup.
Apply:
- if (c > 0) { - flb_sds_cat_safe(&name, ".", 1); - } - syslog_normalize_cat(rp, &name); + if (c > 0 && flb_sds_cat_safe(&name, ".", 1) != 0) { + flb_sds_destroy(name); + return NULL; + } + if (syslog_normalize_cat(rp, &name) != 0) { + flb_sds_destroy(name); + return NULL; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/out_syslog/syslog.c(4 hunks)plugins/out_syslog/syslog_conf.c(7 hunks)plugins/out_syslog/syslog_conf.h(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
plugins/out_syslog/syslog_conf.c (3)
src/flb_sds.c (3)
flb_sds_cat_safe(204-214)flb_sds_create_size(92-95)flb_sds_destroy(389-399)src/flb_record_accessor.c (2)
flb_ra_create(271-358)flb_ra_destroy(232-248)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
plugins/out_syslog/syslog.c (3)
src/flb_record_accessor.c (1)
flb_ra_get_value_object(803-814)src/flb_ra_key.c (1)
flb_ra_key_value_destroy(842-851)src/flb_sds.c (1)
flb_sds_create_len(58-76)
🔇 Additional comments (2)
plugins/out_syslog/syslog_conf.c (1)
22-27: Includes look right for new RA usageAdding flb_record_accessor.h, flb_ra_parser.h, flb_utils.h, mk_core.h ensures types are available.
plugins/out_syslog/syslog.c (1)
601-627: Backward compatibility concern requires manual verification and testingThe code analysis shows that plain keys (e.g.,
syslog_message_key message) are passed directly toflb_ra_create()without being wrapped into record accessor syntax. Tests intests/runtime/out_syslog.cuse plain keys extensively (e.g., "msg", "s_key", "h_key") and would pass if plain key lookup works correctly.However, the actual behavior cannot be definitively determined through static analysis. The two proposed options are valid approaches to ensure backward compatibility:
- Option A (config-time wrapping): Modify
syslog_conf.cto detect plain keys lacking$and wrap them as$['keyname']before callingflb_ra_create().- Option B (runtime fallback): Add fallback logic in the lookup code to try direct map access if RA lookup fails and the key lacks
$.Running the existing tests or checking recent commits would clarify whether a regression actually exists.
Signed-off-by: Simon Bouchard <[email protected]>
1 participant
Added nested keys support in out_syslog plugin by allowing the use of Record Accessors for all syslog properties.
Addresses issue #2168 and PR #2516
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:
fluent-bit-files.zip
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