lib: cmetrics: upgrade to v2.2.0 - #12083
Conversation
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
📝 WalkthroughWalkthroughCMetrics adds timestamp-based expiration APIs, preserves aggregation temporality through MsgPack round trips, improves Prometheus allocation-error handling and non-finite bucket formatting, removes an untyped-value restriction, and introduces Docker-based CentOS 7 builds. ChangesCMetrics changes
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant cmt_expire
participant cmt_map_metrics_expire
participant cmt_map_metric_destroy
cmt_expire->>cmt_map_metrics_expire: expire metric maps at threshold
cmt_map_metrics_expire->>cmt_map_metric_destroy: destroy older metrics
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce207a66e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| struct cfl_list *head; | ||
| struct cmt_metric *metric; | ||
|
|
||
| cfl_list_foreach_safe(head, tmp, &map->metrics) { |
There was a problem hiding this comment.
When cmt_expire() is used on a metric with zero labels, the sample is stored in map->metric (cmt_map_metric_get() returns &map->metric and sets metric_static_set for labels_count == 0), not in map->metrics. This loop only scans map->metrics, so a stale no-label counter/gauge/histogram is still emitted after the expiration cutoff; for example, a scrape target that stops exporting a no-label series will keep that old value forever. Please handle metric_static_set/map->metric too, taking care to release any type-specific buffers before clearing it.
Useful? React with 👍 / 👎.
![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
🤖 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/CMakeLists.txt`:
- Around line 8-9: Fix the CMT_VERSION expression in cmt_version.h.in by adding
+ operators between the major, minor, and patch terms, using CMT_VERSION_MAJOR *
10000 + CMT_VERSION_MINOR * 100 + CMT_VERSION_PATCH.
In `@lib/cmetrics/tests/prometheus_parser.c`:
- Around line 1713-1725: Update test_issue_274 to initialize cmt to NULL, guard
cmt_decode_prometheus_destroy(cmt) so it runs only when cmt is non-NULL, and
replace the // issue reference with an Apache-style /* ... */ comment.
🪄 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: d85a40bb-2585-4a71-891b-165f8d21c8a4
📒 Files selected for processing (20)
lib/cmetrics/.dockerignorelib/cmetrics/.github/workflows/build.yamllib/cmetrics/CMakeLists.txtlib/cmetrics/dockerfiles/Dockerfile.centos7lib/cmetrics/include/cmetrics/cmetrics.hlib/cmetrics/include/cmetrics/cmt_map.hlib/cmetrics/src/cmetrics.clib/cmetrics/src/cmt_decode_msgpack.clib/cmetrics/src/cmt_decode_prometheus.clib/cmetrics/src/cmt_decode_prometheus.llib/cmetrics/src/cmt_encode_msgpack.clib/cmetrics/src/cmt_encode_prometheus.clib/cmetrics/src/cmt_map.clib/cmetrics/src/cmt_untyped.clib/cmetrics/tests/CMakeLists.txtlib/cmetrics/tests/data/issue_274.txtlib/cmetrics/tests/expire.clib/cmetrics/tests/histogram.clib/cmetrics/tests/msgpack_temporality.clib/cmetrics/tests/prometheus_parser.c
| set(CMT_VERSION_MINOR 2) | ||
| set(CMT_VERSION_PATCH 0) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the CMT_VERSION macro in the version header template has correct operators.
# Check the actual content of the version header template
sed -n '26,36p' lib/cmetrics/include/cmetrics/cmt_version.h.in
# Also check if CMT_VERSION is used anywhere in the codebase
rg -n '\bCMT_VERSION\b' lib/cmetrics/ --type c --type h -g '!build/**' -C2Repository: fluent/fluent-bit
Length of output: 1050
Fix the CMT_VERSION expression in lib/cmetrics/include/cmetrics/cmt_version.h.in. It currently expands without + operators between the terms, so the generated macro is invalid C when used; it should be CMT_VERSION_MAJOR * 10000 + CMT_VERSION_MINOR * 100 + CMT_VERSION_PATCH.
🤖 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/CMakeLists.txt` around lines 8 - 9, Fix the CMT_VERSION
expression in cmt_version.h.in by adding + operators between the major, minor,
and patch terms, using CMT_VERSION_MAJOR * 10000 + CMT_VERSION_MINOR * 100 +
CMT_VERSION_PATCH.
| // reproduces https://github.com/fluent/cmetrics/issues/274 | ||
| void test_issue_274() | ||
| { | ||
| int status; | ||
| struct cmt *cmt; | ||
| cfl_sds_t in_buf = read_file(CMT_TESTS_DATA_PATH "/issue_274.txt"); | ||
| size_t in_size = cfl_sds_len(in_buf); | ||
|
|
||
| status = cmt_decode_prometheus_create(&cmt, in_buf, in_size, NULL); | ||
| TEST_CHECK(status == 0); | ||
| cfl_sds_destroy(in_buf); | ||
| cmt_decode_prometheus_destroy(cmt); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Initialize cmt to NULL and guard the destroy call.
If cmt_decode_prometheus_create fails, it does not set *out_cmt, leaving cmt uninitialized. The unconditional cmt_decode_prometheus_destroy(cmt) then invokes undefined behavior. The existing test_issue_fluent_bit_9267 handles this correctly with cmt = NULL and a NULL guard. Additionally, the // comment on line 1713 should use /* ... */ style per the project's Apache-style C conventions.
🔧 Proposed fix
-// reproduces https://github.com/fluent/cmetrics/issues/274
+/* reproduces https://github.com/fluent/cmetrics/issues/274 */
void test_issue_274()
{
int status;
- struct cmt *cmt;
+ struct cmt *cmt = NULL;
cfl_sds_t in_buf = read_file(CMT_TESTS_DATA_PATH "/issue_274.txt");
size_t in_size = cfl_sds_len(in_buf);
status = cmt_decode_prometheus_create(&cmt, in_buf, in_size, NULL);
TEST_CHECK(status == 0);
cfl_sds_destroy(in_buf);
- cmt_decode_prometheus_destroy(cmt);
+ if (cmt != NULL) {
+ cmt_decode_prometheus_destroy(cmt);
+ }
}📝 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.
| // reproduces https://github.com/fluent/cmetrics/issues/274 | |
| void test_issue_274() | |
| { | |
| int status; | |
| struct cmt *cmt; | |
| cfl_sds_t in_buf = read_file(CMT_TESTS_DATA_PATH "/issue_274.txt"); | |
| size_t in_size = cfl_sds_len(in_buf); | |
| status = cmt_decode_prometheus_create(&cmt, in_buf, in_size, NULL); | |
| TEST_CHECK(status == 0); | |
| cfl_sds_destroy(in_buf); | |
| cmt_decode_prometheus_destroy(cmt); | |
| } | |
| /* reproduces https://github.com/fluent/cmetrics/issues/274 */ | |
| void test_issue_274() | |
| { | |
| int status; | |
| struct cmt *cmt = NULL; | |
| cfl_sds_t in_buf = read_file(CMT_TESTS_DATA_PATH "/issue_274.txt"); | |
| size_t in_size = cfl_sds_len(in_buf); | |
| status = cmt_decode_prometheus_create(&cmt, in_buf, in_size, NULL); | |
| TEST_CHECK(status == 0); | |
| cfl_sds_destroy(in_buf); | |
| if (cmt != NULL) { | |
| cmt_decode_prometheus_destroy(cmt); | |
| } | |
| } |
🤖 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/tests/prometheus_parser.c` around lines 1713 - 1725, Update
test_issue_274 to initialize cmt to NULL, guard
cmt_decode_prometheus_destroy(cmt) so it runs only when cmt is non-NULL, and
replace the // issue reference with an Apache-style /* ... */ comment.
Source: Coding guidelines
1 participant
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