lib: cmetrics: upgrade to v2.2.1#12088
Open
edsiper wants to merge 2 commits into
Open
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:
📝 WalkthroughWalkthroughCMetrics adds synchronized label-aware metric indexing, arena-backed OpenTelemetry encoding, revised remote-write staleness handling, lossless MessagePack strings, opt-in benchmarks, repository workflows, and expanded regression coverage. ChangesCMetrics runtime and validation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant cmt_map_metric_get
participant metric_hash_lookup
participant metric_prepare_storage
Caller->>cmt_map_metric_get: request metric with labels and write_op
cmt_map_metric_get->>metric_hash_lookup: find hash and matching labels
metric_hash_lookup-->>cmt_map_metric_get: return existing or indexed metric
cmt_map_metric_get->>metric_prepare_storage: prepare lazy storage
metric_prepare_storage-->>Caller: return metric
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/benchmarks/benchmark.c`:
- Around line 115-120: Check the return value of cmt_create() immediately in
benchmark_lookup, benchmark_update, benchmark_prometheus, and
benchmark_opentelemetry before passing cmt to create_series(). On NULL, return
the same failure result with appropriate cleanup, matching the existing handling
in benchmark_opentelemetry_mixed, while preserving the current counter
allocation failure paths.
In `@lib/cmetrics/src/cmt_cat.c`:
- Around line 889-902: The summary reuse validation after summary_lookup must
compare the label-key schema, not just quantiles. Before merging a matching
summary, compare each label key and its ordering against the destination
summary’s labels, while retaining the existing quantile checks; reject the reuse
path and free labels on any key mismatch. Do not add a redundant label-count
check.
In `@lib/cmetrics/src/cmt_map.c`:
- Around line 86-107: Update cmt_map_metric_destroy so it clears
map->last_metric whenever it points to the metric being destroyed, regardless of
metric->hash_indexed. Keep hash-list removal and indexed-count updates
conditional on hash_indexed, while preserving metric->map cleanup and preventing
metric_hash_lookup from dereferencing freed memory.
- Around line 32-41: Update cmt_map_metric_destroy() to acquire map_lock()
before modifying metric_buckets, indexed_metric_count, or last_metric, and
release it with map_unlock() on every exit path. Preserve the existing
destruction behavior while ensuring concurrent cmt_map_metric_get() and
cmt_map_metrics_expire() operations cannot traverse the map during unlinking.
🪄 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: ed6893ea-2caa-4f0e-99c9-adebcbf055fa
📒 Files selected for processing (32)
lib/cmetrics/.github/workflows/build.yamllib/cmetrics/AGENTS.mdlib/cmetrics/CMakeLists.txtlib/cmetrics/README.mdlib/cmetrics/benchmarks/CMakeLists.txtlib/cmetrics/benchmarks/README.mdlib/cmetrics/benchmarks/benchmark.clib/cmetrics/benchmarks/run-perf.shlib/cmetrics/docs/label-value-handling.mdlib/cmetrics/include/cmetrics/cmt_decode_prometheus.hlib/cmetrics/include/cmetrics/cmt_map.hlib/cmetrics/include/cmetrics/cmt_metric.hlib/cmetrics/include/cmetrics/cmt_mpack_utils_defs.hlib/cmetrics/src/cmt_cat.clib/cmetrics/src/cmt_decode_msgpack.clib/cmetrics/src/cmt_decode_prometheus.clib/cmetrics/src/cmt_encode_opentelemetry.clib/cmetrics/src/cmt_encode_prometheus_remote_write.clib/cmetrics/src/cmt_filter.clib/cmetrics/src/cmt_map.clib/cmetrics/src/cmt_mpack_utils.clib/cmetrics/src/cmt_summary.clib/cmetrics/tests/CMakeLists.txtlib/cmetrics/tests/cat.clib/cmetrics/tests/counter.clib/cmetrics/tests/encoding.clib/cmetrics/tests/expire.clib/cmetrics/tests/filter.clib/cmetrics/tests/format_conversion.clib/cmetrics/tests/issues.clib/cmetrics/tests/prometheus_parser.clib/cmetrics/tests/summary.c
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/cmetrics/src/cmt_decode_msgpack.c (1)
1336-1345: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for implicitly-static summary quantile-count decoding.
None of the summary tests in
tests/cat.cexercise the zero-label ("implicitly static") path throughunpack_metric_array_entry; they all use non-empty label sets, so this fix (lines 1339-1340) has no direct test coverage in the reviewed diff. Consider adding a msgpack round-trip test for a summary created with 0 labels that assertssum_quantiles_counton the decoded map matches the originalquantiles_count.🤖 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_msgpack.c` around lines 1336 - 1345, Add a regression test in tests/cat.c covering a msgpack round trip for a summary created with zero labels, exercising unpack_metric_array_entry’s implicitly static path. Assert that the decoded map’s sum_quantiles_count matches the original quantiles_count, while preserving the existing non-empty-label summary tests.
🤖 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.
Nitpick comments:
In `@lib/cmetrics/src/cmt_decode_msgpack.c`:
- Around line 1336-1345: Add a regression test in tests/cat.c covering a msgpack
round trip for a summary created with zero labels, exercising
unpack_metric_array_entry’s implicitly static path. Assert that the decoded
map’s sum_quantiles_count matches the original quantiles_count, while preserving
the existing non-empty-label summary tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ea519f4-1a5e-41c7-94c7-1236fa8cbc46
📒 Files selected for processing (32)
lib/cmetrics/.github/workflows/build.yamllib/cmetrics/AGENTS.mdlib/cmetrics/CMakeLists.txtlib/cmetrics/README.mdlib/cmetrics/benchmarks/CMakeLists.txtlib/cmetrics/benchmarks/README.mdlib/cmetrics/benchmarks/benchmark.clib/cmetrics/benchmarks/run-perf.shlib/cmetrics/docs/label-value-handling.mdlib/cmetrics/include/cmetrics/cmt_decode_prometheus.hlib/cmetrics/include/cmetrics/cmt_map.hlib/cmetrics/include/cmetrics/cmt_metric.hlib/cmetrics/include/cmetrics/cmt_mpack_utils_defs.hlib/cmetrics/src/cmt_cat.clib/cmetrics/src/cmt_decode_msgpack.clib/cmetrics/src/cmt_decode_prometheus.clib/cmetrics/src/cmt_encode_opentelemetry.clib/cmetrics/src/cmt_encode_prometheus_remote_write.clib/cmetrics/src/cmt_filter.clib/cmetrics/src/cmt_map.clib/cmetrics/src/cmt_mpack_utils.clib/cmetrics/src/cmt_summary.clib/cmetrics/tests/CMakeLists.txtlib/cmetrics/tests/cat.clib/cmetrics/tests/counter.clib/cmetrics/tests/encoding.clib/cmetrics/tests/expire.clib/cmetrics/tests/filter.clib/cmetrics/tests/format_conversion.clib/cmetrics/tests/issues.clib/cmetrics/tests/prometheus_parser.clib/cmetrics/tests/summary.c
🚧 Files skipped from review as they are similar to previous changes (27)
- lib/cmetrics/AGENTS.md
- lib/cmetrics/benchmarks/run-perf.sh
- lib/cmetrics/README.md
- lib/cmetrics/.github/workflows/build.yaml
- lib/cmetrics/benchmarks/README.md
- lib/cmetrics/benchmarks/CMakeLists.txt
- lib/cmetrics/src/cmt_decode_prometheus.c
- lib/cmetrics/tests/CMakeLists.txt
- lib/cmetrics/include/cmetrics/cmt_mpack_utils_defs.h
- lib/cmetrics/CMakeLists.txt
- lib/cmetrics/src/cmt_cat.c
- lib/cmetrics/include/cmetrics/cmt_map.h
- lib/cmetrics/tests/summary.c
- lib/cmetrics/tests/filter.c
- lib/cmetrics/src/cmt_filter.c
- lib/cmetrics/tests/prometheus_parser.c
- lib/cmetrics/tests/encoding.c
- lib/cmetrics/include/cmetrics/cmt_decode_prometheus.h
- lib/cmetrics/include/cmetrics/cmt_metric.h
- lib/cmetrics/docs/label-value-handling.md
- lib/cmetrics/src/cmt_mpack_utils.c
- lib/cmetrics/src/cmt_summary.c
- lib/cmetrics/tests/format_conversion.c
- lib/cmetrics/benchmarks/benchmark.c
- lib/cmetrics/src/cmt_encode_prometheus_remote_write.c
- lib/cmetrics/src/cmt_map.c
- lib/cmetrics/src/cmt_encode_opentelemetry.c
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/cmetrics/src/cmt_encode_opentelemetry.c (1)
29-66: 🩺 Stability & Availability | 🔵 TrivialRun ASAN/Valgrind to validate the heap-to-arena ownership transition.
This change moves all temporary protobuf encoding structures from heap to arena allocation. Per the project's memory-safety review guidelines, ownership changes should be validated with AddressSanitizer or Valgrind to detect invalid frees, use-after-free, or double-free issues — especially in the destroy paths and
destroy_opentelemetry_contextteardown ordering (line 2904–2909:destroy_metrics_dataruns beforecfl_arena_destroy).Suggested verification:
# Build with ASAN cmake -S lib/cmetrics -B build/cmetrics-asan \ -DCMAKE_C_FLAGS="-fsanitize=address -fno-omit-frame-pointer" \ -DCMAKE_EXE_LINKER_FLAGS="-fsanitize=address" cmake --build build/cmetrics-asan ctest --test-dir build/cmetrics-asan -R opentelemetry --output-on-failure # Or with Valgrind cmake --build lib/cmetrics/build ctest --test-dir lib/cmetrics/build -R opentelemetry \ --valgrind --output-on-failureBased on learnings, use AddressSanitizer or Valgrind for ownership changes, following
docs/ai/memory-safety-review.md. Also runscripts/agent-verify.shbefore handoff.Also applies to: 2895-2911, 2979-3015
🤖 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_opentelemetry.c` around lines 29 - 66, Validate the arena ownership transition in create_encoder_arena, destroy_opentelemetry_context, and the related destroy paths using AddressSanitizer or Valgrind, covering opentelemetry tests for invalid frees, use-after-free, and double frees. Confirm teardown ordering keeps destroy_metrics_data before cfl_arena_destroy, then run scripts/agent-verify.sh before handoff.Source: Learnings
🤖 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.
Nitpick comments:
In `@lib/cmetrics/src/cmt_encode_opentelemetry.c`:
- Around line 29-66: Validate the arena ownership transition in
create_encoder_arena, destroy_opentelemetry_context, and the related destroy
paths using AddressSanitizer or Valgrind, covering opentelemetry tests for
invalid frees, use-after-free, and double frees. Confirm teardown ordering keeps
destroy_metrics_data before cfl_arena_destroy, then run scripts/agent-verify.sh
before handoff.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45d24223-711b-4f4b-805f-c79dbf829c11
📒 Files selected for processing (47)
lib/cmetrics/.github/workflows/build.yamllib/cmetrics/AGENTS.mdlib/cmetrics/CLAUDE.mdlib/cmetrics/CMakeLists.txtlib/cmetrics/README.mdlib/cmetrics/benchmarks/CMakeLists.txtlib/cmetrics/benchmarks/README.mdlib/cmetrics/benchmarks/benchmark.clib/cmetrics/benchmarks/run-perf.shlib/cmetrics/docs/ai/bug-fix.mdlib/cmetrics/docs/ai/code-review.mdlib/cmetrics/docs/ai/cross-repository.mdlib/cmetrics/docs/ai/dependency-update.mdlib/cmetrics/docs/ai/investigate.mdlib/cmetrics/docs/ai/memory-safety-review.mdlib/cmetrics/docs/ai/performance-review.mdlib/cmetrics/docs/architecture.mdlib/cmetrics/docs/dependencies.mdlib/cmetrics/docs/label-value-handling.mdlib/cmetrics/include/cmetrics/cmt_decode_prometheus.hlib/cmetrics/include/cmetrics/cmt_map.hlib/cmetrics/include/cmetrics/cmt_metric.hlib/cmetrics/include/cmetrics/cmt_mpack_utils_defs.hlib/cmetrics/scripts/agent-build.shlib/cmetrics/scripts/agent-test.shlib/cmetrics/scripts/agent-verify.shlib/cmetrics/src/cmt_cat.clib/cmetrics/src/cmt_decode_msgpack.clib/cmetrics/src/cmt_decode_prometheus.clib/cmetrics/src/cmt_encode_opentelemetry.clib/cmetrics/src/cmt_encode_prometheus_remote_write.clib/cmetrics/src/cmt_filter.clib/cmetrics/src/cmt_map.clib/cmetrics/src/cmt_mpack_utils.clib/cmetrics/src/cmt_summary.clib/cmetrics/tests/CMakeLists.txtlib/cmetrics/tests/cat.clib/cmetrics/tests/counter.clib/cmetrics/tests/encoding.clib/cmetrics/tests/expire.clib/cmetrics/tests/filter.clib/cmetrics/tests/format_conversion.clib/cmetrics/tests/issues.clib/cmetrics/tests/opentelemetry.clib/cmetrics/tests/prometheus_parser.clib/cmetrics/tests/summary.ctests/runtime/processor_labels.c
🚧 Files skipped from review as they are similar to previous changes (37)
- lib/cmetrics/docs/ai/performance-review.md
- lib/cmetrics/tests/CMakeLists.txt
- lib/cmetrics/scripts/agent-build.sh
- lib/cmetrics/include/cmetrics/cmt_decode_prometheus.h
- lib/cmetrics/benchmarks/CMakeLists.txt
- lib/cmetrics/scripts/agent-test.sh
- lib/cmetrics/scripts/agent-verify.sh
- lib/cmetrics/docs/ai/memory-safety-review.md
- lib/cmetrics/.github/workflows/build.yaml
- lib/cmetrics/docs/ai/cross-repository.md
- lib/cmetrics/benchmarks/run-perf.sh
- lib/cmetrics/src/cmt_decode_prometheus.c
- lib/cmetrics/include/cmetrics/cmt_metric.h
- lib/cmetrics/docs/ai/bug-fix.md
- lib/cmetrics/src/cmt_mpack_utils.c
- tests/runtime/processor_labels.c
- lib/cmetrics/AGENTS.md
- lib/cmetrics/CMakeLists.txt
- lib/cmetrics/include/cmetrics/cmt_mpack_utils_defs.h
- lib/cmetrics/src/cmt_decode_msgpack.c
- lib/cmetrics/docs/label-value-handling.md
- lib/cmetrics/tests/summary.c
- lib/cmetrics/include/cmetrics/cmt_map.h
- lib/cmetrics/src/cmt_filter.c
- lib/cmetrics/tests/prometheus_parser.c
- lib/cmetrics/tests/counter.c
- lib/cmetrics/tests/format_conversion.c
- lib/cmetrics/src/cmt_summary.c
- lib/cmetrics/tests/opentelemetry.c
- lib/cmetrics/src/cmt_cat.c
- lib/cmetrics/README.md
- lib/cmetrics/benchmarks/benchmark.c
- lib/cmetrics/src/cmt_encode_prometheus_remote_write.c
- lib/cmetrics/tests/filter.c
- lib/cmetrics/tests/cat.c
- lib/cmetrics/src/cmt_map.c
- lib/cmetrics/tests/encoding.c
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