out_azure_blob: add path templating support comparable to s3 #11178
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds path-templating and resolution to the Azure Blob output: implements token expansion (UUID, time tokens, strftime, record-accessors) via Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Ingest as Ingest/Flush
participant Resolver as azb_resolve_path()
participant Expander as Token Expander
participant DB as SQLite (azb_db_*)
participant URI as URI builders (azb_uri_*/azb_effective_path)
participant Creator as create/commit routines
Ingest->>Resolver: resolve path_prefix(tag, timestamp, record)
activate Resolver
Resolver->>Expander: expand tokens (%L, %3N, %9N), strftime, $UUID, record-accessors
Expander-->>Resolver: resolved, trimmed path_prefix
Resolver-->>Ingest: return resolved path_prefix
deactivate Resolver
Ingest->>DB: insert file(..., path_prefix)
DB-->>Ingest: rowid / ack
Ingest->>URI: build blob URI(path_prefix, name)
activate URI
URI->>URI: effective_path = azb_effective_path(ctx, path_prefix)
URI-->>Ingest: blob URI (with effective_path + SAS if present)
deactivate URI
Ingest->>Creator: create/append/commit blob(path_prefix, blob_name, parts)
activate Creator
Creator->>DB: update/commit metadata (with path_prefix)
Creator-->>Ingest: success / retry / error
deactivate Creator
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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
🧹 Nitpick comments (2)
plugins/out_azure_blob/azure_blob.c (2)
176-218: Only replaces the first$UUIDoccurrence.The
azb_replace_uuidfunction usesstrstrwhich finds only the first occurrence of$UUID. If a user specifies a path likelogs/$UUID/data/$UUID, only the first placeholder will be replaced.Consider whether multiple UUID placeholders should be supported. If so, a loop would be needed. If single replacement is intentional, document this limitation.
347-366: Minor:strftimereturns 0 for both buffer overflow and empty output.The loop relies on
strftimereturning 0 to indicate the buffer is too small. However,strftimealso returns 0 if the output is an empty string (e.g., if the path contains no format specifiers that produce output, though unlikely). The 4096-byte limit prevents infinite looping, so this is safe, but the behavior could be surprising.If you want to distinguish these cases more explicitly:
while (1) { out_len = strftime(buf, buf_size + 1, path, &tm_utc); - if (out_len > 0) { + if (out_len > 0 || buf_size > flb_sds_len(path) * 2) { break; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
plugins/out_azure_blob/azure_blob.c(15 hunks)plugins/out_azure_blob/azure_blob.h(3 hunks)plugins/out_azure_blob/azure_blob_appendblob.c(1 hunks)plugins/out_azure_blob/azure_blob_appendblob.h(1 hunks)plugins/out_azure_blob/azure_blob_blockblob.c(7 hunks)plugins/out_azure_blob/azure_blob_blockblob.h(1 hunks)plugins/out_azure_blob/azure_blob_conf.c(3 hunks)plugins/out_azure_blob/azure_blob_uri.c(1 hunks)plugins/out_azure_blob/azure_blob_uri.h(1 hunks)tests/internal/CMakeLists.txt(2 hunks)tests/internal/azure_blob_path.c(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
tests/internal/CMakeLists.txtplugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-29T06:25:02.561Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:7-7
Timestamp: 2025-08-29T06:25:02.561Z
Learning: In Fluent Bit, ZSTD (zstandard) compression library is bundled directly in the source tree at `lib/zstd-1.5.7` and is built unconditionally as a static library. Unlike optional external dependencies, ZSTD does not use conditional compilation guards like `FLB_HAVE_ZSTD` and is always available. Headers like `<fluent-bit/flb_zstd.h>` can be included directly without guards.
Applied to files:
plugins/out_azure_blob/azure_blob_conf.cplugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
plugins/out_azure_blob/azure_blob_conf.cplugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-29T06:24:55.855Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:52-56
Timestamp: 2025-08-29T06:24:55.855Z
Learning: ZSTD compression is always available in Fluent Bit and does not require conditional compilation guards. Unlike Arrow/Parquet which use #ifdef FLB_HAVE_ARROW guards, ZSTD is built unconditionally with flb_zstd.c included directly in src/CMakeLists.txt and a bundled ZSTD library at lib/zstd-1.5.7/.
Applied to files:
plugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-29T06:24:26.170Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:39-42
Timestamp: 2025-08-29T06:24:26.170Z
Learning: In Fluent Bit, ZSTD compression support is enabled by default and does not require conditional compilation guards (like #ifdef FLB_HAVE_ZSTD) around ZSTD-related code declarations and implementations.
Applied to files:
plugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components, ZSTD support is always available and doesn't need build-time conditionals.
Applied to files:
plugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-29T06:24:44.797Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:26-26
Timestamp: 2025-08-29T06:24:44.797Z
Learning: In Fluent Bit, ZSTD support is always available and enabled by default. The build system automatically detects and uses either the system libzstd library or builds the bundled ZSTD version. Unlike other optional dependencies like Arrow which use conditional compilation guards (e.g., FLB_HAVE_ARROW), ZSTD does not require conditional includes or build flags.
Applied to files:
plugins/out_azure_blob/azure_blob.h
🧬 Code graph analysis (8)
plugins/out_azure_blob/azure_blob_uri.h (1)
plugins/out_azure_blob/azure_blob_uri.c (1)
azb_uri_create_blob(137-163)
tests/internal/azure_blob_path.c (4)
src/flb_sds.c (2)
flb_sds_destroy(389-399)flb_sds_create(78-90)src/flb_record_accessor.c (2)
flb_ra_destroy(232-248)flb_ra_create(271-358)plugins/out_azure_blob/azure_blob.c (1)
azb_resolve_path(412-518)include/fluent-bit/flb_time.h (1)
flb_time_set(75-79)
plugins/out_azure_blob/azure_blob_conf.c (1)
src/flb_record_accessor.c (2)
flb_ra_create(271-358)flb_ra_destroy(232-248)
plugins/out_azure_blob/azure_blob_appendblob.c (2)
plugins/out_azure_blob/azure_blob_uri.c (1)
azb_uri_container(107-118)src/flb_sds.c (1)
flb_sds_printf(336-387)
plugins/out_azure_blob/azure_blob_uri.c (1)
src/flb_sds.c (1)
flb_sds_printf(336-387)
plugins/out_azure_blob/azure_blob_blockblob.c (2)
plugins/out_azure_blob/azure_blob_uri.c (1)
azb_uri_container(107-118)src/flb_sds.c (1)
flb_sds_printf(336-387)
plugins/out_azure_blob/azure_blob_appendblob.h (1)
plugins/out_azure_blob/azure_blob_appendblob.c (1)
azb_append_blob_uri(27-53)
plugins/out_azure_blob/azure_blob.h (1)
plugins/out_azure_blob/azure_blob.c (1)
azb_resolve_path(412-518)
🔇 Additional comments (32)
plugins/out_azure_blob/azure_blob.h (1)
27-27: LGTM!The header additions are well-structured:
- The
flb_time.hinclude is required for thestruct flb_timeparameter inazb_resolve_path.- The
path_accessorfield enables tag expansion via record accessor.- The function declaration correctly matches the implementation signature.
Also applies to: 66-66, 171-176
tests/internal/CMakeLists.txt (1)
62-67: LGTM!The CMake integration follows the project's established patterns:
- Conditional inclusion of test file when
FLB_OUT_AZURE_BLOBis enabled mirrors other plugin-specific tests.- Library linking is correctly scoped to only the
flb-it-azure_blob_pathtest target.Also applies to: 227-229
plugins/out_azure_blob/azure_blob_appendblob.h (1)
26-28: LGTM!The function signature update correctly adds
path_prefixparameter to support path templating. Theconst char *type forpath_prefixis appropriate since it's read-only input.plugins/out_azure_blob/azure_blob_conf.c (1)
816-819: LGTM!The cleanup logic correctly destroys the path accessor and nullifies the pointer, following the established pattern for other resources in this function.
plugins/out_azure_blob/azure_blob_uri.h (1)
30-32: LGTM!The updated function signature correctly adds the
path_prefixparameter, consistent with the implementation and the parallel changes inazure_blob_appendblob.h.plugins/out_azure_blob/azure_blob_appendblob.c (1)
27-49: LGTM!The implementation correctly:
- Selects
effective_pathpreferringpath_prefixoverctx->pathwhen provided and non-empty.- Constructs the URI with proper path/tag structure for append blob operations.
- Maintains existing SAS token handling.
The
flb_sds_printfreturn value is not checked, but this follows the existing pattern in the codebase for this function.tests/internal/azure_blob_path.c (7)
13-24: LGTM!The
ctx_cleanuphelper properly handles resource cleanup for bothpathandpath_accessor, with NULL checks before destruction and pointer nullification after.
26-58: LGTM!Good test coverage for basic
$TAGsubstitution with proper setup, assertion, and cleanup flow.
60-94: LGTM!Excellent test for
$TAG[n]indexed tag segment extraction with delimiter parsing.
96-131: LGTM!Comprehensive test for time-based tokens (
%Y,%m,%d,%3N,%9N,%L) using a fixed timestamp for deterministic verification.
133-177: LGTM!Good test for UUID token substitution with validation that:
- The UUID prefix is preserved.
- The generated value is 8 alphanumeric characters.
- No
$UUIDliteral remains in output.
179-209: LGTM!Important edge case test verifying that out-of-bounds tag index (
$TAG[5]with only 2 tag segments) correctly returnsret == 0withresolved == NULL.
211-217: LGTM!Test registration is complete and properly null-terminated.
plugins/out_azure_blob/azure_blob_uri.c (1)
137-162: LGTM!The implementation correctly:
- Adds
path_prefixparameter to enable dynamic path resolution.- Uses the same
effective_pathselection logic asazb_append_blob_urifor consistency.- Properly handles SAS token appending with
?(vs&in appendblob which already has query params).plugins/out_azure_blob/azure_blob_blockblob.h (1)
26-43: LGTM! Consistent API extension for path templating support.The function signatures are updated consistently to accept the new
path_prefixparameter with appropriateconstqualification. The parameter ordering is uniform across all modified functions.plugins/out_azure_blob/azure_blob_blockblob.c (5)
34-61: LGTM! Clean effective_path fallback logic.The function correctly implements the path_prefix fallback pattern: if
path_prefixis provided and non-empty, use it; otherwise fall back toctx->path. The dual check for both NULL and empty string is appropriate.
63-124: LGTM! Proper resource management and path handling.The function correctly handles the path_prefix parameter with proper fallback logic and ensures
encoded_blockidis freed before return in all code paths.
126-165: LGTM! Consistent implementation.The
azb_block_blob_uri_commitfunction follows the same effective_path pattern as other functions in this file.
354-393: LGTM! Proper path_prefix propagation.The function correctly forwards the
path_prefixparameter to the internalazb_block_blob_uri_commitcall.
447-451: LGTM! Correct NULL path_prefix for file parts.Passing
NULLaspath_prefixis appropriate here since file parts use the pre-determinedpathstored in the database rather than dynamically resolved templated paths.plugins/out_azure_blob/azure_blob.c (12)
220-267: LGTM with same single-replacement caveat.The helper correctly handles NULL inputs and memory management. Like
azb_replace_uuid, it only replaces the first occurrence of a token, which is acceptable for the time tokens (%3N,%9N,%L) that are typically used once per path.
269-305: LGTM! Correct sequential token application.The function properly chains time token replacements and handles failures appropriately.
382-410: LGTM! Correct in-place slash trimming.The function properly uses
memmovefor overlapping memory regions and correctly updates the SDS length. Edge cases (NULL input, all-slash paths) are handled.
412-518: LGTM! Well-structured path resolution pipeline.The function implements a clean multi-stage path resolution:
- UUID replacement (before record accessor to preserve
$tokens)- Time token replacement (
%3N,%9N,%L)- strftime formatting
- Record accessor expansion for
$TAGand$TAG[n]- Slash trimming
Memory management is correct across all error paths.
520-542: LGTM! Clean wrapper for tag-based blob creation.The function properly wraps path resolution with blob creation. The defensive NULL check before
flb_sds_destroy(prefix)at lines 529-531 is good practice, even thoughazb_resolve_pathinitializes*out_pathto NULL on entry.
544-555: LGTM! Properly propagates path_prefix.The function correctly forwards the
path_prefixparameter toazb_uri_create_blob.
624-632: LGTM! Correct NULL path_prefix for delete operation.Delete operations target existing blobs by name rather than templated paths, so passing
NULLis appropriate.
836-946: LGTM! Comprehensive path_prefix handling in send_blob.The function correctly:
- Resolves the path at the start using current timestamp
- Propagates
path_prefixto all URI construction and blob operation functions- Cleans up
path_prefixin all exit paths (success, error, and retry)
1663-1670: LGTM! Consistent use of tag-aware blob creation.The callback correctly uses
azb_create_blob_with_tagto ensure path templating is applied when creating blobs.
1794-1800: LGTM! Path templating in chunk ingestion.Consistent with other code paths, this uses
azb_create_blob_with_tagfor path-aware blob creation.
2052-2065: LGTM! Path templating in non-buffered flush.The non-buffered flush path correctly uses
azb_create_blob_with_tagfor consistent path templating behavior.
433-435: path_accessor initialization is correctly implemented.Verification confirms the code works as intended:
path_accessoris conditionally created inazure_blob_conf.c:766when a path template is configured, with proper error handling (lines 767-769). The early return at line 433 is appropriate—if no path template is configured, the function returns 0 (success) with*out_path = NULL, which is the correct behavior.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
plugins/out_azure_blob/azure_blob.c(16 hunks)plugins/out_azure_blob/azure_blob.h(3 hunks)plugins/out_azure_blob/azure_blob_appendblob.c(1 hunks)plugins/out_azure_blob/azure_blob_appendblob.h(1 hunks)plugins/out_azure_blob/azure_blob_blockblob.c(7 hunks)plugins/out_azure_blob/azure_blob_blockblob.h(1 hunks)plugins/out_azure_blob/azure_blob_conf.c(3 hunks)plugins/out_azure_blob/azure_blob_uri.c(1 hunks)plugins/out_azure_blob/azure_blob_uri.h(1 hunks)tests/internal/CMakeLists.txt(2 hunks)tests/internal/azure_blob.c(1 hunks)tests/internal/azure_blob_path.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/internal/CMakeLists.txt
- plugins/out_azure_blob/azure_blob_conf.c
- plugins/out_azure_blob/azure_blob.h
- tests/internal/azure_blob_path.c
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
tests/internal/azure_blob.c
📚 Learning: 2025-08-29T06:24:55.855Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:52-56
Timestamp: 2025-08-29T06:24:55.855Z
Learning: ZSTD compression is always available in Fluent Bit and does not require conditional compilation guards. Unlike Arrow/Parquet which use #ifdef FLB_HAVE_ARROW guards, ZSTD is built unconditionally with flb_zstd.c included directly in src/CMakeLists.txt and a bundled ZSTD library at lib/zstd-1.5.7/.
Applied to files:
tests/internal/azure_blob.c
🧬 Code graph analysis (5)
plugins/out_azure_blob/azure_blob_appendblob.c (1)
src/flb_sds.c (1)
flb_sds_printf(336-387)
plugins/out_azure_blob/azure_blob_appendblob.h (1)
plugins/out_azure_blob/azure_blob_appendblob.c (1)
azb_append_blob_uri(27-53)
tests/internal/azure_blob.c (6)
src/flb_config.c (2)
flb_config_init(233-486)flb_config_exit(488-672)src/flb_upstream.c (1)
flb_upstream_create(290-363)src/flb_sds.c (2)
flb_sds_create(78-90)flb_sds_destroy(389-399)plugins/out_azure_blob/azure_blob_blockblob.c (1)
azb_block_blob_uri(63-124)src/flb_http_client.c (3)
flb_http_client(814-859)flb_http_get_header(1002-1019)flb_http_client_destroy(1688-1695)plugins/out_azure_blob/azure_blob_http.c (1)
azb_http_client_setup(293-366)
plugins/out_azure_blob/azure_blob_uri.c (1)
src/flb_sds.c (1)
flb_sds_printf(336-387)
plugins/out_azure_blob/azure_blob.c (3)
plugins/out_azure_blob/azure_blob_uri.c (1)
azb_uri_create_blob(137-163)plugins/out_azure_blob/azure_blob_appendblob.c (1)
azb_append_blob_uri(27-53)plugins/out_azure_blob/azure_blob_blockblob.c (1)
azb_block_blob_uri(63-124)
🔇 Additional comments (21)
plugins/out_azure_blob/azure_blob_appendblob.h (1)
26-28: LGTM: API signature updated for path templating support.The addition of
path_prefixparameter aligns with the broader path templating changes across the Azure Blob module.Minor note: Lines 27-28 use tabs for indentation while the codebase generally uses 4-space indentation. This is a cosmetic issue that can be addressed if desired.
plugins/out_azure_blob/azure_blob_uri.h (1)
30-32: LGTM: Header declaration updated for path_prefix support.The signature change is consistent with the implementation and other URI-building functions in this PR.
plugins/out_azure_blob/azure_blob_appendblob.c (1)
27-46: LGTM: Path prefix logic correctly implemented.The
effective_pathcomputation properly falls back toctx->pathwhenpath_prefixis NULL or empty, maintaining backward compatibility while enabling path templating.plugins/out_azure_blob/azure_blob_uri.c (1)
137-162: LGTM: Consistent path_prefix handling in azb_uri_create_blob.The implementation follows the same pattern as
azb_append_blob_uri, properly computingeffective_pathand using it for URI construction with appropriate SAS token handling.tests/internal/azure_blob.c (1)
42-89: Test fixture setup and teardown are well-structured.The fixture properly initializes and cleans up resources (config, upstream, connection) in the correct order, with appropriate error handling during initialization.
plugins/out_azure_blob/azure_blob_blockblob.h (1)
26-43: LGTM: Block blob function signatures updated for path templating.All three function declarations (
azb_block_blob_blocklist_uri,azb_block_blob_uri,azb_block_blob_commit_block) consistently add thepath_prefixparameter, aligning with the implementation and broader path templating support.plugins/out_azure_blob/azure_blob_blockblob.c (4)
34-61: LGTM: Consistent path_prefix handling in blocklist URI builder.The
effective_pathpattern is correctly applied, with proper fallback toctx->pathwhenpath_prefixis NULL or empty.
63-124: LGTM: Block blob URI construction properly updated.The function correctly handles both path-prefixed and non-prefixed cases, maintaining all existing functionality including timestamp handling, compression extensions, and SAS token appending.
354-366: LGTM: Commit block function properly propagates path_prefix.The
path_prefixparameter is correctly passed through toazb_block_blob_uri_commit.
447-447: Correct: NULL path_prefix for file parts commit.Passing NULL here is appropriate since file parts committed via
azb_block_blob_commit_file_partsuse the storedpathfromctxrather than dynamically resolved templating.plugins/out_azure_blob/azure_blob.c (11)
184-254: LGTM: UUID replacement implementation is correct.The function properly:
- Counts occurrences before allocation
- Returns the original path unchanged if no $UUID tokens exist (line 207)
- Correctly calculates result length accounting for size difference between token and replacement
- Properly frees the original path and returns the new SDS string
260-306: LGTM: Token replacement helper is well-implemented.The function correctly handles edge cases (NULL input, token not found) and properly manages memory by destroying the input SDS and returning a new one.
311-347: LGTM: Time token expansion correctly chains replacements.The function properly chains
azb_simple_replacecalls, which handle memory cleanup of intermediate SDS strings.
352-433: LGTM: strftime application with proper buffer growth.The implementation correctly handles buffer sizing with safeguards against infinite loops (empty_threshold and 4096 byte limit). Memory is properly freed in all code paths.
438-466: LGTM: Slash trimming correctly modifies SDS in-place.The function properly uses
memmovefor potentially overlapping memory regions and updates the SDS length metadata.
471-577: LGTM: Path resolution function with proper resource cleanup.The function correctly handles:
- Early return when templating is disabled
- Fallback to current time when no timestamp provided
- Sequential application of UUID, time tokens, strftime, and record accessor
- Proper cleanup of msgpack and record accessor resources on all paths
579-601: LGTM: Blob creation wrapper with path resolution.The function properly resolves the path prefix and cleans up on failure. Note that
flb_sds_destroyalready handles NULL safely, so the NULL check at line 588 is defensive but harmless.
895-1004: LGTM: send_blob properly manages path_prefix lifecycle.The function correctly:
- Resolves path_prefix at the beginning using the event timestamp
- Passes path_prefix through all URI building and blob creation calls
- Cleans up path_prefix on all exit paths (early returns and final cleanup)
691-691: Correct: delete_blob passes NULL for path_prefix.Deletion uses the stored path from context rather than dynamically resolved templating, so NULL is appropriate here.
1722-1724: LGTM: Ingest callback uses azb_create_blob_with_tag.The callback correctly uses the new wrapper function to resolve the path template when creating blobs.
2111-2113: LGTM: Flush callback uses azb_create_blob_with_tag.Consistent integration with the path templating system.
|
@codex review |
![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.
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/out_azure_blob/azure_blob.c (2)
161-178: UUID and time token helpers are sound, but%3N/%9N/%Lonly replace the first occurrenceThe overall flow in
azb_replace_uuid(),azb_simple_replace(), andazb_apply_time_tokens()looks correct:
$UUIDis replaced with a single 8‑char random suffix consistently across all occurrences.- Memory ownership is clear: the incoming SDS is always either returned as-is or destroyed exactly once on success/failure.
One functional nit:
azb_apply_time_tokens()usesazb_simple_replace(), which only replaces the first occurrence of each token. If a user puts%3Nor%Lmultiple times in the path, only the first instance is expanded.You can make token expansion robust for repeated tokens with a small loop around
azb_simple_replace():-static flb_sds_t azb_apply_time_tokens(flb_sds_t path, const struct flb_time *timestamp) +static flb_sds_t azb_apply_time_tokens(flb_sds_t path, const struct flb_time *timestamp) { @@ - /* Replace %3N with milliseconds */ - tmp = azb_simple_replace(path, "%3N", ms_buf); - if (!tmp) { - return NULL; - } - path = tmp; + /* Replace all %3N with milliseconds */ + while (1) { + tmp = azb_simple_replace(path, "%3N", ms_buf); + if (tmp == path || !tmp) { + /* No more matches or allocation failure */ + if (!tmp) { + return NULL; + } + break; + } + path = tmp; + } @@ - /* Replace %9N with nanoseconds */ - tmp = azb_simple_replace(path, "%9N", ns_buf); - if (!tmp) { - return NULL; - } - path = tmp; + /* Replace all %9N with nanoseconds */ + while (1) { + tmp = azb_simple_replace(path, "%9N", ns_buf); + if (tmp == path || !tmp) { + if (!tmp) { + return NULL; + } + break; + } + path = tmp; + } @@ - /* Replace %L with nanoseconds */ - tmp = azb_simple_replace(path, "%L", ns_buf); - if (!tmp) { - return NULL; - } - - return tmp; + /* Replace all %L with nanoseconds */ + while (1) { + tmp = azb_simple_replace(path, "%L", ns_buf); + if (tmp == path || !tmp) { + if (!tmp) { + return NULL; + } + break; + } + path = tmp; + } + + return path;(Adjust the “no match” detection depending on how you prefer to signal “unchanged”; the main point is to loop until no further replacements.)
Also applies to: 184-254, 256-347
887-1007:send_blob: path templating integration is correct for logs but risky for BLOB partsPositive points:
- Calling
azb_resolve_path()once persend_blob()and passingpath_prefixthrough toazb_append_blob_uri(),azb_block_blob_uri(),azb_block_blob_commit_block(), andcreate_blob()is the right way to make all Azure operations path-templating-aware.path_prefixownership is handled carefully: only set viaazb_resolve_path(), freed in all early-return and normal-exit paths.Two concerns:
BLOB uploads (
FLB_EVENT_TYPE_BLOBS) also go throughazb_resolve_path()withtag == NULLAs noted in the
azb_resolve_path()comment, this means:
flb_ra_translate()may see a null tag pointer if a user puts$TAGin the template.- Even if templates avoid
$TAG, BLOB uploads will now start honoring time/UUID-based templating, which was not clearly part of the original behavior.If the intent is to support templating only for log-based uploads initially, you can defensively gate the resolve step:
- flb_time_get(&now);
- if (azb_resolve_path(ctx, tag, tag_len, &now, &path_prefix) != 0) {
return FLB_RETRY;- }
- flb_time_get(&now);
- if (tag && tag_len > 0) {
if (azb_resolve_path(ctx, tag, tag_len, &now, &path_prefix) != 0) {return FLB_RETRY;}- }
That keeps existing BLOB behavior unchanged while still enabling templating for logs. 2. **Potential path mismatch for multi-part BLOB commits** For BLOBs (`FLB_EVENT_TYPE_BLOBS`), `send_blob()` builds block URIs via: ```c uri = azb_block_blob_uri(ctx, path_prefix, name, block_id, 0, generated_random_string);whereas
azb_block_blob_commit_file_parts()later commits via:uri = azb_block_blob_blocklist_uri(ctx, NULL, path);So the PUT-block step uses
path_prefix(when non-NULL) but the PUT-block-list commit is always called with aNULLpath_prefix. If a user enables path templating and you do let BLOB traffic go throughazb_resolve_path(), the commit step may target a different blob path than the blocks were written under.If you do decide to support templating for BLOBs, you’ll likely need to persist the resolved
path_prefixalongside the file metadata and pass it intoazb_block_blob_commit_file_parts()so both flows use the same effective path.Given the current PR scope, I recommend at least gating
azb_resolve_path()ontag != NULLas above to avoid surprising BLOB behavior and potential mismatches.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
plugins/out_azure_blob/azure_blob.c(19 hunks)plugins/out_azure_blob/azure_blob.h(3 hunks)plugins/out_azure_blob/azure_blob_appendblob.c(1 hunks)plugins/out_azure_blob/azure_blob_appendblob.h(1 hunks)plugins/out_azure_blob/azure_blob_blockblob.c(7 hunks)plugins/out_azure_blob/azure_blob_blockblob.h(1 hunks)plugins/out_azure_blob/azure_blob_conf.c(3 hunks)plugins/out_azure_blob/azure_blob_uri.c(1 hunks)plugins/out_azure_blob/azure_blob_uri.h(1 hunks)tests/internal/CMakeLists.txt(2 hunks)tests/internal/azure_blob_path.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/out_azure_blob/azure_blob_conf.c
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
tests/internal/CMakeLists.txtplugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
plugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-29T06:24:55.855Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:52-56
Timestamp: 2025-08-29T06:24:55.855Z
Learning: ZSTD compression is always available in Fluent Bit and does not require conditional compilation guards. Unlike Arrow/Parquet which use #ifdef FLB_HAVE_ARROW guards, ZSTD is built unconditionally with flb_zstd.c included directly in src/CMakeLists.txt and a bundled ZSTD library at lib/zstd-1.5.7/.
Applied to files:
plugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-29T06:25:02.561Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:7-7
Timestamp: 2025-08-29T06:25:02.561Z
Learning: In Fluent Bit, ZSTD (zstandard) compression library is bundled directly in the source tree at `lib/zstd-1.5.7` and is built unconditionally as a static library. Unlike optional external dependencies, ZSTD does not use conditional compilation guards like `FLB_HAVE_ZSTD` and is always available. Headers like `<fluent-bit/flb_zstd.h>` can be included directly without guards.
Applied to files:
plugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-29T06:24:26.170Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:39-42
Timestamp: 2025-08-29T06:24:26.170Z
Learning: In Fluent Bit, ZSTD compression support is enabled by default and does not require conditional compilation guards (like #ifdef FLB_HAVE_ZSTD) around ZSTD-related code declarations and implementations.
Applied to files:
plugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components, ZSTD support is always available and doesn't need build-time conditionals.
Applied to files:
plugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-29T06:24:44.797Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:26-26
Timestamp: 2025-08-29T06:24:44.797Z
Learning: In Fluent Bit, ZSTD support is always available and enabled by default. The build system automatically detects and uses either the system libzstd library or builds the bundled ZSTD version. Unlike other optional dependencies like Arrow which use conditional compilation guards (e.g., FLB_HAVE_ARROW), ZSTD does not require conditional includes or build flags.
Applied to files:
plugins/out_azure_blob/azure_blob.h
🧬 Code graph analysis (8)
plugins/out_azure_blob/azure_blob.h (1)
plugins/out_azure_blob/azure_blob.c (1)
azb_resolve_path(471-577)
plugins/out_azure_blob/azure_blob_blockblob.h (1)
plugins/out_azure_blob/azure_blob_blockblob.c (3)
azb_block_blob_blocklist_uri(34-61)azb_block_blob_uri(63-124)azb_block_blob_commit_block(354-393)
tests/internal/azure_blob_path.c (3)
plugins/out_azure_blob/azure_blob.c (1)
azb_resolve_path(471-577)include/fluent-bit/flb_time.h (1)
flb_time_set(75-79)plugins/out_azure_blob/azure_blob_uri.c (1)
azb_uri_create_blob(155-181)
plugins/out_azure_blob/azure_blob_appendblob.c (1)
plugins/out_azure_blob/azure_blob_uri.c (2)
azb_uri_container(107-118)azb_effective_path(137-153)
plugins/out_azure_blob/azure_blob_uri.h (1)
plugins/out_azure_blob/azure_blob_uri.c (2)
azb_uri_create_blob(155-181)azb_effective_path(137-153)
plugins/out_azure_blob/azure_blob.c (3)
plugins/out_azure_blob/azure_blob_uri.c (1)
azb_uri_create_blob(155-181)plugins/out_azure_blob/azure_blob_appendblob.c (1)
azb_append_blob_uri(27-53)plugins/out_azure_blob/azure_blob_blockblob.c (1)
azb_block_blob_uri(63-124)
plugins/out_azure_blob/azure_blob_uri.c (1)
src/flb_sds.c (1)
flb_sds_printf(336-387)
plugins/out_azure_blob/azure_blob_appendblob.h (1)
plugins/out_azure_blob/azure_blob_appendblob.c (1)
azb_append_blob_uri(27-53)
🔇 Additional comments (16)
plugins/out_azure_blob/azure_blob_appendblob.h (1)
26-28: Append-blob URI signature updated for path_prefix supportFunction now takes
const char *path_prefix, matching the implementation and enabling reuse ofazb_effective_pathwithout mutating the prefix.tests/internal/CMakeLists.txt (2)
62-67: Conditional inclusion of azure_blob_path test is correct
azure_blob_path.cis only compiled whenFLB_OUT_AZURE_BLOBis enabled, which aligns the test with the plugin’s availability and avoids build issues when the plugin is disabled.
227-229: Selective linking of azure_blob_path test against Azure Blob pluginLinking
flb-it-azure_blob_pathwithflb-plugin-out_azure_blobunder the sameFLB_OUT_AZURE_BLOBguard cleanly wires the test to the plugin without affecting other internal tests.plugins/out_azure_blob/azure_blob.h (1)
27-27: New path-templating state and azb_resolve_path API look consistentIncluding
flb_time.h, addingpath_templating_enabled, and declaringazb_resolve_pathmatch the implementation inazure_blob.cand how the tests use the function (out_patheither NULL or a heap-allocatedflb_sds_towned by the caller).Also applies to: 66-66, 171-175
plugins/out_azure_blob/azure_blob_appendblob.c (1)
27-29: Append-blob URIs now honor effective_path and templated prefixesUsing
azb_effective_path(ctx, path_prefix)and branching on a non-emptyeffective_pathbrings append-blob URIs in line withazb_uri_create_blob, correctly omitting the prefix when templating resolves to an empty path and still appending SAS tokens as before.Also applies to: 32-32, 39-42
plugins/out_azure_blob/azure_blob_uri.h (1)
30-34: URI helper declarations aligned with path_prefix handling
azb_uri_create_bloband the newazb_effective_pathdeclaration match their definitions and clearly expose a path-prefix–aware URI-building API for callers.plugins/out_azure_blob/azure_blob_uri.c (1)
137-153: azb_effective_path semantics correctly separate templated and static pathsWhen
path_templating_enabledis true,azb_effective_pathalways returns the caller-suppliedpath_prefix(including NULL for “no prefix”), so URIs never fall back to the raw template inctx->path; when templating is disabled, it reusespath_prefixif non-empty and otherwise falls back toctx->path, preserving existing behavior.azb_uri_create_blobthen formats the URI from thiseffective_pathin the expected way.Also applies to: 155-170
tests/internal/azure_blob_path.c (1)
1-294: Comprehensive tests for Azure Blob path templating and effective URI behaviorThe suite covers
$TAG,$TAG[n], time tokens, single and repeated$UUIDtokens (including equality across uses), the empty-resolution case where$TAG[5]yields NULL, and the resultingazb_uri_create_blobbehavior when the resolved prefix is NULL;ctx_cleanuphandles allocated SDS fields, so the tests exercise the new templating flow without obvious leaks.plugins/out_azure_blob/azure_blob_blockblob.h (1)
26-34: Block-blob APIs extended with path_prefix consistentlyAdding
const char *path_prefixto the blocklist, per-block, and commit helpers matches the updated implementations and gives block blobs the same effective-path / templating behavior as append blobs.Also applies to: 38-43
plugins/out_azure_blob/azure_blob_blockblob.c (2)
34-61: Blocklist URI now path-prefix aware; existing file-part commit behavior preserved
azb_block_blob_blocklist_uri()correctly introducespath_prefixand usesazb_effective_path()so log paths can honor per-record prefixes while still falling back whenpath_prefix == NULL.azb_block_blob_commit_file_parts()passingNULLkeeps multi-part file commit using the container + configured base path only, which matches previous behavior for blob uploads. No functional or memory-management issues here.Also applies to: 447-457
63-124: Block blob PUT/commit URIs correctly propagatepath_prefixThe updated
azb_block_blob_uri(),azb_block_blob_uri_commit(), andazb_block_blob_commit_block()consistently computeeffective_pathviaazb_effective_path(ctx, path_prefix)and build URIs with or without the prefix.encoded_blockidis always freed, and no new leak or error path was introduced. Layout matches the append-blob and create-blob helpers, so path templating behavior stays uniform across blob types.Also applies to: 126-165, 353-393
plugins/out_azure_blob/azure_blob.c (5)
34-38: New headers align with added functionalityIncluding
flb_record_accessor.hand<string.h>matches the new use of record accessors,strstr,strlen,memcpy, etc. This is the right place to add them; no issues.
579-601:azb_create_blob_with_tagcorrectly couples per-tag path resolution with blob creation
azb_create_blob_with_tag()cleanly wraps:
- Resolving a
path_prefixviaazb_resolve_path().- Passing that prefix into
create_blob()so the blob is created under the same templated path thatsend_blob()uses.- Always destroying
prefixon both success and failure.The retry semantics (returning
FLB_RETRYon resolution failure) are consistent with surrounding code.
603-683:create_blobrefactor: better cleanup and path-prefix supportThe new
create_blob(struct flb_azure_blob *ctx, const char *path_prefix, char *name)implementation looks good:
- Uses
azb_uri_create_blob(ctx, path_prefix, name)so the create step and subsequent writes share the same path resolution logic.- Introduces a
statusvariable andcleanup_create:label to ensureuri, the HTTP client, and the upstream connection are always released on all paths.- Returns
FLB_RETRYwhere previously some error paths just returned a generic error, which should integrate better with the caller’s retry logic.No functional issues spotted here.
685-760:delete_blobrefactor mirrorscreate_bloband looks safe
delete_blob()now:
- Uses
azb_uri_create_blob(ctx, NULL, name)(no per-record prefix, matching previous behavior).- Uses a
statusvariable pluscleanup_delete:to guarantee the HTTP client and upstream connection are released in all branches.- Returns
FLB_RETRYon transport or HTTP-status errors, consistent with the rest of the plugin.This refactor improves robustness without changing intended semantics.
1725-1733: Call sites ofazb_create_blob_with_tagare consistent and correctAll places that previously did a “create blob then resend” dance now call
azb_create_blob_with_tag():
cb_azure_blob_ingest(timer-based upload of buffered files).ingest_all_chunks(startup replay).- Both buffered and non-buffered branches in
cb_azure_blob_flush.They correctly:
- Pass the tag as both the logical blob name and the tag used for path templating.
- Use
flb_sds_len(tag_sds)/tag_lenfor the length.- Only resend when
azb_create_blob_with_tag()returnsFLB_OK.This keeps the create + send behavior consistent with the new path templating model.
Also applies to: 1857-1860, 2029-2033, 2115-2127
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugins/out_azure_blob/azure_blob.c (1)
311-347: Note: Custom time tokens replace first occurrence only.The
azb_apply_time_tokensfunction usesazb_simple_replace, which only replaces the first occurrence of each token (%3N,%9N,%L). If a user specifies a path likelogs/%3N/%3N, only the first%3Nis replaced with milliseconds; the second remains as literal%3Nand will be handled bystrftimelater (likely producing unexpected results).This is acceptable because:
- It's unusual to repeat the same precision token multiple times
- Standard
strftimetokens (like%Y) will still be replaced correctly in all occurrences byazb_apply_strftimeHowever, document this limitation in user-facing docs to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
plugins/out_azure_blob/azure_blob.c(19 hunks)plugins/out_azure_blob/azure_blob.h(3 hunks)plugins/out_azure_blob/azure_blob_appendblob.c(1 hunks)plugins/out_azure_blob/azure_blob_appendblob.h(1 hunks)plugins/out_azure_blob/azure_blob_blockblob.c(7 hunks)plugins/out_azure_blob/azure_blob_blockblob.h(1 hunks)plugins/out_azure_blob/azure_blob_conf.c(3 hunks)plugins/out_azure_blob/azure_blob_uri.c(1 hunks)plugins/out_azure_blob/azure_blob_uri.h(1 hunks)tests/internal/CMakeLists.txt(2 hunks)tests/internal/azure_blob_path.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/internal/CMakeLists.txt
- plugins/out_azure_blob/azure_blob_uri.h
- plugins/out_azure_blob/azure_blob_conf.c
- plugins/out_azure_blob/azure_blob_appendblob.c
- plugins/out_azure_blob/azure_blob_appendblob.h
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
plugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-29T06:24:55.855Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:52-56
Timestamp: 2025-08-29T06:24:55.855Z
Learning: ZSTD compression is always available in Fluent Bit and does not require conditional compilation guards. Unlike Arrow/Parquet which use #ifdef FLB_HAVE_ARROW guards, ZSTD is built unconditionally with flb_zstd.c included directly in src/CMakeLists.txt and a bundled ZSTD library at lib/zstd-1.5.7/.
Applied to files:
plugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
plugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-29T06:25:02.561Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:7-7
Timestamp: 2025-08-29T06:25:02.561Z
Learning: In Fluent Bit, ZSTD (zstandard) compression library is bundled directly in the source tree at `lib/zstd-1.5.7` and is built unconditionally as a static library. Unlike optional external dependencies, ZSTD does not use conditional compilation guards like `FLB_HAVE_ZSTD` and is always available. Headers like `<fluent-bit/flb_zstd.h>` can be included directly without guards.
Applied to files:
plugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-29T06:24:26.170Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:39-42
Timestamp: 2025-08-29T06:24:26.170Z
Learning: In Fluent Bit, ZSTD compression support is enabled by default and does not require conditional compilation guards (like #ifdef FLB_HAVE_ZSTD) around ZSTD-related code declarations and implementations.
Applied to files:
plugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components, ZSTD support is always available and doesn't need build-time conditionals.
Applied to files:
plugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-29T06:24:44.797Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:26-26
Timestamp: 2025-08-29T06:24:44.797Z
Learning: In Fluent Bit, ZSTD support is always available and enabled by default. The build system automatically detects and uses either the system libzstd library or builds the bundled ZSTD version. Unlike other optional dependencies like Arrow which use conditional compilation guards (e.g., FLB_HAVE_ARROW), ZSTD does not require conditional includes or build flags.
Applied to files:
plugins/out_azure_blob/azure_blob.h
🧬 Code graph analysis (5)
plugins/out_azure_blob/azure_blob.h (1)
plugins/out_azure_blob/azure_blob.c (1)
azb_resolve_path(486-607)
tests/internal/azure_blob_path.c (4)
src/flb_sds.c (2)
flb_sds_destroy(389-399)flb_sds_create(78-90)plugins/out_azure_blob/azure_blob.c (1)
azb_resolve_path(486-607)include/fluent-bit/flb_time.h (1)
flb_time_set(75-79)plugins/out_azure_blob/azure_blob_uri.c (1)
azb_uri_create_blob(155-181)
plugins/out_azure_blob/azure_blob_blockblob.c (1)
plugins/out_azure_blob/azure_blob_uri.c (2)
azb_uri_container(107-118)azb_effective_path(137-153)
plugins/out_azure_blob/azure_blob_blockblob.h (1)
plugins/out_azure_blob/azure_blob_blockblob.c (3)
azb_block_blob_blocklist_uri(34-61)azb_block_blob_uri(63-124)azb_block_blob_commit_block(354-393)
plugins/out_azure_blob/azure_blob.c (8)
src/flb_sds.c (4)
flb_sds_create_size(92-95)flb_sds_destroy(389-399)flb_sds_create_len(58-76)flb_sds_printf(336-387)include/fluent-bit/flb_compat.h (1)
gmtime_r(75-81)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)src/flb_record_accessor.c (3)
flb_ra_create(271-358)flb_ra_translate(628-633)flb_ra_destroy(232-248)plugins/out_azure_blob/azure_blob_uri.c (1)
azb_uri_create_blob(155-181)src/flb_http_client.c (1)
flb_http_client_destroy(1688-1695)plugins/out_azure_blob/azure_blob_appendblob.c (1)
azb_append_blob_uri(27-53)plugins/out_azure_blob/azure_blob_blockblob.c (3)
azb_block_blob_uri(63-124)azb_block_blob_id_blob(219-272)azb_block_blob_commit_block(354-393)
🔇 Additional comments (11)
plugins/out_azure_blob/azure_blob.h (1)
27-27: LGTM: Clean header additions for path templating support.The additions integrate well:
flb_time.hinclude provides timestamp types forazb_resolve_pathpath_templating_enabledflag cleanly extends the context structureazb_resolve_pathsignature is clear and follows Fluent Bit conventions (out-parameter for result, return int for status)Also applies to: 66-66, 171-175
plugins/out_azure_blob/azure_blob_uri.c (1)
137-181: LGTM: Path-prefix aware URI construction is correct.The
azb_effective_pathhelper cleanly implements the fallback logic:
- When
path_templating_enabledis TRUE, honors the resolvedpath_prefix(even if NULL/empty)- Otherwise falls back to
ctx->pathThe
azb_uri_create_blobcorrectly handles emptyeffective_pathby building/<tag>URIs, which addresses the past review concern about empty templates falling back to unresolved literal paths.tests/internal/azure_blob_path.c (1)
1-294: LGTM: Comprehensive test coverage for path templating.The test suite validates all key scenarios:
- Tag substitution (
$TAG,$TAG[n])- Time token expansion (
%Y,%m,%d,%3N,%9N,%L)- UUID generation and consistency
- Edge cases (empty resolution, NULL prefix URI construction)
The
ctx_cleanuphelper ensures proper resource management between tests, and all tests follow a clear arrange-act-assert pattern.plugins/out_azure_blob/azure_blob_blockblob.h (1)
26-43: LGTM: Block blob signatures correctly extended for path templating.All three functions now accept
const char *path_prefix, which correctly propagates the resolved path through the block blob URI and commit operations. Theconstqualifier appropriately indicates read-only access.plugins/out_azure_blob/azure_blob_blockblob.c (1)
34-165: LGTM: Block blob implementation correctly propagates path_prefix.All URI constructors (
azb_block_blob_blocklist_uri,azb_block_blob_uri,azb_block_blob_uri_commit) consistently:
- Accept
path_prefixparameter- Compute
effective_pathviaazb_effective_path(ctx, path_prefix)- Build URIs with
/<effective_path>/<name>when non-empty, else/<name>- Preserve SAS token handling in all branches
The
azb_block_blob_commit_blockcorrectly forwardspath_prefixthrough the commit flow.Also applies to: 354-393
plugins/out_azure_blob/azure_blob.c (6)
157-178: LGTM: Random string generation for UUID tokens.The
generate_random_string_blobfunction correctly generates 8-character alphanumeric strings using a properly seeded PRNG combining multiple entropy sources.
184-254: LGTM: UUID replacement correctly replaces all occurrences.The
azb_replace_uuidfunction properly:
- Counts all
$UUIDoccurrences before generating the random value- Replaces all instances with the same 8-character string
- Pre-calculates result length to avoid reallocation
- Handles edge cases (no matches, memory allocation failures)
This ensures consistent UUID values across multiple tokens in a single path template.
352-448: LGTM: strftime handling with proper error checking.The
azb_apply_strftimefunction correctly:
- Uses adaptive buffer growth for large format expansions
- Treats
out_len == 0after retry loop as an explicit error (addresses past review comment)- Logs meaningful error messages for invalid or too-long templates
- Handles resource cleanup on all error paths
486-607: LGTM: Path resolution orchestrates token expansion correctly.The
azb_resolve_pathfunction properly:
- Guards against NULL
ctx->pathwhen templating is enabled (addresses past review comment at lines 511-514)- Applies transformations in correct order: UUID → custom time tokens → strftime → tag expansion
- Handles NULL tag by using empty string for record accessor (addresses past review comment at lines 581-584)
- Cleans up all intermediate allocations on success and error paths
- Returns NULL
*out_pathfor empty resolved paths, allowing callers to distinguish "no prefix" from "error"The per-call record accessor construction is noted in past reviews as a performance optimization opportunity (pre-create at config time). This is acceptable for the current PR and can be addressed in future work.
609-631: LGTM: Path-prefix propagation is consistent throughout blob operations.The updates correctly propagate
path_prefix:
azb_create_blob_with_tagresolves the path and forwards tocreate_blobsend_blobresolves the path at line 937 and passes it to all URI constructors and commit operations- All error paths properly clean up
path_prefixallocations (lines 944, 956, 973, 996, 1034-1036)delete_blobcorrectly passes NULL (deletion doesn't use templating)Memory management is correct with cleanup on both success and error paths.
Also applies to: 633-713, 917-1039
1756-1758: LGTM: Call sites updated to use tag-aware blob creation.All blob creation call sites in
cb_azure_blob_ingest,ingest_all_chunks, andcb_azure_blob_flushcorrectly replaced directcreate_blobcalls withazb_create_blob_with_tag, which properly resolves the path template before creating the blob.Also applies to: 1887-1889, 2059-2059, 2145-2147
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/out_azure_blob/azure_blob_blockblob.c (1)
395-457: Templated paths for BLOB uploads likely still break block list commitFor buffered BLOB uploads (
FLB_EVENT_TYPE_BLOBS):
send_blob()first resolves a per-call prefix via
azb_resolve_path(ctx, tag, tag_len, &now, &path_prefix)(tag isNULLhere) and passes thatpath_prefixintoazb_block_blob_uri()when uploading each block.- Later,
azb_block_blob_commit_file_parts()builds the block list URI as:
uri = azb_block_blob_blocklist_uri(ctx, ctx->path, path);Given
azb_effective_path():
- When
ctx->path_templating_enabled == FLB_TRUE(any non-emptypath), it always returns itspath_prefixargument and never consultsctx->path.- In
send_blob(),path_prefixis the resolved template (after$UUID, time tokens,strftime, record accessor).- In
azb_block_blob_commit_file_parts(),path_prefixis the raw template stringctx->path(e.g."logs/$TAG/%Y/%m/%d/$UUID"), with no resolution.As a result, for BLOBS when
pathcontains any templating tokens:
- Data blocks are written under
/resolved_prefix/<name>...(e.g./logs/2025/12/10/…),- But the subsequent Put Block List is sent to
/ctx->path/<name>?comp=blocklist(e.g./logs/$TAG/%Y/%m/%d/$UUID/<name>?comp=blocklist),so the commit will not see the uploaded blocks and the buffered blob can never finalize.
This doesn’t affect cases where
pathis static and has no tokens (raw and resolved prefixes coincide), but it does undermine the new templating feature for BLOBS.Consider:
- Either explicitly disabling path templating for BLOBS (documenting it and bypassing
azb_resolve_path()/ using plainctx->paththere), or- Storing a per-file resolved prefix when the BLOB file is first registered (e.g., in the DB alongside
file_path) and passing that stored prefix into bothsend_blob()andazb_block_blob_commit_file_parts()so they target the same blob path.At minimum, this deserves tests in
azure_blob_path(or a companion test) that cover BLOBS with templated paths to avoid regressions.
🧹 Nitpick comments (6)
plugins/out_azure_blob/azure_blob_appendblob.h (1)
26-28: Append blob URI API extension is consistent with path-prefix designThe new
azb_append_blob_uri(struct flb_azure_blob *ctx, const char *path_prefix, char *tag)matches the implementation and the broaderazb_uri_create_blob/azb_effective_pathpattern. Consider makingtagaconst char *in both declaration and definition in a follow‑up, since it is not modified.plugins/out_azure_blob/azure_blob_conf.c (2)
23-23: flb_record_accessor include appears unused in this TU
azure_blob_conf.cno longer uses record accessor APIs after switching to a simplepath_templating_enabledflag, so<fluent-bit/flb_record_accessor.h>can likely be dropped to avoid confusion and keep dependencies minimal.
817-817: Resetting templating flag on destroy is harmless but optionalClearing
ctx->path_templating_enabledinflb_azure_blob_conf_destroyis not strictly necessary because the whole context is freed immediately after, but it doesn’t hurt and keeps the teardown logic explicit.plugins/out_azure_blob/azure_blob.c (3)
157-178: Avoid reseeding the global PRNG on every random string generation
generate_random_string_blob()callssrand()on each invocation:unsigned int seed = (unsigned int)(time(NULL) ^ clock() ^ getpid()); srand(seed);Since
rand()/srand()are process‑global, this has a few drawbacks:
- Concurrent calls from different threads will trample each other’s RNG state.
- Frequent reseeding (especially within the same second) can reduce randomness and produce correlated IDs.
Consider:
- Seeding once per process/worker (e.g., in
cb_azure_blob_initorcb_worker_init), or- Using an existing Fluent Bit helper for random bytes / IDs instead of
rand().The rest of the function (bounded charset, explicit terminator) is fine.
256-347: Time-token expansion is correct but only handles the first occurrence per token
azb_apply_time_tokens()plusazb_simple_replace():
- Correctly derive
%3N(milliseconds) and%9N/%L(nanoseconds) fromtimestamp->tm.tv_nsec.- Replace each token at most once because
azb_simple_replace()is deliberately “first occurrence only”.If you expect templates like
"logs/%3N/%3N"to replace both instances, you might consider looping untilstrstr()no longer finds the token. If single-occurrence semantics are acceptable (as the comments suggest), the current implementation is fine.
917-1038: send_blob(): path_prefix threading is coherent; minor dead branch remainsThe updated
send_blob():
- Always calls
azb_resolve_path()to computepath_prefix(a no‑op when templating is disabled).- Passes
path_prefixintoazb_append_blob_uri()/azb_block_blob_uri()and, for log-type blocks, intoazb_block_blob_commit_block()andcreate_blob(), ensuring a single, consistent prefix is used for create, write, and commit.- Cleans up
path_prefixin all exit paths, including early failures.There is a small leftover:
void *payload_buf = data; /* ... */ if (payload_buf != data) { flb_sds_destroy(payload_buf); }
payload_bufis never reassigned in this function, andhttp_send_blob()frees its own compressed buffer internally, so this condition is always false and can be safely removed in a later cleanup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
plugins/out_azure_blob/azure_blob.c(19 hunks)plugins/out_azure_blob/azure_blob.h(3 hunks)plugins/out_azure_blob/azure_blob_appendblob.c(1 hunks)plugins/out_azure_blob/azure_blob_appendblob.h(1 hunks)plugins/out_azure_blob/azure_blob_blockblob.c(7 hunks)plugins/out_azure_blob/azure_blob_blockblob.h(1 hunks)plugins/out_azure_blob/azure_blob_conf.c(3 hunks)plugins/out_azure_blob/azure_blob_uri.c(1 hunks)plugins/out_azure_blob/azure_blob_uri.h(1 hunks)tests/internal/CMakeLists.txt(2 hunks)tests/internal/azure_blob_path.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- plugins/out_azure_blob/azure_blob.h
- plugins/out_azure_blob/azure_blob_uri.c
- tests/internal/azure_blob_path.c
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
tests/internal/CMakeLists.txt
📚 Learning: 2025-08-29T06:25:02.561Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:7-7
Timestamp: 2025-08-29T06:25:02.561Z
Learning: In Fluent Bit, ZSTD (zstandard) compression library is bundled directly in the source tree at `lib/zstd-1.5.7` and is built unconditionally as a static library. Unlike optional external dependencies, ZSTD does not use conditional compilation guards like `FLB_HAVE_ZSTD` and is always available. Headers like `<fluent-bit/flb_zstd.h>` can be included directly without guards.
Applied to files:
plugins/out_azure_blob/azure_blob_conf.c
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
plugins/out_azure_blob/azure_blob_conf.c
🧬 Code graph analysis (4)
plugins/out_azure_blob/azure_blob_uri.h (1)
plugins/out_azure_blob/azure_blob_uri.c (2)
azb_uri_create_blob(155-181)azb_effective_path(137-153)
plugins/out_azure_blob/azure_blob_blockblob.c (1)
plugins/out_azure_blob/azure_blob_uri.c (2)
azb_uri_container(107-118)azb_effective_path(137-153)
plugins/out_azure_blob/azure_blob_blockblob.h (1)
plugins/out_azure_blob/azure_blob_blockblob.c (3)
azb_block_blob_blocklist_uri(34-61)azb_block_blob_uri(63-124)azb_block_blob_commit_block(354-393)
plugins/out_azure_blob/azure_blob.c (4)
src/flb_record_accessor.c (3)
flb_ra_create(271-358)flb_ra_translate(628-633)flb_ra_destroy(232-248)plugins/out_azure_blob/azure_blob_uri.c (1)
azb_uri_create_blob(155-181)plugins/out_azure_blob/azure_blob_appendblob.c (1)
azb_append_blob_uri(27-53)plugins/out_azure_blob/azure_blob_blockblob.c (1)
azb_block_blob_uri(63-124)
🔇 Additional comments (19)
tests/internal/CMakeLists.txt (2)
62-67: Azure blob path tests correctly gated on plugin build optionConditionally adding
azure_blob_path.ctoUNIT_TESTS_FILESbehindFLB_OUT_AZURE_BLOBmatches the existing pattern for feature‑gated tests and prevents build issues when the plugin is disabled.
227-229: Linking azure_blob_path tests against the output plugin is appropriateRestricting the extra link of
flb-plugin-out_azure_blobto theflb-it-azure_blob_pathtest keeps other tests unaffected while ensuring the new path‑templating tests have the symbols they need.plugins/out_azure_blob/azure_blob_uri.h (1)
30-34: Centralized path-prefix handling via azb_effective_path looks soundExtending
azb_uri_create_blobwithconst char *path_prefixand introducingazb_effective_path(...)cleanly separates “template resolution” (inazure_blob.c) from low-level URI assembly. Call sites now have to pass the already‑resolved prefix, which is satisfied by the newazb_resolve_path()flow.plugins/out_azure_blob/azure_blob_conf.c (1)
586-599: Path sanitization and templating flag placement is correctRunning path sanitization after applying remote configuration and enabling
ctx->path_templating_enabledonly when the resulting path is non‑empty ensures:
- remote overrides participate in templating,
- trailing slashes are stripped once, and
- no templating work is done when
pathis unset or becomes empty.This also addresses the earlier concern about remote overrides not updating templating behavior.
plugins/out_azure_blob/azure_blob_appendblob.c (1)
27-46: Append blob URIs now honor resolved path_prefix correctlyUsing
azb_effective_path(ctx, path_prefix)and emitting either"/<effective_path>/<tag>?comp=appendblock"or"/<tag>?comp=appendblock"aligns append blob URIs with the new templating flow:
- For templated paths, the resolved prefix from
azb_resolve_path()is used.- For static/non‑templated paths,
ctx->pathremains the fallback viaazb_effective_path.SAS token handling is preserved.
plugins/out_azure_blob/azure_blob_blockblob.h (1)
26-43: Block blob APIs consistently extended with path_prefixAdding
const char *path_prefixtoazb_block_blob_blocklist_uri,azb_block_blob_uri, andazb_block_blob_commit_blockkeeps the block-blob surface consistent with append-blob and generic blob URI creation. This matches the implementations inazure_blob_blockblob.cand allows all block operations to share the same resolved prefix.plugins/out_azure_blob/azure_blob_blockblob.c (4)
34-61: Blocklist URI builder aligns with effective_path semantics
azb_block_blob_blocklist_uri()now mirrors the general pattern:
- builds the container URI,
- derives
effective_path = azb_effective_path(ctx, path_prefix),- prefixes
/effective_path/when non-empty, and- appends
?comp=blocklistand SAS parameters.This is consistent with how data-block URIs and commit URIs work and looks correct.
63-124: Block blob data URIs correctly incorporate path_prefix and compressionThe updated
azb_block_blob_uri():
- encodes
blockid,- selects
.gzvs""based onctx->compress_blob,- uses
effective_path = azb_effective_path(ctx, path_prefix)to decide whether to prepend a directory segment, and- maintains SAS token behavior.
This keeps data-block writes consistent with the new path templating model.
126-165: Commit URI uses effective_path consistently for log-type blocks
azb_block_blob_uri_commit()now also takespath_prefixand routes it throughazb_effective_path, so for log-type block uploads the same resolved prefix used inazb_block_blob_uri()will be reused when committing via?comp=blocklist.
353-393: Commit-block helper correctly threads path_prefix through to commit URI
azb_block_blob_commit_block()now takesconst char *path_prefixand passes it through toazb_block_blob_uri_commit(). This ensures that, for log-type block uploads, the commit request targets the same blob path used for the individual blocks.plugins/out_azure_blob/azure_blob.c (9)
34-38: New includes correctly cover record accessor and string helpersIncluding
<fluent-bit/flb_record_accessor.h>and<string.h>is appropriate now thatazb_resolve_path()uses record accessors and functions likestrstr,snprintf, andmemmove.
180-254: UUID placeholder replacement is well-structured and ownership-safe
azb_replace_uuid():
- Counts
$UUIDoccurrences first, bails out early if there are none, and reuses the original SDS in that case.- Allocates a correctly-sized result (
result_len + 1), copies segments around each token, and replaces each with the same 8‑char random suffix.- Frees the original
pathSDS only after the new one is successfully constructed.This gives deterministic
$UUIDexpansion per call and keeps memory ownership clear.
349-448: strftime application now fails fast on invalid or too-long formats
azb_apply_strftime():
- Handles a provided timestamp or falls back to
flb_time_get().- Grows the buffer incrementally up to a 4KB ceiling.
- After the loop, explicitly treats
out_len == 0as an error, logs a clear message, frees all temporaries, and returnsNULLinstead of silently producing an empty path.This addresses the earlier concern where bad formats would quietly erase the prefix. Memory handling on all paths looks correct.
453-481: Slash trimming helper is simple and robust
azb_trim_slashes()removes leading and trailing/in place while updating the SDS length, preventing double separators when concatenating with container URIs. It behaves sensibly for edge cases like all‑slash or empty paths.
483-607: azb_resolve_path() composes templated paths safely and defensivelyThe path resolution pipeline:
- Exits quickly when
path_templating_enabledis false orctx->pathis NULL.- Clones
ctx->pathinto a working SDS.- Applies, in order:
$UUIDexpansion (azb_replace_uuid),- custom time tokens (
%3N,%9N,%L),strftime()formatting.- Builds a temporary empty msgpack map and a
flb_record_accessorfrom the intermediate string, then usesflb_ra_translate()with a safe fallback fortag == NULL(empty string) to expand$TAG/$TAG[n].- Trims leading/trailing slashes and treats an empty final string as “no prefix”, returning success with
*out_path == NULL.Error handling is consistent: every intermediate allocation (SDS, RA, msgpack buffers) is freed on failure, and callers can reliably distinguish “no template” from “hard failure” via the return code and
*out_path.
609-631: azb_create_blob_with_tag correctly shares path resolution with create_blob
azb_create_blob_with_tag():
- Resolves
prefixviaazb_resolve_path(ctx, tag, tag_len, NULL, &prefix).- Always destroys
prefixafter callingcreate_blob(), and propagates errors viaFLB_RETRY.This helper keeps
create_blob()unaware of tags/templating while ensuring blob creation uses the exact same resolved prefix as uploads.
633-713: create_blob()’s new path_prefix-aware implementation and cleanup look correctKey points in the updated
create_blob():
- Uses
azb_uri_create_blob(ctx, path_prefix, name)so the same resolved prefix is applied as in append/block uploads.- On HTTP‑level failures sets
status = FLB_RETRYand jumps to a singlecleanup_createblock that destroys the client, releases the connection, and freesurionce.- Early failures (no URI, no connection, no client) still bail out promptly without touching uninitialized resources.
This reduces duplication and keeps resource management clear.
715-790: delete_blob() mirrors the new cleanup pattern but always ignores path templatingThe refactored
delete_blob():
- Uses the same
status+cleanup_deletepattern ascreate_blob(), which simplifies resource cleanup.- Calls
azb_uri_create_blob(ctx, NULL, name), i.e., it never passes apath_prefix.Given
azb_effective_path()returns itspath_prefixverbatim when templating is enabled, this means deletes will always target/container/<name>regardless ofctx->pathor any templating. Today this function is only used in the BLOB buffering path (stale/aborted files), not for log blobs, but it’s worth double‑checking whether BLOB users expect deletes to honor a configured path prefix as well.
1751-1762: CREATE_BLOB handling now correctly reuses templated prefixesIn
cb_azure_blob_ingest(),ingest_all_chunks(), and the non-bufferedcb_azure_blob_flush()paths, theCREATE_BLOBbranch now calls:ret = azb_create_blob_with_tag(ctx, tag_sds, (int) flb_sds_len(tag_sds), tag_sds);(or the analogous call with
event_chunk->tag), then retriessend_blob().This ensures blob creation uses the same resolved
path_prefix(including$TAG,$UUID, and time tokens) as the subsequent upload, avoiding mismatches between the created blob location and where the data is written.Also applies to: 1886-1892, 2144-2157
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
plugins/out_azure_blob/azure_blob.c(28 hunks)plugins/out_azure_blob/azure_blob.h(3 hunks)plugins/out_azure_blob/azure_blob_appendblob.c(1 hunks)plugins/out_azure_blob/azure_blob_appendblob.h(1 hunks)plugins/out_azure_blob/azure_blob_blockblob.c(8 hunks)plugins/out_azure_blob/azure_blob_blockblob.h(1 hunks)plugins/out_azure_blob/azure_blob_conf.c(2 hunks)plugins/out_azure_blob/azure_blob_db.c(11 hunks)plugins/out_azure_blob/azure_blob_db.h(6 hunks)plugins/out_azure_blob/azure_blob_uri.c(1 hunks)plugins/out_azure_blob/azure_blob_uri.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- plugins/out_azure_blob/azure_blob.h
- plugins/out_azure_blob/azure_blob_uri.h
- plugins/out_azure_blob/azure_blob_appendblob.c
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
plugins/out_azure_blob/azure_blob_conf.c
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
plugins/out_azure_blob/azure_blob.c
🧬 Code graph analysis (6)
plugins/out_azure_blob/azure_blob_appendblob.h (1)
plugins/out_azure_blob/azure_blob_appendblob.c (1)
azb_append_blob_uri(27-53)
plugins/out_azure_blob/azure_blob_blockblob.c (2)
plugins/out_azure_blob/azure_blob_uri.c (2)
azb_uri_container(107-118)azb_effective_path(137-153)src/flb_sds.c (1)
flb_sds_printf(336-387)
plugins/out_azure_blob/azure_blob_db.c (2)
src/flb_sqldb.c (1)
flb_sqldb_close(129-149)lib/cfl/src/cfl_sds.c (2)
cfl_sds_create(113-125)cfl_sds_destroy(127-137)
plugins/out_azure_blob/azure_blob_blockblob.h (1)
plugins/out_azure_blob/azure_blob_blockblob.c (6)
azb_block_blob_blocklist_uri(34-61)azb_block_blob_uri(63-124)azb_block_blob_id_logs(171-210)azb_block_blob_id_blob(219-272)azb_block_blob_commit_block(354-393)azb_block_blob_commit_file_parts(395-460)
plugins/out_azure_blob/azure_blob_db.h (1)
plugins/out_azure_blob/azure_blob_db.c (1)
azb_db_file_oldest_ready(947-1035)
plugins/out_azure_blob/azure_blob_uri.c (1)
src/flb_sds.c (1)
flb_sds_printf(336-387)
🔇 Additional comments (20)
plugins/out_azure_blob/azure_blob_appendblob.h (1)
26-28: LGTM: Path prefix parameter added cleanly.The function signature update correctly adds
path_prefixas a const parameter betweenctxandtag, and the implementation in azure_blob_appendblob.c properly usesazb_effective_path(ctx, path_prefix)to resolve the path before URI construction.plugins/out_azure_blob/azure_blob_db.h (1)
34-34: LGTM: Path prefix schema and API changes are consistent.The SQL schema additions (path_prefix TEXT column), INSERT/SELECT updates, and function signature extensions are all consistent and properly handle the nullable path_prefix. The additive schema change maintains backward compatibility with existing databases, and the migration will be handled at runtime.
Also applies to: 57-58, 139-140, 169-169, 186-186, 223-224, 232-233
plugins/out_azure_blob/azure_blob_blockblob.h (1)
26-28: LGTM: Consistent path prefix propagation with const correctness.All function signatures correctly add
path_prefixas the first parameter afterctxand applyconstqualifiers to string parameters, improving type safety. The changes are consistent with the implementations in azure_blob_blockblob.c.Also applies to: 29-34, 38-43, 44-46
plugins/out_azure_blob/azure_blob_conf.c (2)
585-599: LGTM: Path sanitization correctly updates SDS metadata.The trailing slash removal now properly updates the SDS length metadata via
flb_sds_len_setafter truncation (Line 592), addressing the previous review concern. Thepath_templating_enabledflag is set appropriately when a non-empty path exists after sanitization.
817-817: LGTM: Proper cleanup of templating flag.The
path_templating_enabledflag is correctly reset toFLB_FALSEduring context destruction, ensuring clean teardown.plugins/out_azure_blob/azure_blob_uri.c (2)
137-153: LGTM: Path resolution logic correctly handles templating mode.The
azb_effective_pathhelper correctly returnspath_prefixwhen path templating is enabled, preventing fallback to the unresolvedctx->pathtemplate. The logic properly handles:
- NULL path_prefix (no resolution yet) → returns NULL → URI uses tag only
- Empty string path_prefix (resolved to empty) → returns "" → URI uses tag only
- Non-empty path_prefix (resolved value) → returns value → URI includes prefix
This ensures templated paths are never used directly in URIs, addressing the earlier concern about template literals appearing in blob paths.
155-181: LGTM: URI construction properly integrates effective path.The URI builder correctly uses
azb_effective_path(ctx, path_prefix)and handles both NULL and empty-string cases via theeffective_path && effective_path[0] != '\0'check, ensuring clean URIs without double slashes or template literals.plugins/out_azure_blob/azure_blob.c (6)
162-200: LGTM: Random string generation now uses cryptographically secure RNG.The refactored
generate_random_string_blobnow callsflb_random_bytesfor secure random generation and falls back to a time-seeded PRNG only if that fails. The fallback ensures robustness while the primary path provides better entropy for UUID generation.
202-276: LGTM: Token replacement functions are correct and memory-safe.The
azb_replace_uuid,azb_simple_replace, andazb_apply_time_tokensfunctions correctly:
- Count occurrences and pre-allocate result buffers
- Use
memcpyfor efficient string building- Return early when tokens are not found
- Destroy the input SDS and return the new result
- Handle allocation failures by cleaning up and returning NULL
Also applies to: 278-328, 330-369
371-470: LGTM: strftime expansion handles edge cases correctly.The
azb_apply_strftimefunction now properly treatsout_len == 0after the growth loop as an error (Lines 445-456), logging an appropriate message and returning NULL instead of silently creating an empty path. This addresses the previous review concern about malformed format strings.
505-629: LGTM: Path resolution function is robust and complete.The
azb_resolve_pathfunction correctly:
- Returns early (Lines 529-536) when templating is disabled or
ctx->pathis NULL, addressing the defensive guard concern- Synthesizes an empty tag (Lines 600-606) when the input tag is NULL to prevent
flb_ra_translatefrom receiving a NULL pointer- Applies transformations in the correct order: UUID → time tokens → strftime → record accessor
- Cleans up all temporary resources on every path
- Handles empty resolved paths (Line 622-625) by returning an empty string rather than NULL
This addresses all previously raised concerns about null safety and edge-case handling.
631-711: LGTM: Blob lifecycle functions properly handle path_prefix.Both
create_blobanddelete_blobcorrectly:
- Accept
path_prefixas a parameter- Use structured cleanup with goto labels
- Pass
path_prefixto URI construction (create) or NULL (delete, which is correct since deletion doesn't require templating)- Release resources on all paths
Also applies to: 713-788
915-1049: LGTM: send_blob correctly resolves and propagates path_prefix.The
send_blobfunction properly:
- Resolves the path when
path_prefixis not provided (Lines 937-945), callingazb_resolve_pathwith the event timestamp- Cleans up
tmp_path_prefixon all error paths- Passes the resolved
path_prefixto URI construction functions for both append and block blobs- Handles the CREATE_BLOB retry case by creating the blob with
path_prefixand re-uploading with the same prefix, ensuring consistent pathsplugins/out_azure_blob/azure_blob_db.c (5)
28-68: LGTM: Database migration is safe and well-implemented.The
ensure_path_prefix_columnfunction correctly:
- Uses
PRAGMA table_infoto detect the column's presence- Executes
ALTER TABLE ADD COLUMNonly when needed- Handles all error cases with appropriate logging
- Finishes prepared statements properly
This migration is safe for existing databases and allows graceful schema evolution.
284-288: LGTM: Migration is invoked at the right time.The
ensure_path_prefix_columncall inazb_db_openruns after enabling foreign keys and before preparing statements, ensuring the schema is up-to-date before any queries execute.
362-413: LGTM: File insertion correctly handles nullable path_prefix.The
azb_db_file_insertfunction properly bindspath_prefixto column 6 when present (Lines 385-390) or binds NULL when absent, maintaining schema consistency. The binding index is correct relative to the SQL INSERT statement defined in the header.
784-885: LGTM: File part retrieval handles path_prefix with proper cleanup.The
azb_db_file_part_get_nextfunction correctly:
- Retrieves
path_prefixfrom column 10 (Line 821)- Creates a CFL SDS string when the value is not NULL (Lines 839-844)
- Sets
*path_prefixto NULL when the column is NULL (Lines 842-844)- Cleans up all allocated strings on any allocation failure (Lines 849-873)
- Propagates the path_prefix to the caller (Line 880)
The error handling ensures no memory leaks on partial allocation failures.
947-1035: LGTM: Oldest file retrieval handles path_prefix with cascading cleanup.The
azb_db_file_oldest_readyfunction retrievespath_prefixfrom column 4 (Line 996) and follows the same robust pattern asfile_part_get_next, with proper:
- NULL handling for missing values (Lines 1009-1011)
- CFL SDS allocation (Line 998)
- Cascading cleanup on allocation failures (Lines 1000-1007)
- Initialization of all output parameters on error paths (Lines 1018-1030)
plugins/out_azure_blob/azure_blob_blockblob.c (2)
34-61: LGTM: Block blob URI functions correctly integrate effective_path.All three URI construction functions (
azb_block_blob_blocklist_uri,azb_block_blob_uri, andazb_block_blob_uri_commit) follow the same correct pattern:
- Retrieve the container URI base
- Call
azb_effective_path(ctx, path_prefix)to resolve the actual path- Conditionally format the URI with or without the resolved path
- Append SAS token when configured
The path resolution logic is consistent and handles both NULL and empty-string effective paths correctly.
Also applies to: 63-124, 126-165
354-393: LGTM: Block commit functions properly propagate path_prefix.Both
azb_block_blob_commit_blockandazb_block_blob_commit_file_partscorrectly:
- Accept
path_prefixas a const parameter- Pass it through to URI construction functions
- Maintain consistent path_prefix handling across single-block commits (line 366) and multi-part commits (line 449)
The XML payload construction in
commit_file_partsis unchanged and correctly builds the block list from part IDs.Also applies to: 395-460
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
plugins/out_azure_blob/azure_blob_uri.c (1)
167-180: Missing error handling forflb_sds_printfcalls.The
flb_sds_printfcalls at lines 170, 173, and 177 don't check return values. While this matches the existing pattern inazb_uri_container(line 116), the updatedazb_append_blob_urinow properly validates these calls. Consider adding consistent error handling:if (effective_path && effective_path[0] != '\0') { - flb_sds_printf(&uri, "/%s/%s", effective_path, tag); + if (flb_sds_printf(&uri, "/%s/%s", effective_path, tag) == -1) { + flb_sds_destroy(uri); + return NULL; + } } else { - flb_sds_printf(&uri, "/%s", tag); + if (flb_sds_printf(&uri, "/%s", tag) == -1) { + flb_sds_destroy(uri); + return NULL; + } } if (ctx->atype == AZURE_BLOB_AUTH_SAS && ctx->sas_token) { - flb_sds_printf(&uri, "?%s", ctx->sas_token); + if (flb_sds_printf(&uri, "?%s", ctx->sas_token) == -1) { + flb_sds_destroy(uri); + return NULL; + } }plugins/out_azure_blob/azure_blob.c (2)
282-328: Consider replacing all occurrences, not just the first.
azb_simple_replaceonly replaces the first occurrence of the token. While this works for%3N,%9N, and%Lif users only use them once, paths likelogs/%3N/%3Nwould only have the first%3Nexpanded.If multiple occurrences should be supported, consider a loop similar to
azb_replace_uuid. If single replacement is intentional, adding a brief comment would clarify this design choice.
1597-1600: Minor: Redundant assignment after destroy.Setting
path_prefix = NULLaftercfl_sds_destroyis harmless but unnecessary since the variable goes out of scope. This pattern repeats at lines 1643-1645, 1667-1669, 1691-1693, 1715-1718. Consider removing the redundant assignments for consistency with other cleanup patterns in this file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
plugins/out_azure_blob/azure_blob.c(28 hunks)plugins/out_azure_blob/azure_blob.h(3 hunks)plugins/out_azure_blob/azure_blob_appendblob.c(1 hunks)plugins/out_azure_blob/azure_blob_appendblob.h(1 hunks)plugins/out_azure_blob/azure_blob_blockblob.c(8 hunks)plugins/out_azure_blob/azure_blob_blockblob.h(1 hunks)plugins/out_azure_blob/azure_blob_conf.c(2 hunks)plugins/out_azure_blob/azure_blob_db.c(11 hunks)plugins/out_azure_blob/azure_blob_db.h(6 hunks)plugins/out_azure_blob/azure_blob_uri.c(1 hunks)plugins/out_azure_blob/azure_blob_uri.h(1 hunks)tests/internal/CMakeLists.txt(2 hunks)tests/internal/azure_blob_path.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/internal/CMakeLists.txt
- plugins/out_azure_blob/azure_blob_appendblob.h
- plugins/out_azure_blob/azure_blob_uri.h
- plugins/out_azure_blob/azure_blob_conf.c
- plugins/out_azure_blob/azure_blob.h
- tests/internal/azure_blob_path.c
- plugins/out_azure_blob/azure_blob_blockblob.h
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
plugins/out_azure_blob/azure_blob.c
🧬 Code graph analysis (6)
plugins/out_azure_blob/azure_blob_uri.c (1)
src/flb_sds.c (1)
flb_sds_printf(336-387)
plugins/out_azure_blob/azure_blob_appendblob.c (2)
plugins/out_azure_blob/azure_blob_uri.c (2)
azb_uri_container(107-118)azb_effective_path(137-153)src/flb_sds.c (2)
flb_sds_printf(336-387)flb_sds_destroy(389-399)
plugins/out_azure_blob/azure_blob.c (6)
src/flb_sds.c (4)
flb_sds_create_size(92-95)flb_sds_destroy(389-399)flb_sds_create_len(58-76)flb_sds_printf(336-387)src/flb_record_accessor.c (2)
flb_ra_create(271-358)flb_ra_destroy(232-248)plugins/out_azure_blob/azure_blob_uri.c (1)
azb_uri_create_blob(155-181)plugins/out_azure_blob/azure_blob_appendblob.c (1)
azb_append_blob_uri(27-62)plugins/out_azure_blob/azure_blob_blockblob.c (1)
azb_block_blob_uri(63-124)plugins/out_azure_blob/azure_blob_db.c (1)
azb_db_file_insert(362-413)
plugins/out_azure_blob/azure_blob_db.h (1)
plugins/out_azure_blob/azure_blob_db.c (1)
azb_db_file_oldest_ready(947-1035)
plugins/out_azure_blob/azure_blob_db.c (2)
src/flb_sqldb.c (1)
flb_sqldb_close(129-149)lib/cfl/src/cfl_sds.c (2)
cfl_sds_create(113-125)cfl_sds_destroy(127-137)
plugins/out_azure_blob/azure_blob_blockblob.c (1)
plugins/out_azure_blob/azure_blob_uri.c (2)
azb_uri_container(107-118)azb_effective_path(137-153)
🔇 Additional comments (15)
plugins/out_azure_blob/azure_blob_appendblob.c (1)
27-62: LGTM! Robust error handling for URI construction.The function properly validates
flb_sds_printfreturn values at all three construction sites (lines 42, 48, 55) and correctly cleans up theuribefore returningNULLon failure. Theazb_effective_pathintegration aligns with the path templating design.plugins/out_azure_blob/azure_blob_uri.c (1)
137-153: LGTM! Clear logic for effective path resolution.The helper correctly prioritizes:
- Returns
path_prefixwhen templating is enabled (resolved path takes precedence)- Returns
path_prefixif explicitly provided and non-empty- Falls back to
ctx->pathfor legacy behaviorThis design ensures resolved templates are used while maintaining backward compatibility.
plugins/out_azure_blob/azure_blob_db.h (3)
28-40: LGTM! Schema extended correctly for path_prefix.The
path_prefix TEXTcolumn is properly nullable, allowing existing rows to have NULL values during migration. The column placement and type are appropriate.
56-58: LGTM! INSERT statement correctly includes path_prefix.The parameter order (source, destination, path, size, created, path_prefix) matches the bind order in the implementation at
azure_blob_db.clines 379-390.
129-154: LGTM! Query includes path_prefix for part retrieval.The
f.path_prefixat column index 10 aligns with the implementation'ssqlite3_column_text(ctx->stmt_get_next_file_part, 10)call.plugins/out_azure_blob/azure_blob.c (4)
162-200: LGTM! Robust random string generation with fallback.The implementation correctly:
- Uses
flb_random_bytesfor cryptographic quality randomness- Falls back to a time-seeded LCG if random bytes fail
- Properly allocates and frees the random buffer
- Handles edge case of
length == 0
206-276: LGTM! Correct $UUID replacement with consistent suffix.The function properly:
- Returns early if no occurrences found (avoids unnecessary allocation)
- Generates a single random suffix reused for all
$UUIDoccurrences- Correctly calculates
result_lenaccounting for token size difference- Handles memory cleanup by destroying the original path and returning the new result
508-629: LGTM! Comprehensive path resolution with proper null safety.The function correctly addresses the past review concerns:
- Line 533-536: Guards against
ctx->path == NULLwhen templating is enabled- Lines 599-609: Synthesizes empty tag when
tag == NULLforflb_ra_translate- Proper cleanup of msgpack resources and intermediate SDS strings on all error paths
- Returns 0 with
*out_path = expanded(empty string) when template resolves to empty, which is a valid result
1533-1543: LGTM! Correctly handles legacy DB rows without path_prefix.The fallback logic at lines 1539-1541 properly addresses the past review concern: it only uses
ctx->pathas fallback whenpath_templating_enabled == FLB_FALSE, preventing unresolved template tokens from leaking into commit URIs for legacy rows.plugins/out_azure_blob/azure_blob_db.c (3)
30-68: LGTM! Safe schema migration for path_prefix column.The migration approach is correct:
- Uses
PRAGMA table_infoto detect existing column (avoids duplicate ADD COLUMN error)ALTER TABLE ADD COLUMNin SQLite allows NULL for existing rows- Properly finalizes the prepared statement before returning
- Called during
azb_db_openbefore preparing statements, ensuring the column exists
839-864: LGTM! Correct handling of nullable path_prefix from DB.The logic properly:
- Creates
local_prefixonly whentmp_prefixis non-NULL (lines 839-844)- Validates allocation succeeded when
tmp_prefixwas provided (line 849)- Cleans up
local_prefixon subsequent allocation failures (lines 858-860)- Propagates NULL when the column value is NULL (line 880)
994-1011: LGTM! Consistent path_prefix handling in oldest_ready query.Same correct pattern as
azb_db_file_part_get_next:
- Creates SDS only when column is non-NULL
- Proper cleanup chain on allocation failure
- Sets
*path_prefix = NULLfor NULL column valuesplugins/out_azure_blob/azure_blob_blockblob.c (3)
34-61: LGTM! Blocklist URI construction now uses path_prefix.The function correctly integrates
azb_effective_pathto determine the path component, addressing the past review concern about commit requests ignoring the configured path prefix.
63-124: LGTM! Block blob URI properly handles path_prefix.The function correctly:
- Uses
azb_effective_pathfor path resolution- Handles both
ms > 0andms == 0cases with and without effective_path- Properly cleans up
encoded_blockidbefore returning
395-460: LGTM! Commit file parts now receives path_prefix from caller.The function signature update at line 397 (
const char *path_prefix) and the call toazb_block_blob_blocklist_uri(ctx, path_prefix, path)at line 449 ensures the commit request targets the same path where parts were uploaded, addressing the past review concern about path drift between upload and commit.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
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_azure_blob/azure_blob_uri.c (1)
169-178: Missing error handling forflb_sds_printfcalls.The
flb_sds_printfcalls at lines 170, 173, and 177 don't check for failure. If allocation fails, the function returns a malformed URI. This is inconsistent with the error handling inazb_append_blob_uri.Apply this diff to add error handling:
if (effective_path && effective_path[0] != '\0') { - flb_sds_printf(&uri, "/%s/%s", effective_path, tag); + if (flb_sds_printf(&uri, "/%s/%s", effective_path, tag) == NULL) { + flb_sds_destroy(uri); + return NULL; + } } else { - flb_sds_printf(&uri, "/%s", tag); + if (flb_sds_printf(&uri, "/%s", tag) == NULL) { + flb_sds_destroy(uri); + return NULL; + } } if (ctx->atype == AZURE_BLOB_AUTH_SAS && ctx->sas_token) { - flb_sds_printf(&uri, "?%s", ctx->sas_token); + if (flb_sds_printf(&uri, "?%s", ctx->sas_token) == NULL) { + flb_sds_destroy(uri); + return NULL; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
plugins/out_azure_blob/azure_blob.c(28 hunks)plugins/out_azure_blob/azure_blob.h(3 hunks)plugins/out_azure_blob/azure_blob_appendblob.c(1 hunks)plugins/out_azure_blob/azure_blob_appendblob.h(1 hunks)plugins/out_azure_blob/azure_blob_blockblob.c(8 hunks)plugins/out_azure_blob/azure_blob_blockblob.h(1 hunks)plugins/out_azure_blob/azure_blob_conf.c(2 hunks)plugins/out_azure_blob/azure_blob_db.c(11 hunks)plugins/out_azure_blob/azure_blob_db.h(6 hunks)plugins/out_azure_blob/azure_blob_uri.c(1 hunks)plugins/out_azure_blob/azure_blob_uri.h(1 hunks)tests/internal/CMakeLists.txt(2 hunks)tests/internal/azure_blob_path.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- plugins/out_azure_blob/azure_blob_conf.c
- plugins/out_azure_blob/azure_blob.h
- plugins/out_azure_blob/azure_blob_db.h
- tests/internal/azure_blob_path.c
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
tests/internal/CMakeLists.txt
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
plugins/out_azure_blob/azure_blob.c
🧬 Code graph analysis (7)
plugins/out_azure_blob/azure_blob_uri.h (1)
plugins/out_azure_blob/azure_blob_uri.c (2)
azb_uri_create_blob(155-181)azb_effective_path(137-153)
plugins/out_azure_blob/azure_blob_uri.c (1)
src/flb_sds.c (1)
flb_sds_printf(336-387)
plugins/out_azure_blob/azure_blob_appendblob.c (2)
plugins/out_azure_blob/azure_blob_uri.c (2)
azb_uri_container(107-118)azb_effective_path(137-153)src/flb_sds.c (2)
flb_sds_printf(336-387)flb_sds_destroy(389-399)
plugins/out_azure_blob/azure_blob_db.c (2)
src/flb_sqldb.c (1)
flb_sqldb_close(129-149)lib/cfl/src/cfl_sds.c (2)
cfl_sds_create(113-125)cfl_sds_destroy(127-137)
plugins/out_azure_blob/azure_blob_appendblob.h (1)
plugins/out_azure_blob/azure_blob_appendblob.c (1)
azb_append_blob_uri(27-62)
plugins/out_azure_blob/azure_blob.c (6)
src/flb_sds.c (3)
flb_sds_create_size(92-95)flb_sds_destroy(389-399)flb_sds_printf(336-387)src/flb_record_accessor.c (3)
flb_ra_create(271-358)flb_ra_translate(628-633)flb_ra_destroy(232-248)plugins/out_azure_blob/azure_blob_uri.c (1)
azb_uri_create_blob(155-181)plugins/out_azure_blob/azure_blob_appendblob.c (1)
azb_append_blob_uri(27-62)plugins/out_azure_blob/azure_blob_blockblob.c (1)
azb_block_blob_uri(63-124)plugins/out_azure_blob/azure_blob_db.c (2)
azb_db_file_insert(362-413)azb_db_file_oldest_ready(947-1035)
plugins/out_azure_blob/azure_blob_blockblob.c (2)
plugins/out_azure_blob/azure_blob_uri.c (2)
azb_uri_container(107-118)azb_effective_path(137-153)src/flb_sds.c (1)
flb_sds_printf(336-387)
🔇 Additional comments (24)
tests/internal/CMakeLists.txt (2)
62-67: LGTM!The conditional inclusion of
azure_blob_path.cfollows the established pattern used for other feature-specific tests in this file.
227-229: LGTM!The conditional linking of the Azure Blob plugin to its test target follows the same pattern as other plugin-specific test linking (e.g.,
flb-awsat lines 223-225).plugins/out_azure_blob/azure_blob_appendblob.h (1)
26-28: LGTM!The updated signature correctly adds
path_prefixand appliesconstqualifiers to both string parameters, matching the implementation.plugins/out_azure_blob/azure_blob_uri.h (1)
30-34: LGTM!The updated
azb_uri_create_blobsignature and newazb_effective_pathdeclaration align with their implementations and maintain proper const correctness.plugins/out_azure_blob/azure_blob_uri.c (1)
137-153: LGTM!The
azb_effective_pathhelper correctly prioritizes the resolvedpath_prefixwhen templating is enabled or when a non-empty prefix is provided, falling back toctx->pathotherwise.plugins/out_azure_blob/azure_blob_blockblob.h (1)
26-46: LGTM!The updated function signatures consistently add
path_prefixparameters and applyconstqualifiers, aligning with the path-prefix-aware design implemented across the Azure Blob module.plugins/out_azure_blob/azure_blob_db.c (5)
30-68: LGTM: Schema migration for path_prefix column is well-implemented.The
ensure_path_prefix_columnfunction correctly:
- Uses parameterized PRAGMA query to inspect schema
- Properly finalizes the statement before ALTER TABLE
- Handles both "column exists" and "column missing" cases
- Returns appropriate error codes
284-288: LGTM: Schema migration called at appropriate point in database open.The migration is correctly placed after table creation and foreign keys enablement, ensuring the schema is ready before prepared statements are compiled.
385-390: LGTM: path_prefix binding handles NULL correctly.The conditional binding with
sqlite3_bind_nullfor NULL path_prefix values is the correct approach for optional columns.
839-864: Allocation failure check is correct but cleanup could be simplified.The error handling correctly frees all allocated resources when
cfl_sds_createfails forlocal_prefix. The logic properly handles the case wheretmp_prefixis non-NULL but allocation fails.
994-1011: LGTM: path_prefix retrieval with proper NULL handling and cleanup.The code correctly handles the case where
path_prefixcolumn may be NULL in the database, and properly destroys previously allocated resources on allocation failure.plugins/out_azure_blob/azure_blob.c (8)
158-200: LGTM: Improved RNG implementation with proper fallback.The updated
generate_random_string_blobnow usesflb_random_byteswith a sensible fallback to a time-seeded LCG when the secure RNG fails. Memory is properly allocated and freed.
206-276: LGTM: UUID replacement correctly handles memory ownership.The function properly:
- Returns the original path unchanged when no
$UUIDtokens exist- Generates a single random suffix for all occurrences (consistent behavior)
- Destroys the input
pathand returns a new SDS- Handles allocation failures by destroying the input and returning NULL
282-328:azb_simple_replaceonly replaces the first occurrence.The function is documented to replace "the first occurrence" but
azb_apply_time_tokenscalls it sequentially for%3N,%9N, and%L. If a user's path contains multiple occurrences of the same token (e.g.,/%3N/%3N/), only the first will be replaced.Consider whether this is intentional or if all occurrences should be replaced like
azb_replace_uuiddoes. If intentional, the current behavior is fine for typical use cases.
445-456: Good: Previous review feedback addressed - strftime zero-length now errors.The code now correctly treats
out_len == 0after the growth loop as an explicit error, logging the issue and returning NULL instead of silently producing an empty path.
508-629: Good: Previous review feedback addressed - NULL guards and empty tag handling.The function now:
- Returns early if
ctx->pathis NULL (line 533-536)- Synthesizes an empty tag when
tagis NULL (lines 599-606)This addresses the edge cases identified in past reviews.
937-945: Double-free risk ifazb_resolve_pathfails after allocatingtmp_path_prefix.When
azb_resolve_pathreturns non-zero,tmp_path_prefixis destroyed at line 940-941. However,azb_resolve_pathalready sets*out_path = NULLon early returns (lines 527, 531, 536) or destroys/NULLs the path on error paths. The checkif (tmp_path_prefix)is safe, but the pattern is slightly inconsistent.The current code is correct because the destroy paths within
azb_resolve_pathdon't set*out_pathbefore returning -1 on some paths, but the NULL check here protects against that.
1533-1541: Good: Previous review feedback addressed - legacy DB rows handled correctly.The fallback to
ctx->pathis now correctly gated byctx->path_templating_enabled == FLB_FALSE, preventing unresolved template tokens from leaking into commit URIs for legacy database rows.
2193-2196: The comment at line 2196 "send_blob handles CREATE_BLOB internally" is accurate. Thesend_blobfunction (lines 1021-1030) does handle theCREATE_BLOBcase by callingcreate_bloband retrying the upload. The buffering-enabled path (lines 2113-2128) correctly delegates tosend_blobwithout redundantCREATE_BLOBhandling. No issues found.plugins/out_azure_blob/azure_blob_blockblob.c (5)
34-61: Good: Past review feedback addressed - blocklist URI now uses path_prefix.The function now correctly uses
azb_effective_path(ctx, path_prefix)to determine the effective path, addressing the previous issue where block list commits ignored the configured path prefix.
63-124: LGTM: Block blob URI construction correctly integrates path_prefix.The function properly:
- Uses
azb_effective_pathto resolve the effective path- Handles both with-path and without-path URI construction
- Maintains correct SAS token handling
126-165: LGTM: Commit URI construction correctly uses path_prefix.The function follows the same pattern as other URI builders, correctly using
azb_effective_pathfor path resolution.
354-393: LGTM: Commit block correctly propagates path_prefix.The function properly passes
path_prefixthrough toazb_block_blob_uri_commit.
395-460: Good: Past review feedback addressed - file parts commit uses path_prefix.The function now accepts
path_prefixand passes it toazb_block_blob_blocklist_uriat line 449, ensuring the commit targets the same path where parts were uploaded.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
plugins/out_azure_blob/azure_blob_appendblob.h (1)
26-28: Minor formatting inconsistency.The signature is correct, but lines 27-28 use tabs for indentation while other headers in this PR (e.g.,
azure_blob_uri.h) use spaces for parameter alignment. Consider aligning with the project's formatting conventions for consistency.flb_sds_t azb_append_blob_uri(struct flb_azure_blob *ctx, - const char *path_prefix, - const char *tag); + const char *path_prefix, + const char *tag);plugins/out_azure_blob/azure_blob_uri.c (1)
116-117: Missing error check forflb_sds_printfinazb_uri_container.While not part of the new changes, the error handling pattern established in the updated
azb_uri_create_blobfunction is not applied here. Ifflb_sds_printffails, the function returns a potentially incomplete URI.- flb_sds_printf(&uri, "%s%s", ctx->base_uri, ctx->container_name); - return uri; + if (flb_sds_printf(&uri, "%s%s", ctx->base_uri, ctx->container_name) == NULL) { + flb_sds_destroy(uri); + return NULL; + } + return uri;plugins/out_azure_blob/azure_blob.c (1)
202-369: UUID and time token expansion logic is correct; only first‑occurrence replacement is a small behavioral nit.
azb_replace_uuid:
- Counts
$UUIDoccurrences, generates a single 8‑char suffix, and replaces all occurrences in one pass with correct size calculations and ownership transfer (old SDS destroyed, new one returned). This ensures all$UUIDtokens in a path share the same value, which is desirable.azb_simple_replaceandazb_apply_time_tokens:
%3N,%9N, and%Lare expanded fromstruct flb_timeusing fixed‑width zero‑padded formats; memory ownership and error paths are handled cleanly.- Note that
azb_simple_replaceonly replaces the first occurrence of each token. If a user ever repeats%3N/%9N/%Lmultiple times in one template, only the first will be expanded. If you want stricter S3‑style parity, consider loopingazb_simple_replaceuntil the token is no longer found, but this is an optional enhancement, not a blocker.plugins/out_azure_blob/azure_blob_db.c (1)
28-68: Schema migration helper forpath_prefixis correct; consider finalizing on prepare error.
ensure_path_prefix_column:
- Uses
PRAGMA table_infoto detect whetherout_azure_blob_filesalready has apath_prefixcolumn.- If missing, issues a single
ALTER TABLE ... ADD COLUMN path_prefix TEXT;and logs on failure.- Returns 0 when the column exists or is added successfully, and
azb_db_openwill close the DB and abort setup if this step fails.The only minor nit is that when
sqlite3_prepare_v2fails,stmtisn’t finalized before returning. In practice this is a one‑off statement at startup, so it’s very low impact, but you could callsqlite3_finalize(stmt)in theret != SQLITE_OKbranch for completeness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
plugins/out_azure_blob/azure_blob.c(28 hunks)plugins/out_azure_blob/azure_blob.h(3 hunks)plugins/out_azure_blob/azure_blob_appendblob.c(1 hunks)plugins/out_azure_blob/azure_blob_appendblob.h(1 hunks)plugins/out_azure_blob/azure_blob_blockblob.c(8 hunks)plugins/out_azure_blob/azure_blob_blockblob.h(1 hunks)plugins/out_azure_blob/azure_blob_conf.c(2 hunks)plugins/out_azure_blob/azure_blob_db.c(11 hunks)plugins/out_azure_blob/azure_blob_db.h(6 hunks)plugins/out_azure_blob/azure_blob_uri.c(1 hunks)plugins/out_azure_blob/azure_blob_uri.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/out_azure_blob/azure_blob.h
- plugins/out_azure_blob/azure_blob_conf.c
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
plugins/out_azure_blob/azure_blob.c
🧬 Code graph analysis (7)
plugins/out_azure_blob/azure_blob_uri.h (1)
plugins/out_azure_blob/azure_blob_uri.c (2)
azb_uri_create_blob(155-190)azb_effective_path(137-153)
plugins/out_azure_blob/azure_blob_appendblob.c (2)
plugins/out_azure_blob/azure_blob_uri.c (2)
azb_uri_container(107-118)azb_effective_path(137-153)src/flb_sds.c (1)
flb_sds_printf(336-387)
plugins/out_azure_blob/azure_blob_db.h (1)
plugins/out_azure_blob/azure_blob_db.c (1)
azb_db_file_oldest_ready(947-1035)
plugins/out_azure_blob/azure_blob_blockblob.c (1)
plugins/out_azure_blob/azure_blob_uri.c (2)
azb_uri_container(107-118)azb_effective_path(137-153)
plugins/out_azure_blob/azure_blob.c (4)
plugins/out_azure_blob/azure_blob_uri.c (1)
azb_uri_create_blob(155-190)plugins/out_azure_blob/azure_blob_appendblob.c (1)
azb_append_blob_uri(27-62)plugins/out_azure_blob/azure_blob_blockblob.c (1)
azb_block_blob_uri(63-124)plugins/out_azure_blob/azure_blob_db.c (1)
azb_db_file_insert(362-413)
plugins/out_azure_blob/azure_blob_uri.c (1)
src/flb_sds.c (2)
flb_sds_printf(336-387)flb_sds_destroy(389-399)
plugins/out_azure_blob/azure_blob_appendblob.h (1)
plugins/out_azure_blob/azure_blob_appendblob.c (1)
azb_append_blob_uri(27-62)
🔇 Additional comments (24)
plugins/out_azure_blob/azure_blob_uri.h (1)
30-34: LGTM!The updated function signatures properly introduce the
path_prefixparameter and the newazb_effective_pathhelper. Theconstqualifiers on input parameters are appropriate for read-only string arguments.plugins/out_azure_blob/azure_blob_db.h (5)
56-58: LGTM!The INSERT statement correctly includes
path_prefixin both the column list and VALUES clause, maintaining proper parameter binding order.
139-140: LGTM!The SELECT query correctly retrieves
path_prefix(column index 10) alongsidedestination(column index 9) for use in URI construction.
168-176: LGTM!The query correctly includes
f.path_prefixin the SELECT list (column index 4), which aligns with the implementation inazb_db_file_oldest_readythat retrieves it viasqlite3_column_text(..., 4).
182-187: LGTM!The function signatures are correctly updated to propagate
path_prefix:
azb_db_file_insert: acceptspath_prefixas input for storageazb_db_file_part_get_next: returnspath_prefixas output for URI constructionazb_db_file_oldest_ready: returnspath_prefixas output for commit operationsAlso applies to: 217-224, 231-233
28-40: Migration for existing databases is properly handled via ALTER TABLE.The
ensure_path_prefix_column()function checks if the column exists before adding it, and the ALTER TABLE statement with error handling executes during database initialization inazb_db_open(). New installations receive the column directly inCREATE TABLE IF NOT EXISTS, while existing databases have it added on upgrade. The nullable TEXT column maintains backward compatibility.plugins/out_azure_blob/azure_blob_uri.c (2)
137-153: LGTM!The
azb_effective_pathhelper correctly implements the path resolution priority:
- When templating is enabled, always use the resolved
path_prefix- When templating is disabled but
path_prefixis provided, use it- Fall back to
ctx->pathonly when no override is providedThis allows callers to pass NULL or empty
path_prefixwhen templating resolves to nothing, resulting in URIs without a path prefix.
155-189: LGTM!The updated
azb_uri_create_blobproperly:
- Uses
azb_effective_pathto determine the path component- Checks all
flb_sds_printfreturn values for NULL and cleans up on failure- Handles both path-prefixed and non-prefixed URI formats correctly
plugins/out_azure_blob/azure_blob_appendblob.c (1)
27-61: LGTM!The implementation correctly:
- Adds the
path_prefixparameter and usesazb_effective_pathfor consistent path resolution- Applies
constqualifiers to input parameters- Checks all
flb_sds_printfreturn values for NULL (addressing the previous review feedback about incorrect-1checks)- Properly cleans up the URI on failure before returning NULL
The structure mirrors
azb_uri_create_blob, providing a consistent pattern across URI construction functions.plugins/out_azure_blob/azure_blob_blockblob.h (1)
26-46: Header prototypes match implementation and correctly threadpath_prefix.The updated function signatures (adding
const char *path_prefixand tightening const‑correctness) align with the definitions inazure_blob_blockblob.cand with the new path templating flow; no issues from an API or ABI standpoint.plugins/out_azure_blob/azure_blob.c (10)
158-200: Random string generator is robust and well‑scoped.
generate_random_string_blobcorrectly prefersflb_random_bytesand only falls back to a local PRNG on failure, uses a bounded alphanumeric charset, handles allocation failure by emitting a deterministic placeholder, and always NUL‑terminates within a caller‑supplied buffer. No issues here.
371-470:azb_apply_strftimenow fails fast on invalid or too‑long format strings and cleans up correctly.
- Uses either the event timestamp or current time, converts via
gmtime_r, and grows the buffer up to a sane cap while callingstrftime.- The new
if (out_len == 0)block logs an explicit error, frees the temporary buffer, and destroys the input SDS before returningNULL, so malformed or overlong templates no longer silently collapse to an empty prefix.- All allocation failures (
flb_malloc,flb_realloc,flb_sds_create_len) free intermediate resources and preserve existing errno logging.This behavior is sound and fixes the earlier “empty prefix on bad format” problem.
505-629:azb_resolve_path: sequencing, NULL‑safety, and ownership are handled carefully.
- Early‑exit guards are correct:
out_pathis validated and zeroed.- If templating is disabled or
ctx->pathis NULL, it returns 0 with*out_path == NULL, letting callers treat this as “no prefix”.- Transformation pipeline is coherent:
- Clone
ctx->pathinto a private SDS.- Apply
$UUID(single shared suffix), then%3N/%9N/%L, thenstrftimeusing the provided timestamp or current time.- Build a temporary record accessor from the partially processed path and use it to expand
$TAG/$TAG[n]against an empty record and the given tag.- Trim leading/trailing slashes and return the final SDS (including the intentional zero‑length case).
tag == NULLis handled by substituting an empty string soflb_ra_translatenever sees a NULL pointer.- All allocations (SDS, msgpack_sbuffer, unpacked object, RA) are released on all error paths; the caller owns only
*out_pathon success.Overall this function looks correct, and the behavior when templating is disabled (returning 0 without allocating) matches expectations.
631-711:create_blobcorrectly incorporatespath_prefixand unifies error handling.
- URI construction now goes through
azb_uri_create_blob(ctx, path_prefix, name), which in turn usesazb_effective_path, so both templated prefixes and non‑templatedctx->pathare honored.- All network and HTTP failure paths set
statustoFLB_RETRYand jump to a singlecleanup_createlabel that destroys the HTTP client, releases the connection, and frees the URI if allocated.- On success, it logs the created blob after stripping
&sig=from the URI to avoid leaking SAS signatures.No functional issues spotted; this is a solid refactor.
713-788:delete_blobis now consistent withcreate_blobin cleanup behavior.
- Still targets the container root (
path_prefix == NULL), which matches previous semantics for deleting blobs by name.- Uses a
statusvariable andcleanup_deletelabel to guarantee the HTTP client, upstream connection, and URI are destroyed exactly once in all branches.- Returns
FLB_RETRYfor I/O failures and non‑201 statuses while logging details, which is appropriate for the caller’s retry logic.No correctness issues here.
915-1049:send_blob: path templating is resolved once per call and reused across retries and commits.Key points:
Single resolution of templated prefix
- Accepts an optional
resolved_path_prefix. When NULL, it:
- Captures
nowonce withflb_time_get,- Calls
azb_resolve_path(ctx, tag, tag_len, &now, &tmp_path_prefix),- Reuses
path_prefix = tmp_path_prefixfor URI creation, CREATE_BLOB, and commit.- This avoids re‑expanding
$UUID/time tokens and RA templates on CREATE_BLOB retries, fixing the historical “path drift” issue.URI construction
- Append blobs:
azb_append_blob_uri(ctx, path_prefix, tag).- Block blobs (logs):
azb_block_blob_uri(ctx, path_prefix, tag, block_id, ms, random_str).- Block blobs (BLOBs):
azb_block_blob_uri(ctx, path_prefix, name, block_id, 0, random_str).- All use
path_prefixconsistently, whether templated or static.CREATE_BLOB handling
- On initial
CREATE_BLOBfromhttp_send_blob, callscreate_blob(ctx, path_prefix, name)then re‑callshttp_send_blobwith the same URI.- For log events, both the initial success path and the retry path commit via
azb_block_blob_commit_block(ctx, path_prefix, block_id, tag, ms, random_str), ensuring the block list is always finalized.Resource management
tmp_path_prefixis destroyed on all early returns where it may be set.ref_name,uri,generated_random_string, andblock_idare freed in all success/error paths.- Compression buffers are still owned/freed inside
http_send_blob;send_blobno longer attempts to free them directly (thepayload_buf != databranch is effectively a no‑op, but harmless).Overall, the function now cleanly separates “resolve template once” from “send and possibly create+retry”, which is the right model for templated paths.
1308-1384:process_blob_chunk: BLOB file entries correctly capture the resolved prefix into the DB.
- For each BLOB log event, you:
- Extract
source,file_path, andfile_sizeviaflb_input_blob_file_get_info.- Resolve the path prefix with
azb_resolve_path(ctx, event_chunk->tag, tag_len, &log_event.timestamp, &path_prefix).- Insert a DB row via
azb_db_file_insert(ctx, source, ctx->real_endpoint, file_path, path_prefix, file_size).- If path resolution fails, you log and skip that file while cleaning up
path_prefix,file_path, andsource.- On success, you immediately destroy
path_prefix(the DB stores its own copy) and then register parts.This wires
path_prefixinto the buffering layer correctly so later resume/commit logic can use the same resolved prefix.
1405-1721: Buffered BLOB upload/resume path now uses the storedpath_prefixconsistently for both PUT blocks and commit.
azb_db_file_oldest_readynow returns apath_prefixSDS, which you:
- Assign to
commit_prefix = path_prefix.- For non‑templated configurations (
ctx->path_templating_enabled == FLB_FALSE) andcommit_prefix == NULL, fall back toctx->path. This preserves pre‑templating behavior for legacy rows where no prefix was stored.- For templated configurations, you intentionally do not fall back to the raw template string, avoiding commits to unresolved paths like
logs/$TAG/%Y/%m/%d.
azb_block_blob_commit_file_partsis then called with thiscommit_prefix, and the returnedpath_prefixSDS is destroyed exactly once afterward.When uploading individual parts:
azb_db_file_part_get_nextnow returnspart_path_prefix, which you pass asresolved_path_prefixintosend_blob.- For new rows, this is the resolved prefix captured at ingestion time;
send_blobreuses it without re‑templating.- For legacy rows without a stored prefix:
- With templating disabled,
azb_effective_pathfalls back toctx->path, matching past behavior.- With templating enabled, the lack of a stored prefix means the plugin intentionally uploads/commits at the container root, avoiding use of an unresolved template.
All new SDS values (
path_prefix,part_path_prefix) are freed in every success and error path (including early returns on endpoint mismatch, read failures, and DB errors).This end‑to‑end threading of
path_prefixthrough resume and commit looks correct and is careful about both legacy and templated configurations.
1750-1971: Ingestion helpers now rely onsend_blobfor templated paths without re‑implementing path logic.
- Both
cb_azure_blob_ingestandingest_all_chunks:
- Construct payloads and tags as before.
- Call
send_blob(..., tag, flb_sds_len(tag), NULL, payload, payload_size)for log events, lettingsend_blobresolve the prefix and handle CREATE_BLOB internally.- No additional path resolution is performed here, which prevents accidental double‑templating or drift between ingest and retry paths.
These call sites now correctly delegate path templating to the central helper.
2026-2216: Flush path for both buffered and non‑buffered logs correctly defers path templating tosend_blob.
- Buffered branch:
- After assembling
final_payload, callssend_blob(config, i_ins, ctx, FLB_EVENT_TYPE_LOGS, ctx->btype, (char *) tag_name, 0, (char *) tag_name, tag_len, NULL, final_payload, final_payload_size);.- Non‑buffered branch:
- After
azure_blob_format, callssend_blobwith the event tag andresolved_path_prefix == NULL.- The explicit comment noting “send_blob handles CREATE_BLOB internally” matches the current implementation: no outer
CREATE_BLOBhandling remains here.This keeps the flush logic path‑agnostic and avoids redundant retry/create code.
plugins/out_azure_blob/azure_blob_db.c (3)
362-413:azb_db_file_insertnow persistspath_prefixcleanly.
- Adds a
path_prefixargument and binds it as the 6th parameter:
- Non‑NULL prefixes are stored as TEXT.
- A NULL pointer results in a SQL NULL, so legacy and non‑templated cases are distinguishable.
- Locking (
azb_db_lock/unlock) and error handling onsqlite3_stepremain unchanged and correct.This wiring matches how the rest of the code treats optional prefixes.
784-885:azb_db_file_part_get_nextcorrectly returns destination andpath_prefixwith safe cleanup.
- Initializes
*file_path,*destination, and*path_prefixto NULL before any DB work.- Reads:
- Path string from column 6,
- Destination from column 9,
- Optional
path_prefixfrom column 10.- Allocates:
pathandlocal_destinationviacfl_sds_create.local_prefixonly iftmp_prefixis non‑NULL.- If any allocation fails (including only the prefix when it’s present), it destroys any partial SDS values, unlocks the DB, and returns
-1.- Marks the part as in progress via
azb_db_file_part_in_progresswhile still holding the DB mutex, then exposes all three SDS outputs to the caller.This function’s extended signature and memory handling look correct and align with the BLOB upload caller expectations.
947-1035:azb_db_file_oldest_readynow surfacespath_prefixto callers with proper error handling.
- After reading
file_id, path, concatenatedpart_ids, andsource, the function:
- Reads column 4 as an optional
path_prefixand, when non‑NULL, allocates a SDS viacfl_sds_create.- On prefix allocation failure, destroys
source,part_ids, andpath, clears and resets the statement, unlocks the DB, and returns-1.- In the
SQLITE_DONEand generic error branches, it explicitly sets*path,*part_ids,*source, and*path_prefixto NULL before returning.This gives the caller a clear contract:
path_prefixis either a valid SDS they own or NULL, with no leaks on any path.plugins/out_azure_blob/azure_blob_blockblob.c (1)
34-50: Block‑blob URIs now respect the resolvedpath_prefixconsistently across PUT and commit.
azb_block_blob_blocklist_uri,azb_block_blob_uri, andazb_block_blob_uri_commitnow:
- Accept a
const char *path_prefixand derive aneffective_pathviaazb_effective_path(ctx, path_prefix).- Use
effective_pathonly when it is non‑NULL and non‑empty, otherwise they build URIs at the container root.azb_block_blob_commit_block:
- Propagates
path_prefixintoazb_block_blob_uri_commit, so single‑block log uploads are committed under the same prefix used for the PUT.azb_block_blob_commit_file_parts:
- Calls
azb_block_blob_blocklist_uri(ctx, path_prefix, path)so buffered BLOB uploads commit their block lists under the same (possibly templated) prefix recorded in the DB.- All new code paths reuse existing allocation and cleanup patterns (URIs are freed on all early returns, payload SDS is destroyed after use), with no new leaks introduced.
This ties the path templating and DB‑stored prefixes cleanly into the block‑blob PUT and commit flows.
Also applies to: 63-105, 126-155, 353-397, 395-459
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/out_azure_blob/azure_blob.c (1)
1419-1613: Treat non-FLB_OKreturns fromazb_block_blob_commit_file_partsas failureIn
cb_azb_blob_file_upload, the commit result is currently checked only forret == -1:ret = azb_block_blob_commit_file_parts(ctx, file_id, file_path, part_ids, commit_prefix); if (ret == -1) { flb_plg_error(ctx->ins, "cannot commit blob file parts for file id=%" PRIu64 " path=%s", file_id, file_path); } else { /* treat as success */ }But
azb_block_blob_commit_file_parts()returns the status fromazb_block_blob_put_block_list(), which usesFLB_OKandFLB_RETRY(not-1) for most outcomes. As written,FLB_RETRY(e.g., network error or 404 due to a mismatched path) is treated as success: the code logs the file as committed, enqueues a success notification, and deletes the DB row, potentially losing data that was never actually made visible in Azure.Consider tightening the check so only
FLB_OKis treated as success and everything else as failure:- ret = azb_block_blob_commit_file_parts(ctx, file_id, file_path, part_ids, commit_prefix); - if (ret == -1) { - flb_plg_error(ctx->ins, "cannot commit blob file parts for file id=%" PRIu64 " path=%s", - file_id, file_path); - } - else { + ret = azb_block_blob_commit_file_parts(ctx, file_id, file_path, part_ids, commit_prefix); + if (ret != FLB_OK) { + flb_plg_error(ctx->ins, + "cannot commit blob file parts for file id=%" PRIu64 " path=%s (status=%d)", + file_id, file_path, ret); + } + else { flb_plg_info(ctx->ins, "blob file '%s' (id=%" PRIu64 ") committed successfully", file_path, file_id); /* existing success handling... */ }This keeps commit semantics explicit and avoids marking buffered blob uploads as successfully committed when the block list request actually failed or needs a retry.
🧹 Nitpick comments (2)
plugins/out_azure_blob/azure_blob_uri.c (1)
132-135: Inconsistent error handling:flb_sds_printfreturn values not checked.Unlike the updated
azb_uri_containerandazb_uri_create_blobfunctions, this function does not check the return values offlb_sds_printfcalls. On allocation failure, the function would return a corrupted URI.Apply this diff for consistent error handling:
flb_sds_printf(&uri, "?restype=container"); + if (uri == NULL) { + return NULL; + } if (ctx->atype == AZURE_BLOB_AUTH_SAS && ctx->sas_token) { - flb_sds_printf(&uri, "&%s", ctx->sas_token); + if (flb_sds_printf(&uri, "&%s", ctx->sas_token) == NULL) { + flb_sds_destroy(uri); + return NULL; + } }plugins/out_azure_blob/azure_blob.c (1)
631-711:send_blobpath-prefix handling and CREATE_BLOB retry flow are correct
create_blob()/delete_blob()now acceptpath_prefixand build URIs viaazb_uri_create_blob(ctx, path_prefix, name), so blob creation/deletion is executed against the same resolved prefix that uploads use.send_blob():
- resolves
path_prefixonce viaazb_resolve_path()when the caller does not provide a resolved prefix, and reuses that value for the entire operation (initial upload, CREATE_BLOB retry, and commit).- correctly bypasses re-resolution when a DB-stored prefix is provided (blob multi-part flows), eliminating path drift on retries.
- builds append and block blob URIs with the chosen
path_prefixand a per-call random suffix, and keeps the same suffix (generated_random_string) for the commit step.- for block blobs with
FLB_EVENT_TYPE_LOGS, callsazb_block_blob_commit_block()after a successful initial upload and again after a successful CREATE_BLOB retry, so log blocks are always committed.- All call sites (
cb_azure_blob_ingest,ingest_all_chunks,cb_azure_blob_flush,cb_azb_blob_file_upload) now pass either NULL (letsend_blobresolve) or a DB-derived prefix, so the effective path used for uploads and later commits is consistent.Aside from some now-dead cleanup in
send_blobrelated topayload_buf(always equal todata), the control flow and prefix usage look correct.Also applies to: 713-790, 917-1063, 1419-1747, 1768-2242
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
plugins/out_azure_blob/azure_blob.c(28 hunks)plugins/out_azure_blob/azure_blob.h(3 hunks)plugins/out_azure_blob/azure_blob_appendblob.c(1 hunks)plugins/out_azure_blob/azure_blob_appendblob.h(1 hunks)plugins/out_azure_blob/azure_blob_blockblob.c(8 hunks)plugins/out_azure_blob/azure_blob_blockblob.h(1 hunks)plugins/out_azure_blob/azure_blob_conf.c(2 hunks)plugins/out_azure_blob/azure_blob_db.c(18 hunks)plugins/out_azure_blob/azure_blob_db.h(9 hunks)plugins/out_azure_blob/azure_blob_uri.c(2 hunks)plugins/out_azure_blob/azure_blob_uri.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/out_azure_blob/azure_blob_conf.c
- plugins/out_azure_blob/azure_blob_uri.h
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
plugins/out_azure_blob/azure_blob.hplugins/out_azure_blob/azure_blob.c
📚 Learning: 2025-08-29T06:24:55.855Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:52-56
Timestamp: 2025-08-29T06:24:55.855Z
Learning: ZSTD compression is always available in Fluent Bit and does not require conditional compilation guards. Unlike Arrow/Parquet which use #ifdef FLB_HAVE_ARROW guards, ZSTD is built unconditionally with flb_zstd.c included directly in src/CMakeLists.txt and a bundled ZSTD library at lib/zstd-1.5.7/.
Applied to files:
plugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-29T06:25:02.561Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:7-7
Timestamp: 2025-08-29T06:25:02.561Z
Learning: In Fluent Bit, ZSTD (zstandard) compression library is bundled directly in the source tree at `lib/zstd-1.5.7` and is built unconditionally as a static library. Unlike optional external dependencies, ZSTD does not use conditional compilation guards like `FLB_HAVE_ZSTD` and is always available. Headers like `<fluent-bit/flb_zstd.h>` can be included directly without guards.
Applied to files:
plugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
plugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-29T06:24:26.170Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:39-42
Timestamp: 2025-08-29T06:24:26.170Z
Learning: In Fluent Bit, ZSTD compression support is enabled by default and does not require conditional compilation guards (like #ifdef FLB_HAVE_ZSTD) around ZSTD-related code declarations and implementations.
Applied to files:
plugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components, ZSTD support is always available and doesn't need build-time conditionals.
Applied to files:
plugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-29T06:24:44.797Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:26-26
Timestamp: 2025-08-29T06:24:44.797Z
Learning: In Fluent Bit, ZSTD support is always available and enabled by default. The build system automatically detects and uses either the system libzstd library or builds the bundled ZSTD version. Unlike other optional dependencies like Arrow which use conditional compilation guards (e.g., FLB_HAVE_ARROW), ZSTD does not require conditional includes or build flags.
Applied to files:
plugins/out_azure_blob/azure_blob.h
🧬 Code graph analysis (9)
plugins/out_azure_blob/azure_blob.h (1)
plugins/out_azure_blob/azure_blob.c (1)
azb_resolve_path(508-629)
plugins/out_azure_blob/azure_blob_appendblob.h (1)
plugins/out_azure_blob/azure_blob_appendblob.c (1)
azb_append_blob_uri(27-62)
plugins/out_azure_blob/azure_blob_appendblob.c (2)
plugins/out_azure_blob/azure_blob_uri.c (2)
azb_uri_container(107-121)azb_effective_path(140-156)src/flb_sds.c (2)
flb_sds_printf(336-387)flb_sds_destroy(389-399)
plugins/out_azure_blob/azure_blob_uri.c (1)
src/flb_sds.c (2)
flb_sds_printf(336-387)flb_sds_destroy(389-399)
plugins/out_azure_blob/azure_blob_db.h (1)
plugins/out_azure_blob/azure_blob_db.c (2)
azb_db_file_get_next_stale(546-609)azb_db_file_oldest_ready(979-1067)
plugins/out_azure_blob/azure_blob.c (4)
src/flb_record_accessor.c (2)
flb_ra_create(271-358)flb_ra_destroy(232-248)plugins/out_azure_blob/azure_blob_appendblob.c (1)
azb_append_blob_uri(27-62)plugins/out_azure_blob/azure_blob_blockblob.c (3)
azb_block_blob_id_logs(171-210)azb_block_blob_uri(63-124)azb_block_blob_commit_block(354-393)plugins/out_azure_blob/azure_blob_db.c (2)
azb_db_file_insert(365-416)azb_db_file_oldest_ready(979-1067)
plugins/out_azure_blob/azure_blob_blockblob.c (1)
plugins/out_azure_blob/azure_blob_uri.c (2)
azb_uri_container(107-121)azb_effective_path(140-156)
plugins/out_azure_blob/azure_blob_db.c (2)
src/flb_sqldb.c (1)
flb_sqldb_close(129-149)lib/cfl/src/cfl_sds.c (2)
cfl_sds_create(113-125)cfl_sds_destroy(127-137)
plugins/out_azure_blob/azure_blob_blockblob.h (1)
plugins/out_azure_blob/azure_blob_blockblob.c (6)
azb_block_blob_blocklist_uri(34-61)azb_block_blob_uri(63-124)azb_block_blob_id_logs(171-210)azb_block_blob_id_blob(219-272)azb_block_blob_commit_block(354-393)azb_block_blob_commit_file_parts(395-460)
🔇 Additional comments (21)
plugins/out_azure_blob/azure_blob.h (3)
27-27: LGTM!The
flb_time.hinclude is correctly added to support thestruct flb_timeparameter in the newazb_resolve_pathfunction declaration.
66-66: LGTM!The
path_templating_enabledfield is appropriately placed within the configuration section of the context struct and usesinttype consistent with Fluent Bit'sFLB_TRUE/FLB_FALSEconventions.
171-175: LGTM!The
azb_resolve_pathfunction signature is well-designed:
- Uses
constcorrectly for input parameters- Returns resolved path via output pointer, allowing differentiation between "no template" (returns 0,
*out_path = NULL) and errors (returns -1)- Accepts optional timestamp for deterministic path generation in tests
plugins/out_azure_blob/azure_blob_appendblob.h (1)
26-28: LGTM!The updated function signature correctly adds the
path_prefixparameter and appliesconstcorrectness to both string parameters. This aligns with the path-prefix threading pattern used consistently across the PR.plugins/out_azure_blob/azure_blob_appendblob.c (1)
27-61: LGTM!The implementation correctly:
- Uses
azb_effective_pathto determine the appropriate path based on templating state- Applies proper
NULLchecks for allflb_sds_printfcalls (addressing the previous review feedback)- Cleans up the URI on any failure path before returning
NULL- Maintains consistent error handling with the rest of the codebase
plugins/out_azure_blob/azure_blob_uri.c (3)
116-120: LGTM!Proper error handling with
NULLcheck and cleanup on failure.
140-156: LGTM!The
azb_effective_pathhelper correctly implements the path resolution logic:
- When templating is enabled, it always returns the resolved
path_prefix(honoring empty resolved paths)- Falls back to
ctx->pathonly when templating is disabled and no explicit prefix is providedThis addresses the concern from the past review about empty template paths using unresolved prefixes.
158-192: LGTM!The updated
azb_uri_create_blobfunction:
- Correctly integrates
azb_effective_pathfor path resolution- Has comprehensive
NULLchecks on allflb_sds_printfcalls- Properly cleans up resources on any failure path
plugins/out_azure_blob/azure_blob_db.h (9)
56-58: LGTM!The INSERT statement correctly includes
path_prefixin both the column list and VALUES clause, with the parameter binding using@path_prefix.
79-88: LGTM!The query correctly adds
path_prefixto the SELECT list for aborted file retrieval.
91-98: LGTM!The stale file query correctly includes
path_prefixin the result set.
129-154: LGTM!The next file part query correctly adds
path_prefix(column index 10) alongsidedestinationin the JOIN result.
168-176: LGTM!The oldest file query correctly includes
f.path_prefixin the aggregated result set.
182-187: LGTM!Function signature correctly updated to accept
path_prefixfor file insertion.
199-210: LGTM!Both
azb_db_file_get_next_abortedandazb_db_file_get_next_stalesignatures correctly updated to returnpath_prefixas an output parameter.
219-235: LGTM!Function signatures for
azb_db_file_part_get_nextandazb_db_file_oldest_readycorrectly updated to includepath_prefixoutput parameters, aligning with the SQL query changes.
28-40: No database migration issue. Theensure_path_prefix_column()function handles schema migration by checking if thepath_prefixcolumn exists (usingPRAGMA table_info), and adds it viaALTER TABLEfor existing databases if needed. This is called during database initialization inazb_db_open()before statement preparation, ensuring both new and existing databases are properly updated.plugins/out_azure_blob/azure_blob_blockblob.h (1)
26-46: Path-prefix-aware blockblob prototypes look consistent and correctThe added
path_prefixparameters andconstqualifiers align with the implementations and call sites; the header surface now cleanly exposes path-prefix-aware URIs and commit helpers.plugins/out_azure_blob/azure_blob_db.c (1)
28-71: DB schema migration andpath_prefixpropagation look correct
ensure_path_prefix_column()cleanly detects/creates thepath_prefixcolumn viaPRAGMA table_info+ALTER TABLE, with proper statement finalization and error handling.azb_db_open()invokes the migration once per open before preparing statements, and closes the DB on failure, so partial migrations won’t leak handles.azb_db_file_insert()safely bindspath_prefixas TEXT or NULL, matching how other string fields are handled.- The getters (
azb_db_file_get_next_stale,azb_db_file_get_next_aborted,azb_db_file_part_get_next,azb_db_file_oldest_ready) now allocatecfl_sds_tforpath_prefixwhen present, initialize outputs to NULL, and consistently clean up on all error paths. Legacy rows without the column remain represented aspath_prefix == NULL, which upper layers then interpret explicitly.Overall, the DB layer changes correctly carry
path_prefixthrough without leaks or regressions for older databases.Also applies to: 287-291, 365-416, 546-609, 611-682, 816-917, 979-1067
plugins/out_azure_blob/azure_blob_blockblob.c (1)
34-61: Block-blob URIs now correctly honor resolvedpath_prefixThe updated helpers:
- Use
azb_effective_path(ctx, path_prefix)so templated paths rely solely on the resolved prefix, while non-templated configs still fall back toctx->path.- Correctly distinguish between
NULLand empty-string prefixes, treating""as “no prefix” while still avoiding a fallback to the raw template.- Thread
path_prefixconsistently throughazb_block_blob_blocklist_uri,azb_block_blob_uri,azb_block_blob_uri_commit,azb_block_blob_commit_block, andazb_block_blob_commit_file_parts, so uploads and blocklist commits now target the same blob paths for both log and BLOB flows.No correctness issues spotted in the new logic.
Also applies to: 63-125, 126-165, 353-393, 395-459
plugins/out_azure_blob/azure_blob.c (1)
158-629: Templating helpers andazb_resolve_pathpipeline look sound
generate_random_string_blob()usesflb_random_bytes()with a sane fallback PRNG and gracefully handles OOM / zero-length requests.azb_replace_uuid()replaces all$UUIDtokens in one pass with a fixed 8‑char random suffix, preserving the rest of the template and correctly managing SDS ownership (destroying the input only when a new buffer is allocated).azb_apply_time_tokens()andazb_apply_strftime()apply%3N/%9N/%Land standardstrftimeformatting in a robust way, with clear error paths and explicit logging when the format is invalid or too long.azb_trim_slashes()safely normalizes leading/trailing slashes in-place, keeping SDS metadata (len) in sync.azb_resolve_path():
- short-circuits when templating is disabled or
ctx->pathis NULL,- applies UUID → time tokens →
strftime→ record accessor ($TAG,$TAG[n]) in the expected order,- handles
tag == NULLby substituting an empty tag string so the record accessor never sees a NULL pointer,- returns
*out_path == NULLwhen no templating is effectively applied, and an empty SDS (length 0) when the template legitimately resolves to an empty prefix, which higher layers then treat as “no prefix”.Overall, the path resolution pipeline is correct, defensive against NULLs, and matches the intended S3-style templating behavior.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
plugins/out_azure_blob/azure_blob.c(26 hunks)plugins/out_azure_blob/azure_blob.h(3 hunks)plugins/out_azure_blob/azure_blob_appendblob.c(1 hunks)plugins/out_azure_blob/azure_blob_appendblob.h(1 hunks)plugins/out_azure_blob/azure_blob_blockblob.c(8 hunks)plugins/out_azure_blob/azure_blob_blockblob.h(1 hunks)plugins/out_azure_blob/azure_blob_conf.c(2 hunks)plugins/out_azure_blob/azure_blob_db.c(18 hunks)plugins/out_azure_blob/azure_blob_db.h(9 hunks)plugins/out_azure_blob/azure_blob_uri.c(2 hunks)plugins/out_azure_blob/azure_blob_uri.h(1 hunks)tests/internal/CMakeLists.txt(2 hunks)tests/internal/azure_blob_path.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- plugins/out_azure_blob/azure_blob_uri.h
- tests/internal/CMakeLists.txt
- tests/internal/azure_blob_path.c
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
plugins/out_azure_blob/azure_blob.hplugins/out_azure_blob/azure_blob.c
📚 Learning: 2025-08-29T06:24:55.855Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:52-56
Timestamp: 2025-08-29T06:24:55.855Z
Learning: ZSTD compression is always available in Fluent Bit and does not require conditional compilation guards. Unlike Arrow/Parquet which use #ifdef FLB_HAVE_ARROW guards, ZSTD is built unconditionally with flb_zstd.c included directly in src/CMakeLists.txt and a bundled ZSTD library at lib/zstd-1.5.7/.
Applied to files:
plugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-29T06:25:02.561Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:7-7
Timestamp: 2025-08-29T06:25:02.561Z
Learning: In Fluent Bit, ZSTD (zstandard) compression library is bundled directly in the source tree at `lib/zstd-1.5.7` and is built unconditionally as a static library. Unlike optional external dependencies, ZSTD does not use conditional compilation guards like `FLB_HAVE_ZSTD` and is always available. Headers like `<fluent-bit/flb_zstd.h>` can be included directly without guards.
Applied to files:
plugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
plugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-29T06:24:26.170Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:39-42
Timestamp: 2025-08-29T06:24:26.170Z
Learning: In Fluent Bit, ZSTD compression support is enabled by default and does not require conditional compilation guards (like #ifdef FLB_HAVE_ZSTD) around ZSTD-related code declarations and implementations.
Applied to files:
plugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components, ZSTD support is always available and doesn't need build-time conditionals.
Applied to files:
plugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-08-29T06:24:44.797Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:26-26
Timestamp: 2025-08-29T06:24:44.797Z
Learning: In Fluent Bit, ZSTD support is always available and enabled by default. The build system automatically detects and uses either the system libzstd library or builds the bundled ZSTD version. Unlike other optional dependencies like Arrow which use conditional compilation guards (e.g., FLB_HAVE_ARROW), ZSTD does not require conditional includes or build flags.
Applied to files:
plugins/out_azure_blob/azure_blob.h
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
plugins/out_azure_blob/azure_blob_conf.c
🧬 Code graph analysis (6)
plugins/out_azure_blob/azure_blob.h (1)
plugins/out_azure_blob/azure_blob.c (2)
azb_resolve_path(544-665)azb_commit_prefix_with_fallback(219-236)
plugins/out_azure_blob/azure_blob_db.h (1)
plugins/out_azure_blob/azure_blob_db.c (4)
azb_db_file_get_next_stale(546-609)azb_db_file_part_uploaded(919-944)azb_db_file_part_delivery_attempts(946-977)azb_db_file_oldest_ready(979-1067)
plugins/out_azure_blob/azure_blob.c (3)
plugins/out_azure_blob/azure_blob_uri.c (1)
azb_uri_create_blob(165-200)plugins/out_azure_blob/azure_blob_appendblob.c (1)
azb_append_blob_uri(27-62)plugins/out_azure_blob/azure_blob_blockblob.c (1)
azb_block_blob_uri(63-124)
plugins/out_azure_blob/azure_blob_blockblob.c (1)
plugins/out_azure_blob/azure_blob_uri.c (2)
azb_uri_container(107-121)azb_effective_path(147-163)
plugins/out_azure_blob/azure_blob_db.c (2)
src/flb_sqldb.c (1)
flb_sqldb_close(129-149)lib/cfl/src/cfl_sds.c (2)
cfl_sds_create(113-125)cfl_sds_destroy(127-137)
plugins/out_azure_blob/azure_blob_appendblob.h (1)
plugins/out_azure_blob/azure_blob_appendblob.c (1)
azb_append_blob_uri(27-62)
🔇 Additional comments (26)
plugins/out_azure_blob/azure_blob_uri.c (1)
116-119: LGTM!The NULL checks for
flb_sds_printfare correct, ensuring cleanup on allocation failures. The effective_path logic properly centralizes path resolution for URI construction.Also applies to: 132-141, 179-196
plugins/out_azure_blob/azure_blob_db.c (3)
30-71: Well-designed migration logic.The
ensure_path_prefix_columnfunction properly checks for column existence before attempting to add it, ensuring idempotent schema migration. The error handling includes proper statement cleanup on all paths.
287-291: Verify migration executes before statement preparation.The migration call occurs after table creation and foreign key enablement but before prepared statements are created. This ensures the column exists when statements referencing it are prepared. Good ordering.
388-393: Correct NULL handling for path_prefix throughout database operations.All database operations consistently:
- Bind NULL when path_prefix is absent (insert)
- Check for NULL before creating SDS (retrieval)
- Initialize output to NULL on error paths
- Include path_prefix in cleanup on allocation failures
The error handling ensures no leaks when path_prefix allocation fails after other fields are allocated.
Also applies to: 578-588, 648-658, 871-876
plugins/out_azure_blob/azure_blob_blockblob.c (5)
34-61: LGTM: Path prefix correctly threaded through blocklist URI construction.The function now accepts
path_prefix, determines theeffective_pathviaazb_effective_path, and correctly incorporates it into the blocklist URI when present. SAS token handling is preserved.
63-124: LGTM: Block blob URI construction properly uses path prefix.The function correctly determines
effective_pathfrompath_prefixand applies it consistently across all URI construction branches (with/without timestamp, with/without effective_path).
126-169: LGTM: Commit URI now includes path prefix with added safety checks.The function properly incorporates
path_prefixviaazb_effective_path. The added null checks forctx,tag, andstr(Lines 136-138) improve robustness.
358-397: LGTM: Path prefix properly propagated to commit URI.The function correctly forwards
path_prefixtoazb_block_blob_uri_commit, ensuring consistent path resolution throughout the commit flow.
399-464: LGTM: Block list commit now uses path prefix.The function correctly forwards
path_prefixtoazb_block_blob_blocklist_uriat Line 453, addressing previous concerns about block list commits targeting incorrect blob paths.plugins/out_azure_blob/azure_blob.c (17)
162-200: LGTM: Improved random string generation with proper fallback.The rewritten implementation uses
flb_random_bytesfor cryptographic-quality randomness and gracefully falls back to a time-seeded PRNG if that fails. Memory management is correct, and the zero-length edge case is handled.
202-217: LGTM: Token detection helper is straightforward.Simple utility to detect template tokens in paths. Null-safe and correctly identifies
$or%characters.
219-236: LGTM: Fallback logic addresses legacy buffer concerns.This function correctly prioritizes
db_prefix, then falls back toctx->pathonly when safe (templating disabled or no tokens present). Returning NULL for templated paths without a resolved prefix prevents committing to incorrect blob locations, addressing the concerns in past reviews about legacy buffered uploads.
238-312: LGTM: UUID replacement correctly uses single random value.All
$UUIDtokens in the path are replaced with the same 8-character random string, ensuring consistency. Memory management is correct (destroys input, returns new SDS), and the function handles allocation failures.
314-364: LGTM: Simple single-replacement helper is correct.Replaces the first occurrence of
tokenwithreplacement. Memory management is proper (destroys input, returns new SDS). Null-safe.
366-405: Time token expansion works correctly but note the single-replacement behavior.The function expands
%3N(milliseconds),%9N(nanoseconds), and%L(nanoseconds). Sinceazb_simple_replaceonly replaces the first occurrence, paths with multiple instances of the same time token will only have the first one expanded. This is likely acceptable but worth noting if users report unexpected behavior.
407-506: LGTM: strftime expansion with proper error handling for invalid formats.Lines 481-492 correctly address the past review concern by treating
out_len == 0(invalid or too-long strftime format) as an error with explicit logging, rather than silently creating an empty path. Buffer growth and cleanup are correct.
541-665: LGTM: Path resolution with defensive null handling.Lines 569-571 add a defensive check for NULL
ctx->pathwhen templating is enabled, and Lines 635-645 synthesize an empty string whentagis NULL (for BLOB uploads), both addressing previous review feedback. The resolution sequence (UUID → time tokens → strftime → record accessor → trim) is correct, and all resources are properly cleaned up on error paths.
667-747: LGTM: Blob creation now uses path prefix.The function correctly accepts and forwards
path_prefixtoazb_uri_create_blob. The cleanup logic usinggotoand astatusvariable ensures resources are properly released on all exit paths.
749-826: LGTM: Blob deletion now includes path prefix.The function now accepts and forwards
path_prefixtoazb_uri_create_blobat Line 760, addressing the previous concern about DELETE operations omitting the configured prefix for templated paths.
953-1089: LGTM: Blob upload with resolved path prefix and proper commit handling.Lines 973-981 resolve the path prefix once if not provided, storing it in
tmp_path_prefix. The function correctly propagatespath_prefixthrough all URI construction calls. Lines 1065-1070 ensure that block uploads following blob creation are also committed, addressing the previous concern about uncommitted blocks after the CREATE_BLOB retry path. Memory management properly cleans uptmp_path_prefixon all exit paths.
1385-1424: LGTM: Blob chunk processing resolves and persists path prefix.Lines 1393-1407 resolve the path prefix for each blob file using the event timestamp, and Line 1410 stores it in the database via
azb_db_file_insert. This ensures that multi-part blob uploads can be committed with the correct resolved prefix. Cleanup ofpath_prefixis correct.
1461-1762: LGTM: File upload callback comprehensively handles path prefixes.The callback now retrieves
path_prefixfrom the database for all blob operations (stale cleanup at Line 1496, aborted cleanup at Line 1523, commit at Line 1574, and part upload at Line 1652). Line 1585 usesazb_commit_prefix_with_fallbackto safely handle NULL prefixes from legacy database rows, addressing previous concerns. Allpath_prefixvariables are properly cleaned up.
1872-1872: LGTM: Blob ingestion delegates path resolution to send_blob.The call site passes
NULLforresolved_path_prefix, allowingsend_blobto resolve the path once internally and use it consistently throughout the upload and commit flow.
1993-1994: LGTM: Chunk ingestion delegates path resolution to send_blob.Passing
NULLforresolved_path_prefixis correct;send_blobwill resolve the path internally at Lines 973-981.
2157-2158: LGTM: Buffered flush delegates path resolution.The buffered upload path correctly passes
NULLforresolved_path_prefix, relying onsend_blobto resolve the path. Note that the outerCREATE_BLOBretry logic has been removed, eliminating the risk of double path resolution mentioned in past reviews.
2237-2237: LGTM: Non-buffered flush delegates path resolution.Consistent with other call sites, this passes
NULLforresolved_path_prefix, allowingsend_blobto handle path resolution internally.
Signed-off-by: Nico Berlee <[email protected]>
fixes fluent#7515 Signed-off-by: Nico Berlee <[email protected]>
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@edsiper I fixed all codex and coderabbit findings |
(refs fluent/fluent-bit#11178) Signed-off-by: Nico Berlee <[email protected]>
2 participants
fixes #7515
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:
see valgrind log
valgrind.log
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
out_azure_blob: document path templating fluent-bit-docs#2302
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
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.