logs_to_metrics: Support optional value_field for counters #10055
Conversation
databus23
requested review from
edsiper,
fujimotos,
koleini and
leonardo-albertovich
as code owners
databus23
force-pushed
the
logs_to_metric_counter_value
branch
2 times, most recently
from
68cd15e to
e725188
Compare
databus23
changed the title
databus23
force-pushed
the
logs_to_metric_counter_value
branch
2 times, most recently
from
2c1822f to
db182ed
Compare
|
Valgrind output for unit tests: |
This documents a feature added with this PR: fluent/fluent-bit#10055 Signed-off-by: Fabian Ruff <[email protected]>
|
@edsiper Kindly ping. At least the docs-required label can be removed |
|
@leonardo-albertovich kindly ping. Any chance to get a response |
|
@koleini @fujimotos Kindly ping. Any change that is is being looked at? It has tests, docs, all the things. I would love to see some progress on my PR. Thanks |
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
@patrick-stephens can we get a review here, the docs PR fluent/fluent-bit-docs#1584 is waiting on this code merge? |
|
This needs a rebase and @cosmo0920 I think is ok? |
This needs rebase off master and needs to verify the CI results especially for Linux and Windows unit testing results. |
Support incrementing counters by a specified value. If a `value_field` is specified for a counter, the counter is incremented by the given value instead of just counting the number of records. This allows tracking different metrics, such as the total number of bytes sent or received in an access log. Signed-off-by: Fabian Ruff <[email protected]>
databus23
force-pushed
the
logs_to_metric_counter_value
branch
from
db182ed to
5c28512
Compare
WalkthroughInitialization now always validates Changes
Sequence Diagram(s)sequenceDiagram
participant Log as Log Entry
participant Filter as log_to_metrics filter
participant RA as Record Accessor
participant CMT as cmetrics (counter)
Log->>Filter: deliver record
alt filter has `value_field`
Filter->>RA: use `value_ra` to extract field
RA-->>Filter: value (string/float/int)
Filter->>Filter: convert → double (or log warning/error)
Filter->>CMT: cmt_counter_add(metric, double_value)
else no `value_field`
Filter->>CMT: cmt_counter_add(metric, 1)
end
CMT-->>Filter: metric updated
Note over Filter,RA: `value_ra` created at init and freed on teardown
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
|
As requested I rebased the branch on master |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
plugins/filter_log_to_metrics/log_to_metrics.c (1)
937-950: Consider extracting type‑conversion logic to reduce duplication.The same type‑conversion logic for STRING/FLOAT/INT appears in three places: counter (lines 937–950), gauge (lines 970–985), and histogram (lines 1000–1015). Extracting this into a helper function would improve maintainability.
Example helper:
static int extract_numeric_value(struct flb_ra_value *rval, double *out_value, struct flb_filter_instance *f_ins) { if (rval->type == FLB_RA_STRING) { if (sscanf(rval->val.string, "%lf", out_value) != 1) { flb_plg_error(f_ins, "cannot parse value_field as numeric"); return -1; } } else if (rval->type == FLB_RA_FLOAT) { *out_value = rval->val.f64; } else if (rval->type == FLB_RA_INT) { *out_value = (double)rval->val.i64; } else { flb_plg_error(f_ins, "cannot convert given value to metric"); return -1; } return 0; }Then replace the three duplicated blocks with:
if (extract_numeric_value(rval, &counter_value, f_ins) != 0) { flb_ra_key_value_destroy(rval); break; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugins/filter_log_to_metrics/log_to_metrics.c(4 hunks)tests/runtime/filter_log_to_metrics.c(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
plugins/filter_log_to_metrics/log_to_metrics.c (3)
lib/cmetrics/src/cmt_counter.c (2)
cmt_counter_inc(100-117)cmt_counter_add(119-135)src/flb_record_accessor.c (3)
flb_ra_create(271-358)flb_ra_get_value_object(803-814)flb_ra_destroy(232-248)src/flb_ra_key.c (1)
flb_ra_key_value_destroy(842-851)
tests/runtime/filter_log_to_metrics.c (1)
src/flb_lib.c (10)
flb_create(138-220)flb_service_set(647-673)flb_input(261-271)flb_input_set(300-330)flb_filter(287-297)flb_filter_set(613-644)flb_output(274-284)flb_output_set(515-546)flb_start(914-925)flb_lib_push(774-801)
🔇 Additional comments (4)
plugins/filter_log_to_metrics/log_to_metrics.c (2)
646-663: LGTM! Validation logic correctly updated.The refactored validation now permits counters to operate without
value_fieldwhile still requiring it for gauge and histogram modes. The record accessor is properly initialized and stored inctx->value_rafor reuse.
1104-1104: LGTM! Documentation correctly updated.The config map description now accurately reflects that
value_fieldcan be used with counter, gauge, or histogram modes.tests/runtime/filter_log_to_metrics.c (2)
60-60: LGTM! Test properly declared and registered.The new test function is correctly declared and added to the test list.
Also applies to: 122-122
326-392: LGTM! Comprehensive test for counter with value_field.The test correctly validates that when
value_fieldis set to "duration", the counter increments by the numeric value from that field (20) rather than by 1. Pushing 5 messages with duration=20 produces an expected counter value of 100.0. The test also validates metadata (namespace, subsystem, name, description) and labels.
| if (rval->type == FLB_RA_STRING) { | ||
| sscanf(rval->val.string, "%lf", &counter_value); |
There was a problem hiding this comment.
Check sscanf return value to avoid undefined behavior.
Line 938 uses sscanf but ignores its return value. If the string is not a valid number, counter_value remains uninitialized or retains a stale value, leading to incorrect metrics.
Apply this diff:
if (rval->type == FLB_RA_STRING) {
- sscanf(rval->val.string, "%lf", &counter_value);
+ if (sscanf(rval->val.string, "%lf", &counter_value) != 1) {
+ flb_plg_error(f_ins, "cannot parse value_field as numeric");
+ flb_ra_key_value_destroy(rval);
+ break;
+ }
}📝 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.
| if (rval->type == FLB_RA_STRING) { | |
| sscanf(rval->val.string, "%lf", &counter_value); | |
| if (rval->type == FLB_RA_STRING) { | |
| if (sscanf(rval->val.string, "%lf", &counter_value) != 1) { | |
| flb_plg_error(f_ins, "cannot parse value_field as numeric"); | |
| flb_ra_key_value_destroy(rval); | |
| break; | |
| } | |
| } |
🤖 Prompt for AI Agents
In plugins/filter_log_to_metrics/log_to_metrics.c around lines 937-938, the
sscanf call ignores its return value which can leave counter_value uninitialized
if parsing fails; replace or guard the sscanf usage by checking its return
(expecting 1) and only assign counter_value on success, otherwise handle the
error path (e.g., skip processing this metric, set a safe default, and/or log an
error); alternatively use strtod with an endptr to robustly detect invalid input
and validate endptr before using counter_value.
There was a problem hiding this comment.
Thanks coderabbitai for the suggestion but if you check the existing uses of sscanf in the file they also don't check the return value as you suggested.
There was a problem hiding this comment.
@databus23 Thanks — you’re right that the other sscanf sites follow the same pattern. To keep things consistent and safer across all modes, here’s a minimal patch that guards all three sscanf calls. On parse failure we log and skip the emission, avoiding a silent add of 0.
diff --git a/plugins/filter_log_to_metrics/log_to_metrics.c b/plugins/filter_log_to_metrics/log_to_metrics.c
--- a/plugins/filter_log_to_metrics/log_to_metrics.c
+++ b/plugins/filter_log_to_metrics/log_to_metrics.c
@@
- if (rval->type == FLB_RA_STRING) {
- sscanf(rval->val.string, "%lf", &counter_value);
- }
+ if (rval->type == FLB_RA_STRING) {
+ if (sscanf(rval->val.string, "%lf", &counter_value) != 1) {
+ flb_plg_error(f_ins, "cannot parse value_field as numeric");
+ flb_ra_key_value_destroy(rval);
+ rval = NULL;
+ break;
+ }
+ }
@@
- if (rval->type == FLB_RA_STRING) {
- sscanf(rval->val.string, "%lf", &gauge_value);
- }
+ if (rval->type == FLB_RA_STRING) {
+ if (sscanf(rval->val.string, "%lf", &gauge_value) != 1) {
+ flb_plg_error(f_ins, "cannot parse value_field as numeric");
+ flb_ra_key_value_destroy(rval);
+ rval = NULL;
+ break;
+ }
+ }
@@
- if (rval->type == FLB_RA_STRING) {
- sscanf(rval->val.string, "%lf", &histogram_value);
- }
+ if (rval->type == FLB_RA_STRING) {
+ if (sscanf(rval->val.string, "%lf", &histogram_value) != 1) {
+ flb_plg_error(f_ins, "cannot parse value_field as numeric");
+ flb_ra_key_value_destroy(rval);
+ rval = NULL;
+ break;
+ }
+ }If you’d prefer to align with how buckets are parsed, we can switch these three sites to a strtod-based helper (or reuse convert_double) instead; happy to provide that variant too.
Incorporate changes that were introduced with fluent@e6bc684 Signed-off-by: Fabian Ruff <[email protected]>
databus23
force-pushed
the
logs_to_metric_counter_value
branch
from
640d668 to
5b8e260
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugins/filter_log_to_metrics/log_to_metrics.c (1)
918-949: Counter parsing: validate input, enforce monotonicity, and use plugin log macro.
- Use strtod with endptr and errno; avoid unchecked sscanf.
- Reject NaN/Inf and negative values (counters must be non-decreasing).
- Prefer flb_plg_warn(ctx->ins, ...) over flb_warn for consistency.
Apply this diff:
case FLB_LOG_TO_METRICS_COUNTER: // If value_field is not set, increment counter by 1 if (ctx->value_field == NULL || strlen(ctx->value_field) == 0) { ret = cmt_counter_inc(ctx->c, ts, label_count, label_values); break; } // If value_field is set, increment counter by value rval = flb_ra_get_value_object(ctx->value_ra, map); if (!rval) { - flb_warn("given value field is empty or not existent"); + flb_plg_warn(ctx->ins, "value_field empty or not present; skipping counter update"); break; } - if (rval->type == FLB_RA_STRING) { - sscanf(rval->val.string, "%lf", &counter_value); - } - else if (rval->type == FLB_RA_FLOAT) { - counter_value = rval->val.f64; - } - else if (rval->type == FLB_RA_INT) { - counter_value = (double)rval->val.i64; - } - else { + { + double parsed = 0.0; + int ok = 1; + + if (rval->type == FLB_RA_STRING) { + errno = 0; + char *endptr = NULL; + parsed = strtod(rval->val.string, &endptr); + if (errno == ERANGE || endptr == rval->val.string || *endptr != '\0') { + ok = 0; + } + } + else if (rval->type == FLB_RA_FLOAT) { + parsed = rval->val.f64; + } + else if (rval->type == FLB_RA_INT) { + parsed = (double) rval->val.i64; + } + else { + ok = 0; + } + + if (!ok || !isfinite(parsed) || parsed < 0.0) { + flb_plg_error(f_ins, "invalid counter value (non-numeric, NaN/Inf, or negative)"); + flb_ra_key_value_destroy(rval); + break; + } + counter_value = parsed; + } - ret = cmt_counter_add(ctx->c, ts, counter_value, + ret = cmt_counter_add(ctx->c, ts, counter_value, label_count, label_values); flb_ra_key_value_destroy(rval); rval = NULL; break;Add missing headers:
#include <stdio.h> +#include <errno.h> +#include <math.h>Optional: declare
double counter_valueinside this case to reduce scope. See separate note below.
🧹 Nitpick comments (1)
plugins/filter_log_to_metrics/log_to_metrics.c (1)
840-840: Narrow variable scope of counter_value.Declare
double counter_valueinside the COUNTER case block where it’s used; reduces lifetime and accidental reuse.- double counter_value = 0;Then inside the FLB_LOG_TO_METRICS_COUNTER case, before parsing:
double counter_value = 0.0;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/filter_log_to_metrics/log_to_metrics.c(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/filter_log_to_metrics/log_to_metrics.c (3)
lib/cmetrics/src/cmt_counter.c (2)
cmt_counter_inc(100-117)cmt_counter_add(119-135)src/flb_record_accessor.c (1)
flb_ra_get_value_object(803-814)src/flb_ra_key.c (1)
flb_ra_key_value_destroy(842-851)
🔇 Additional comments (2)
plugins/filter_log_to_metrics/log_to_metrics.c (2)
1093-1093: Docstring update LGTM.Config map text now reflects counter support for value_field.
646-653: No change needed for value_field check
The conditionctx->mode > 0already applies only to GAUGE (1) and HISTOGRAM (2), since COUNTER is defined as 0. Remove the suggested diff for this check.Likely an incorrect or invalid review comment.
4 participants
Support incrementing counters by a specified value
If a
value_fieldis specified for a counter metric, the counter is incremented by the given value instead of just incrementing by 1.This allows counting more things then just the number of records, such as the total number of bytes sent or received from an access log.
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
See docs PR for a configuration example: filter_logs_to_metrics: document field_value usage for counters fluent-bit-docs#1584
Documentation
Doc PR: fluent/fluent-bit-docs#1584
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
New Features
Bug Fixes
Tests