cmetrics: prevent appending .0 to scientific notation in bucket_value_to_string#11654
cmetrics: prevent appending .0 to scientific notation in bucket_value_to_string#11654Pigueiras wants to merge 1 commit intofluent:masterfrom
Conversation
📝 WalkthroughWalkthroughAdjusted Prometheus bucket value formatting: Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested labels
Suggested reviewers
Poem
🚥 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 |
There was a problem hiding this comment.
Pull request overview
Fixes Prometheus histogram bucket label formatting by preventing bucket_value_to_string() from appending a trailing .0 when %g emits scientific notation (e.g., avoiding invalid labels like 1e+08.0).
Changes:
- Update
bucket_value_to_string()to skip appending.0when the formatted value is in scientific notation (...e...). - Add clarifying inline comments around the formatting logic.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Only append .0 if it's already a decimal or the number | ||
| // is NOT in scientific notation |
There was a problem hiding this comment.
The new inline comment is inaccurate and inconsistent with the surrounding style: the condition checks for the absence of a decimal point, but the comment says "append .0 if it's already a decimal". Consider switching to the file’s existing /* ... */ comment style and rewording to match the actual condition (append .0 only when there is no decimal point and the value is not in scientific notation).
| // Only append .0 if it's already a decimal or the number | |
| // is NOT in scientific notation | |
| /* Append .0 only when there is no decimal point and the number | |
| * is not in scientific notation. | |
| */ |
| len = snprintf(str, 64, "%g", val); | ||
| cfl_sds_len_set(str, len); | ||
|
|
||
| if (!strchr(str, '.')) { | ||
| // Only append .0 if it's already a decimal or the number | ||
| // is NOT in scientific notation | ||
| if (!strchr(str, '.') && !strchr(str, 'e')) { | ||
| cfl_sds_cat_safe(&str, ".0", 2); | ||
| } |
There was a problem hiding this comment.
Please add a regression unit test to cover this formatting behavior (large histogram bucket boundaries that are rendered in scientific notation should not get a trailing ".0" in the exported Prometheus le label). This helps prevent reintroducing malformed labels like "1e+08.0".
There was a problem hiding this comment.
It looks great but we need to register changes which intent to cmetrics into https://github.com/fluent/cmetrics. This is because under lib directory is handled as bundled libraries.
![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
🧹 Nitpick comments (1)
lib/cmetrics/src/cmt_encode_prometheus.c (1)
373-380: Consider unifying float-to-string formatting across encoders to prevent drift.
bucket_value_to_string()is now fixed, but similar logic inlib/cmetrics/src/cmt_encode_splunk_hec.c:34-57still appends.0based only on'.'presence. A shared helper (or parity patch) would avoid repeating this bug class.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/cmetrics/src/cmt_encode_prometheus.c` around lines 373 - 380, The Prometheus encoder appends ".0" by checking only for '.' and 'e' which duplicates logic present in cmt_encode_splunk_hec.c and can drift; extract this into a shared helper (e.g., cmt_float_to_string or bucket_value_to_string as a common utility) that takes the double, writes it to an SDS/string buffer using snprintf("%g"), ensures cfl_sds_len_set is applied, and appends ".0" only when there is no '.' and no 'e' (i.e., integer not in scientific notation). Replace the inline formatting in cmt_encode_prometheus.c (the snippet using snprintf/strchr) and the corresponding logic in lib/cmetrics/src/cmt_encode_splunk_hec.c (lines ~34-57) to call the new helper so both encoders use the exact same formatting routine.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/cmetrics/src/cmt_encode_prometheus.c`:
- Around line 376-377: Replace the two // comment lines with a /* ... */
block-style comment and tighten the wording to clearly state the condition: e.g.
"/* Append \".0\" only when the numeric string contains a decimal point or does
not use scientific notation. */". Apply this block comment immediately above the
logic that decides whether to append ".0" in cmt_encode_prometheus.c so the
intent is unambiguous and the comment follows the project's block-comment
guideline.
---
Nitpick comments:
In `@lib/cmetrics/src/cmt_encode_prometheus.c`:
- Around line 373-380: The Prometheus encoder appends ".0" by checking only for
'.' and 'e' which duplicates logic present in cmt_encode_splunk_hec.c and can
drift; extract this into a shared helper (e.g., cmt_float_to_string or
bucket_value_to_string as a common utility) that takes the double, writes it to
an SDS/string buffer using snprintf("%g"), ensures cfl_sds_len_set is applied,
and appends ".0" only when there is no '.' and no 'e' (i.e., integer not in
scientific notation). Replace the inline formatting in cmt_encode_prometheus.c
(the snippet using snprintf/strchr) and the corresponding logic in
lib/cmetrics/src/cmt_encode_splunk_hec.c (lines ~34-57) to call the new helper
so both encoders use the exact same formatting routine.
🪄 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: c4622f9f-e3bb-4bd2-9139-ceb5d4cb38a9
📒 Files selected for processing (1)
lib/cmetrics/src/cmt_encode_prometheus.c
Pigueiras
force-pushed
the
fix11651
branch
2 times, most recently
from
63863ec to
bc16164
Compare
…bels `bucket_value_to_string` used %g to format histogram bucket boundaries, which switches to scientific notation for values >= 1e6. The subsequent check for a missing decimal point would then append ".0", producing malformed labels like "1e+06.0" that Prometheus rejects with: ``` bucket label "le" is missing or has a malformed value of "1e+08.0" ``` Skip the ".0" suffix when the string already contains 'e', since scientific notation is valid in Prometheus `le` labels without it. Fixes fluent#11651 Signed-off-by: Luis Pigueiras <luis.pigueiras@cern.ch>
Pigueiras
changed the title
|
@Pigueiras thanks for this PR. please submit the fix to https://github.com/fluent/cmetrics for review |
Sorry @edsiper @cosmo0920, I didn't know it was a separate repository. I did the PR in fluent/cmetrics#261, let me know if you need anything else. Thanks a lot for your review! |
bucket_value_to_stringused %g to format histogram bucket boundaries, which switches to scientific notation for values >= 1e6. The subsequent check for a missing decimal point would then append ".0", producing malformed labels like "1e+06.0" that Prometheus rejects with:Skip the ".0" suffix when the string already contains 'e', since scientific notation is valid in Prometheus
lelabels without it.Fixes #11651
Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
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