out_vivo_exporter: add versioned API endpoints #11040
Conversation
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
|
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. WalkthroughLog formatting now accepts the input instance for source resolution and consolidates OTLP vs non-OTLP packaging into a single MsgPack→JSON pipeline; HTTP endpoints move to versioned /api/v1/* paths, content serving is exported, stream APIs report start/end/next IDs, cmetrics internal metrics endpoint added, and exporter stores config in context. Changes
Sequence DiagramsequenceDiagram
participant Client
participant HTTP as HTTP Handler
participant Stream as Stream Manager
participant Logs as Log Formatter
participant Output as Output Buffer
Client->>HTTP: GET /api/v1/logs
HTTP->>HTTP: headers_set_common(ctx, request)
HTTP->>Stream: vivo_stream_get_content(from,to,limit,&start,&end,&next)
Stream->>Stream: init IDs (-1), iterate entries, set start/end, set next
Stream-->>HTTP: content buffer + start/end/next
HTTP->>Output: set headers Start/End/Next
HTTP-->>Client: Response (body + headers)
rect rgb(220,235,250)
Note over Logs: Log processing (on flush)
Logs->>Logs: format_logs(src_ins, event_chunk, config)
Logs->>Logs: find_map_value() for metadata, track group attributes
alt OTLP present
Logs->>Logs: append otlp map (resource/scope)
else non-OTLP
Logs->>Logs: append flb_group map (metadata/body)
end
Logs-->>Output: JSON payload (msgpack→JSON) with newline
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Potential attention points:
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
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".
| /* Convert the complete msgpack structure to JSON */ | ||
| out_js = flb_msgpack_raw_to_json_sds(tmp_sbuf.data, tmp_sbuf.size, | ||
| config->json_escape_unicode); | ||
|
|
||
| msgpack_sbuffer_destroy(&tmp_sbuf); | ||
|
|
||
| return out_buf; | ||
| /* append a newline */ | ||
| flb_sds_cat_safe(&out_js, "\n", 1); | ||
|
|
||
| if (!out_js) { |
There was a problem hiding this comment.
If flb_msgpack_raw_to_json_sds fails, out_js is NULL but flb_sds_cat_safe(&out_js, "\n", 1) is called before checking for the error. flb_sds_cat_safe dereferences the pointer, so this path will segfault instead of propagating a graceful error whenever the JSON conversion runs out of memory or receives invalid data. The previous implementation checked the return value before appending, so the crash is new to this change.
Useful? React with 👍 / 👎.
| ph->vid = vid; | ||
|
|
||
| /* Set HTTP URI callbacks */ | ||
| mk_vhost_handler(ph->ctx, vid, "/logs", cb_logs, ctx); | ||
| mk_vhost_handler(ph->ctx, vid, "/metrics", cb_metrics, ctx); | ||
| mk_vhost_handler(ph->ctx, vid, "/traces", cb_traces, ctx); | ||
| mk_vhost_handler(ph->ctx, vid, "/api/v1/logs", cb_logs, ctx); | ||
| mk_vhost_handler(ph->ctx, vid, "/api/v1/metrics", cb_metrics, ctx); | ||
| mk_vhost_handler(ph->ctx, vid, "/api/v1/traces", cb_traces, ctx); | ||
| mk_vhost_handler(ph->ctx, vid, "/api/v1/internal/metrics", cb_internal_metrics, ctx); |
There was a problem hiding this comment.
The new /api/v1/internal/metrics handler unconditionally includes and calls flb_me_get_cmetrics and cmt_encode_msgpack_create. These symbols are only available when Fluent Bit is built with FLB_METRICS enabled, but the file lacks any #ifdef FLB_HAVE_METRICS guard. Building the plugin with metrics disabled now results in an implicit‑declaration/undefined‑reference build failure. Consider compiling this handler only when FLB_HAVE_METRICS is defined or providing stubs.
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/out_vivo_exporter/vivo_stream.c (1)
71-87: Race: duplicate stream IDs under concurrencye->id is assigned in vivo_stream_entry_create() before taking stream_mutex, while vs->entries_added is incremented in vivo_stream_append() under lock. Two threads can read the same entries_added and produce duplicate IDs. Assign the ID and increment counters atomically under the lock in vivo_stream_append(), and stop setting e->id in vivo_stream_entry_create().
Apply:
@@ - e->id = vivo_stream_get_new_id(vs); @@ - /* add entry to the end of the list */ + /* assign id atomically and add entry to the end of the list */ + e->id = vs->entries_added; + vs->entries_added++; mk_list_add(&e->_head, &vs->entries); @@ - vs->entries_added++;Optionally remove now-unused vivo_stream_get_new_id() to avoid confusion.
Also applies to: 221-253
plugins/out_vivo_exporter/vivo.c (1)
362-416: Initialization error paths leak resources and config map set is duplicated
- flb_output_config_map_set() is called twice; keep one.
- On failures after allocating ctx/streams/http, resources aren’t unwound (leaks on error).
Refactor with a single config map call and structured cleanup.
@@ - ret = flb_output_config_map_set(ins, (void *) ctx); - if (ret == -1) { - flb_free(ctx); - return -1; - } - flb_output_set_context(ins, ctx); - - /* Load config map */ - ret = flb_output_config_map_set(ins, (void *) ctx); - if (ret == -1) { - return -1; - } + ret = flb_output_config_map_set(ins, (void *) ctx); + if (ret == -1) { + flb_free(ctx); + return -1; + } + flb_output_set_context(ins, ctx); @@ - ctx->stream_logs = vivo_stream_create(ctx); - if (!ctx->stream_logs) { - return -1; - } + ctx->stream_logs = vivo_stream_create(ctx); + if (!ctx->stream_logs) { flb_free(ctx); return -1; } @@ - ctx->stream_metrics = vivo_stream_create(ctx); - if (!ctx->stream_metrics) { - return -1; - } + ctx->stream_metrics = vivo_stream_create(ctx); + if (!ctx->stream_metrics) { vivo_stream_destroy(ctx->stream_logs); flb_free(ctx); return -1; } @@ - ctx->stream_traces = vivo_stream_create(ctx); - if (!ctx->stream_traces) { - return -1; - } + ctx->stream_traces = vivo_stream_create(ctx); + if (!ctx->stream_traces) { + vivo_stream_destroy(ctx->stream_metrics); + vivo_stream_destroy(ctx->stream_logs); + flb_free(ctx); + return -1; + } @@ - if (!ctx->http) { - flb_plg_error(ctx->ins, "could not initialize HTTP server, aborting"); - return -1; - } + if (!ctx->http) { + flb_plg_error(ctx->ins, "could not initialize HTTP server, aborting"); + vivo_stream_destroy(ctx->stream_traces); + vivo_stream_destroy(ctx->stream_metrics); + vivo_stream_destroy(ctx->stream_logs); + flb_free(ctx); + return -1; + } @@ - ret = vivo_http_server_start(ctx->http); - if (ret == -1) { - return -1; - } + ret = vivo_http_server_start(ctx->http); + if (ret == -1) { + vivo_http_server_destroy(ctx->http); + vivo_stream_destroy(ctx->stream_traces); + vivo_stream_destroy(ctx->stream_metrics); + vivo_stream_destroy(ctx->stream_logs); + flb_free(ctx); + return -1; + }
🧹 Nitpick comments (8)
plugins/out_vivo_exporter/vivo_stream.c (1)
148-154: Minor: prealloc without lock may under-size bufferbuf = flb_sds_create_size(vs->current_bytes_size) is done before taking stream_mutex; size may grow meanwhile. Harmless due to flb_sds_cat_safe reallocation, but move read under lock for tighter sizing if this is hot.
plugins/out_vivo_exporter/vivo.c (2)
32-53: Use memcmp for MsgPack raw string keysKeys are length-delimited; memcmp avoids any surprises with embedded NULs.
- if (map->via.map.ptr[i].key.via.str.size == key_len && - strncmp(map->via.map.ptr[i].key.via.str.ptr, key, key_len) == 0) { + if (map->via.map.ptr[i].key.via.str.size == key_len && + memcmp(map->via.map.ptr[i].key.via.str.ptr, key, key_len) == 0) {
87-92: Remove unused out_buf and handle newline append failureout_buf is allocated but never used; also check flb_sds_cat_safe result.
@@ - flb_sds_t out_buf = NULL; @@ - out_buf = flb_sds_create_size((event_chunk->size * 2) / 4); - if (!out_buf) { - flb_errno(); - return NULL; - } @@ - /* append a newline */ - flb_sds_cat_safe(&out_js, "\n", 1); + /* append a newline */ + if (flb_sds_cat_safe(&out_js, "\n", 1) < 0) { + flb_sds_destroy(out_js); + return NULL; + } @@ - if (!out_js) { - flb_sds_destroy(out_buf); - return NULL; - } - - /* Replace out_buf with the complete JSON */ - flb_sds_destroy(out_buf); - return out_js; + return out_js;Also applies to: 274-291, 280-286
plugins/out_vivo_exporter/vivo_http.c (5)
69-91: CORS and Content-Type centralization is good; add Authorization to allowed headers.
Most clients send Authorization; current allow-list would block it on preflight.Apply:
mk_http_header(request, "Access-Control-Allow-Headers", sizeof("Access-Control-Allow-Headers") - 1, - "Origin, X-Requested-With, Content-Type, Accept", - sizeof("Origin, X-Requested-With, Content-Type, Accept") - 1); + "Origin, X-Requested-With, Content-Type, Accept, Authorization", + sizeof("Origin, X-Requested-With, Content-Type, Accept, Authorization") - 1);
92-109: Expose-headers: use exact header names as sent on the wire.
The list uses lowercase while actual headers use macro-defined names (likely “Vivo-Stream-*”). Case-insensitive matching usually works, but align for clarity.mk_http_header(request, "Access-Control-Expose-Headers", sizeof("Access-Control-Expose-Headers") - 1, - "vivo-stream-start-id, vivo-stream-end-id, vivo-stream-next-id", - sizeof("vivo-stream-start-id, vivo-stream-end-id, vivo-stream-next-id") - 1); + "Vivo-Stream-Start-ID, Vivo-Stream-End-ID, Vivo-Stream-Next-ID", + sizeof("Vivo-Stream-Start-ID, Vivo-Stream-End-ID, Vivo-Stream-Next-ID") - 1);
149-154: Empty payload: consider 204 No Content.
You set 200 then return without a body. 204 better signals emptiness; clients still read the Vivo-Stream-Next-ID header you already set.- if (flb_sds_len(payload) == 0) { - flb_sds_destroy(payload); - flb_sds_destroy(str_next); - return; - } + if (flb_sds_len(payload) == 0) { + flb_sds_destroy(payload); + mk_http_status(request, 204); + return; + }(If you keep 200 for compatibility, ignore.)
Please confirm client expectations regarding status code when no entries are available.
214-259: Internal metrics endpoint: good; add Cache-Control: no-store and small nits.
Avoid intermediary caches for ephemeral metrics; keep headers consistent with JSON response.mk_http_status(request, 200); headers_set_common(ctx, request); + mk_http_header(request, + "Cache-Control", sizeof("Cache-Control") - 1, + "no-store", sizeof("no-store") - 1); mk_http_send(request, json, flb_sds_len(json), NULL); mk_http_done(request);Optional: If cmetrics supports direct JSON encoding, you can bypass msgpack->json conversion for efficiency.
I can provide a version using the cmetrics JSON encoder if desired.
305-308: Consider backward-compatibility strategy for API endpoint change.The breaking change is confirmed: commit b2b5ce0 renamed endpoints from
/logs,/metrics,/tracesto/api/v1/logs,/api/v1/metrics,/api/v1/traces. Adding temporary backward-compatibility aliases is a recommended best practice for smooth client migration.However, no external documentation or examples referencing the old vivo exporter endpoints were found in the codebase, suggesting limited external client impact. The aliases remain a good practice but may be optional depending on your versioning and stability commitments.
mk_vhost_handler(ph->ctx, vid, "/api/v1/logs", cb_logs, ctx); mk_vhost_handler(ph->ctx, vid, "/api/v1/metrics", cb_metrics, ctx); mk_vhost_handler(ph->ctx, vid, "/api/v1/traces", cb_traces, ctx); mk_vhost_handler(ph->ctx, vid, "/api/v1/internal/metrics", cb_internal_metrics, ctx); + /* Temporary backward-compat aliases (remove in a future major release) */ + mk_vhost_handler(ph->ctx, vid, "/logs", cb_logs, ctx); + mk_vhost_handler(ph->ctx, vid, "/metrics", cb_metrics, ctx); + mk_vhost_handler(ph->ctx, vid, "/traces", cb_traces, ctx);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
plugins/out_vivo_exporter/vivo.c(4 hunks)plugins/out_vivo_exporter/vivo.h(2 hunks)plugins/out_vivo_exporter/vivo_http.c(5 hunks)plugins/out_vivo_exporter/vivo_http.h(2 hunks)plugins/out_vivo_exporter/vivo_stream.c(3 hunks)plugins/out_vivo_exporter/vivo_stream.h(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
plugins/out_vivo_exporter/vivo_http.h (1)
plugins/out_vivo_exporter/vivo_http.c (1)
vivo_http_serve_content(111-178)
plugins/out_vivo_exporter/vivo.c (5)
src/flb_mp.c (6)
flb_mp_map_header_init(316-328)flb_mp_map_header_append(345-349)flb_mp_array_header_init(330-342)flb_mp_array_header_append(351-355)flb_mp_array_header_end(367-375)flb_mp_map_header_end(357-365)src/flb_input.c (1)
flb_input_name(790-797)src/flb_log_event_decoder.c (2)
flb_log_event_decoder_next(310-406)flb_log_event_decoder_destroy(147-179)src/flb_pack.c (1)
flb_msgpack_raw_to_json_sds(1026-1085)src/flb_sds.c (2)
flb_sds_cat_safe(204-214)flb_sds_destroy(389-399)
plugins/out_vivo_exporter/vivo_http.c (4)
plugins/out_vivo_exporter/vivo_stream.c (1)
vivo_stream_get_content(137-201)src/flb_sds.c (3)
flb_sds_create_size(92-95)flb_sds_printf(336-387)flb_sds_destroy(389-399)src/flb_metrics_exporter.c (1)
flb_me_get_cmetrics(266-372)src/flb_pack.c (1)
flb_msgpack_raw_to_json_sds(1026-1085)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (30)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
- GitHub Check: PR - fuzzing test
- GitHub Check: pr-compile-without-cxx (3.31.6)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-centos-7
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
🔇 Additional comments (6)
plugins/out_vivo_exporter/vivo.h (1)
26-27: ctx->config pointer added — clarify ownership/lifetimeLooks good. Please document that vivo_exporter does not own struct flb_config (valid until cb_vivo_exit) to avoid misuse later.
Also applies to: 45-46
plugins/out_vivo_exporter/vivo_http.h (1)
28-33: New stream ID headers and public serve helper — LGTMHeader names and prototype align with vivo_http.c usage. No issues spotted.
Also applies to: 62-63
plugins/out_vivo_exporter/vivo_stream.h (1)
55-59: API change verification complete - all call sites correctly updatedThe signature change is properly integrated:
- All declarations in the header (vivo_stream.h:55-58) and implementation (vivo_stream.c:137-140) include stream_next_id parameter
- The single call site in vivo_http.c:129-131 correctly passes &stream_next_id
- The implementation correctly assigns vs->entries_added to stream_next_id (line 164)
The optional documentation suggestion remains unimplemented—no inline comments explain that stream_next_id represents the next entry ID counter (set from vs->entries_added).
plugins/out_vivo_exporter/vivo_http.c (3)
22-26: Includes look appropriate for new functionality.
Required headers for config, msgpack-to-JSON, and cmetrics encoding are correctly added.
142-148: No issues found—VIVO_STREAM_NEXT_ID is properly defined and correctly used.The macro is defined in vivo_http.h (line 30) as
#define VIVO_STREAM_NEXT_ID "Vivo-Stream-Next-ID", and it's used correctly in vivo_http.c (line 146). The header name casing follows proper HTTP header conventions. All related stream ID macros (START, END, NEXT) are defined consistently and used throughout the codebase.
111-118: No issues found; all call sites properly finalize responses.Verification shows all three call sites of
vivo_http_serve_content()correctly callmk_http_done()immediately after invocation (lines 187–188, 198–199, 209–210). No non-callback callers were discovered. The function's contract is being honored in the current codebase.
| str_next = flb_sds_create_size(32); | ||
| flb_sds_printf(&str_next, "%" PRId64, stream_next_id); | ||
|
|
||
| mk_http_header(request, | ||
| VIVO_STREAM_NEXT_ID, sizeof(VIVO_STREAM_NEXT_ID) - 1, | ||
| str_next, flb_sds_len(str_next)); | ||
|
|
There was a problem hiding this comment.
Possible NULL deref from unchecked flb_sds_printf; use stack buffers.
flb_sds_printf can return NULL on allocation failure; passing NULL to flb_sds_len/mk_http_header can crash. Same pattern exists for start/end IDs.
Replace all three sds prints with snprintf stack buffers:
- str_next = flb_sds_create_size(32);
- flb_sds_printf(&str_next, "%" PRId64, stream_next_id);
- mk_http_header(request,
- VIVO_STREAM_NEXT_ID, sizeof(VIVO_STREAM_NEXT_ID) - 1,
- str_next, flb_sds_len(str_next));
+ {
+ char next_buf[32];
+ int next_len = snprintf(next_buf, sizeof(next_buf), "%" PRId64, stream_next_id);
+ if (next_len < 0) { mk_http_status(request, 500); return; }
+ mk_http_header(request, VIVO_STREAM_NEXT_ID, sizeof(VIVO_STREAM_NEXT_ID) - 1,
+ next_buf, (size_t) next_len);
+ }- str_start = flb_sds_create_size(32);
- flb_sds_printf(&str_start, "%" PRId64, stream_start_id);
- str_end = flb_sds_create_size(32);
- flb_sds_printf(&str_end, "%" PRId64, stream_end_id);
- mk_http_header(request,
- VIVO_STREAM_START_ID, sizeof(VIVO_STREAM_START_ID) - 1,
- str_start, flb_sds_len(str_start));
- mk_http_header(request,
- VIVO_STREAM_END_ID, sizeof(VIVO_STREAM_END_ID) - 1,
- str_end, flb_sds_len(str_end));
+ {
+ char start_buf[32], end_buf[32];
+ int start_len = snprintf(start_buf, sizeof(start_buf), "%" PRId64, stream_start_id);
+ int end_len = snprintf(end_buf, sizeof(end_buf), "%" PRId64, stream_end_id);
+ if (start_len < 0 || end_len < 0) { mk_http_status(request, 500); return; }
+ mk_http_header(request, VIVO_STREAM_START_ID, sizeof(VIVO_STREAM_START_ID) - 1,
+ start_buf, (size_t) start_len);
+ mk_http_header(request, VIVO_STREAM_END_ID, sizeof(VIVO_STREAM_END_ID) - 1,
+ end_buf, (size_t) end_len);
+ }Also remove now-unneeded flb_sds_destroy(str_start/str_end/str_next).
Also applies to: 156-168
|
@codex review |
edsiper
changed the title
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".
| /* Convert the complete msgpack structure to JSON */ | ||
| out_js = flb_msgpack_raw_to_json_sds(tmp_sbuf.data, tmp_sbuf.size, | ||
| config->json_escape_unicode); | ||
|
|
||
| msgpack_sbuffer_destroy(&tmp_sbuf); | ||
|
|
||
| return out_buf; | ||
| /* append a newline */ | ||
| flb_sds_cat_safe(&out_js, "\n", 1); | ||
|
|
||
| if (!out_js) { |
There was a problem hiding this comment.
After converting the msgpack buffer with flb_msgpack_raw_to_json_sds, the code immediately calls flb_sds_cat_safe(&out_js, "\n", 1) and only then checks if (!out_js). flb_sds_cat_safe dereferences the pointer passed in, so if flb_msgpack_raw_to_json_sds fails (e.g. out of memory or malformed data), this dereference will segfault instead of propagating the error. The previous version checked for NULL before operating on the buffer. Consider testing out_js for NULL before calling flb_sds_cat_safe so the function can return an error rather than crashing.
Useful? React with 👍 / 👎.
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/out_vivo_exporter/vivo.c (1)
343-343: Check return value offlb_sds_cat_safe.Unlike the updated
format_logsfunction (lines 286-290), this call toflb_sds_cat_safedoesn't check the return value. If the append fails due to allocation failure, thejsonbuffer may be unchanged or invalid, and the code will proceed to write potentially corrupted data to the stream.Apply this diff to add error handling:
- flb_sds_cat_safe(&json, "\n", 1); + if (flb_sds_cat_safe(&json, "\n", 1) < 0) { + flb_sds_destroy(json); + flb_plg_error(ctx->ins, "cannot append newline to JSON"); + return -1; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/out_vivo_exporter/vivo.c(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/out_vivo_exporter/vivo.c (5)
src/flb_mp.c (6)
flb_mp_map_header_init(316-328)flb_mp_map_header_append(345-349)flb_mp_array_header_init(330-342)flb_mp_array_header_append(351-355)flb_mp_array_header_end(367-375)flb_mp_map_header_end(357-365)src/flb_input.c (1)
flb_input_name(790-797)src/flb_log_event_decoder.c (2)
flb_log_event_decoder_next(310-406)flb_log_event_decoder_destroy(147-179)src/flb_pack.c (1)
flb_msgpack_raw_to_json_sds(1026-1085)src/flb_sds.c (2)
flb_sds_destroy(389-399)flb_sds_cat_safe(204-214)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (30)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-without-cxx (3.31.6)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-centos-7
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
- GitHub Check: PR - fuzzing test
🔇 Additional comments (6)
plugins/out_vivo_exporter/vivo.c (6)
32-53: LGTM: Helper function is well-implemented.The
find_map_valuefunction properly validates inputs, safely iterates the map, and correctly compares keys.
274-290: Good: Null check now properly placed before string concatenation.The past review comments flagged that
flb_sds_cat_safewas called before checking ifout_jsis NULL. This is now fixed—line 280 checksif (!out_js)before line 286 callsflb_sds_cat_safe, preventing the segfault on JSON conversion failure.
297-324: LGTM: Function signature correctly updated.The addition of the
src_insparameter is consistent with theformat_logssignature change and properly passes the input instance for source resolution.
374-374: LGTM: Config stored in context for later use.Storing the config pointer in the context enables functions like
format_logsto access configuration without requiring it as a parameter in all intermediate calls.
440-440: LGTM: Input instance properly passed to log processing.The addition of the
insparameter correctly threads the input instance through tologs_event_chunk_append, enabling source name resolution informat_logs.
163-179: Based on the decoder implementation shown in the output, the code's assumption is verified as correct. The log event decoder guarantees pointer identity for group metadata and attributes within the same group:
- When a
GROUP_STARTrecord is encountered, the decoder stores the metadata and attributes pointers in its context (lines 385-386)- For all subsequent records in the same group, the decoder copies these same pointer values from context to each event (lines 400-401)
- This ensures all events within a group share identical pointer values
The pointer inequality comparison in vivo.c correctly identifies mismatches because events from different groups will have different pointer values, while events within the same group will always have matching pointers.
| int i; | ||
| char *name; | ||
| flb_sds_t out_js; | ||
| flb_sds_t out_buf = NULL; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused variable out_buf.
The variable out_buf is allocated at line 87 but never written to or read from. It's only destroyed on error paths and at the end. This is dead code left over from the previous implementation that accumulated JSON per record.
Apply this diff to remove the unused variable:
int result;
char *name;
flb_sds_t out_js;
- flb_sds_t out_buf = NULL;
msgpack_sbuffer tmp_sbuf;
msgpack_packer tmp_pck; return NULL;
}
- out_buf = flb_sds_create_size((event_chunk->size * 2) / 4);
- if (!out_buf) {
- flb_errno();
- return NULL;
- }
-
/* Create temporary msgpack buffer */
msgpack_sbuffer_init(&tmp_sbuf); msgpack_sbuffer_destroy(&tmp_sbuf);
if (!out_js) {
- flb_sds_destroy(out_buf);
return NULL;
} /* append a newline */
if (flb_sds_cat_safe(&out_js, "\n", 1) < 0) {
flb_sds_destroy(out_js);
- flb_sds_destroy(out_buf);
return NULL;
}
- /* Replace out_buf with the complete JSON */
- flb_sds_destroy(out_buf);
return out_js;Also applies to: 87-91, 281-281, 288-288, 293-293
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
Improvements