lib: updates for cmetrics and monkey #10870
Conversation
Signed-off-by: Eduardo Silva <[email protected]>
WalkthroughRenames and retargets the Prometheus text decoder CMake symbols, gates its public header/API behind CMT_HAVE_PROMETHEUS_TEXT_DECODER, conditionally installs headers, always includes Prometheus remote-write/protobuf sources, fixes several cmetrics logic issues and tests, bumps Monkey patch version and request init, and upgrades CI actions to v5. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant CMake as Configure
participant Build as Build System
participant Install as Installer
Dev->>CMake: configure (CMT_PROMETHEUS_TEXT_DECODER on/off)
CMake->>Build: check for FLEX & BISON
alt FLEX && BISON found
CMake->>Build: define CMT_HAVE_PROMETHEUS_TEXT_DECODER
CMake->>Build: generate lexer/parser and enable API
CMake->>Install: include cmt_decode_prometheus.h
else missing tools
CMake-->>Build: skip text-decoder generation
CMake-->>Install: exclude cmt_decode_prometheus.h
end
Note right of Build: Prometheus remote-write + protobuf sources compiled unconditionally
sequenceDiagram
autonumber
participant mk as mk_http
participant Req as request
participant Stream as stream
mk->>Req: mk_http_request_init()
Req->>Req: init in_headers (type, dynamic, callbacks, buffer, bytes_total)
Req->>Stream: in_headers.stream = &request->stream
Req->>Stream: append in_headers to stream.inputs
Note right of mk: previous manual init in mk_http_init removed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
✨ Finishing Touches
🧪 Generate unit tests
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/monkey/mk_server/mk_http.c (1)
1252-1266: Leaked FD and wrong descriptor used for error page streaming.You open fd for the custom error page but pass sr->file_fd to mk_stream_in_file, leaking fd and likely sending from an unrelated/invalid descriptor.
Apply this fix:
- /* open file */ - fd = open(entry->real_path, server->open_flags); + /* open file */ + fd = open(entry->real_path, server->open_flags); if (fd == -1) { break; } - /* This fd seems to be leaked, we need to verify this logic */ + /* Stream will own/close 'fd' via in_file once enqueued */ /* Outgoing headers */ sr->headers.content_length = finfo.size; sr->headers.real_length = finfo.size; mk_header_prepare(cs, sr, server); /* Stream setup */ - mk_stream_in_file(&sr->stream, &sr->in_file, sr->file_fd, + mk_stream_in_file(&sr->stream, &sr->in_file, fd, finfo.size, 0, NULL, NULL); return MK_EXIT_OK;lib/cmetrics/tests/CMakeLists.txt (1)
17-22: Define the new option in the top-level CMakeLists.txt
Root CMakeLists.txt must declareoption(CMT_BUILD_PROMETHEUS_TEXT_DECODER "Enable Prometheus text decoder" OFF)immediately after the
project()call so the flag propagates to all subdirectories.
🧹 Nitpick comments (7)
lib/monkey/mk_server/mk_http.c (2)
98-107: Early in_headers setup is good; also prep in_headers_extra (or confirm it’s wired).Great move initializing and enqueuing the headers IOV input up-front. Two follow-ups:
- If sr->headers._extra_rows is used, ensure in_headers_extra is likewise initialized during request init (or confirmed to be fully set up and appended inside mk_header_prepare). Otherwise, later increments to in_headers_extra.bytes_total won’t be flushed.
Apply this diff to mirror the initialization (deferring list_add until populated by header prepare logic):
/* Initialize headers input stream */ request->in_headers.type = MK_STREAM_IOV; request->in_headers.dynamic = MK_FALSE; request->in_headers.cb_consumed = NULL; request->in_headers.cb_finished = NULL; request->in_headers.buffer = NULL; request->in_headers.bytes_total = 0; request->in_headers.stream = &request->stream; mk_list_add(&request->in_headers._head, &request->stream.inputs); + + /* Prepare optional extra-headers IOV input; will be populated on demand */ + request->in_headers_extra.type = MK_STREAM_IOV; + request->in_headers_extra.dynamic = MK_FALSE; + request->in_headers_extra.cb_consumed = NULL; + request->in_headers_extra.cb_finished = NULL; + request->in_headers_extra.buffer = NULL; + request->in_headers_extra.bytes_total = 0; + request->in_headers_extra.stream = &request->stream; + /* NOTE: append to stream.inputs only when actually populated */
1338-1346: Double-check in_headers_extra is actually attached to the stream when used.You increment in_headers_extra.bytes_total when headers._extra_rows exists, but if in_headers_extra was never appended to sr->stream.inputs, those bytes won’t be written. Confirm mk_header_prepare attaches in_headers_extra whenever _extra_rows is present; otherwise, append it here before mk_channel_write.
lib/cmetrics/src/cmt_histogram.c (1)
38-39: Comment mismatches implementation.The “+inf” bucket is handled in the counts array, not in
upper_bounds. Consider clarifying this comment to avoid future off-by-one mistakes.lib/cmetrics/tests/issues.c (2)
25-26: Gate Prometheus includes with the feature macro (clarity).Since the tests are conditionally compiled, consider guarding these includes too for symmetry and to avoid accidental header exposure when the feature is off.
-#include <cmetrics/cmt_decode_prometheus.h> -#include <cmetrics/cmt_encode_prometheus.h> +#ifdef CMT_HAVE_PROMETHEUS_TEXT_DECODER +#include <cmetrics/cmt_decode_prometheus.h> +#include <cmetrics/cmt_encode_prometheus.h> +#endif
94-122: Solid test; consider skipping timestamps to minimize noise.Using CMT_FALSE avoids extra timestamp output and makes the intent crisper; the substring check still works either way.
- result = cmt_encode_prometheus_create(cmt, CMT_TRUE); + result = cmt_encode_prometheus_create(cmt, CMT_FALSE);lib/cmetrics/CMakeLists.txt (2)
82-87: Option rename is clear and aligned with feature scope.CMT_PROMETHEUS_TEXT_DECODER reads better. Consider a compatibility shim for external consumers using the old name.
option(CMT_PROMETHEUS_TEXT_DECODER "Enable prometheus text format decoder (requires Flex/Bison)" Yes) +set(CMT_ENABLE_PROMETHEUS_DECODER "${CMT_PROMETHEUS_TEXT_DECODER}" CACHE BOOL + "Deprecated: use CMT_PROMETHEUS_TEXT_DECODER" FORCE)
169-184: Add explicit enable/disable STATUS messages for Prometheus text decoder
In lib/cmetrics/CMakeLists.txt (around line 179), extend the existing Flex/Bison gate with anelse()branch that setsCMT_BUILD_PROMETHEUS_TEXT_DECODERto 0 and emits a clear disable message, and add amessage(STATUS …)in the enable path to summarize the Flex/Bison executables:if(FLEX_FOUND AND BISON_FOUND) set(CMT_BUILD_PROMETHEUS_TEXT_DECODER 1) CMT_DEFINITION(CMT_HAVE_PROMETHEUS_TEXT_DECODER) + message(STATUS "Prometheus text decoder: enabled (Flex: ${FLEX_EXECUTABLE}; Bison: ${BISON_EXECUTABLE})") else() set(CMT_BUILD_PROMETHEUS_TEXT_DECODER 0) + message(STATUS "Prometheus text decoder: disabled (Flex/Bison not found)") endif()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
lib/cmetrics/.github/workflows/build.yaml(4 hunks)lib/cmetrics/.github/workflows/lint.yaml(2 hunks)lib/cmetrics/.github/workflows/packages.yaml(4 hunks)lib/cmetrics/CMakeLists.txt(4 hunks)lib/cmetrics/include/CMakeLists.txt(1 hunks)lib/cmetrics/include/cmetrics/cmt_decode_prometheus.h(2 hunks)lib/cmetrics/src/CMakeLists.txt(3 hunks)lib/cmetrics/src/cmt_cat.c(1 hunks)lib/cmetrics/src/cmt_decode_prometheus.c(1 hunks)lib/cmetrics/src/cmt_histogram.c(1 hunks)lib/cmetrics/tests/CMakeLists.txt(1 hunks)lib/cmetrics/tests/issues.c(2 hunks)lib/monkey/CMakeLists.txt(1 hunks)lib/monkey/mk_server/mk_http.c(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/cmetrics/tests/issues.c (2)
lib/cmetrics/src/cmt_decode_prometheus.c (2)
cmt_decode_prometheus_create(91-142)cmt_decode_prometheus_destroy(144-147)lib/cmetrics/src/cmt_encode_prometheus.c (2)
cmt_encode_prometheus_create(491-538)cmt_encode_prometheus_destroy(540-543)
⏰ 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). (29)
- 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 (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- 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-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, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: pr-compile-centos-7
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 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_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: PR - fuzzing test
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 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_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=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 (-DFLB_SIMD=Off, 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_SMALL=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_SMALL=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=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_UNDEFINED=On, 3.31.6, gcc, g++)
🔇 Additional comments (13)
lib/monkey/CMakeLists.txt (1)
26-26: Approve version bump to 1.8.5; no stale 1.8.4 in lib/monkey
CMakeLists and mk_info.h.in macros are correctly updated; unable to locatedocs/manpages/templates—please manually verify any documentation or manpage templates for lingering 1.8.4 references.lib/cmetrics/.github/workflows/build.yaml (1)
22-22: actions/checkout → v5 looks good.Upgrade is straightforward and consistent across jobs.
Also applies to: 139-139, 173-173, 203-203
lib/cmetrics/.github/workflows/lint.yaml (2)
13-13: actions/checkout v5LGTM.
22-22: actions/checkout v5LGTM.
lib/cmetrics/.github/workflows/packages.yaml (1)
21-21: Workflow actions bumped to v5Checkout and download-artifact upgrades look correct.
Also applies to: 57-57, 84-84, 112-112
lib/cmetrics/src/cmt_decode_prometheus.c (1)
177-179: Good fix: initialize ns/subsystem when no '_' present.Prevents uninitialized outputs and aligns with
reset_contextfree logic.lib/cmetrics/include/CMakeLists.txt (1)
1-22: No update needed: BUILD and HAVE macros are correctly paired
Confirmed in lib/cmetrics/CMakeLists.txt that CMT_BUILD_PROMETHEUS_TEXT_DECODER and CMT_HAVE_PROMETHEUS_TEXT_DECODER are set together under the same if(FLEX_FOUND AND BISON_FOUND) block, so header installation gating matches compile-time symbols.lib/cmetrics/tests/issues.c (1)
126-128: Test registration looks good.lib/cmetrics/src/CMakeLists.txt (3)
1-9: Rename of gating var is consistent with top-level CMake.
69-74: Appending Flex/Bison outputs under the new flag looks correct.
41-48: Protobuf-C runtime is vendored in cmetrics/src/external/protobuf-c.c, so the hard dependency is satisfied and no build changes are needed.lib/cmetrics/include/cmetrics/cmt_decode_prometheus.h (2)
23-26: Header gating pattern is correct.Including cmt_info.h unconditionally while hiding the decoder API behind CMT_HAVE_PROMETHEUS_TEXT_DECODER is the right balance.
117-118: Guard close matches the open; no API leakage when disabled.
lib/cmetrics/CMakeLists.txt
Outdated
| message(STATUS "include <ctype.h> | ||
| #include <errno.h> | ||
| #include <math.h> | ||
| #include <stdarg.h> | ||
| #include <stdint.h> | ||
| #include <cmetrics/cmetrics.h> | ||
| #include <cmetrics/cmt_gauge.h> | ||
| #include <cmetrics/cmt_untyped.h> | ||
| #include <cmetrics/cmt_histogram.h> | ||
| #include <cmetrics/cmt_summary.h> | ||
| #include <cmetrics/cmt_counter.h>Found Bison keg installed by Homebrew at ${BREW_BISON_PREFIX}") |
There was a problem hiding this comment.
Fix accidental paste in message(): C header lines leaked into status output.
This will spew a large multi-line string into CMake logs. Replace with the intended concise message.
- message(STATUS "include <ctype.h>
-#include <errno.h>
-#include <math.h>
-#include <stdarg.h>
-#include <stdint.h>
-
-#include <cmetrics/cmetrics.h>
-#include <cmetrics/cmt_gauge.h>
-#include <cmetrics/cmt_untyped.h>
-#include <cmetrics/cmt_histogram.h>
-#include <cmetrics/cmt_summary.h>
-#include <cmetrics/cmt_counter.h>Found Bison keg installed by Homebrew at ${BREW_BISON_PREFIX}")
+ message(STATUS "Found Bison keg installed by Homebrew at ${BREW_BISON_PREFIX}")🤖 Prompt for AI Agents
In lib/cmetrics/CMakeLists.txt around lines 27 to 38, a block of C header lines
was accidentally pasted into a message(STATUS ...) call, causing a large
multi-line string to be emitted to CMake logs; replace the pasted C headers with
the intended concise status message (for example "Found Bison keg installed by
Homebrew at ${BREW_BISON_PREFIX}") so message(STATUS ...) only outputs the
single intended line and remove the stray header text.
| for (i = 0; i < histogram->buckets->count; i++) { | ||
| /* histogram buckets are always integers, no need to convert them */ | ||
| metric_dst->hist_buckets[i] += metric_src->hist_buckets[i]; | ||
| } |
There was a problem hiding this comment.
Histogram concatenation drops the +Inf bucket.
hist_buckets has length count + 1; iterating with < count excludes the +Inf bucket at index count, producing incorrect merged histograms.
Apply:
- for (i = 0; i < histogram->buckets->count; i++) {
+ for (i = 0; i <= histogram->buckets->count; i++) {📝 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.
| for (i = 0; i < histogram->buckets->count; i++) { | |
| /* histogram buckets are always integers, no need to convert them */ | |
| metric_dst->hist_buckets[i] += metric_src->hist_buckets[i]; | |
| } | |
| for (i = 0; i <= histogram->buckets->count; i++) { | |
| /* histogram buckets are always integers, no need to convert them */ | |
| metric_dst->hist_buckets[i] += metric_src->hist_buckets[i]; | |
| } |
🤖 Prompt for AI Agents
In lib/cmetrics/src/cmt_cat.c around lines 111 to 114, the loop iterates only up
to histogram->buckets->count - 1 and thus omits the +Inf bucket at index count;
update the iteration to include the final bucket (either change the loop
condition to iterate up to and including count, or compute bucket_len =
histogram->buckets->count + 1 and loop i < bucket_len) so
metric_dst->hist_buckets accumulates the +Inf bucket as well.
|
|
||
| /* besides buckets set by the user, we add an implicit bucket for +inf */ | ||
| upper_bounds = calloc(1, sizeof(double) * (count + 1)); | ||
| upper_bounds = calloc(1, sizeof(double) * count + 1); |
There was a problem hiding this comment.
Fix calloc under-allocation (parentheses bug).
Allocates sizeof(double) * count + 1 bytes instead of (count + 1) * sizeof(double) (or just count * sizeof(double)). This can corrupt memory when writing bucket bounds.
Apply one of the safe patterns (consistent with the rest of this file, upper_bounds has length = count):
- upper_bounds = calloc(1, sizeof(double) * count + 1);
+ upper_bounds = calloc(count, sizeof(double));📝 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.
| upper_bounds = calloc(1, sizeof(double) * count + 1); | |
| upper_bounds = calloc(count, sizeof(double)); |
🤖 Prompt for AI Agents
In lib/cmetrics/src/cmt_histogram.c around line 39, the calloc call is
under-allocating due to missing parentheses (it does sizeof(double) * count +
1); replace it with a correct allocation matching the intended length: use
calloc(count, sizeof(double)) if upper_bounds length == count (or calloc(count +
1, sizeof(double)) if an extra slot is needed), ensuring the multiplication is
done before addition; keep the allocation pattern consistent with the rest of
the file and check for NULL return as done elsewhere.
Signed-off-by: Eduardo Silva <[email protected]>
edsiper
force-pushed
the
lib-updates-20250910
branch
from
d24bad8 to
6163314
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/cmetrics/src/cmt_histogram.c (1)
39-41: Fix calloc under-allocation (precedence bug).
sizeof(double) * count + 1allocates one extra byte, not an extra element; this under-allocates and risks OOB writes. Allocate exactlycountdoubles (the +Inf bucket is handled separately in the metric array).- upper_bounds = calloc(1, sizeof(double) * count + 1); + /* allocate 'count' bounds; +Inf bucket is handled separately */ + upper_bounds = calloc(count, sizeof(double));
🧹 Nitpick comments (3)
lib/cmetrics/src/cmt_histogram.c (1)
110-111: Normalize calloc usage to the canonical form to prevent future precedence bugs.Use
calloc(count, sizeof(double))consistently; avoids repeating this class of mistake.- upper_bounds = calloc(1, sizeof(double) * count); + upper_bounds = calloc(count, sizeof(double));- upper_bounds = calloc(1, sizeof(double) * count); + upper_bounds = calloc(count, sizeof(double));Also applies to: 157-159
lib/cmetrics/src/CMakeLists.txt (2)
1-1: Rename is fine; add a compat alias and align gating var.To avoid breaking cached builds or external scripts still setting CMT_BUILD_PROMETHEUS_DECODER, add a small alias + deprecation notice, and ensure the rest of the tree consistently uses the new name.
Add near the top of this file (or the parent CMakeLists):
# Backward-compat for older cache/CI if (DEFINED CMT_BUILD_PROMETHEUS_DECODER AND NOT DEFINED CMT_BUILD_PROMETHEUS_TEXT_DECODER) set(CMT_BUILD_PROMETHEUS_TEXT_DECODER "${CMT_BUILD_PROMETHEUS_DECODER}") message(DEPRECATION "CMT_BUILD_PROMETHEUS_DECODER is deprecated; use CMT_BUILD_PROMETHEUS_TEXT_DECODER") endif()Also verify downstream guards (headers/tests) consistently key off the same final symbol (e.g., CMT_HAVE_PROMETHEUS_TEXT_DECODER vs build option).
69-74: Gate lexer/parser outputs on tool availability to prevent configure-time failures.If the option is ON but Flex/Bison aren’t present, configuration may fail. Either key off a “HAVE” variable set when tools are found, or add tool checks here.
Apply one of:
- if (CMT_BUILD_PROMETHEUS_TEXT_DECODER) + if (CMT_BUILD_PROMETHEUS_TEXT_DECODER AND FLEX_FOUND AND BISON_FOUND)—or, if a consolidated symbol exists—
- if (CMT_BUILD_PROMETHEUS_TEXT_DECODER) + if (CMT_HAVE_PROMETHEUS_TEXT_DECODER)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
lib/cmetrics/.github/workflows/build.yaml(4 hunks)lib/cmetrics/.github/workflows/lint.yaml(2 hunks)lib/cmetrics/.github/workflows/packages.yaml(4 hunks)lib/cmetrics/CMakeLists.txt(3 hunks)lib/cmetrics/include/CMakeLists.txt(1 hunks)lib/cmetrics/include/cmetrics/cmt_decode_prometheus.h(2 hunks)lib/cmetrics/src/CMakeLists.txt(3 hunks)lib/cmetrics/src/cmt_cat.c(1 hunks)lib/cmetrics/src/cmt_decode_prometheus.c(1 hunks)lib/cmetrics/src/cmt_histogram.c(1 hunks)lib/cmetrics/tests/CMakeLists.txt(1 hunks)lib/cmetrics/tests/issues.c(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- lib/cmetrics/.github/workflows/build.yaml
- lib/cmetrics/tests/CMakeLists.txt
- lib/cmetrics/tests/issues.c
- lib/cmetrics/include/CMakeLists.txt
- lib/cmetrics/src/cmt_cat.c
- lib/cmetrics/include/cmetrics/cmt_decode_prometheus.h
- lib/cmetrics/.github/workflows/packages.yaml
- lib/cmetrics/CMakeLists.txt
- lib/cmetrics/src/cmt_decode_prometheus.c
- lib/cmetrics/.github/workflows/lint.yaml
⏰ 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). (29)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- 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 (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 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_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_SIMD=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_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- 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 (-DSANITIZE_ADDRESS=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_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
- 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, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 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, gcc, g++)
- 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, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-centos-7
- 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-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
- GitHub Check: PR - fuzzing test
| # Add Prometheus remote write decoder (always available, only needs protobuf) | ||
| set(src ${src} | ||
| cmt_decode_prometheus_remote_write.c | ||
| # Prometheus related protobuf files | ||
| external/remote.pb-c.c | ||
| external/types.pb-c.c | ||
| ) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Always-on remote write: ensure protobuf-c is linked and propagate it; also prefer list(APPEND).
Compiling external/*.pb-c.c introduces a hard link-time dependency on protobuf-c. cmetrics-static currently doesn’t declare/propagate that, which can break final linkage with static libs due to order/transitivity.
- Within this hunk, switch to list(APPEND) for clarity:
- set(src ${src}
- cmt_decode_prometheus_remote_write.c
- # Prometheus related protobuf files
- external/remote.pb-c.c
- external/types.pb-c.c
- )
+ list(APPEND src
+ cmt_decode_prometheus_remote_write.c
+ # Prometheus remote write protobufs
+ external/remote.pb-c.c
+ external/types.pb-c.c
+ )- Ensure cmetrics-static links protobuf-c and propagates it (adjust the exact target name to match your build, e.g., protobuf-c or protobuf-c::protobuf-c):
# Replace the existing line with a keyworded form and add protobuf-c
target_link_libraries(cmetrics-static
PUBLIC mpack-static cfl-static fluent-otel-proto protobuf-c)- If protobuf-c discovery isn’t guaranteed earlier, fail fast with a clear message (optional):
if (NOT TARGET protobuf-c AND NOT PROTOBUF_C_FOUND)
message(FATAL_ERROR "protobuf-c is required when building Prometheus remote write decoder")
endif()Verification script to confirm discovery/links:
🏁 Script executed:
#!/bin/bash
# Check old var usages
rg -n --hidden -C2 '\bCMT_BUILD_PROMETHEUS_DECODER\b'
# Check protobuf-c discovery/targets
rg -n -C2 'find_package\((ProtobufC|protobuf-c)\b|pkg_check_modules\((PROTOBUF_C|protobuf-c)\b|target_link_libraries\([^)]+protobuf-?c' cmake CMakeLists.txt lib -g '!**/build/**'Length of output: 0
🏁 Script executed:
#!/bin/bash
set -e
# Search for any find_package(ProtobufC) or find_package(protobuf-c) in CMake files
rg -n --type cmake 'find_package\([^)]*(ProtobufC|protobuf-c)' -C2 .
# Search for pkg_check_modules(PROTOBUF_C) in CMake files
rg -n --type cmake 'pkg_check_modules\([^)]*PROTOBUF_C' -C2 .
# Search for target_link_libraries references to protobuf libraries
rg -n 'target_link_libraries\([^)]+protobuf' -C2 .
# Check for the custom build flag CMT_BUILD_PROMETHEUS_DECODER
rg -n 'CMT_BUILD_PROMETHEUS_DECODER' -C2 .Length of output: 0
🏁 Script executed:
#!/bin/bash
set -e
sed -n '1,200p' lib/cmetrics/src/CMakeLists.txtLength of output: 2668
Always-on remote write: link protobuf-c and switch to list(APPEND)
- In lib/cmetrics/src/CMakeLists.txt (around the block that adds
external/*.pb-c.c), replacewithset(src ${src} cmt_decode_prometheus_remote_write.c external/remote.pb-c.c external/types.pb-c.c )
list(APPEND src cmt_decode_prometheus_remote_write.c external/remote.pb-c.c external/types.pb-c.c )
- Add a
find_package(ProtobufC REQUIRED)(or your preferred discovery) before appending the.pb-c.cfiles. - Update the static target to propagate protobuf-c:
target_link_libraries(cmetrics-static PUBLIC mpack-static cfl-static fluent-otel-proto protobuf-c )
- (Optional) Fail fast if protobuf-c isn’t available:
if (NOT TARGET protobuf-c AND NOT PROTOBUF_C_FOUND) message(FATAL_ERROR "protobuf-c is required for the Prometheus remote-write decoder") endif()
🤖 Prompt for AI Agents
In lib/cmetrics/src/CMakeLists.txt around lines 41 to 48, replace the set(...)
that adds cmt_decode_prometheus_remote_write.c and the generated
external/*.pb-c.c files with list(APPEND src ...) so the files are appended
instead of resetting the variable; add a find_package(ProtobufC REQUIRED) (or
equivalent discovery) before appending the .pb-c.c files; update the
cmetrics-static target to link against protobuf-c by adding protobuf-c to
target_link_libraries(cmetrics-static PUBLIC ...); and (optionally) add a short
check that fails fast with message(FATAL_ERROR ...) if protobuf-c isn’t found
(i.e., if neither TARGET protobuf-c nor PROTOBUF_C_FOUND).
Signed-off-by: Eduardo Silva <[email protected]>
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)
src/http_server/flb_http_server_http1.c (1)
86-94: Bug: SDS length not updated after memmove, leaving stale tail bytes.After compacting the buffer, the cfl_sds length header remains unchanged. Downstream cfl_sds_len() calls will see the old length. Update the SDS length.
if ((session_buffer_length - request_length) > 0) { - session_buffer_length -= request_length; - - memmove(session_buffer, - &session_buffer[request_length], - session_buffer_length); - - session_buffer[session_buffer_length] = '\0'; + session_buffer_length -= request_length; + + memmove(session_buffer, + &session_buffer[(size_t) request_length], + session_buffer_length); + + cfl_sds_set_len(session_buffer, session_buffer_length); + session_buffer[session_buffer_length] = '\0'; }
🧹 Nitpick comments (3)
src/http_server/flb_http_server_http1.c (3)
456-460: Redundant zeroing before init — remove duplicate memset.dummy_mk_http_request_init() already memset()s the request. The extra memset here is redundant and could mask future cleanup paths.
- memset(&session->inner_request, 0, sizeof(struct mk_http_request));
20-22: Add time.h for time(NULL).Some toolchains won’t pull time() via indirect includes; include it explicitly.
#include <fluent-bit/http_server/flb_http_server.h> #include <string.h> +#include <time.h>If you adopt the ssize_t change above, also include:
+#include <sys/types.h>
491-498: Silence unused-parameter warnings.buffer and length aren’t used; cast to void to keep builds clean.
int flb_http1_server_session_ingest(struct flb_http1_server_session *session, unsigned char *buffer, size_t length) { int result; + (void) buffer; + (void) length;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/http_server/flb_http_server_http1.c(2 hunks)
⏰ 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). (29)
- 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_ARROW=On, 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_SIMD=Off, 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_SIMD=On, 3.31.6, clang, clang++)
- 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=Off, 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_COVERAGE=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: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- 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-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: 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: pr-compile-centos-7
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
- GitHub Check: PR - fuzzing test
🔇 Additional comments (2)
src/http_server/flb_http_server_http1.c (2)
21-21: Include for memset/memmove is correct — LGTM.Required for memset/memmove used in this file.
63-81: Incorrect — mk_http_parser_request_size returns size_t; remove the -1 checkmk_http_parser_request_size is declared as
static inline size_t mk_http_parser_request_size(...)in lib/monkey/include/monkey/mk_http_parser.h, so therequest_length == -1check in src/http_server/flb_http_server_http1.c is ineffective. Keeprequest_lengthassize_tand remove the== -1branch (or change the parser API to return a signed error sentinel if needed).Likely an incorrect or invalid review comment.
Upgrade the following bundled libraries:
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
Refactor
Tests
Chores