out_arvancloud_cloudlogs: add new output plugin for ArvanCloud CloudLogs #11095
Conversation
javadnasrolahi
requested review from
celalettin1286,
edsiper,
fujimotos,
koleini,
niedbalski and
patrick-stephens
as code owners
|
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 a new ArvanCloud CloudLogs output plugin: build flags and option, plugin registration and build stanza, public header, configuration/lifecycle creation and teardown, and full plugin implementation including formatting, timestamp parsing/formatting, optional gzip, and HTTP flush logic. Changes
Sequence Diagram(s)sequenceDiagram
participant FB as Fluent Bit Core
participant Init as cb_arvancloud_init
participant Cfg as flb_arvancloud_conf_create
participant Ctx as Plugin Context
participant Flush as cb_arvancloud_flush
participant Format as arvancloud_format
participant HTTP as Fluent Bit HTTP Client
participant AC as ArvanCloud API
FB->>Init: plugin init
Init->>Cfg: create & validate config (apikey, proxy, keys)
Cfg->>Ctx: allocate context, setup upstream (https, host, port, uri)
Ctx-->>FB: return context
loop For each event chunk
FB->>Flush: flush events
Flush->>Format: decode events, build JSON logs array
Format->>Format: resolve timestamps & logType per record
Format-->>Flush: formatted payload
alt gzip enabled
Flush->>Flush: gzip compress payload
end
Flush->>HTTP: POST payload (headers + optional Authorization)
HTTP->>AC: send request
AC-->>HTTP: respond (status)
HTTP-->>Flush: deliver status
alt 2xx
Flush-->>FB: FLB_OK
else retryable (5xx / network)
Flush-->>FB: FLB_RETRY
else client error (4xx)
Flush-->>FB: FLB_ERROR
end
end
FB->>Init: shutdown
Init->>Ctx: flb_arvancloud_conf_destroy -> free resources
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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
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".
| c = flb_http_client(u_conn, FLB_HTTP_POST, ctx->uri, | ||
| final_payload, final_payload_size, | ||
| NULL, 0, // host=NULL, port=0 -> use from u_conn | ||
| NULL, 0); // proxy=NULL |
There was a problem hiding this comment.
The flush path always creates the HTTP client with host=NULL and proxy=NULL, even when proxy was parsed during configuration. Because the upstream was created with TLS against the proxy host, the code now attempts to start a TLS handshake with the proxy and never performs an HTTP CONNECT or sets the real Host header. Standard HTTP proxies require a plain CONNECT request before TLS, so any configuration that specifies a proxy will fail to deliver logs. Pass the proxy information (and target host) to flb_http_client or drop the proxy option.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CMakeLists.txt(1 hunks)cmake/plugins_options.cmake(1 hunks)plugins/CMakeLists.txt(1 hunks)plugins/out_arvancloud_cloudlogs/CMakeLists.txt(1 hunks)plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs.c(1 hunks)plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs.h(1 hunks)plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs_conf.c(1 hunks)
🧰 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:
cmake/plugins_options.cmakeCMakeLists.txt
📚 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, the correct CMake flag for using system librdkafka is `FLB_PREFER_SYSTEM_LIB_KAFKA=ON`.
Applied to files:
cmake/plugins_options.cmakeCMakeLists.txt
🧬 Code graph analysis (3)
plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs.h (1)
plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs_conf.c (2)
flb_arvancloud_conf_create(27-156)flb_arvancloud_conf_destroy(158-189)
plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs_conf.c (7)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1298-1321)src/flb_output.c (2)
flb_output_get_property(1096-1099)flb_output_upstream_set(1556-1647)src/flb_sds.c (2)
flb_sds_create(78-90)flb_sds_destroy(389-399)src/flb_utils.c (1)
flb_utils_url_split(1441-1534)src/flb_record_accessor.c (2)
flb_ra_create(271-358)flb_ra_destroy(232-248)src/flb_upstream.c (2)
flb_upstream_create(290-363)flb_upstream_destroy(656-698)
plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs.c (11)
include/fluent-bit/flb_compat.h (1)
gmtime_r(75-81)src/flb_strptime.c (1)
flb_strptime(247-251)src/flb_log_event_decoder.c (3)
flb_log_event_decoder_init(103-120)flb_log_event_decoder_next(309-456)flb_log_event_decoder_destroy(151-178)src/flb_record_accessor.c (1)
flb_ra_translate(628-633)src/flb_sds.c (3)
flb_sds_destroy(389-399)flb_sds_create_size(92-95)flb_sds_cat(120-141)src/flb_pack.c (1)
flb_msgpack_raw_to_json_sds(1026-1085)src/flb_upstream.c (2)
flb_upstream_conn_get(711-844)flb_upstream_conn_release(862-947)src/flb_gzip.c (1)
flb_gzip_compress(157-252)src/flb_http_client.c (5)
flb_http_client(814-859)flb_http_add_header(963-995)flb_http_set_content_encoding_gzip(1123-1132)flb_http_do(1572-1632)flb_http_client_destroy(1688-1695)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs_conf.c (2)
flb_arvancloud_conf_create(27-156)flb_arvancloud_conf_destroy(158-189)
This patch adds a new output plugin that sends logs to ArvanCloud CloudLogs service. The plugin supports: - GZIP compression - Configurable timestamp formats - Custom log fields mapping - Retry logic and error handling The plugin has been tested with ArvanCloud CloudLogs API and successfully delivers logs with proper formatting and timestamps. Signed-off-by: Javad Nasrolahi <[email protected]>
javadnasrolahi
force-pushed
the
add-out-arvancloud-cloudlogs
branch
from
c414de0 to
b14d48d
Compare
patrick-stephens
left a comment
patrick-stephens
left a comment
There was a problem hiding this comment.
Please follow the PR template and ensure everything it requires is covered - in particular support for all targets.
Please respond to all comments raised by Coderabbit in some way to indicate whether it is relevant or not.
| DEFINE_OPTION(FLB_OUT_UDP "Enable UDP output plugin" ON) | ||
| DEFINE_OPTION(FLB_OUT_VIVO_EXPORTER "Enable Vivo exporter output plugin" ON) | ||
| DEFINE_OPTION(FLB_OUT_WEBSOCKET "Enable Websocket output plugin" ON) | ||
| DEFINE_OPTION(FLB_OUT_ARVANCLOUD_CLOUDLOGS "Enable ArvanCloud CloudLogs output plugin" ON) |
There was a problem hiding this comment.
Will this build for all Linux, macOS and Windows targets with no extra dependencies required?
If it will not build for Windows or macOS then configure the defaults under the cmake/ directory for those targets.
If it needs more build or runtime dependencies then please update the container definitions for both the OSS images under dockerfiles/ and also the package builds under packaging/distros/
There was a problem hiding this comment.
yes, This plugin will build for all Linux, macOS, and Windows targets with no extra dependencies required.
…rvancloud_conf_destroy in ArvanCloud CloudLogs plugin configuration
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs_conf.c (1)
158-189: Memory leak still present: config-map-populated fields not freed.As previously noted,
flb_output_config_map_set()allocates SDS strings for configuration fields likeapi_key,log_type,log_type_key,tag_key,timestamp_key, andtimestamp_format. These are not being freed in the destructor, causing memory leaks on every plugin teardown.Apply the previously suggested diff to add cleanup for these fields:
if (ctx->proxy) { flb_sds_destroy(ctx->proxy); } + if (ctx->api_key) { + flb_sds_destroy(ctx->api_key); + } if (ctx->proxy_host) { flb_free(ctx->proxy_host); } if (ctx->scheme) { flb_sds_destroy(ctx->scheme); } if (ctx->host) { flb_sds_destroy(ctx->host); } if (ctx->uri) { flb_sds_destroy(ctx->uri); } + if (ctx->log_type) { + flb_sds_destroy(ctx->log_type); + } + if (ctx->log_type_key) { + flb_sds_destroy(ctx->log_type_key); + } if (ctx->ra_log_type_key) { flb_ra_destroy(ctx->ra_log_type_key); } + if (ctx->tag_key) { + flb_sds_destroy(ctx->tag_key); + } + if (ctx->timestamp_key) { + flb_sds_destroy(ctx->timestamp_key); + } if (ctx->ra_timestamp_key) { flb_ra_destroy(ctx->ra_timestamp_key); } + if (ctx->timestamp_format) { + flb_sds_destroy(ctx->timestamp_format); + }
🧹 Nitpick comments (1)
plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs_conf.c (1)
129-131: Simplify:flb_sds_destroyhandles NULL safely.The explicit NULL check is redundant since
flb_sds_destroy()already handles NULL pointers gracefully.Apply this diff:
- if (ctx->uri) { - flb_sds_destroy(ctx->uri); - } + flb_sds_destroy(ctx->uri);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs_conf.c(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs_conf.c (7)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1298-1321)src/flb_output.c (2)
flb_output_get_property(1096-1099)flb_output_upstream_set(1556-1647)src/flb_sds.c (2)
flb_sds_create(78-90)flb_sds_destroy(389-399)src/flb_utils.c (1)
flb_utils_url_split(1441-1534)src/flb_record_accessor.c (2)
flb_ra_create(271-358)flb_ra_destroy(232-248)src/flb_upstream.c (2)
flb_upstream_create(290-363)flb_upstream_destroy(656-698)
🔇 Additional comments (5)
plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs_conf.c (5)
53-57: Past issue resolved: proper cleanup on config map failure.The error path now correctly calls the destructor to clean up any partially-initialized fields from
flb_output_config_map_set().
59-78: LGTM: proxy parsing and memory management.The proxy URL parsing correctly transfers ownership of the host string to
ctx->proxy_hostand frees the other components.
80-93: LGTM: HTTPS enforcement and required field validation.Forcing TLS and validating the required API key early prevents configuration errors from propagating.
95-115: LGTM: conditional record accessor initialization.The record accessors are correctly initialized only when configured, with clear error messages and proper cleanup.
138-156: LGTM: upstream creation and configuration.The upstream is correctly created with either proxy or direct connection settings, and properly configured with TLS flags.
…r enhanced functionality
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs.c (1)
419-422: Proxy configuration never propagated to HTTP client.When
proxyis configured, the HTTP client receivesNULLfor both host and proxy parameters. This causes incorrectHostheaders and prevents CONNECT tunneling, so proxied requests fail. Pass the destination host/port and proxy string so the client can negotiate the tunnel correctly.Apply this diff:
c = flb_http_client(u_conn, FLB_HTTP_POST, ctx->uri, final_payload, final_payload_size, - NULL, 0, // host=NULL, port=0 -> use from u_conn - NULL, 0); // proxy=NULL + ctx->host, ctx->port, + ctx->proxy, 0);plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs_conf.c (1)
159-189: Past review concern remains: config-map SDS fields still leak.The destructor still doesn't free SDS strings populated by
flb_output_config_map_set()on lines 53. Fieldsapi_key,log_type,log_type_key,tag_key,timestamp_key, andtimestamp_formatare allocated as SDS by the config map but never destroyed here, leaking memory on every teardown.Apply this diff to plug the leaks:
if (ctx->proxy) { flb_sds_destroy(ctx->proxy); } + if (ctx->api_key) { + flb_sds_destroy(ctx->api_key); + } if (ctx->proxy_host) { flb_free(ctx->proxy_host); } if (ctx->scheme) { flb_sds_destroy(ctx->scheme); } if (ctx->host) { flb_sds_destroy(ctx->host); } if (ctx->uri) { flb_sds_destroy(ctx->uri); } + if (ctx->log_type) { + flb_sds_destroy(ctx->log_type); + } + if (ctx->log_type_key) { + flb_sds_destroy(ctx->log_type_key); + } if (ctx->ra_log_type_key) { flb_ra_destroy(ctx->ra_log_type_key); } + if (ctx->tag_key) { + flb_sds_destroy(ctx->tag_key); + } + if (ctx->timestamp_key) { + flb_sds_destroy(ctx->timestamp_key); + } if (ctx->ra_timestamp_key) { flb_ra_destroy(ctx->ra_timestamp_key); } + if (ctx->timestamp_format) { + flb_sds_destroy(ctx->timestamp_format); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs.c(1 hunks)plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs_conf.c(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs_conf.c (7)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1298-1321)src/flb_output.c (2)
flb_output_get_property(1096-1099)flb_output_upstream_set(1556-1647)src/flb_sds.c (2)
flb_sds_create(78-90)flb_sds_destroy(389-399)src/flb_utils.c (1)
flb_utils_url_split(1441-1534)src/flb_record_accessor.c (2)
flb_ra_create(271-358)flb_ra_destroy(232-248)src/flb_upstream.c (2)
flb_upstream_create(290-363)flb_upstream_destroy(656-698)
plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs.c (12)
include/fluent-bit/flb_compat.h (1)
gmtime_r(75-81)src/flb_strptime.c (1)
flb_strptime(247-251)src/flb_mp.c (1)
flb_mp_count(43-46)src/flb_log_event_decoder.c (3)
flb_log_event_decoder_init(103-120)flb_log_event_decoder_next(309-456)flb_log_event_decoder_destroy(151-178)src/flb_record_accessor.c (1)
flb_ra_translate(628-633)src/flb_sds.c (3)
flb_sds_destroy(389-399)flb_sds_create_size(92-95)flb_sds_cat(120-141)src/flb_pack.c (1)
flb_msgpack_raw_to_json_sds(1026-1085)src/flb_upstream.c (2)
flb_upstream_conn_get(711-844)flb_upstream_conn_release(862-947)src/flb_gzip.c (1)
flb_gzip_compress(157-252)src/flb_http_client.c (5)
flb_http_client(814-859)flb_http_add_header(963-995)flb_http_set_content_encoding_gzip(1123-1132)flb_http_do(1572-1632)flb_http_client_destroy(1688-1695)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs_conf.c (2)
flb_arvancloud_conf_create(27-157)flb_arvancloud_conf_destroy(159-190)
🔇 Additional comments (5)
plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs.c (5)
60-134: LGTM: Robust timestamp parsing with auto-detection.The function properly validates inputs, attempts user-provided format or auto-detects ISO8601 patterns, and correctly extracts fractional seconds with nanosecond precision scaling.
136-351: Well-structured formatting logic with proper fallbacks.The function correctly:
- Initializes and cleans up the decoder
- Implements priority-based log_type extraction (record field → configured default)
- Handles timezone-aware timestamp parsing with awareness of
timegm()side effects (Line 276 comment)- Falls back gracefully to event timestamps on parse failures
- Properly cleans up intermediate SDS allocations
462-523: Appropriate HTTP response handling with retry logic.The status code mapping is correct:
- 2xx → success
- 400 → error (client fault, no retry)
- 401/403 → error (auth failure, no retry)
- 429 → retry (rate limit)
- 5xx → retry (server fault)
- Others → error
536-555: LGTM: Init and exit callbacks properly delegate to config lifecycle functions.The callbacks correctly create and destroy the plugin context with appropriate error handling.
558-622: LGTM: Config map and plugin registration are well-defined.The configuration options are clearly documented with appropriate defaults, and the plugin struct correctly declares network output with TLS support. Including
test_formatter.callbackenables testing of the formatting logic.
| static size_t format_timestamp_rfc3339(char *buffer, size_t buffer_size, | ||
| time_t seconds, long nsec) | ||
| { | ||
| struct tm tm; | ||
| size_t s; | ||
| int len; | ||
|
|
||
| gmtime_r(&seconds, &tm); | ||
| s = strftime(buffer, buffer_size - 1, "%Y-%m-%dT%H:%M:%S", &tm); | ||
| len = snprintf(buffer + s, buffer_size - 1 - s, | ||
| ".%06ldZ", (long) (nsec / 1000)); | ||
| return s + len; | ||
| } |
There was a problem hiding this comment.
Add error handling for strftime and verify buffer space.
strftime can return 0 on failure or insufficient buffer space, but the code proceeds to snprintf without checking. If s == 0 or if the buffer is too small, the subsequent snprintf could write beyond bounds or produce incomplete output.
Apply this diff to add validation:
static size_t format_timestamp_rfc3339(char *buffer, size_t buffer_size,
time_t seconds, long nsec)
{
struct tm tm;
size_t s;
int len;
gmtime_r(&seconds, &tm);
s = strftime(buffer, buffer_size - 1, "%Y-%m-%dT%H:%M:%S", &tm);
+ if (s == 0 || s >= buffer_size - 1) {
+ return 0;
+ }
len = snprintf(buffer + s, buffer_size - 1 - s,
".%06ldZ", (long) (nsec / 1000));
+ if (len < 0 || (size_t)len >= buffer_size - 1 - s) {
+ return 0;
+ }
return s + len;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static size_t format_timestamp_rfc3339(char *buffer, size_t buffer_size, | |
| time_t seconds, long nsec) | |
| { | |
| struct tm tm; | |
| size_t s; | |
| int len; | |
| gmtime_r(&seconds, &tm); | |
| s = strftime(buffer, buffer_size - 1, "%Y-%m-%dT%H:%M:%S", &tm); | |
| len = snprintf(buffer + s, buffer_size - 1 - s, | |
| ".%06ldZ", (long) (nsec / 1000)); | |
| return s + len; | |
| } | |
| static size_t format_timestamp_rfc3339(char *buffer, size_t buffer_size, | |
| time_t seconds, long nsec) | |
| { | |
| struct tm tm; | |
| size_t s; | |
| int len; | |
| gmtime_r(&seconds, &tm); | |
| s = strftime(buffer, buffer_size - 1, "%Y-%m-%dT%H:%M:%S", &tm); | |
| if (s == 0 || s >= buffer_size - 1) { | |
| return 0; | |
| } | |
| len = snprintf(buffer + s, buffer_size - 1 - s, | |
| ".%06ldZ", (long) (nsec / 1000)); | |
| if (len < 0 || (size_t)len >= buffer_size - 1 - s) { | |
| return 0; | |
| } | |
| return s + len; | |
| } |
🤖 Prompt for AI Agents
In plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs.c around lines 41 to
53, the code calls strftime and immediately calls snprintf without validating
strftime's return or ensuring enough buffer space; update the function to check
the return value s from strftime (if s == 0 treat as error and return 0),
compute remaining = buffer_size - s and ensure remaining > 0 before calling
snprintf, limit the snprintf length to remaining (e.g., use snprintf with
remaining and check its return for negative or >= remaining), and on any failure
return 0 (or an error indicator) to avoid buffer overruns and incomplete
timestamps.
…pport proxy configurations
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs.c (1)
41-53: Add error handling forstrftimeandsnprintfto prevent buffer overruns.The missing validation of
strftime's return value and buffer space checks before callingsnprintfcan lead to buffer overruns if the buffer is too small or ifstrftimefails. This issue was previously flagged and remains unaddressed.Apply the previously suggested fix:
static size_t format_timestamp_rfc3339(char *buffer, size_t buffer_size, time_t seconds, long nsec) { struct tm tm; size_t s; int len; gmtime_r(&seconds, &tm); s = strftime(buffer, buffer_size - 1, "%Y-%m-%dT%H:%M:%S", &tm); + if (s == 0 || s >= buffer_size - 1) { + return 0; + } len = snprintf(buffer + s, buffer_size - 1 - s, ".%06ldZ", (long) (nsec / 1000)); + if (len < 0 || (size_t)len >= buffer_size - 1 - s) { + return 0; + } return s + len; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs.c(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs.c (13)
include/fluent-bit/flb_compat.h (1)
gmtime_r(75-81)src/flb_strptime.c (1)
flb_strptime(247-251)src/flb_mp.c (1)
flb_mp_count(43-46)src/flb_log_event_decoder.c (3)
flb_log_event_decoder_init(103-120)flb_log_event_decoder_next(309-456)flb_log_event_decoder_destroy(151-178)src/flb_record_accessor.c (1)
flb_ra_translate(628-633)src/flb_sds.c (3)
flb_sds_destroy(389-399)flb_sds_create_size(92-95)flb_sds_cat(120-141)src/flb_pack.c (1)
flb_msgpack_raw_to_json_sds(1026-1085)src/flb_upstream.c (2)
flb_upstream_conn_get(711-844)flb_upstream_conn_release(862-947)src/flb_gzip.c (1)
flb_gzip_compress(157-252)src/flb_http_client.c (5)
flb_http_client(814-859)flb_http_add_header(963-995)flb_http_set_content_encoding_gzip(1123-1132)flb_http_do(1572-1632)flb_http_client_destroy(1688-1695)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs_conf.c (2)
flb_arvancloud_conf_create(27-157)flb_arvancloud_conf_destroy(159-190)src/flb_output.c (1)
flb_output_set_context(1524-1527)
🔇 Additional comments (2)
plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs.c (2)
246-306: Well-implemented timestamp extraction with proper timezone handling.The timestamp extraction logic correctly handles timezone-aware timestamps by reading
gmtoffbefore callingtimegm()(which may modify the struct) and applying the offset to convert to UTC. The fallback to event timestamp with a warning when parsing fails is appropriate.
466-534: Appropriate HTTP response handling with correct retry logic.The HTTP status code handling correctly distinguishes between retryable errors (429, 5xx) and non-retryable errors (4xx except 429), with helpful logging for debugging. This follows best practices for resilient HTTP clients.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs.c(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs.c (12)
include/fluent-bit/flb_compat.h (1)
gmtime_r(75-81)src/flb_strptime.c (1)
flb_strptime(247-251)src/flb_mp.c (1)
flb_mp_count(43-46)src/flb_log_event_decoder.c (3)
flb_log_event_decoder_init(103-120)flb_log_event_decoder_next(309-456)flb_log_event_decoder_destroy(151-178)src/flb_record_accessor.c (1)
flb_ra_translate(628-633)src/flb_sds.c (3)
flb_sds_destroy(389-399)flb_sds_create_size(92-95)flb_sds_cat(120-141)src/flb_pack.c (1)
flb_msgpack_raw_to_json_sds(1026-1085)src/flb_upstream.c (2)
flb_upstream_conn_get(711-844)flb_upstream_conn_release(862-947)src/flb_gzip.c (1)
flb_gzip_compress(157-252)src/flb_http_client.c (5)
flb_http_client(814-859)flb_http_add_header(963-995)flb_http_set_content_encoding_gzip(1123-1132)flb_http_do(1572-1632)flb_http_client_destroy(1688-1695)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs_conf.c (2)
flb_arvancloud_conf_create(27-157)flb_arvancloud_conf_destroy(159-190)
| if (parse_timestamp_string(timestamp_value, fmt, | ||
| &parsed_tm, &parsed_nsec) == 0) { | ||
| /* | ||
| * Successfully parsed, use the extracted timestamp | ||
| * Convert flb_tm to time_t, accounting for timezone offset | ||
| * IMPORTANT: Read gmtoff BEFORE calling timegm() as timegm() may modify the struct | ||
| */ | ||
| gmtoff_value = flb_tm_gmtoff(&parsed_tm); | ||
| time_before_adjustment = timegm(&parsed_tm.tm); | ||
| parsed_time = time_before_adjustment - gmtoff_value; | ||
|
|
||
| s = format_timestamp_rfc3339(time_formatted, | ||
| sizeof(time_formatted), | ||
| parsed_time, parsed_nsec); | ||
| parse_success = 1; | ||
| } else { | ||
| /* Parse failed, log warning and fall back to event timestamp */ | ||
| flb_plg_warn(ctx->ins, | ||
| "Failed to parse timestamp from field '%s': %s, " | ||
| "falling back to event timestamp", | ||
| ctx->timestamp_key, timestamp_value); | ||
| } | ||
|
|
||
| flb_sds_destroy(timestamp_value); | ||
| } | ||
|
|
||
| /* If parsing failed or extraction failed, use event timestamp */ | ||
| if (!parse_success) { | ||
| s = format_timestamp_rfc3339(time_formatted, | ||
| sizeof(time_formatted), | ||
| t.tm.tv_sec, t.tm.tv_nsec); | ||
| } |
There was a problem hiding this comment.
Timezone offset lost when timestamp_key has fractional seconds
Line 269 falls back to the %Y-%m-%dT%H:%M:%S pattern whenever the record contains fractional seconds (e.g. 2025-10-23T15:00:00.123+03:30). That parse succeeds, but it leaves the trailing +03:30 untouched, so flb_tm_gmtoff(tm_out) stays at 0. A few lines later we convert with timegm() and subtract the zero offset, so the event is emitted as 15:00 UTC instead of 11:30 UTC—a 3½‑hour drift for every such record. This breaks timestamp fidelity for the common case of ISO‑8601 strings with both fractional seconds and offsets.
Please preserve the offset by parsing it after the fractional block (or by stripping the fractional part before calling the %z pattern). One way is to reuse result / frac_ptr to locate the timezone and update flb_tm_gmtoff(tm_out) before returning:
@@
- char *result;
- const char *frac_ptr;
+ char *result;
+ const char *frac_ptr;
+ const char *tz_ptr;
@@
- frac_value = 0;
- digit_count = 0;
+ frac_value = 0;
+ digit_count = 0;
+ tz_ptr = NULL;
@@
- if (frac_ptr) {
+ if (frac_ptr) {
frac_ptr++; /* Skip the dot */
@@
if (digit_count > 0) {
while (digit_count < 9) {
frac_value *= 10;
digit_count++;
}
*nsec_out = frac_value;
}
- }
+ tz_ptr = frac_ptr;
+ }
+ else if (result && *result != '\0') {
+ tz_ptr = result;
+ }
+
+ if (tz_ptr && *tz_ptr != '\0') {
+ if (*tz_ptr == '.') {
+ while (*tz_ptr >= '0' && *tz_ptr <= '9') {
+ tz_ptr++;
+ }
+ }
+ if (*tz_ptr == 'Z' || *tz_ptr == 'z') {
+ flb_tm_gmtoff(tm_out) = 0;
+ }
+ else if (*tz_ptr == '+' || *tz_ptr == '-') {
+ int sign = (*tz_ptr == '-') ? -1 : 1;
+ int hours;
+ int minutes;
+
+ tz_ptr++;
+ if (sscanf(tz_ptr, "%2d:%2d", &hours, &minutes) != 2 &&
+ sscanf(tz_ptr, "%2d%2d", &hours, &minutes) != 2) {
+ return -1;
+ }
+ flb_tm_gmtoff(tm_out) = sign * (hours * 3600 + minutes * 60);
+ }
+ }Any equivalent fix that retains the numeric offset works too. Otherwise, records coming from most ISO‑8601 sources will carry incorrect timestamps.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs.c around lines 269-300,
the code parses timestamps with fractional seconds but loses the timezone offset
because the fractional component prevents the `%z` pattern from being applied;
as a result flb_tm_gmtoff(&parsed_tm) stays zero and the final UTC conversion is
wrong. Fix by preserving/reading the numeric timezone offset when fractional
seconds are present: after parse_timestamp_string (or inside it) locate the
timezone substring (reuse result/frac_ptr or scan from the fractional-pointer to
the '+'/'-' or 'Z'), parse the numeric offset into a gmtoff_value (or populate
parsed_tm's gmtoff field) and then read gmtoff_value BEFORE calling timegm();
alternatively strip the fractional part before parsing with a `%z` pattern so
the timezone is parsed normally—either approach must ensure gmtoff_value is
correctly set and used when computing parsed_time.
| if (ctx->proxy) { | ||
| c = flb_http_client(u_conn, FLB_HTTP_POST, ctx->uri, | ||
| final_payload, final_payload_size, | ||
| ctx->host, ctx->port, | ||
| ctx->proxy, 0); | ||
| } |
There was a problem hiding this comment.
TLS upstream breaks plain HTTP proxy support
Line 419 relies on the proxy path, but flb_arvancloud_conf_create() (see arvancloud_cloudlogs_conf.c) still sets io_flags = FLB_IO_TLS before calling flb_upstream_create() even when proxy is http://…. That makes flb_upstream_conn_get() attempt a TLS handshake with the proxy host using the ArvanCloud certificate (SNI napi.arvancloud.ir) before we ever issue CONNECT, so the connection is rejected and every flush retries forever behind a standard HTTP proxy.
Please drop the TLS flag for plain proxies and only enable it when the proxy scheme is https, while still keeping the TLS context so the client can negotiate after CONNECT. For example:
diff --git a/plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs_conf.c b/plugins/out_arvancloud_cloudlogs/arvancloud_cloudlogs_conf.c
@@
- int io_flags;
+ int io_flags;
+ int proxy_use_tls;
@@
- io_flags = 0;
+ io_flags = 0;
+ proxy_use_tls = FLB_FALSE;
@@
- ctx->proxy_host = host;
- ctx->proxy_port = atoi(port);
+ ctx->proxy_host = host;
+ ctx->proxy_port = atoi(port);
+ if (protocol && strcmp(protocol, "https") == 0) {
+ proxy_use_tls = FLB_TRUE;
+ }
@@
- if (ctx->proxy) {
- upstream = flb_upstream_create(config, ctx->proxy_host,
- ctx->proxy_port, io_flags, ins->tls);
+ if (ctx->proxy) {
+ int proxy_flags = proxy_use_tls ? FLB_IO_TLS : 0;
+
+ upstream = flb_upstream_create(config, ctx->proxy_host,
+ ctx->proxy_port,
+ proxy_flags, ins->tls);After this change we CONNECT over plain TCP when the proxy is http://, and TLS still protects the traffic once the tunnel is established. Without it, any deployment behind a conventional HTTP proxy is unusable.
Committable suggestion skipped: line range outside the PR's diff.
2 participants
This patch adds a new output plugin that sends logs to ArvanCloud CloudLogs service. The plugin supports:
The plugin has been tested with ArvanCloud CloudLogs API and successfully delivers logs with proper formatting and timestamps.
Summary by CodeRabbit