Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds comprehensive NULL-safety, bounds/range checks and defensive validation across cmetrics encoders/decoders; replaces position-based label emission with index/key-aligned traversal; refactors Prometheus remote-write to use per-time-series wrapper objects with dynamic sample growth; expands test coverage; bumps package patch and CI action versions. ChangesNULL-Safety & Label/Time-series Refactor
Sequence DiagramsequenceDiagram
participant Client
participant Decoder
participant Validator
participant TimeSeriesMgr
participant Encoder
participant Output
Client->>Decoder: Submit metric payload
Decoder->>Validator: Parse labels/fields
Validator->>Validator: NULL, bounds, enum checks
alt Invalid input
Validator-->>Decoder: Reject
Decoder-->>Client: Decode failure
else Valid
Validator-->>Decoder: Normalized data
Decoder->>TimeSeriesMgr: Lookup/create time-series wrapper
TimeSeriesMgr-->>Decoder: Return wrapper
Decoder->>Encoder: Emit metrics using wrapper
Encoder->>Encoder: Index/key-aligned label emission (NULL-guarded)
Encoder->>Output: Produce formatted output
Output-->>Client: Encoded payload
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
💡 Codex Reviewfluent-bit/lib/cmetrics/src/cmt_decode_prometheus_remote_write.c Lines 578 to 582 in 7a044f0  Preserve histogram type when metadata is sparse
When a remote-write request has more time series than metadata entries, this new bounds check leaves ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
edsiper
force-pushed
the
lib-cmetrics-2.1.3
branch
from
7a044f0 to
9e8f54d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/cmetrics/src/cmt_encode_influx.c (1)
329-365:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTrack emitted tags, not raw label count.
When a NULL key/value hits the
continue, the separator logic still uses the originalnfrom Line 327. That leaves trailing commas, and it can even emit a comma with no following tag when the first surviving label comes after one or more skipped entries. The resulting line is invalid Influx line protocol.Suggested fix
- if (static_labels > 0 || has_namespace == CMT_TRUE) { - cfl_sds_cat_safe(buf, ",", 1); - } - label_k = cfl_list_entry_first(&map->label_keys, struct cmt_map_label, _head); - i = 1; + i = 0; label_index = 0; cfl_list_foreach(head, &metric->labels) { if (label_index >= label_key_count) { @@ if (label_k->name == NULL || label_v->name == NULL) { label_index++; label_k = cfl_list_entry_next(&label_k->_head, struct cmt_map_label, _head, &map->label_keys); continue; } + + if (i > 0 || static_labels > 0 || has_namespace == CMT_TRUE) { + cfl_sds_cat_safe(buf, ",", 1); + } /* key */ append_string(buf, label_k->name); cfl_sds_cat_safe(buf, "=", 1); append_string(buf, label_v->name); - - if (i < n) { - cfl_sds_cat_safe(buf, ",", 1); - } i++;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/cmetrics/src/cmt_encode_influx.c` around lines 329 - 365, The loop uses the original n to decide where to emit commas, but skips NULL keys/values so trailing commas can be produced; change the logic to track the number of tags actually emitted (e.g., emitted_count) instead of relying on n: initialize emitted_count to 0 before the loop, and inside the cfl_list_foreach only increment emitted_count when you successfully write a label (after append_string for key/value); use emitted_count to decide comma placement (preferably add a comma before writing a label when emitted_count > 0) so you never emit a trailing comma. Update references in this block including variables n, i, label_k, label_v, append_string, and cfl_sds_cat_safe to use the new emitted_count logic.
🧹 Nitpick comments (1)
lib/cmetrics/src/cmt_encode_prometheus_remote_write.c (1)
635-655: 💤 Low value
new_capacity == 0guard is unreachable given initialization invariants.
samples_capacityis initialized tolabel_set_hash_matches(≥ 1, due to thelabel_set_hash_matches++guard at line 403), sosamples_capacity * 2 >= 2when this branch is first reached. Thenew_capacity == 0check on lines 637–639 is dead code. Harmless as a safety net, but may mislead future readers into thinking capacity can start at zero.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/cmetrics/src/cmt_encode_prometheus_remote_write.c` around lines 635 - 655, The check for new_capacity == 0 in the growth logic of time_series->samples_capacity is dead because samples_capacity is initialized from label_set_hash_matches which is guaranteed ≥1; remove the unreachable guard (the if (new_capacity == 0) branch) in the block that reallocs time_series->data.samples and update comments to reflect that samples_capacity will always be ≥1 so new_capacity = samples_capacity * 2 will never be zero; reference the growth block that uses time_series->samples_capacity, new_capacity, realloc on time_series->data.samples and initialization via label_set_hash_matches to locate the code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/cmetrics/include/cmetrics/cmt_encode_prometheus_remote_write.h`:
- Around line 42-46: The public struct cmt_prometheus_time_series was altered by
adding the samples_capacity field which breaks ABI; revert that field from the
public header so the original struct layout and offsets remain unchanged, and if
you need to carry additional state create a new type (e.g.
cmt_prometheus_time_series_v2) or move samples_capacity into an internal/private
struct used only inside the library; update any uses of
Prometheus__TimeSeries/cmt_prometheus_time_series within the code to use the new
internal/variant type and prepare a proper minor/major version bump rather than
shipping this as a patch.
In `@lib/cmetrics/src/cmt_decode_opentelemetry.c`:
- Around line 273-277: The code currently treats length == 0 as "compute with
strlen" which causes unsafe strlen() on non-null-terminated protobuf byte
buffers; modify the label creation logic (e.g., create_label) to distinguish
"use strlen" from "use provided length" — either use a sentinel like SIZE_MAX to
mean "compute C-string length" or add a new helper (e.g.,
create_label_from_bytes or create_label_with_len) that always respects the
provided len (including zero) and does not call strlen when data may not be
NUL-terminated; update callers that pass protobuf bytes (the code paths that
forward protobuf data and lengths) to call the new helper or pass SIZE_MAX
appropriately so zero-length attributes produce an empty cfl_sds without
invoking strlen.
In `@lib/cmetrics/src/cmt_decode_prometheus_remote_write.c`:
- Around line 455-467: Rollback paths in cmt_decode_prometheus_remote_write
currently free the label list and metric struct but forget to free
metric->hist_buckets, leaking the bucket buffer; update each rollback branch
(the one after allocating metric->hist_buckets at the first allocation and the
other two branches noted around lines 485-493 and 507-515) to check if
metric->hist_buckets != NULL and free it before calling
destroy_label_list(&metric->labels), cfl_list_del(&metric->_head) and
free(metric), so all allocated resources are released on error.
In `@lib/cmetrics/src/cmt_encode_prometheus.c`:
- Around line 400-409: The fallback formatting uses "%.15g" which is
insufficient for IEEE 754 binary64 round-tripping and can mis-round
bucket/quantile label values; update the fallback call in the block that
computes len/parsed (the snprintf that currently uses "%.15g") to use "%.17g"
instead, leaving the surrounding logic (variables str, val, parsed, len and the
cfl_sds_len_set and subsequent dot/exponent checks) unchanged so labels preserve
full 17-digit precision for exact matching.
In `@lib/cmetrics/src/cmt_encode_splunk_hec.c`:
- Around line 301-334: The loop emits commas based on the original label count
(n) and i, but skips NULL labels which causes extra or double commas; fix
cmt_encode_splunk_hec by introducing a boolean or counter (e.g., emitted_any)
and use it to prefix subsequent label entries with a comma only when a previous
label was actually written, updating the code paths around label_k/label_v
emission (inside the cfl_list_foreach over metric->labels) to set emitted_any
when you call cfl_sds_cat_safe to write a label and to check emitted_any before
emitting a comma, removing reliance on i/n and label_index for comma
bookkeeping.
---
Outside diff comments:
In `@lib/cmetrics/src/cmt_encode_influx.c`:
- Around line 329-365: The loop uses the original n to decide where to emit
commas, but skips NULL keys/values so trailing commas can be produced; change
the logic to track the number of tags actually emitted (e.g., emitted_count)
instead of relying on n: initialize emitted_count to 0 before the loop, and
inside the cfl_list_foreach only increment emitted_count when you successfully
write a label (after append_string for key/value); use emitted_count to decide
comma placement (preferably add a comma before writing a label when
emitted_count > 0) so you never emit a trailing comma. Update references in this
block including variables n, i, label_k, label_v, append_string, and
cfl_sds_cat_safe to use the new emitted_count logic.
---
Nitpick comments:
In `@lib/cmetrics/src/cmt_encode_prometheus_remote_write.c`:
- Around line 635-655: The check for new_capacity == 0 in the growth logic of
time_series->samples_capacity is dead because samples_capacity is initialized
from label_set_hash_matches which is guaranteed ≥1; remove the unreachable guard
(the if (new_capacity == 0) branch) in the block that reallocs
time_series->data.samples and update comments to reflect that samples_capacity
will always be ≥1 so new_capacity = samples_capacity * 2 will never be zero;
reference the growth block that uses time_series->samples_capacity,
new_capacity, realloc on time_series->data.samples and initialization via
label_set_hash_matches to locate the code to change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 574839b4-3db2-42e9-954d-13125bcda5b0
⛔ Files ignored due to path filters (1)
lib/cmetrics/tests/data/otlp_null_label_histogram.binis excluded by!**/*.bin
📒 Files selected for processing (17)
lib/cmetrics/.github/workflows/build.yamllib/cmetrics/.github/workflows/packages.yamllib/cmetrics/CMakeLists.txtlib/cmetrics/include/cmetrics/cmt_encode_prometheus_remote_write.hlib/cmetrics/src/cmt_decode_opentelemetry.clib/cmetrics/src/cmt_decode_prometheus_remote_write.clib/cmetrics/src/cmt_encode_cloudwatch_emf.clib/cmetrics/src/cmt_encode_influx.clib/cmetrics/src/cmt_encode_msgpack.clib/cmetrics/src/cmt_encode_opentelemetry.clib/cmetrics/src/cmt_encode_prometheus.clib/cmetrics/src/cmt_encode_prometheus_remote_write.clib/cmetrics/src/cmt_encode_splunk_hec.clib/cmetrics/src/cmt_encode_text.clib/cmetrics/tests/decoding.clib/cmetrics/tests/histogram.clib/cmetrics/tests/opentelemetry.c
edsiper
force-pushed
the
lib-cmetrics-2.1.3
branch
from
9e8f54d to
f901ba8
Compare
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/cmetrics/src/cmt_decode_prometheus_remote_write.c (2)
142-223:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPrevent
value_index_listfrom overrunning oncemap->label_keysgrows.
value_index_listis fixed at 128 entries, but Lines 201 and 212 index it with positions derived frommap->label_keys, which is cumulative across previously decoded series. A request with multiple sparse series can therefore read/write past this buffer even though each individual series still satisfiesn_labels <= 127.Suggested fix
size_t map_label_index; size_t map_label_count; + size_t value_index_capacity; struct cfl_list *label_iterator; struct cmt_map_label *current_label; size_t label_index; @@ - value_index_list = calloc(128, sizeof(void *)); + value_index_capacity = cfl_list_size(&map->label_keys) + n_labels; + value_index_list = calloc(value_index_capacity, sizeof(void *));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/cmetrics/src/cmt_decode_prometheus_remote_write.c` around lines 142 - 223, The value_index_list is a fixed 128-entry buffer that can be indexed by indices derived from map->label_keys (and new keys added by append_new_map_label_key), which can overflow; replace the fixed allocation with a dynamically sized allocation and bounds checks: compute an allocation count (e.g. size_t alloc_count = cfl_list_size(&map->label_keys) + n_labels), allocate value_index_list = calloc(alloc_count, sizeof(void *)), use alloc_count when checking/assigning value_index_list[label_index] (ensure label_index < alloc_count before writing), and free as before; update any code that assumes 128 entries to use alloc_count instead.
637-653:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMatch metadata by metric family name, not by array index.
The current code at lines 637-653 assumes
write->metadata[i]aligns withwrite->timeseries[i], but the Prometheus remote write protocol provides no such guarantee. When metadata is sparse or reordered, non-histogram time series without a corresponding metadata entry at their index position fall back to the incorrect GAUGE type default. Resolve metadata by iterating throughwrite->metadata[]and matchingmetric_family_nameagainst the extracted metric name from the time series labels instead of relying on positional correspondence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/cmetrics/src/cmt_decode_prometheus_remote_write.c` around lines 637 - 653, The code incorrectly assumes write->metadata[i] corresponds to write->timeseries[i]; instead, for each timeseries (loop using ts and index i) iterate over write->metadata[0..meta_count-1] and find the metadata entry whose metric_family_name matches the metric name extracted from the timeseries labels (the label parsed earlier in this function), then use that matched entry to set type and metric_description; if a histogram metadata is found set type = PROMETHEUS__METRIC_METADATA__METRIC_TYPE__HISTOGRAM and description, otherwise fall back to the matched metadata's type/help, and only if no matching metadata exists use the existing defaults (GAUGE/"-"). Ensure you replace direct uses of write->metadata[i] with the matched metadata pointer and handle nulls safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/cmetrics/src/cmt_decode_opentelemetry.c`:
- Around line 650-658: The code currently treats missing/unsupported OTLP
attribute values as an explicit empty string by calling
append_new_metric_label_value(metric, "", ...), which conflates "label omitted"
with an actual empty-string label; update the branches handling
absent/unsupported values (the fallthrough cases around
OPENTELEMETRY__PROTO__COMMON__V1__ANY_VALUE__VALUE_STRING_VALUE and the other
affected blocks at the noted ranges) to call
append_new_metric_label_value(metric, NULL, 0) to represent a NULL/omitted label
slot, and only pass "" (or a zero-length byte buffer) when
attribute->value->string_value is non-NULL and legitimately empty; reference the
append_new_metric_label_value and attribute->value->value_case checks when
making these replacements so omitted/invalid attributes remain NULL labels while
real empty strings stay as "".
In `@lib/cmetrics/src/cmt_encode_opentelemetry.c`:
- Around line 3366-3375: The current logic treats NULL label values as empty
strings by counting all sample labels in label_name_count and always calling
initialize_string_attribute; change it to count only non-NULL sample label
values when computing attribute_count (i.e., iterate sample->labels and
increment only for entries where label_value->name != NULL) and adjust the size
check to compare non-NULL count against map->label_count; additionally, before
calling initialize_string_attribute (and when advancing label_name), skip any
label entries whose label_value->name == NULL so no OTLP attribute is created
for omitted labels. Update the same pattern in the other similar blocks (around
the ranges noted) so attribute_count and attribute initialization both ignore
NULL label values.
In `@lib/cmetrics/src/cmt_encode_prometheus.c`:
- Around line 326-328: The conditions that gate emitting Prometheus labels
currently use strlen(label_v->name) which filters out empty label values and
collapses distinct series; update both occurrences so they only check for
non-NULL pointers (i.e., use label_k->name != NULL && label_v->name != NULL) and
remove the strlen(...) test so empty strings ("") are preserved as valid
Prometheus label values; ensure you change the same check at the other
occurrence (the second block referencing label_k->name and label_v->name) so
both code paths treat empty label values as present.
---
Outside diff comments:
In `@lib/cmetrics/src/cmt_decode_prometheus_remote_write.c`:
- Around line 142-223: The value_index_list is a fixed 128-entry buffer that can
be indexed by indices derived from map->label_keys (and new keys added by
append_new_map_label_key), which can overflow; replace the fixed allocation with
a dynamically sized allocation and bounds checks: compute an allocation count
(e.g. size_t alloc_count = cfl_list_size(&map->label_keys) + n_labels), allocate
value_index_list = calloc(alloc_count, sizeof(void *)), use alloc_count when
checking/assigning value_index_list[label_index] (ensure label_index <
alloc_count before writing), and free as before; update any code that assumes
128 entries to use alloc_count instead.
- Around line 637-653: The code incorrectly assumes write->metadata[i]
corresponds to write->timeseries[i]; instead, for each timeseries (loop using ts
and index i) iterate over write->metadata[0..meta_count-1] and find the metadata
entry whose metric_family_name matches the metric name extracted from the
timeseries labels (the label parsed earlier in this function), then use that
matched entry to set type and metric_description; if a histogram metadata is
found set type = PROMETHEUS__METRIC_METADATA__METRIC_TYPE__HISTOGRAM and
description, otherwise fall back to the matched metadata's type/help, and only
if no matching metadata exists use the existing defaults (GAUGE/"-"). Ensure you
replace direct uses of write->metadata[i] with the matched metadata pointer and
handle nulls safely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2fde565a-3a0f-4400-8d18-a8dea7ea03e8
⛔ Files ignored due to path filters (1)
lib/cmetrics/tests/data/otlp_null_label_histogram.binis excluded by!**/*.bin
📒 Files selected for processing (17)
lib/cmetrics/.github/workflows/build.yamllib/cmetrics/.github/workflows/packages.yamllib/cmetrics/CMakeLists.txtlib/cmetrics/src/cmt_decode_opentelemetry.clib/cmetrics/src/cmt_decode_prometheus_remote_write.clib/cmetrics/src/cmt_encode_cloudwatch_emf.clib/cmetrics/src/cmt_encode_influx.clib/cmetrics/src/cmt_encode_msgpack.clib/cmetrics/src/cmt_encode_opentelemetry.clib/cmetrics/src/cmt_encode_prometheus.clib/cmetrics/src/cmt_encode_prometheus_remote_write.clib/cmetrics/src/cmt_encode_splunk_hec.clib/cmetrics/src/cmt_encode_text.clib/cmetrics/tests/decoding.clib/cmetrics/tests/histogram.clib/cmetrics/tests/null_label.clib/cmetrics/tests/opentelemetry.c
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/cmetrics/src/cmt_encode_prometheus.c (1)
320-339: 💤 Low valueIndex-aligned label traversal correctly preserves empty values.
Replacing the previous
strlen(label_v->name)filter withlabel_index >= label_key_countbounding plus aNULL-only check onlabel_k->name/label_v->namekeeps{foo=""}distinguishable from a missing label, which addresses the earlier concern flagged in the previous review round.One thing to be aware of: when
label_key_count == 0,label_kis left uninitialized at lines 322‑324 and the loop relies on thelabel_index >= label_key_countbreak at line 326 to never read it. That is correct, but some compilers may emit-Wmaybe-uninitializedsince the early-break invariant is not statically obvious. If that warning shows up in CI, initializinglabel_k = NULLat declaration silences it without changing behavior.♻️ Optional defensive init
- struct cmt_map_label *label_k; + struct cmt_map_label *label_k = NULL;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/cmetrics/src/cmt_encode_prometheus.c` around lines 320 - 339, The variable label_k can remain uninitialized when label_key_count == 0 which triggers -Wmaybe-uninitialized; initialize label_k (e.g., set it to NULL) at declaration before the if (label_key_count > 0) branch so the loop and early-break invariant stay the same but compilers won't warn—update the declaration of label_k near label_key_count and keep the existing uses in cfl_list_entry_first/cfl_list_entry_next and the metric->labels traversal unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/cmetrics/tests/decoding.c`:
- Around line 282-288: The tests currently only destroy decoded_context on
successful decodes which can leak memory when
cmt_decode_prometheus_remote_write_create partially allocates and returns
failure; update each decode block (e.g., the calls to
cmt_decode_prometheus_remote_write_create and the surrounding test code that
checks ret and uses decoded_context) to always call
cmt_decode_prometheus_remote_write_destroy(decoded_context) guarded by a
null-check immediately after the decode attempt (regardless of ret), and then
set decoded_context = NULL; apply this unconditional cleanup pattern for the
other similar blocks referenced (lines covering the other decode calls).
In `@lib/cmetrics/tests/opentelemetry.c`:
- Around line 1615-1618: Two tests dereference decoded_context immediately after
calling cfl_list_entry_first (yielding struct cmt *decoded_context) and risk
SIGSEGV if the list is empty; add the same NULL guard pattern used in
test_opentelemetry_many_attributes: after assigning decoded_context =
cfl_list_entry_first(&result_list, struct cmt, _head) call
TEST_CHECK(decoded_context != NULL); and wrap the subsequent accesses (e.g.,
reading decoded_context->gauges and using gauge = cfl_list_entry_first(...)) in
if (decoded_context != NULL) { ... } in both affected tests
(test_opentelemetry_zero_length_bytes_attribute and the other test block around
the first cfl_list_entry_first) to prevent dereferencing NULL.
---
Nitpick comments:
In `@lib/cmetrics/src/cmt_encode_prometheus.c`:
- Around line 320-339: The variable label_k can remain uninitialized when
label_key_count == 0 which triggers -Wmaybe-uninitialized; initialize label_k
(e.g., set it to NULL) at declaration before the if (label_key_count > 0) branch
so the loop and early-break invariant stay the same but compilers won't
warn—update the declaration of label_k near label_key_count and keep the
existing uses in cfl_list_entry_first/cfl_list_entry_next and the metric->labels
traversal unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 177888bb-4c73-4a98-bc6f-39e288ad7650
⛔ Files ignored due to path filters (1)
lib/cmetrics/tests/data/otlp_null_label_histogram.binis excluded by!**/*.bin
📒 Files selected for processing (20)
lib/cmetrics/.github/workflows/build.yamllib/cmetrics/.github/workflows/packages.yamllib/cmetrics/CMakeLists.txtlib/cmetrics/src/cmt_decode_msgpack.clib/cmetrics/src/cmt_decode_opentelemetry.clib/cmetrics/src/cmt_decode_prometheus_remote_write.clib/cmetrics/src/cmt_encode_cloudwatch_emf.clib/cmetrics/src/cmt_encode_influx.clib/cmetrics/src/cmt_encode_msgpack.clib/cmetrics/src/cmt_encode_opentelemetry.clib/cmetrics/src/cmt_encode_prometheus.clib/cmetrics/src/cmt_encode_prometheus_remote_write.clib/cmetrics/src/cmt_encode_splunk_hec.clib/cmetrics/src/cmt_encode_text.clib/cmetrics/src/cmt_map.clib/cmetrics/tests/decoding.clib/cmetrics/tests/encoding.clib/cmetrics/tests/histogram.clib/cmetrics/tests/null_label.clib/cmetrics/tests/opentelemetry.c
✅ Files skipped from review due to trivial changes (2)
- lib/cmetrics/CMakeLists.txt
- lib/cmetrics/.github/workflows/packages.yaml
🚧 Files skipped from review as they are similar to previous changes (12)
- lib/cmetrics/.github/workflows/build.yaml
- lib/cmetrics/src/cmt_encode_msgpack.c
- lib/cmetrics/src/cmt_map.c
- lib/cmetrics/tests/histogram.c
- lib/cmetrics/src/cmt_encode_cloudwatch_emf.c
- lib/cmetrics/src/cmt_encode_splunk_hec.c
- lib/cmetrics/src/cmt_encode_influx.c
- lib/cmetrics/tests/null_label.c
- lib/cmetrics/src/cmt_decode_msgpack.c
- lib/cmetrics/src/cmt_encode_prometheus_remote_write.c
- lib/cmetrics/src/cmt_decode_prometheus_remote_write.c
- lib/cmetrics/src/cmt_encode_text.c
edsiper
force-pushed
the
lib-cmetrics-2.1.3
branch
from
b678e3a to
ad5f8f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/cmetrics/src/cmt_encode_prometheus.c`:
- Around line 403-410: The snprintf/strtod check only detects precision loss but
misses cases where "%g" emitted exponent form (e/E) yet parses back equal (e.g.,
1e+06), so update the logic in the block that fills str (using snprintf, parsed
= strtod(...), cfl_sds_len_set) to also force the higher-precision fallback when
the first "%g" output contains 'e' or 'E' (i.e., after snprintf and before
deciding not to redo, test if strchr(str,'e') || strchr(str,'E') and if so set
len = snprintf(str, 64, "%.17g", val) and then call cfl_sds_len_set(str, len));
reference symbols: snprintf, parsed, strtod, cfl_sds_len_set, strchr, val, str.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d049183-ced6-46d7-b7d7-e9a55b58635f
⛔ Files ignored due to path filters (1)
lib/cmetrics/tests/data/otlp_null_label_histogram.binis excluded by!**/*.bin
📒 Files selected for processing (20)
lib/cmetrics/.github/workflows/build.yamllib/cmetrics/.github/workflows/packages.yamllib/cmetrics/CMakeLists.txtlib/cmetrics/src/cmt_decode_msgpack.clib/cmetrics/src/cmt_decode_opentelemetry.clib/cmetrics/src/cmt_decode_prometheus_remote_write.clib/cmetrics/src/cmt_encode_cloudwatch_emf.clib/cmetrics/src/cmt_encode_influx.clib/cmetrics/src/cmt_encode_msgpack.clib/cmetrics/src/cmt_encode_opentelemetry.clib/cmetrics/src/cmt_encode_prometheus.clib/cmetrics/src/cmt_encode_prometheus_remote_write.clib/cmetrics/src/cmt_encode_splunk_hec.clib/cmetrics/src/cmt_encode_text.clib/cmetrics/src/cmt_map.clib/cmetrics/tests/decoding.clib/cmetrics/tests/encoding.clib/cmetrics/tests/histogram.clib/cmetrics/tests/null_label.clib/cmetrics/tests/opentelemetry.c
✅ Files skipped from review due to trivial changes (3)
- lib/cmetrics/.github/workflows/build.yaml
- lib/cmetrics/CMakeLists.txt
- lib/cmetrics/src/cmt_decode_msgpack.c
🚧 Files skipped from review as they are similar to previous changes (14)
- lib/cmetrics/src/cmt_encode_msgpack.c
- lib/cmetrics/.github/workflows/packages.yaml
- lib/cmetrics/tests/histogram.c
- lib/cmetrics/src/cmt_encode_opentelemetry.c
- lib/cmetrics/src/cmt_encode_influx.c
- lib/cmetrics/src/cmt_encode_text.c
- lib/cmetrics/src/cmt_map.c
- lib/cmetrics/tests/null_label.c
- lib/cmetrics/src/cmt_encode_cloudwatch_emf.c
- lib/cmetrics/src/cmt_encode_splunk_hec.c
- lib/cmetrics/tests/encoding.c
- lib/cmetrics/src/cmt_encode_prometheus_remote_write.c
- lib/cmetrics/src/cmt_decode_prometheus_remote_write.c
- lib/cmetrics/src/cmt_decode_opentelemetry.c
edsiper
force-pushed
the
lib-cmetrics-2.1.3
branch
from
ad5f8f0 to
220c577
Compare
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
edsiper
force-pushed
the
lib-cmetrics-2.1.3
branch
from
220c577 to
36df7e3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Bug Fixes
Tests
Chores