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:
📝 WalkthroughWalkthroughAdds a configurable Changes
Sequence DiagramsequenceDiagram
participant Flush as Flush callback
participant S3 as S3 plugin
participant Helper as s3_format_event_chunk
participant OTLP as OTLP converter
participant Store as S3 storage
Flush->>S3: deliver event chunk
S3->>Helper: s3_format_event_chunk(chunk, out_format, log_key)
alt out_format == otlp_json or otlp_json_pretty
Helper->>OTLP: convert msgpack (logs/metrics/traces)
OTLP-->>Helper: OTLP JSON / pretty JSON (or NULL)
Helper-->>S3: formatted_payload or NULL
S3->>Store: s3_put_object(formatted_payload)
else json_lines
Helper->>Helper: extract `log_key` or marshal JSON Lines
Helper-->>S3: formatted_payload
S3->>Store: enqueue/buffer/upload via existing path
end
Store-->>S3: success / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
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 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: 7928c53340
ℹ️ 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".
![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 the current code and only fix it if needed.
Inline comments:
In `@plugins/out_s3/s3.c`:
- Around line 710-712: The code sets ins->event_type |= FLB_OUTPUT_METRICS |
FLB_OUTPUT_TRACES when ctx->out_format == FLB_PACK_JSON_FORMAT_OTLP, which
advertises metrics/traces support that cb_s3_flush cannot safely handle (it
still computes file_first_log_time via log-style decoding). Revert/remove the OR
of FLB_OUTPUT_METRICS and FLB_OUTPUT_TRACES in that branch (leave event_type
as-is for now) or gate it behind a new capability check, and only re-enable
setting those bits after cb_s3_flush (and any helpers that compute
file_first_log_time and object naming) is updated to properly decode and handle
OTLP/metrics/traces payloads.
🪄 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: 0f8b6775-4a4e-4c76-ad5c-c1b61c4cc265
📒 Files selected for processing (2)
plugins/out_s3/s3.cplugins/out_s3/s3.h
cosmo0920
force-pushed
the
cosmo0920-handle-otlp-json-format-on-s3
branch
from
7928c53 to
bd1fcad
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_s3/s3.c`:
- Around line 3799-3825: The current path using flb_opentelemetry_*_to_otlp_json
(e.g. flb_opentelemetry_logs_to_otlp_json,
flb_opentelemetry_metrics_msgpack_to_otlp_json,
flb_opentelemetry_traces_msgpack_to_otlp_json) returns complete OTLP JSON
documents per engine chunk but the out_s3 buffering concatenates chunks raw,
producing invalid "doc1doc2..." objects; fix by either (A) merging multiple OTLP
JSON documents into a single valid JSON container before returning/ buffering
(e.g., wrap items into a top-level array or merge their inner arrays/objects) or
(B) force upload-one-object-per-chunk for OTLP JSON paths by signalling the
caller to flush immediately instead of buffering; update the branch handling
ctx->out_format == FLB_PACK_JSON_FORMAT_OTLP (and the similar code at the other
occurrence noted) to implement one of these approaches so S3 objects remain
valid OTLP JSON rather than concatenated documents.
- Around line 693-708: When parsing the "format" in cb_s3_init() (the block that
calls flb_pack_to_json_format_type and sets ctx->out_format), validate that if
the selected format is FLB_PACK_JSON_FORMAT_OTLP (otlp_json) then no log_key is
configured: detect the presence of a log_key (via
flb_output_get_property("log_key", ins) or ctx->log_key) and if both are present
call flb_plg_error on ctx->ins with a descriptive message and return -1 to fail
fast; also add the same check in the other format-parsing site that mirrors this
logic (the similar block referenced around the other occurrence) so
s3_format_event_chunk() won’t silently ignore ctx->log_key in OTLP mode.
🪄 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: ccf4a2ac-4f19-40ea-a4d9-e5e29fba7ed7
📒 Files selected for processing (2)
plugins/out_s3/s3.cplugins/out_s3/s3.h
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_s3/s3.c`:
- Around line 3814-3829: The conversion calls
(flb_opentelemetry_logs_to_otlp_json,
flb_opentelemetry_metrics_msgpack_to_otlp_json,
flb_opentelemetry_traces_msgpack_to_otlp_json) can return NULL and set the
out-parameter result on error; update the branches that call these functions to
check if the returned json is NULL, and when NULL call flb_plg_error(ctx->ins,
"could not convert event chunk to OTLP JSON: %d", result) and return -1 to match
out_stdout/out_kafka behavior—do this for the LOGS, METRICS and TRACES paths
surrounding the use of the result variable and ctx->ins.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
♻️ Duplicate comments (1)
plugins/out_s3/s3.c (1)
710-715:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject
log_keyforotlp_json_prettytoo.
ctx->log_keyis only blocked forotlp_json, but the pretty OTLP path also ignores it ins3_format_event_chunk(). That makesformat=otlp_json_prettysilently change the output shape instead of failing fast.Suggested fix
- if (ctx->out_format == FLB_PACK_JSON_FORMAT_OTLP && + if ((ctx->out_format == FLB_PACK_JSON_FORMAT_OTLP || + ctx->out_format == FLB_PACK_JSON_FORMAT_OTLP_PRETTY) && ctx->log_key != NULL) { flb_plg_error(ctx->ins, - "'log_key' is not supported when format=otlp_json"); + "'log_key' is not supported when format is " + "otlp_json or otlp_json_pretty"); return -1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/out_s3/s3.c` around lines 710 - 715, The current check in s3.c only rejects ctx->log_key when ctx->out_format == FLB_PACK_JSON_FORMAT_OTLP, but the pretty OTLP variant (FLB_PACK_JSON_FORMAT_OTLP_PRETTY) also ignores log_key in s3_format_event_chunk(), causing silent output changes; update the conditional that logs the error (the block using flb_plg_error and return -1) to also include FLB_PACK_JSON_FORMAT_OTLP_PRETTY (i.e., check for either FLB_PACK_JSON_FORMAT_OTLP || FLB_PACK_JSON_FORMAT_OTLP_PRETTY) so both OTLP JSON flavors reject log_key up front and fail fast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@plugins/out_s3/s3.c`:
- Around line 710-715: The current check in s3.c only rejects ctx->log_key when
ctx->out_format == FLB_PACK_JSON_FORMAT_OTLP, but the pretty OTLP variant
(FLB_PACK_JSON_FORMAT_OTLP_PRETTY) also ignores log_key in
s3_format_event_chunk(), causing silent output changes; update the conditional
that logs the error (the block using flb_plg_error and return -1) to also
include FLB_PACK_JSON_FORMAT_OTLP_PRETTY (i.e., check for either
FLB_PACK_JSON_FORMAT_OTLP || FLB_PACK_JSON_FORMAT_OTLP_PRETTY) so both OTLP JSON
flavors reject log_key up front and fail fast.
caac199 to
b27f8ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_s3/s3.c`:
- Around line 3983-3990: The OTLP direct PutObject branch bypasses
upload_data()/put_all_chunks(), so data never gets compressed even though
s3_put_object() sets Content-Encoding; add a helper (e.g.,
s3_put_marshaled_object) that takes the same args as s3_put_object (ctx, tag,
file_first_log_time, body, body_size), performs the existing compression
pre-processing using flb_aws_compression_compress when ctx->compression !=
FLB_AWS_COMPRESS_NONE, calls s3_put_object() with the possibly-compressed buffer
and size, frees any temporary compressed buffer, and return its result; then
replace the direct call site in the OTLP branch to call this new helper instead
of s3_put_object() so advertised Content-Encoding matches the actual payload.
- Around line 710-715: cb_s3_init currently rejects ctx->log_key only when
ctx->out_format == FLB_PACK_JSON_FORMAT_OTLP but not when the pretty OTLP
variant is used, causing s3_format_event_chunk to ignore log_key for
FLB_PACK_JSON_FORMAT_OTLP_PRETTY and silently change output; update the
validation in cb_s3_init to also check for FLB_PACK_JSON_FORMAT_OTLP_PRETTY
(i.e., treat both FLB_PACK_JSON_FORMAT_OTLP and FLB_PACK_JSON_FORMAT_OTLP_PRETTY
the same), log the same flb_plg_error message referencing "'log_key' is not
supported when format=otlp_json" and return -1 so the unsupported combination is
rejected early (also verify s3_format_event_chunk is consistent with this
behavior).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
cosmo0920
force-pushed
the
cosmo0920-handle-otlp-json-format-on-s3
branch
from
b6cb79d to
3b99023
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ca3029122
ℹ️ 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".
| if [[ "$FLB_OPT" =~ COVERAGE ]]; then | ||
| # Coverage build requires larger coroutine stack. | ||
| if [[ "$FLB_OPT" =~ COVERAGE || | ||
| "$FLB_OPT" =~ SANITIZE_ADDRESS || |
There was a problem hiding this comment.
Probably can just do a check for == FLB_SANITIZE_* rather than use a regex compare with a specific string - or use the regex compare to just check if it contains?
There was a problem hiding this comment.
This SEGV issue which seems to be run out coroutine memory just occurring only for SANITIZE_ADDRESS and SANITIZE_UNDEFINED so gating for FLB_SANITIZE_* is a bit of larger condition than we needed.
There was a problem hiding this comment.
Ah right, in which case probably use a direct comparison rather than regex as just looks bit weird to me but fine.
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
…nitizers Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
3 participants
With using otel input, we need to encode hex strings from binary encoded key-value like span_id and trace_id.
I added fromat parameter to handle binary encoded keys.
2nd attempt of resolving #11630.
Closes #11630.
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:
Sample OTEL payloads with ingested by curl:
And out_s3 encodes binary encoded records into hex encoding with concatenated records include of chunks:
{ "resourceLogs": [ { "resource": { "attributes": [ { "key": "service.name", "value": { "stringValue": "my.service" } } ] }, "scopeLogs": [ { "scope": { "name": "my.library", "version": "1.0.0", "attributes": [ { "key": "my.scope.attribute", "value": { "stringValue": "somescopeattribute" } } ] }, "logRecords": [ { "timeUnixNano": "1774877764000000000", "observedTimeUnixNano": "1774877764000000000", "severityNumber": 2, "severityText": "INFO", "traceId": "c413d5b5fea3657325ff663320c7fd10", "spanId": "77651bbd5ce0ce99", "body": { "kvlistValue": { "values": [ { "key": "specversion", "value": { "stringValue": "1.0" } }, { "key": "id", "value": { "stringValue": "3c344379-2eee-404b-b144-79ffbee05861" } } ] } } }, { "timeUnixNano": "1774877764000000000", "observedTimeUnixNano": "1774877764000000000", "severityNumber": 2, "severityText": "INFO", "traceId": "c413d5b5fea3657325ff663320c7fd10", "spanId": "77651bbd5ce0ce99", "body": { "kvlistValue": { "values": [ { "key": "specversion", "value": { "stringValue": "1.0" } }, { "key": "id", "value": { "stringValue": "3c344379-2eee-404b-b144-79ffbee05861" } } ] } } } ] } ] } ] }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
Bug Fixes / Reliability
log_keyis disallowed with OTLP formats to prevent incompatible output.