out_cloudwatch_logs: reuse entity record accessors across records - #12256
Open
ParakhJaggi wants to merge 1 commit into
Open
out_cloudwatch_logs: reuse entity record accessors across records#12256ParakhJaggi wants to merge 1 commit into
ParakhJaggi wants to merge 1 commit into
Conversation
parse_entity() built and destroyed ten record accessors for every single record when add_entity is enabled. The paths are compile time constants, so this repeated the same parse work per record and produced a steady churn of allocations and frees proportional to the ingested log volume. Compile the accessors once during plugin init and store them on the context, following the same pattern already used for ra_group and ra_stream, then release them in flb_cloudwatch_ctx_destroy(). The accessors are only built when add_entity is enabled, so deployments that do not use entity support are unaffected. The paths and the field map are indexed by the same ENTITY_RA_* slots so the two tables cannot drift out of order. Signed-off-by: ParakhJaggi <parakhjaggi@gmail.com>
📝 WalkthroughWalkthroughCloudWatch entity record accessors are stored in an indexed context array. The output initializes them once when ChangesCloudWatch entity accessor lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/out_cloudwatch_logs/cloudwatch_api.c`:
- Around line 1268-1329: Add unit tests in the existing CloudWatch test suite
covering entity_ra_init() success, partial flb_ra_create() failure, and cleanup
through entity_ra_destroy() that releases only initialized accessors. Exercise
parse_entity() with missing fields and non-string values, verifying fields and
counters remain correct without crashes or invalid updates. Reuse the existing
CloudWatch test helpers and accessor-path symbols rather than adding unrelated
production changes.
🪄 Autofix
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 Plus
Run ID: 65897885-8745-4be0-a1d0-362aa7935cc6
📒 Files selected for processing (4)
plugins/out_cloudwatch_logs/cloudwatch_api.cplugins/out_cloudwatch_logs/cloudwatch_api.hplugins/out_cloudwatch_logs/cloudwatch_logs.cplugins/out_cloudwatch_logs/cloudwatch_logs.h
Comment on lines
+1268
to
1329
| int entity_ra_init(struct flb_cloudwatch *ctx) | ||
| { | ||
| int i; | ||
|
|
||
| for (i = 0; i < ENTITY_RA_MAX; i++) { | ||
| ctx->entity_ra[i] = flb_ra_create((char *) entity_ra_paths[i], | ||
| FLB_FALSE); | ||
| if (ctx->entity_ra[i] == NULL) { | ||
| flb_plg_error(ctx->ins, "Could not parse entity record accessor %s", | ||
| entity_ra_paths[i]); | ||
| entity_ra_destroy(ctx); | ||
| return -1; | ||
| } | ||
| } | ||
|
|
||
| return 0; | ||
| } | ||
|
|
||
| void parse_entity(struct flb_cloudwatch *ctx, entity *entity, | ||
| msgpack_object map, int map_size) | ||
| { | ||
| struct flb_record_accessor *ra; | ||
| struct flb_ra_value *val; | ||
| int i; | ||
|
|
||
| struct { | ||
| const char *path; | ||
| char **field; | ||
| int *filter_count; | ||
| int *found_flag; | ||
| } field_map[] = { | ||
| {"$kubernetes['aws_entity_service_name']", &entity->key_attributes->name, | ||
| } field_map[ENTITY_RA_MAX] = { | ||
| [ENTITY_RA_SERVICE_NAME] = {&entity->key_attributes->name, | ||
| &entity->filter_count, &entity->service_name_found}, | ||
| {"$kubernetes['aws_entity_environment']", &entity->key_attributes->environment, | ||
| [ENTITY_RA_ENVIRONMENT] = {&entity->key_attributes->environment, | ||
| &entity->filter_count, &entity->environment_found}, | ||
| {"$kubernetes['namespace_name']", &entity->attributes->namespace, | ||
| [ENTITY_RA_NAMESPACE] = {&entity->attributes->namespace, | ||
| NULL, NULL}, | ||
| {"$kubernetes['host']", &entity->attributes->node, NULL, NULL}, | ||
| {"$kubernetes['aws_entity_cluster']", &entity->attributes->cluster_name, | ||
| [ENTITY_RA_NODE] = {&entity->attributes->node, NULL, NULL}, | ||
| [ENTITY_RA_CLUSTER] = {&entity->attributes->cluster_name, | ||
| &entity->filter_count, NULL}, | ||
| {"$kubernetes['aws_entity_workload']", &entity->attributes->workload, | ||
| [ENTITY_RA_WORKLOAD] = {&entity->attributes->workload, | ||
| &entity->filter_count, NULL}, | ||
| {"$kubernetes['aws_entity_name_source']", &entity->attributes->name_source, | ||
| [ENTITY_RA_NAME_SOURCE] = {&entity->attributes->name_source, | ||
| &entity->filter_count, &entity->name_source_found}, | ||
| {"$kubernetes['aws_entity_platform']", &entity->attributes->platform_type, | ||
| [ENTITY_RA_PLATFORM] = {&entity->attributes->platform_type, | ||
| &entity->filter_count, NULL}, | ||
| {"$aws_entity_ec2_instance_id", &entity->attributes->instance_id, | ||
| [ENTITY_RA_INSTANCE_ID] = {&entity->attributes->instance_id, | ||
| &entity->root_filter_count, NULL}, | ||
| {"$aws_entity_account_id", &entity->key_attributes->account_id, | ||
| &entity->root_filter_count, NULL}, | ||
| {NULL, NULL, NULL, NULL} | ||
| [ENTITY_RA_ACCOUNT_ID] = {&entity->key_attributes->account_id, | ||
| &entity->root_filter_count, NULL} | ||
| }; | ||
|
|
||
| for (i = 0; field_map[i].path; i++) { | ||
| ra = flb_ra_create((char *) field_map[i].path, FLB_FALSE); | ||
| if (!ra) { | ||
|
|
||
| for (i = 0; i < ENTITY_RA_MAX; i++) { | ||
| if (ctx->entity_ra[i] == NULL) { | ||
| continue; | ||
| } | ||
| val = flb_ra_get_value_object(ra, map); | ||
|
|
||
| val = flb_ra_get_value_object(ctx->entity_ra[i], map); | ||
| if (val) { | ||
| set_entity_field(field_map[i].field, val, field_map[i].filter_count, | ||
| field_map[i].found_flag); | ||
| flb_ra_key_value_destroy(val); | ||
| } | ||
|
|
||
| flb_ra_destroy(ra); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 --glob '*.{c,h}' \
'\b(entity_ra_init|entity_ra_destroy|parse_entity|flb_ra_create)\s*\(' \
plugins/out_cloudwatch_logs tests
rg -n -C 4 --glob '*.{c,h}' \
'add_entity|aws_entity_service_name|aws_entity_environment|FLB_RA_STRING' \
plugins/out_cloudwatch_logs testsRepository: fluent/fluent-bit
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching cloudwatch tests:\n'
fd -i 'cloudwatch' tests | sed -n '1,80p'
printf '\nReferences to entity RA/entity fields/services in out_cloudwatch_logs:\n'
rg -n --glob '*.{c,h}' \
'entity_ra_init|entity_ra_destroy|parse_entity|set_entity_field|add_entity|aws_entity_service_name|aws_entity_environment|aws_entity_cluster|aws_entity_workload|aws_entity_ec2_instance_id|aws_entity_account_id|ENTITY_RA_|filter_count|found_flag' \
plugins/out_cloudwatch_logs tests | sed -n '1,220p'
printf '\nRelevant init/cleanup code:\n'
sed -n '140,172p;508,526p' plugins/out_cloudwatch_logs/cloudwatch_logs.c
sed -n '1200,1235p;1238,1345p' plugins/out_cloudwatch_logs/cloudwatch_api.cRepository: fluent/fluent-bit
Length of output: 15857
Add unit coverage for entity accessor lifecycle.
tests/runtime/out_cloudwatch.c does not cover the new entity_ra_init()/entity_ra_destroy() path or parse_entity() field handling. Add tests for successful initialization, partial accessor initialization failure, missing fields, and non-string entity fields, including cleanup that releases only initialized accessors.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/out_cloudwatch_logs/cloudwatch_api.c` around lines 1268 - 1329, Add
unit tests in the existing CloudWatch test suite covering entity_ra_init()
success, partial flb_ra_create() failure, and cleanup through
entity_ra_destroy() that releases only initialized accessors. Exercise
parse_entity() with missing fields and non-string values, verifying fields and
counters remain correct without crashes or invalid updates. Reuse the existing
CloudWatch test helpers and accessor-path symbols rather than adding unrelated
production changes.
Source: Coding guidelines
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.
parse_entity()built and destroyed ten record accessors for every single record wheneveradd_entityis enabled:The ten paths are compile time constants, so this re-parses the same accessor expressions on every record and produces a churn of allocations and frees proportional to ingested log volume.
This change compiles the accessors once during plugin init and stores them on the context, following the pattern already used for
ra_groupandra_stream, then releases them inflb_cloudwatch_ctx_destroy().Notes:
add_entityis enabled, so deployments that do not use entity support allocate nothing extra.ENTITY_RA_*slots using designated initializers, so the two tables cannot silently drift out of order.flb_ra_create()was skipped silently per record; now a failure is reported once and fails plugin init, which is consistent with how the existinglog_group_templateandlog_stream_templateaccessors are handled.We hit this on an EKS cluster running the CloudWatch Container Insights daemonset with
add_entity true, where fluent-bit is on the per record path for every container log line on the node.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