out_cloudwatch: add entity support and remove unnecessary log content #10585
Conversation
Signed-off-by: Zhihong Lin <zhiholin@amazon.com>
There was a problem hiding this comment.
-
Code style needs to be updated, while I did not comment all the code its expected the whole patch is aligned to https://github.com/fluent/fluent-bit/blob/master/CONTRIBUTING.md#coding-style
-
Use record accesso API for key/value pairs lookup
Signed-off-by: Zhihong Lin <zhiholin@amazon.com>
WalkthroughThe changes introduce an "entity" metadata feature to the CloudWatch Logs plugin. This includes new data structures for entity attributes, logic to extract and filter entity-related fields from log records, enrich outgoing CloudWatch payloads with entity information, and configuration options to control this behavior. Supporting macros and memory management routines are also added. Changes
Sequence Diagram(s)sequenceDiagram
participant LogRecord
participant CloudWatchPlugin
participant Entity
participant AWSCloudWatch
LogRecord->>CloudWatchPlugin: Submit log event
CloudWatchPlugin->>Entity: Parse entity fields from log record
CloudWatchPlugin->>Entity: Remove entity fields from log body
CloudWatchPlugin->>CloudWatchPlugin: Enrich payload with entity JSON (if enabled)
CloudWatchPlugin->>AWSCloudWatch: Send PutLogEvents(payload)
AWSCloudWatch-->>CloudWatchPlugin: Respond with status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
🔇 Additional comments (16)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
zhihonl
force-pushed
the
cloudwatch-entity
branch
from
35d423d to
9bb75a9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
plugins/out_cloudwatch_logs/cloudwatch_api.c (1)
637-642: Add comment to clarify intentionally empty blockThe empty else if block is intentional but could be clearer with a comment explaining that root-level aws_entity fields are filtered by not packing them.
else if (root_filtered_fields > 0 && root_kv.key.type == MSGPACK_OBJECT_STR && root_kv.key.via.str.size > AWS_ENTITY_PREFIX_LEN && strncmp(root_kv.key.via.str.ptr, AWS_ENTITY_PREFIX, AWS_ENTITY_PREFIX_LEN) == 0) { + /* Skip root-level aws_entity fields - they are filtered by not packing them */ }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/out_cloudwatch_logs/cloudwatch_api.c(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (25)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: PR - fuzzing test
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: pr-compile-centos-7
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
🔇 Additional comments (2)
plugins/out_cloudwatch_logs/cloudwatch_api.c (2)
2029-2030: Consider security implications of debug loggingThe debug logs print full HTTP response data and payload which might contain sensitive information or PII. Ensure these logs don't leak sensitive data in production environments.
Consider adding a configuration option to control verbose debug logging or redact sensitive fields from the debug output.
1198-1204: Add null checks before strlen to prevent segfaultsThe code calls strlen on potentially null pointers without checking. If any of these fields are NULL, it will cause a segmentation fault.
if (entity->key_attributes->name == NULL && entity->attributes->name_source == NULL && entity->attributes->workload != NULL) { - entity->key_attributes->name = flb_strndup(entity->attributes->workload, - strlen(entity->attributes->workload)); + size_t workload_len = strlen(entity->attributes->workload); + entity->key_attributes->name = flb_strndup(entity->attributes->workload, + workload_len); entity->attributes->name_source = flb_strndup("K8sWorkload", 11); }However, the NULL check for workload on line 1200 already ensures it's not NULL before the strlen call, so this specific case is safe. The pattern should still be reviewed throughout the codebase for consistency.
Likely an incorrect or invalid review comment.
Signed-off-by: Zhihong Lin <zhiholin@amazon.com>
zhihonl
force-pushed
the
cloudwatch-entity
branch
from
9bb75a9 to
05eaaca
Compare
Signed-off-by: Zhihong Lin <zhiholin@amazon.com>
Signed-off-by: Zhihong Lin <zhiholin@amazon.com>
Signed-off-by: Zhihong Lin <zhiholin@amazon.com>
|
@edsiper PR is good for review again 👀 |
| FLB_CONFIG_MAP_BOOL, "add_entity", "false", | ||
| 0, FLB_TRUE, offsetof(struct flb_cloudwatch, add_entity), | ||
| "add entity to PutLogEvent calls" |
There was a problem hiding this comment.
We will want to add documentation for the new parameter to the new output plugins - https://github.com/fluent/fluent-bit-docs/blob/master/pipeline/outputs/cloudwatch.md#configuration-parameters
|
thank you! |
Background
CloudWatch introduced a new feature called Explored Related which is a UI component that helps user navigate between their telemetry(metrics,logs) and AWS resources. This is done by including a new field in PutLogEvents API which is called Entity. More documentation about this new field can be found here: https://docs.aws.amazon.com/AmazonCloudWatchLogs/latest/APIReference/API_Entity.html
Problem
CloudWatch output plugin is what sends user logs to the backend. It needs to populate the entity field in the PutLogEvents field. As part of this effort, we need to add additional logics in CloudWatch plugin to attach appropriate entity data then make the correct PutLogEvents call.
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:
Screenshot showing the entities are properly attached
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
Summary by CodeRabbit
New Features
Bug Fixes
Chores