out_logdna: remove promoted known keys#11755
Conversation
|
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:
📝 WalkthroughWalkthroughThe LogDNA output plugin adds an Changes
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 docstrings
🧪 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: 69dd8871b6
ℹ️ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/out_logdna/logdna.c (1)
251-271:⚠️ Potential issue | 🔴 CriticalGuard unpack/JSON conversion failures before using
line_json.At line 254,
msgpack_unpack_next()result is not checked; at line 268,strlen(line_json)dereferences without verifying the JSON conversion succeeded. Both can fail (returning error codes or NULL), causing crashes on malformed input or memory allocation failure.Proposed fix
if (ctx->exclude_promoted_keys) { msgpack_unpacked_init(&mp_line_result); off = 0; - msgpack_unpack_next(&mp_line_result, - mp_line_sbuf.data, mp_line_sbuf.size, &off); + ret = msgpack_unpack_next(&mp_line_result, + mp_line_sbuf.data, mp_line_sbuf.size, &off); + if (ret != MSGPACK_UNPACK_SUCCESS) { + msgpack_unpacked_destroy(&mp_line_result); + msgpack_sbuffer_destroy(&mp_line_sbuf); + flb_plg_error(ctx->ins, "could not unpack filtered line payload"); + flb_log_event_decoder_destroy(&log_decoder); + msgpack_sbuffer_destroy(&mp_sbuf); + return NULL; + } line_json = flb_msgpack_to_json_str(1024, &mp_line_result.data, config->json_escape_unicode); msgpack_unpacked_destroy(&mp_line_result); msgpack_sbuffer_destroy(&mp_line_sbuf); } else { line_json = flb_msgpack_to_json_str(1024, log_event.body, config->json_escape_unicode); } + if (!line_json) { + flb_plg_error(ctx->ins, "could not serialize line payload"); + flb_log_event_decoder_destroy(&log_decoder); + msgpack_sbuffer_destroy(&mp_sbuf); + return NULL; + } + len = strlen(line_json);
🧹 Nitpick comments (1)
tests/runtime/out_logdna.c (1)
98-606: Consider extracting a shared test harness helper.Setup/start/push/wait/assert/stop/destroy is repeated across many tests; a small helper would reduce drift and simplify future additions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/runtime/out_logdna.c` around lines 98 - 606, Many tests repeat the identical lifecycle (clear_output_num; flb_create + flb_service_set; flb_input + flb_input_set; flb_output + flb_output_set; flb_output_set_test; flb_start; flb_lib_push; sleep + assert get_output_num; flb_stop; flb_destroy); extract a small helper (e.g., run_logdna_test or execute_test_flow) that accepts the JSON payload, the formatter callback (cb_check_*), output config args (like "exclude_promoted_keys"), and any expected-wait duration, and moves the common sequence into that helper using the existing symbols flb_create, flb_service_set, flb_input, flb_input_set, flb_output, flb_output_set, flb_output_set_test, flb_start, flb_lib_push, clear_output_num/get_output_num/set_output_num, flb_stop and flb_destroy; replace each flb_test_* function body with a single call to the helper passing the proper JSON and callback to reduce duplication and keep per-test differences minimal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/out_logdna/logdna.c`:
- Around line 658-662: Update the config help text for the FLB_CONFIG_MAP_BOOL
entry named "exclude_promoted_keys" (in struct flb_logdna) to explicitly mention
that it also excludes the promoted "severity" field (promoted as "level"); keep
the rest of the description but add ", severity (promoted as level)" or
equivalent phrasing so the behavior is clear to users.
---
Nitpick comments:
In `@tests/runtime/out_logdna.c`:
- Around line 98-606: Many tests repeat the identical lifecycle
(clear_output_num; flb_create + flb_service_set; flb_input + flb_input_set;
flb_output + flb_output_set; flb_output_set_test; flb_start; flb_lib_push; sleep
+ assert get_output_num; flb_stop; flb_destroy); extract a small helper (e.g.,
run_logdna_test or execute_test_flow) that accepts the JSON payload, the
formatter callback (cb_check_*), output config args (like
"exclude_promoted_keys"), and any expected-wait duration, and moves the common
sequence into that helper using the existing symbols flb_create,
flb_service_set, flb_input, flb_input_set, flb_output, flb_output_set,
flb_output_set_test, flb_start, flb_lib_push,
clear_output_num/get_output_num/set_output_num, flb_stop and flb_destroy;
replace each flb_test_* function body with a single call to the helper passing
the proper JSON and callback to reduce duplication and keep per-test differences
minimal.
🪄 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: 4f3df5f1-449e-4b6b-bfba-8d3613d74181
📒 Files selected for processing (4)
plugins/out_logdna/logdna.cplugins/out_logdna/logdna.htests/runtime/CMakeLists.txttests/runtime/out_logdna.c
lecaros
force-pushed
the
lecaros-logdna-remove-known-keys
branch
from
69dd887 to
ec331c9
Compare
lecaros
force-pushed
the
lecaros-logdna-remove-known-keys
branch
from
ec331c9 to
ae0c138
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/runtime/out_logdna.c (2)
133-137: Replace fixedsleep(...)with a bounded poll helper to reduce flakiness and runtime.Using fixed sleeps can be brittle on slow/fast CI and adds avoidable delay. A short polling loop with timeout (e.g., check
get_output_num()every 100ms) makes these tests more deterministic.Also applies to: 234-238, 305-309, 369-373, 451-455, 518-522, 598-602, 642-644
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/runtime/out_logdna.c` around lines 133 - 137, Replace the fixed sleep(2) with a bounded polling helper that repeatedly checks get_output_num() (e.g., every 100ms) until a short timeout (e.g., 2–5s) and then asserts via TEST_CHECK/TEST_MSG if no output; add a helper function (e.g., poll_until_output_or_timeout) used in place of sleep calls so the test uses get_output_num() as the success condition and returns early on success to reduce flakiness; make sure to update all similar occurrences that currently sleep and then call TEST_CHECK/TEST_MSG to use this helper.
98-141: Consider extracting a shared test harness for setup/start/push/assert/teardown.The repeated boilerplate makes maintenance harder and increases the chance of drift between scenarios. A helper that accepts input JSON, output options, and formatter callback would keep these cases concise and consistent.
Also applies to: 199-242, 270-313, 335-377, 417-459, 484-526, 564-606
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/runtime/out_logdna.c` around lines 98 - 141, Extract the repeated setup/start/push/assert/teardown sequence in tests like flb_test_non_duplication into a shared helper (e.g., run_logdna_test) that encapsulates flb_create + flb_service_set, flb_input/flb_input_set, flb_output/flb_output_set, flb_output_set_test, flb_start, flb_lib_push, sleep/assert via get_output_num, and flb_stop/flb_destroy; change each test to call this helper with parameters for input JSON, output options (api_key, exclude_promoted_keys, match, etc.), and the formatter callback (cb_check_non_duplication) so all boilerplate is centralized and tests only pass the differing inputs and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/runtime/out_logdna.c`:
- Around line 133-137: Replace the fixed sleep(2) with a bounded polling helper
that repeatedly checks get_output_num() (e.g., every 100ms) until a short
timeout (e.g., 2–5s) and then asserts via TEST_CHECK/TEST_MSG if no output; add
a helper function (e.g., poll_until_output_or_timeout) used in place of sleep
calls so the test uses get_output_num() as the success condition and returns
early on success to reduce flakiness; make sure to update all similar
occurrences that currently sleep and then call TEST_CHECK/TEST_MSG to use this
helper.
- Around line 98-141: Extract the repeated setup/start/push/assert/teardown
sequence in tests like flb_test_non_duplication into a shared helper (e.g.,
run_logdna_test) that encapsulates flb_create + flb_service_set,
flb_input/flb_input_set, flb_output/flb_output_set, flb_output_set_test,
flb_start, flb_lib_push, sleep/assert via get_output_num, and
flb_stop/flb_destroy; change each test to call this helper with parameters for
input JSON, output options (api_key, exclude_promoted_keys, match, etc.), and
the formatter callback (cb_check_non_duplication) so all boilerplate is
centralized and tests only pass the differing inputs and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd868599-11eb-459c-8190-6347d36c83de
📒 Files selected for processing (4)
plugins/out_logdna/logdna.cplugins/out_logdna/logdna.htests/runtime/CMakeLists.txttests/runtime/out_logdna.c
🚧 Files skipped from review as they are similar to previous changes (3)
- plugins/out_logdna/logdna.h
- tests/runtime/CMakeLists.txt
- plugins/out_logdna/logdna.c
lecaros
force-pushed
the
lecaros-logdna-remove-known-keys
branch
from
ae0c138 to
eefbd5d
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/runtime/out_logdna.c (1)
68-76:⚠️ Potential issue | 🟠 MajorTop-level key assertions can false-pass due escaped
linecontent.
strstr(json, "\"meta\":")/\"level\"/\"app\"/\"file\"can match inside escaped"line"JSON, so promotion regressions may be missed (e.g., no real top-level key, but escaped key still present inline).Suggested hardening
+static int contains_unescaped_token(const char *json, const char *token) +{ + const char *p; + size_t len; + + p = json; + len = strlen(token); + + while ((p = strstr(p, token)) != NULL) { + if (p == json || *(p - 1) != '\\') { + return FLB_TRUE; + } + p += len; + } + + return FLB_FALSE; +} ... - if (!TEST_CHECK(strstr(json, "\"meta\":") != NULL)) { + if (!TEST_CHECK(contains_unescaped_token(json, "\"meta\":"))) { TEST_MSG("missing top-level meta: %s", json); } - if (!TEST_CHECK(strstr(json, "\"level\":") != NULL)) { + if (!TEST_CHECK(contains_unescaped_token(json, "\"level\":"))) { TEST_MSG("missing top-level level: %s", json); }Also applies to: 163-174, 539-547
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/runtime/out_logdna.c` around lines 68 - 76, The assertions use strstr(json, "\"meta\":") / "\"level\"" / "\"app\"" which can match inside an escaped "line" value and false-pass; replace these raw substring checks by parsing the JSON and asserting top-level keys exist (e.g., cJSON_Parse or the project’s JSON helper to get the root object and verify cJSON_GetObjectItem(root, "meta"), "level", "app", "file" are non-NULL), then free the parsed object and fail the test if any top-level key is missing; update all occurrences noted (the shown block and the duplicate locations) and reference the json variable when replacing the checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/runtime/out_logdna.c`:
- Around line 68-76: The assertions use strstr(json, "\"meta\":") / "\"level\""
/ "\"app\"" which can match inside an escaped "line" value and false-pass;
replace these raw substring checks by parsing the JSON and asserting top-level
keys exist (e.g., cJSON_Parse or the project’s JSON helper to get the root
object and verify cJSON_GetObjectItem(root, "meta"), "level", "app", "file" are
non-NULL), then free the parsed object and fail the test if any top-level key is
missing; update all occurrences noted (the shown block and the duplicate
locations) and reference the json variable when replacing the checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61988e71-d2e4-460b-96e9-992b976a31da
📒 Files selected for processing (4)
plugins/out_logdna/logdna.cplugins/out_logdna/logdna.htests/runtime/CMakeLists.txttests/runtime/out_logdna.c
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/out_logdna/logdna.c
lecaros
force-pushed
the
lecaros-logdna-remove-known-keys
branch
from
4285472 to
0f1deee
Compare
|
Hi lecaros! |
Signed-off-by: lecaros <lecaros@chronosphere.io>
Signed-off-by: lecaros <lecaros@chronosphere.io>
lecaros
force-pushed
the
lecaros-logdna-remove-known-keys
branch
from
0f1deee to
86aa1e0
Compare
2 participants
The
out_logdnaplugin promotes certain record keys (meta,level/severity,file,app) to top-level fields in the ingest payload, but also serializes the entire record — including those same keys — as JSON into thelinefield. This causes Mezmo to display duplicate entries (e.g. bothmetaand_meta) in the log viewer.This PR adds a new boolean option
exclude_promoted_keys(default:false) that, when enabled, strips the promoted keys from thelinebody. The default preserves backward compatibility.Addresses #11754
Testing
Before we can approve your change; please submit the following in a comment:
Logs
Config:
Test output:
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
Backport to latest stable release.
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