allow counter metrics to use value_field instead of increment by 1 only #11344
Conversation
|
Warning Rate limit exceeded@Lusitaniae has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 6 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughlog_to_metrics now requires Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 1
🤖 Fix all issues with AI Agents
In @plugins/filter_log_to_metrics/log_to_metrics.c:
- Around line 929-965: The parsing branch must validate the parsed value from
flb_ra_get_value_object/rval and reject non-numeric or negative values before
calling cmt_counter_add: check sscanf return value when rval->type ==
FLB_RA_STRING and handle a non-1 return by logging via flb_plg_error and falling
back to cmt_counter_inc (or skipping adding); for FLB_RA_FLOAT and FLB_RA_INT
verify counter_value >= 0 and if negative log an error and fall back to
cmt_counter_inc (do not call cmt_counter_add with negative), ensuring
flb_ra_key_value_destroy(rval) is called in all error paths and rval is nulled
afterwards.
🧹 Nitpick comments (2)
plugins/filter_log_to_metrics/log_to_metrics.c (2)
646-675: Logic for optional value_field in counter mode looks correct.The initialization properly makes
value_fieldrequired for gauge/histogram modes while keeping it optional for counter mode. Thevalue_rarecord accessor is created consistently in both paths.Minor note: The local
value_fieldvariable populated at lines 653-654 and 666-667 viasnprintfis never used after assignment. Consider removing these dead assignments or the local variable declaration at line 496 if it's no longer needed.
931-936: Consider logging when value_field is configured but not found.When
value_rais set but the field is missing from the record, the code silently falls back to incrementing by 1. This could hide misconfiguration. A debug or warning log would help users troubleshoot unexpected behavior.🔎 Suggested improvement
if (!rval) { - /* No value field, increment by 1 */ + /* value_field configured but not found, increment by 1 */ + flb_plg_debug(f_ins, + "value_field not found in record, incrementing counter by 1"); ret = cmt_counter_inc(ctx->c, ts, label_count, label_values); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/filter_log_to_metrics/log_to_metrics.c
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/filter_log_to_metrics/log_to_metrics.c (3)
src/flb_record_accessor.c (2)
flb_ra_create(271-358)flb_ra_get_value_object(803-814)lib/cmetrics/src/cmt_counter.c (2)
cmt_counter_inc(100-117)cmt_counter_add(119-135)src/flb_ra_key.c (1)
flb_ra_key_value_destroy(842-851)
🔇 Additional comments (1)
plugins/filter_log_to_metrics/log_to_metrics.c (1)
1107-1110: Documentation update looks good.The updated description accurately reflects the new behavior where
value_fieldis required for gauge/histogram modes but optional for counter mode.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
plugins/filter_log_to_metrics/log_to_metrics.c (2)
653-654: Dead code:value_fieldlocal buffer is populated but never used.The local
value_fieldbuffer (declared at line 496) is populated withsnprintfbut never referenced afterward. The code usesctx->value_fielddirectly withflb_ra_create. Consider removing these dead assignments.🔎 Suggested fix
if (ctx->mode == FLB_LOG_TO_METRICS_GAUGE || ctx->mode == FLB_LOG_TO_METRICS_HISTOGRAM) { if (ctx->value_field == NULL || strlen(ctx->value_field) == 0) { flb_plg_error(f_ins, "value_field is required for gauge and histogram modes"); log_to_metrics_destroy(ctx); return -1; } - snprintf(value_field, sizeof(value_field) - 1, "%s", - ctx->value_field); ctx->value_ra = flb_ra_create(ctx->value_field, FLB_TRUE); if (ctx->value_ra == NULL) { flb_plg_error(f_ins, "invalid record accessor key for value_field"); log_to_metrics_destroy(ctx); return -1; } } else if (ctx->mode == FLB_LOG_TO_METRICS_COUNTER && ctx->value_field != NULL && strlen(ctx->value_field) > 0) { /* value_field is optional for counter mode */ - snprintf(value_field, sizeof(value_field) - 1, "%s", - ctx->value_field); ctx->value_ra = flb_ra_create(ctx->value_field, FLB_TRUE);Also remove the unused declaration at line 496:
- char value_field[MAX_METRIC_LENGTH];Also applies to: 666-667
958-964: Inconsistent fallback behavior for unsupported type.When
value_fieldcontains an unsupported type, the code logs an error and breaks without incrementing the counter. However, for other error cases (missing field, parse failure, negative value), the code falls back to incrementing by 1. This inconsistency may cause silent metric drops for unexpected field types.Consider falling back to increment-by-1 for consistency, or document that unsupported types skip the record entirely.
🔎 Suggested fix for consistent fallback
else { flb_plg_error(f_ins, - "cannot convert given value to metric"); + "cannot convert given value to metric, falling back to increment by 1"); flb_ra_key_value_destroy(rval); rval = NULL; - break; + ret = cmt_counter_inc(ctx->c, ts, label_count, + label_values); + break; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/filter_log_to_metrics/log_to_metrics.c
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/filter_log_to_metrics/log_to_metrics.c (3)
src/flb_record_accessor.c (2)
flb_ra_create(271-358)flb_ra_get_value_object(803-814)lib/cmetrics/src/cmt_counter.c (2)
cmt_counter_inc(100-117)cmt_counter_add(119-135)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)
854-854: LGTM!Variable declaration follows the existing pattern for
gauge_valueandhistogram_value, and is properly initialized.
1130-1133: LGTM!Config map description correctly documents that
value_fieldis now supported for counters and is optional in that mode.
This comment was marked as outdated.
This comment was marked as outdated.
|
Our commit linter complains like: So, we need to use |
Lusitaniae
force-pushed
the
feature/counter_value_field
branch
from
28a907c to
1c89b61
Compare
|
Is it worthwhile implementing the nitpick comments? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugins/filter_log_to_metrics/log_to_metrics.c (1)
959-965: Inconsistent error handling for unsupported value_field types.When
value_fieldis configured and present in the record, the error handling is inconsistent:
- Non-numeric string (sscanf fails, line 948): falls back to increment by 1 ✓
- Negative value (line 974): falls back to increment by 1 ✓
- Unsupported type (line 964): breaks without incrementing ✗
For consistency, unsupported types (BOOL, ARRAY, MAP, etc.) should also fall back to increment by 1 rather than silently skipping the record. This would make error handling uniform and ensure counters are always updated even when value_field contains unexpected data.
🔎 Proposed fix to add fallback for unsupported types
else { flb_plg_error(f_ins, - "cannot convert given value to metric"); + "cannot convert given value to metric, falling back to increment by 1"); flb_ra_key_value_destroy(rval); rval = NULL; + ret = cmt_counter_inc(ctx->c, ts, label_count, + label_values); break; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/filter_log_to_metrics/log_to_metrics.c
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/filter_log_to_metrics/log_to_metrics.c (3)
src/flb_record_accessor.c (2)
flb_ra_create(271-358)flb_ra_get_value_object(803-814)lib/cmetrics/src/cmt_counter.c (2)
cmt_counter_inc(100-117)cmt_counter_add(119-135)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)
646-675: Initialization logic correctly implements optional value_field for counters.The distinction between required (gauge/histogram) and optional (counter) value_field is clearly implemented. Error handling for accessor creation failures is appropriate in both branches.
1133-1133: Config description accurately reflects the new behavior.The updated description clearly communicates that
value_fieldis required for gauge/histogram but optional for counter mode.
Lusitaniae
force-pushed
the
feature/counter_value_field
branch
from
1c89b61 to
7579b62
Compare
…ad of increment by 1 only Signed-off-by: Pedro <[email protected]>
Signed-off-by: Pedro <[email protected]>
Signed-off-by: Pedro <[email protected]>
Lusitaniae
force-pushed
the
feature/counter_value_field
branch
from
7579b62 to
07039bc
Compare
Force pushed to address that |
2 participants
It allows users to specify a key (value_field) as the value to increment the counters by, instead of only incrementing by 1.
Closes #11343
Testing:
First test uncommented
#value_field durationUsing value_field
Default (commented out value_field): increment by 1
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
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.