out_azure_blob: add path templating support comparable to s3#11178
out_azure_blob: add path templating support comparable to s3#11178edsiper merged 2 commits intofluent:masterfrom
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.
💡 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.
|
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 <nico.berlee@on2it.net>
|
Rebase to resolve merge conflicts |
Signed-off-by: Nico Berlee <nico.berlee@on2it.net>
fixes fluent#7515 Signed-off-by: Nico Berlee <nico.berlee@on2it.net>
|
Thanks for the patience and work on this. Merging it. |
(refs fluent/fluent-bit#11178) Signed-off-by: Nico Berlee <nico.berlee@on2it.net>
(refs fluent/fluent-bit#11178) Signed-off-by: Nico Berlee <nico.berlee@on2it.net>
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.