Clippy cleanup#89
Closed
x10an14-nav wants to merge 3 commits into
Closed
Conversation
x10an14-nav
commented
Jun 26, 2026
x10an14-nav
commented
Contributor
- 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
Contributor
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(), |
Contributor
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() | ||
| } |
softberries
requested changes
Jul 3, 2026
softberries
left a comment
softberries
left a comment
Member
There was a problem hiding this comment.
The Rust changes themselves check out: we traced every mechanical refactor (branch swaps, map_or/map_or_else argument order, cast_signed, f64::from(i32), the from_secs → from_mins/from_hours conversions, the &LagMetrics signature change) and they're all semantically equivalent to the old code, with no added clones or hot-path cost. CI is green. Two structural things need sorting out before merge, though.
Blocking:
-
This PR is stacked on the unmerged #88, so merging it would land all the nix tooling as a side effect — including the two issues flagged there (the
revCounteval bug inflake.nixand the over-broad.env*/result*gitignore patterns). Please rebase onto main once #88 is resolved, or apply those two fixes here. -
The toolchain bump is partial:
ci.ymlstill pinsdtolnay/rust-toolchain@1.93(×4) while release.yml, post-release.yml, release-plz-pr.yml, and Dockerfile.dev move to 1.96.post-release.ymlbuilds the shipped binaries, so releases now compile with a toolchain CI never tested. Note the bump isn't actually required —Duration::from_mins/from_hoursstabilized in 1.91, and CI passes on 1.93 — so either revert the 1.96 bumps or bumpci.ymlto match; just keep them identical.
Optional follow-ups:
- The cleanup fixes ~100 pedantic/nursery-level lints, but CI runs default clippy (
-D warnings) and there's no[lints.clippy]table orclippy.toml, so nothing enforces the new standard and the codebase will drift back. Encoding the intended level inCargo.toml[lints]would make this PR's work stick. - The effective MSRV is now 1.91 (
from_mins/from_hours); declaringrust-version = "1.91"inCargo.tomlwould document/enforce it. .lock().unwrap_or_else(std::sync::PoisonError::into_inner)now appears at ~19 call sites across four files — a smalllock_ignore_poisonhelper would make the poison policy a one-line change instead of nineteen.- A few of the lint fixes hurt readability: the
super::super::collector::timestamp_sampler::TimestampSampler::cache_sizepath inmanager.rs(a closure satisfies the lint), theconsumer.rspollmatch→map_or_elserewrite that puts the no-message branch before the 30-line success branch, the fully-qualifiedstd::string::ToString::to_string-style paths (prelude names suffice), and widening the deadtimestamp_msfield tou128(any real consumer would need to narrow it again).option_if_let_elsein particular is a known-controversial lint worth allowing rather than obeying.
One genuine behavior change to be aware of (fine as-is): duration_since(UNIX_EPOCH).unwrap() → unwrap_or_default() in registry.rs swaps panic-on-backwards-clock for a silent epoch-0 timestamp — currently inert since timestamp_ms is never read on the export path, but worth remembering if it's ever wired into the OTLP payload.
🤖 Generated with Claude Code
Member
|
The conflict here is fallout from #88 landing first, so I resolved it for you rather than asking for another round: merged main into your branch (your commits untouched - no rebase) taking main's versions of .envrc/.gitignore/flake.nix, plus one commit aligning the remaining Rust pins at 1.96 - ci.yml was still installing 1.93 and the flake pinned 1.93.0. Verified locally on 1.96: fmt, clippy -D warnings, and all tests green. I tried to push this straight to your branch, but GitHub doesn't allow maintainer edits on org-owned forks (the checkbox has no effect for those), so it's sitting on pr89-main-merge in this repo as a fast-forward of your branch. To take it: git fetch git@github.com:softwaremill/klag-exporter.git pr89-main-merge Once CI is green here I'll merge. |
Member
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.