processor_content_modifier: add new otel context for logs attrs#11500
Open
processor_content_modifier: add new otel context for logs attrs#11500
Conversation
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
📝 WalkthroughWalkthroughThis PR introduces support for OpenTelemetry log attributes in the content modifier processor. Changes include a new enum constant, configuration mapping for the "otel_log_attributes" context string, processing logic to extract and handle log attributes, a new public API function for attribute retrieval, and comprehensive test coverage for insert, upsert, delete, and invalid metadata scenarios. Changes
Sequence DiagramsequenceDiagram
participant Log as Log Event
participant Proc as cm_logs_process()
participant Helper as otel_get_log_attributes()
participant OTEL as cm_otel_get_or_create_attributes()
participant Meta as Metadata/KVList
Log->>Proc: FLB_LOG_EVENT_NORMAL with CM_CONTEXT_OTEL_LOG_ATTR
Proc->>Helper: Call otel_get_log_attributes(record)
Helper->>Meta: Extract/read OTLP metadata from record
Helper->>Meta: Create intermediate KVList if needed
Meta->>OTEL: Pass KVList to cm_otel_get_or_create_attributes()
OTEL->>Meta: Retrieve or initialize attributes structure
Meta->>Helper: Return cfl_variant with attributes
Helper->>Proc: Return attributes variant
Proc->>Proc: Wrap in KVLIST variant for processing
Proc->>Proc: Set as current object for modification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
…ntext Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
edsiper
force-pushed
the
processor_content_modifier-otel-logs-attr
branch
from
99803c5 to
ed52699
Compare
![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: 99803c57a5
ℹ️ 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".
Comment on lines
+324
to
+326
| if (ret != 0) { | ||
| cfl_kvlist_destroy(kvlist_tmp); | ||
| return NULL; |
There was a problem hiding this comment.
When cfl_kvlist_insert_kvlist_s fails here, it already destroys the wrapped variant (and therefore the kvlist_tmp payload) before returning non-zero (lib/cfl/src/cfl_kvlist.c calls cfl_variant_destroy in its error path). Calling cfl_kvlist_destroy(kvlist_tmp) again in this branch introduces a double free that can crash the process under allocation/insert failure conditions (e.g., memory pressure).
Useful? React with 👍 / 👎.
![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 (1)
plugins/processor_content_modifier/cm_config.c (1)
196-203: Remove unreachable duplicate logs-context branches in this block.The new
"otel_log_attributes"mapping is correct, but the subsequent duplicate"otel_scope_name"(empty body) and second"otel_scope_version"branches are unreachable and make future edits error-prone.♻️ Proposed cleanup
else if (strcasecmp(ctx->context_str, "otel_log_attributes") == 0) { context = CM_CONTEXT_OTEL_LOG_ATTR; } - else if (strcasecmp(ctx->context_str, "otel_scope_name") == 0) { - } - else if (strcasecmp(ctx->context_str, "otel_scope_version") == 0) { - context = CM_CONTEXT_OTEL_SCOPE_VERSION; - } else { flb_plg_error(ctx->ins, "unknown logs context '%s'", ctx->context_str); return -1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/processor_content_modifier/cm_config.c` around lines 196 - 203, The context-parsing block contains redundant/unreachable branches: remove the empty else-if for strcasecmp(ctx->context_str, "otel_scope_name") == 0 and delete the duplicate else-if for "otel_scope_version", leaving the proper mapping for "otel_log_attributes" (setting context = CM_CONTEXT_OTEL_LOG_ATTR) and a single, correct mapping for "otel_scope_version" (CM_CONTEXT_OTEL_SCOPE_VERSION); also, if a mapping for "otel_scope_name" is required, replace the empty branch with setting the appropriate constant (e.g., CM_CONTEXT_OTEL_SCOPE_NAME) instead of leaving it empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/runtime/processor_content_modifier.c`:
- Around line 89-151: cb_check_metadata_result leaks the incoming record buffer;
ensure you free it on all paths. Add a flb_free(record) (or the appropriate free
used in the test suite) before every early return and before the final return 0
so the buffer is released; locate the freeing calls around
flb_log_event_decoder_destroy(&decoder) and flb_sds_destroy(result) in
cb_check_metadata_result to insert the flb_free(record) calls so both success
and error paths free the record.
---
Nitpick comments:
In `@plugins/processor_content_modifier/cm_config.c`:
- Around line 196-203: The context-parsing block contains redundant/unreachable
branches: remove the empty else-if for strcasecmp(ctx->context_str,
"otel_scope_name") == 0 and delete the duplicate else-if for
"otel_scope_version", leaving the proper mapping for "otel_log_attributes"
(setting context = CM_CONTEXT_OTEL_LOG_ATTR) and a single, correct mapping for
"otel_scope_version" (CM_CONTEXT_OTEL_SCOPE_VERSION); also, if a mapping for
"otel_scope_name" is required, replace the empty branch with setting the
appropriate constant (e.g., CM_CONTEXT_OTEL_SCOPE_NAME) instead of leaving it
empty.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
plugins/processor_content_modifier/cm.hplugins/processor_content_modifier/cm_config.cplugins/processor_content_modifier/cm_logs.cplugins/processor_content_modifier/cm_opentelemetry.htests/runtime/processor_content_modifier.c
Comment on lines
+89
to
+151
| static int cb_check_metadata_result(void *record, size_t size, void *data) | ||
| { | ||
| int ret; | ||
| char *p; | ||
| flb_sds_t result; | ||
| size_t result_size = 1024; | ||
| struct expect_str *expected; | ||
| struct flb_log_event event; | ||
| struct flb_log_event_decoder decoder; | ||
|
|
||
| expected = (struct expect_str *) data; | ||
| result = NULL; | ||
|
|
||
| ret = flb_log_event_decoder_init(&decoder, (char *) record, size); | ||
| if (!TEST_CHECK(ret == FLB_EVENT_DECODER_SUCCESS)) { | ||
| return -1; | ||
| } | ||
|
|
||
| flb_log_event_decoder_read_groups(&decoder, FLB_TRUE); | ||
|
|
||
| ret = flb_log_event_decoder_next(&decoder, &event); | ||
| if (!TEST_CHECK(ret == FLB_EVENT_DECODER_SUCCESS)) { | ||
| flb_log_event_decoder_destroy(&decoder); | ||
| return -1; | ||
| } | ||
|
|
||
| result = flb_sds_create_size(result_size); | ||
| if (!TEST_CHECK(result != NULL)) { | ||
| flb_log_event_decoder_destroy(&decoder); | ||
| return -1; | ||
| } | ||
|
|
||
| ret = flb_msgpack_to_json(result, result_size, event.metadata, FLB_TRUE); | ||
| if (!TEST_CHECK(ret >= 0)) { | ||
| flb_sds_destroy(result); | ||
| flb_log_event_decoder_destroy(&decoder); | ||
| return -1; | ||
| } | ||
|
|
||
| while(expected != NULL && expected->str != NULL) { | ||
| if (expected->found == FLB_TRUE) { | ||
| p = strstr(result, expected->str); | ||
| if(!TEST_CHECK(p != NULL)) { | ||
| flb_error("Expected to find: '%s' in metadata '%s'", | ||
| expected->str, result); | ||
| } | ||
| } | ||
| else { | ||
| p = strstr(result, expected->str); | ||
| if(!TEST_CHECK(p == NULL)) { | ||
| flb_error("'%s' should be removed from metadata '%s'", | ||
| expected->str, result); | ||
| } | ||
| } | ||
|
|
||
| expected++; | ||
| } | ||
|
|
||
| flb_sds_destroy(result); | ||
| flb_log_event_decoder_destroy(&decoder); | ||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
Free callback record buffer in cb_check_metadata_result.
record is not released on either success or error paths here, unlike cb_check_result, so this leaks memory during test runs.
🧹 Proposed fix
static int cb_check_metadata_result(void *record, size_t size, void *data)
{
- int ret;
+ int ret;
+ int result = -1;
@@
ret = flb_log_event_decoder_init(&decoder, (char *) record, size);
if (!TEST_CHECK(ret == FLB_EVENT_DECODER_SUCCESS)) {
- return -1;
+ goto cleanup_record;
}
@@
ret = flb_log_event_decoder_next(&decoder, &event);
if (!TEST_CHECK(ret == FLB_EVENT_DECODER_SUCCESS)) {
flb_log_event_decoder_destroy(&decoder);
- return -1;
+ goto cleanup_record;
}
@@
result = flb_sds_create_size(result_size);
if (!TEST_CHECK(result != NULL)) {
flb_log_event_decoder_destroy(&decoder);
- return -1;
+ goto cleanup_record;
}
@@
ret = flb_msgpack_to_json(result, result_size, event.metadata, FLB_TRUE);
if (!TEST_CHECK(ret >= 0)) {
flb_sds_destroy(result);
flb_log_event_decoder_destroy(&decoder);
- return -1;
+ goto cleanup_record;
}
@@
flb_sds_destroy(result);
flb_log_event_decoder_destroy(&decoder);
+ result = 0;
- return 0;
+cleanup_record:
+ flb_free(record);
+ return result;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/runtime/processor_content_modifier.c` around lines 89 - 151,
cb_check_metadata_result leaks the incoming record buffer; ensure you free it on
all paths. Add a flb_free(record) (or the appropriate free used in the test
suite) before every early return and before the final return 0 so the buffer is
released; locate the freeing calls around
flb_log_event_decoder_destroy(&decoder) and flb_sds_destroy(result) in
cb_check_metadata_result to insert the flb_free(record) calls so both success
and error paths free the record.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extend content_modifier processor with a new
otel_log_attributescontext which directly modify the log record attributes field. Config example: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