Clippy cleanup#89
Open
x10an14-nav wants to merge 3 commits into
Open
Conversation
x10an14-nav
commented
Jun 26, 2026
x10an14-nav
commented
- fix missing compression libs
- chore: release v0.1.16
- chore(helm): update chart and image versions to 0.1.16
- feat: add kafka_consumergroup_group_state metric
- style: fix cargo fmt formatting
- chore: release v0.1.17
- chore(helm): update chart and image versions to 0.1.17
- updated the docs with new metric info
- chore: release v0.1.17
- chore(helm): update chart and image versions to 0.1.17
- chore: release v0.1.18
- chore(helm): update chart and image versions to 0.1.18
- Fix native FFI memory leak and improve memory management
- chore: release v0.1.19
- chore(helm): update chart and image versions to 0.1.19
- Support the new config option in helm chart
- Update helm/klag-exporter/templates/configmap.yaml
- Update helm/klag-exporter/values.yaml
- Update helm/klag-exporter/templates/configmap.yaml
- chore: release v0.1.20
- chore(helm): update chart and image versions to 0.1.20
- fix(grafana): widen Topic Message Rate window to [5m]
- feat(kafka): scaffold admin module for batched Admin API wrappers
- feat(kafka): add batched list_offsets_batched FFI wrapper
- feat(kafka): expose admin_handle for batched Admin API callers
- feat(kafka): add batched describe_consumer_groups_batched FFI wrapper
- feat(kafka): add batched list_consumer_group_offsets_batched FFI wrapper
- perf(kafka): replace sequential describe_consumer_groups with batched Admin API
- feat(kafka): add fetch_watermarks_for_partitions using batched ListOffsets
- perf(collector): rewire hot path to batched Admin API with pre-filtered topics
- refactor(kafka): remove legacy per-partition watermark + single-group offset FFI
- docs(readme): describe batched Admin API collection model and updated sizing guidance
- chore(docker): bump Dockerfile.dev rust image to 1.92
- review: address Copilot PR feedback (Tier 1)
- chore: release v0.1.21
- chore(helm): update chart and image versions to 0.1.21
- perf(runtime): cap tokio blocking-thread pool (default 64)
- perf(kafka): skip member-assignment parsing unless granularity=partition
- perf(collector): add TTL cache for compacted-topic detection
- perf(collector): add TTL cache for filtered monitored-topic set
- perf(kafka): intern topic names as Arc with per-call dedup
- review: address Copilot PR feedback (Tier 2)
- feat(collector): add RateSampler for rate-based time-lag estimation
- feat: switch time-lag default to rate-based estimation (Tier 3)
- docs(readme): document rate vs message time-lag modes
- review: address Copilot PR feedback (Tier 3)
- perf(kafka): parallelize DescribeConsumerGroups chunks
- perf(collector): TTL cache list_consumer_groups (default 60s)
- obs(collector): per-phase elapsed_ms in the cycle-complete debug log
- deprecate: max_concurrent_watermarks (no-op since Tier 1 batched ListOffsets)
- review: address Copilot PR feedback (Tier 4)
- chore: release v0.1.22
- chore(helm): update chart and image versions to 0.1.22
- docs: document timestamp_sampling.mode in test-stack + config example
- review: address Copilot feedback on PR docs: document timestamp_sampling.mode in test-stack + config example #72
- chore: release v0.1.22
- Support the new 'mode' config options in helm chart
- feat: configurable metrics staleness threshold (Metrics staleness time is too less for large clusters #60)
- review: validate poll_interval > 0 and staleness_threshold > 0
- feat(helm): expose exporter.staleness_threshold
- docs: document exporter.staleness_threshold in README
- Update helm/klag-exporter/templates/configmap.yaml
- chore: release v0.1.23
- chore(helm): update chart and image versions to 0.1.23
- helm: Fix typos and add configuration stub
- fix(helm): derive containerPort from config.exporter.http_port
- chore: release v0.1.24
- chore(helm): update chart and image versions to 0.1.24
- devex: add nix tooling for reproducible builds
- upkeep(clippy): clean up 100+ clippy lints
Author
|
This branch is based on #88, based feedback given here: #46 (review) |
x10an14-nav
force-pushed
the
clippy_cleanup
branch
from
57c2150 to
86c83eb
Compare
Contributor
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
This PR is primarily a “clippy cleanup” sweep across the Rust codebase plus dev tooling additions (Nix flake + gitignore tweaks), with several small refactors to simplify code paths and reduce allocations.
Changes:
- Refactors various APIs and call sites to use references,
const fn, and more idiomatic combinators (map_or,map_or_else,is_some_and), plus minor formatting/doc comment touchups. - Adjusts Prometheus/OTel rendering code to use
writeln!and minor metric/label formatting changes. - Adds Nix flake support (
flake.nix,flake.lock) and updates.gitignorefor direnv/Nix artifacts.
Reviewed changes
Copilot reviewed 18 out of 20 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/metrics/types.rs | Minor enum/utility cleanups (const fn, pattern matching), adjusts OtelDataPoint timestamp type. |
| src/metrics/registry.rs | Refactors update API to take &LagMetrics; switches Prometheus output building to writeln!; minor iterator/formatting changes. |
| src/main.rs | Small refactors in cluster-manager spawn and shutdown handling; minor tokio::select! style change. |
| src/leadership/noop.rs | Makes constructor const fn. |
| src/leadership/mod.rs | Makes LeadershipState::is_leader a const fn and uses Self::Leader. |
| src/kafka/consumer.rs | Simplifies mutex poison handling and pool logic; refactors poll handling. |
| src/kafka/client.rs | Refactors mutex poison handling; adjusts RSS reclaimed computation line. |
| src/kafka/admin.rs | Expands explicit bindings imports; refactors small iterator/FFI call patterns; changes helper visibility. |
| src/http/server.rs | Minor string formatting simplification for socket address parsing. |
| src/export/prometheus.rs | Makes constructor const fn; updates tests for new registry update signature. |
| src/config.rs | Refactors defaults to const fn and changes duration constructors for readability. |
| src/collector/timestamp_sampler.rs | Minor combinator refactors and doc formatting. |
| src/collector/rate_sampler.rs | Refactors mutex poison handling and small style changes. |
| src/collector/offset_collector.rs | Refactors mutex poison handling, logging, and doc formatting. |
| src/collector/lag_calculator.rs | Small doc formatting and map_or_else refactor. |
| src/cluster/manager.rs | Adjusts ClusterManager::new to take &ClusterConfig; tweaks durations and logging. |
| flake.nix | Adds Nix flake for reproducible dev/build environments. |
| flake.lock | Locks Nix inputs. |
| .gitignore | Ignores direnv, Nix result*, and .env* files. |
Files excluded by content exclusion policy (1)
- .envrc
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+251
to
+252
| const fn default_cache_ttl() -> Duration { | ||
| Duration::from_mins(1) |
Comment on lines
+267
to
+268
| const fn default_rate_history_max_age() -> Duration { | ||
| Duration::from_mins(10) |
Comment on lines
+299
to
+300
| const fn default_compacted_topics_cache_ttl() -> Duration { | ||
| Duration::from_hours(1) |
Comment on lines
+303
to
+304
| const fn default_metadata_cache_ttl() -> Duration { | ||
| Duration::from_mins(5) |
Comment on lines
+307
to
+308
| const fn default_consumer_groups_cache_ttl() -> Duration { | ||
| Duration::from_mins(1) |
Comment on lines
+315
to
+316
| const fn default_export_interval() -> Duration { | ||
| Duration::from_mins(1) |
Comment on lines
+16
to
+17
| /// Default timeout for a single collection cycle (should be less than `poll_interval`) | ||
| const DEFAULT_COLLECTION_TIMEOUT: Duration = Duration::from_mins(1); |
| registry, | ||
| poll_interval: exporter_config.poll_interval, | ||
| max_backoff: Duration::from_secs(300), | ||
| max_backoff: Duration::from_mins(5), |
| rss_before_kb = rss_before, | ||
| rss_after_kb = rss_after, | ||
| rss_reclaimed_kb = rss_before as i64 - rss_after as i64, | ||
| rss_reclaimed_kb = rss_before.cast_signed() - rss_after.cast_signed(), |
Standardize it across all (`Dockerfile.dev` used a different one from the rest)
I used ``` cargo clippy -- -W clippy::pedantic -W clippy::nursery -W clippy::unwrap_used -A clippy::cast_precision_loss -A clippy::cast_possible_truncation -A clippy::too_many_lines -A clippy::missing_fields_in_debug ``` to reduce the amount of warnings from 201 to 24, and clippy suggestions from 151 to 1. I did this to re-familiarize myself with this code-base, I want to give it another try, our kafka clusters are on the orders of magnitude: - 5+ partitions for biggest topics - 1.5k topics - ~1-3k consumergroups (I am not sure if I'm counting double the ACLs or not, since I know we keep some for rotation purposes per consumer) Thus, when I last tried in february, even after a lot of work on the Rust base (code @ https://github.com/nais/klag-exporter/tree/deploy_our_own_helmchart), we could NOT manage to get `klag-exporter` to scrape lag metrics for an entire cluster reliably within ~20-40 seconds without memory leak. Looking at the code now, I suspect you may have dealt with the librdkafka memory leak I struggled with back then.
x10an14-nav
force-pushed
the
clippy_cleanup
branch
from
86c83eb to
18f8e05
Compare
Contributor
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 22 out of 24 changed files in this pull request and generated 10 comments.
Files excluded by content exclusion policy (1)
- .envrc
Comment on lines
+251
to
253
| const fn default_cache_ttl() -> Duration { | ||
| Duration::from_mins(1) | ||
| } |
Comment on lines
+267
to
269
| const fn default_rate_history_max_age() -> Duration { | ||
| Duration::from_mins(10) | ||
| } |
Comment on lines
+299
to
301
| const fn default_compacted_topics_cache_ttl() -> Duration { | ||
| Duration::from_hours(1) | ||
| } |
Comment on lines
+303
to
305
| const fn default_metadata_cache_ttl() -> Duration { | ||
| Duration::from_mins(5) | ||
| } |
| /// Default timeout for a single collection cycle (should be less than poll_interval) | ||
| const DEFAULT_COLLECTION_TIMEOUT: Duration = Duration::from_secs(60); | ||
| /// Default timeout for a single collection cycle (should be less than `poll_interval`) | ||
| const DEFAULT_COLLECTION_TIMEOUT: Duration = Duration::from_mins(1); |
| registry, | ||
| poll_interval: exporter_config.poll_interval, | ||
| max_backoff: Duration::from_secs(300), | ||
| max_backoff: Duration::from_mins(5), |
|
|
||
| let mut n_infos: usize = 0; | ||
| let infos_ptr = rd_kafka_ListOffsets_result_infos(result, &mut n_infos); | ||
| let infos_ptr = rd_kafka_ListOffsets_result_infos(result, &raw mut n_infos); |
|
|
||
| let mut n: usize = 0; | ||
| let groups_ptr = rd_kafka_DescribeConsumerGroups_result_groups(result, &mut n); | ||
| let groups_ptr = rd_kafka_DescribeConsumerGroups_result_groups(result, &raw mut n); |
|
|
||
| let mut n_groups: usize = 0; | ||
| let groups_ptr = rd_kafka_ListConsumerGroupOffsets_result_groups(result, &mut n_groups); | ||
| let groups_ptr = rd_kafka_ListConsumerGroupOffsets_result_groups(result, &raw mut n_groups); |
| rss_before_kb = rss_before, | ||
| rss_after_kb = rss_after, | ||
| rss_reclaimed_kb = rss_before as i64 - rss_after as i64, | ||
| rss_reclaimed_kb = rss_before.cast_signed() - rss_after.cast_signed(), |
Author
|
Now updated with 1.96 (latest stable) across all CI/tooling I could find. |
Contributor
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 22 out of 24 changed files in this pull request and generated 3 comments.
Files excluded by content exclusion policy (1)
- .envrc
Comment on lines
92
to
96
| /// Copy the librdkafka errstr buffer out as a String. | ||
| pub(crate) fn errstr_to_string(buf: &[c_char]) -> String { | ||
| pub fn errstr_to_string(buf: &[c_char]) -> String { | ||
| // SAFETY: librdkafka null-terminates; buf is a stack array owned by caller. | ||
| unsafe { CStr::from_ptr(buf.as_ptr()).to_string_lossy().to_string() } | ||
| } |
Comment on lines
+98
to
101
| /// Build a C cstring from a Rust &str, returning a `KlagError` on embedded NULs. | ||
| pub fn cstring_or_err(s: &str) -> Result<CString> { | ||
| CString::new(s).map_err(|e| KlagError::Admin(format!("Invalid C string '{s}': {e}"))) | ||
| } |
Comment on lines
103
to
108
| /// Keep the admin client alive for the duration of an FFI call. This type is | ||
| /// intentionally opaque: callers pass `&AdminClient` and we hold references | ||
| /// that must not outlive it. | ||
| pub(crate) fn admin_native_ptr(admin: &AdminClient<DefaultClientContext>) -> *mut rd_kafka_t { | ||
| pub fn admin_native_ptr(admin: &AdminClient<DefaultClientContext>) -> *mut rd_kafka_t { | ||
| admin.inner().native_ptr() | ||
| } |
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
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.