feat: add SumCounter for exact u64 counter sums by josecelano · Pull Request #2 · torrust/torrust-metrics

josecelano · 2026-10-17T12:27:36Z

Closes #1.

Adds SumCounter::sum_counter(&MetricName, &LabelSet) -> Option<u64> on MetricCollection (metric_collection::aggregate::sum). It returns the exact u64 sum of a counter's samples, with no u64 -> f64 round trip. Sum/Avg are unchanged.

Design choices

Trait, not inherent method: matches the existing Sum/Avg style and the shape proposed in Add an exact u64 sum aggregate for counters #1.
Missing metric or gauge name: None, mirroring Sum.
Overflow: delegates to Metric<Counter>::sum (Iterator::sum over u64): panics in debug, wraps in release. Not changed; documented on the trait.
Gauges: out of scope (no integer gauge API).
README documents the new trait.

Tests

Label filter, empty label set, missing metric, gauge name, and (1 << 53) + 1, which SumCounter returns exactly while Sum rounds it to 2^53. Plus a doctest.

Checks

cargo test --all-targets --all-features (265 pass), cargo test --doc, linter all all pass.

The second commit fixes 4 pre-existing clippy::assert_is_empty errors in test code (newer Clippy) so linter clippy passes; it is separate so it can be dropped or split out.

Add a counter-only aggregate on MetricCollection that returns the exact u64 sum instead of round-tripping through f64. Sum and Avg are unchanged. Refs #1 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

🟢 Approval recommended

The additive API preserves existing aggregation behavior, includes focused coverage, and has no identified blocking issues.

No open issues 🎉

What changed in this PR

Adds SumCounter to expose exact u64 counter totals through MetricCollection, addressing #1 without changing Sum or Avg.

Changes:

Adds integer aggregation with tests for filtering, missing metrics, gauges, and precision.
Documents the API and overflow behavior.
Updates four existing test assertions for Clippy compatibility.

File	Description
src/unit.rs	Updates a nonempty-string assertion.
src/sample_collection.rs	Updates a Prometheus-output assertion.
src/metric_collection/aggregate/sum.rs	Adds `SumCounter`, documentation, and tests.
src/label/set.rs	Updates two label-string assertions.
README.md	Documents exact counter aggregation.

_{Review effort: Balanced}

josecelano · 2026-10-19T10:02:44Z

ACK aaa87d4

da2ce7

Reviewed at aaa87d4a4e407f0759208eab738734f6f6fb5000 (post-merge). Recomputed from the bytes at the merged head, against the merge base e7ce6b8; the PR merged on 2026-10-19 at 10:04:44Z as f0facdd27, and main has since moved to 9b018b8 with the v0.2.0 release.

Label: POST_MERGE_FINDINGS — the review request this round answers is recorded on 2026-10-17 at 12:31:11Z and was still pending when the PR merged on the 19th, before this round ran; the findings below stand at the merged head for a follow-up PR, and no ACK follows a merged PR. The form is the tracker's merged-PR review form, applied by analogy because this repository defines none of its own.

API. SumCounter::sum_counter(&self, &MetricName, &LabelSet) -> Option<u64> (src/metric_collection/aggregate/sum.rs:40-42) is the issue's proposed declaration byte for byte, reachable as metric_collection::aggregate::sum::SumCounter through the pub mod chain from src/lib.rs:5; nothing is re-exported. Sum and Avg are untouched: the aggregate diff is +118/−0 in sum.rs. The impl looks the name up in counters and calls Metric<Counter>::sum, the same LabelSet::matches filter Sum uses, so an empty criterion selects every sample; a missing name or a gauge name (names are disjoint across kinds) gives None, as in Sum. Metric<Counter>::sum (src/metric/aggregate/sum.rs:14-20) ends in Iterator::sum into u64, whose addition follows overflow-checks: a panic under the default debug profile, wrapping under the default release profile, as the doc comment says.

Tests. Five tests in it_should_allow_summing_counter_samples_exactly plus one doctest. (1 << 53) + 1 = 9007199254740993 lies midway between the binary64 neighbours 2^53 and 2^53 + 2 (53-bit significand, spacing 2 there), and ties-to-even yields 9007199254740992.0, which the single-sample fixture reaches through Sum's as f64. The doctest uses public paths only. Attribute count: 244 #[test] + 16 rstest cases = 260 at the base, 249 + 16 = 265 at the head, consistent with the 265 reported.

Style commit. Four assert!(!x.is_empty()) on String values in tests become assert_ne!(x, ""), the same truth value (String: PartialEq<&str>). The lint name cannot be attested from the bytes, and whether the base fails clippy depends on the Clippy release and the lint's group; CI runs the floating stable toolchain.

README. The new section names only items that exist with those parameter and return types, and its example has the doctest's call shape; the overflow sentence lives on the trait only.

Merge and release. f0facdd27 has parents e7ce6b8 and aaa87d4a4, the head's tree, and a Tree-SHA512 line that recomputes. The issue closed in the merge second, from the body's Closes #1. (the commit says Refs #1). The release touches Cargo.toml, Cargo.lock and README.md (+3/−3), install snippet included; under Cargo's rules an additive trait would fit 0.1.1, so 0.2.0 is permitted and conservative. Nothing in the new code needs more than 1.88.

Hygiene. Conventional subjects without scope, like the rest of main; author equals committer; git diff --check is clean per commit; no unsafe; no project-words.txt or cspell.json change. Both commits carry an attribution footer naming the review bot; no rule here forbids it.

The review bot. Its five-row file table matches the diff and its summary is accurate; its "no open issues" does not cover the README findings below, which sit outside the diff.

Findings.

F1 [Minor] README.md:122 (pre-existing): no collection sum(&name, &labels) returns Option<u64>.
F2 [Minor] README.md:72 (pre-existing): aggregate::{Sum, Avg} does not resolve.
F3 [Suggestion] Some(0) for a counter with no matching sample is neither documented nor tested.
F4 [Suggestion] the counter lookup is written twice.

Checked, no finding. Declaration and placement against the issue; reachability; Sum/Avg unchanged; shared label matching and empty criterion; both None cases; the overflow sentence; the precision arithmetic; the doctest; the test count; the four style hunks; the README section's names; merge parents, tree and Tree-SHA512; issue closure; release files and version mention; the 1.88 floor; commit hygiene.

Checks

On the loop's compute hub at the merged head, merge base e7ce6b8 (receipt server-gates-sojka-368; toolchain rustc 1.101.0-nightly of 2026-10-16, clippy 0.1.101): cargo build --all-targets --all-features exit 0 (10 s); cargo test --tests --all-targets --all-features exit 0, 265 passed, 0 failed; cargo test --doc exit 0, 1 passed; cargo clippy --all-targets --all-features -- -D warnings exit 0; cargo fmt --check exit 0; linter all exit 0. At the base e7ce6b8, re-measured in its own target directory: 260 passed, 0 failed; cargo test --doc exit 0; cargo clippy --all-targets --all-features -- -D warnings exit 0, so the hub's toolchain does not reproduce the four assertion errors the style commit fixes; linter all exit 0. GitHub checks at this head at posting time: "CodeQL: completed/success; copilot-pull-request-reviewer: completed/success; CI: completed/success; Analyze (rust): completed/success; Analyze (actions): completed/success; CI: completed/success". At posting, main is 9b018b858.

da2ce7 · 2026-10-19T10:59:18Z

 println!("Average response time: {:?}", avg_response_time);
 ```

+`Sum` returns an `f64` for both counters and gauges. To get the exact `u64` total of a


[Minor][F1] Pre-existing, outside the diff: the Type System block at README.md:122 contradicts this sentence.

let counter_sum: Option<u64> = counter_collection.sum(&name, &labels); matches no method at the merged head: the only (&MetricName, &LabelSet) aggregate on MetricKindCollection<Counter> is Sum, which returns Option<f64> (src/metric_collection/aggregate/sum.rs:67-73), and the only one returning Option<u64> is the new sum_counter. With this line now stating that Sum returns an f64 for counters, the README says both. A follow-up can rewrite line 122 as metric_collection.sum_counter(&name, &labels).

da2ce7 · 2026-10-19T10:59:18Z

+counter (no rounding above `2^53`), use `SumCounter`:
+
+```rust
+use torrust_metrics::metric_collection::aggregate::sum::SumCounter;


[Minor][F2] Pre-existing, outside the diff: this import path is right, but the one two blocks up is not.

README.md:72 imports torrust_metrics::metric_collection::aggregate::{Sum, Avg}, while src/metric_collection/aggregate/mod.rs declares only pub mod avg; and pub mod sum; with no re-exports, so that path does not resolve; the traits live at aggregate::sum::Sum and aggregate::avg::Avg. A reader who copies the aggregation section gets one working import and one failing one. The README blocks are compiled nowhere (src/lib.rs does not include the README as crate docs), which is how this went unnoticed. A follow-up can fix line 72, or add pub use re-exports in aggregate/mod.rs, which would also make aggregate::SumCounter valid.

da2ce7 · 2026-10-19T10:59:18Z

+/// Unlike [`Sum`], which returns an `f64` for both counters and gauges, this
+/// trait returns the exact `u64` total, so values above `2^53` are not rounded.
+///
+/// It returns `None` if there is no counter with the given name. A metric with


[Suggestion][F3] Pin the Some(0) boundary next to the None cases.

Both None cases are documented and tested, but the neighbouring case is not: a counter that exists with no sample matching the criterion, or one registered through describe_counter (src/metric_collection/mod.rs:87-93) with no samples, returns Some(0) from Metric<Counter>::sum over an empty filter, not None. That None versus Some(0) line is what a consumer replacing its own integer-total helper relies on. A sentence here, one in the README, and a test such as returning_zero_when_no_sample_matches_the_label_set would settle it.

da2ce7 · 2026-10-19T10:59:18Z

+
+impl SumCounter for MetricCollection {
+    fn sum_counter(&self, metric_name: &MetricName, label_set_criteria: &LabelSet) -> Option<u64> {
+        self.counters


[Suggestion][F4] One counter lookup instead of two.

This body is impl Sum for MetricKindCollection<Counter> (lines 67-73) without the as f64. Implementing SumCounter for MetricKindCollection<Counter> and having both MetricCollection::sum_counter and that Sum impl delegate to it (the latter as .map(|total| total as f64) under the existing cast_precision_loss allowance) keeps the exact and the rounded paths from drifting, and gives the public MetricKindCollection<Counter> the exact sum it now offers only rounded.

…m from torrust-metrics 0.2.0 dc461b1 docs(pr-reviews): [#2560] record the fourth review round in the PR #2561 audit (Jose Celano) 013fd9c docs(issues): [#2560] put the overflow check where a reader looks for it (Jose Celano) e107e05 docs(pr-reviews): [#2560] record Copilot's second review in the PR #2561 audit (Jose Celano) c3d5c65 docs(issues): [#2560] correct the overflow claim: only the f64 rounding goes away (Jose Celano) 578be10 docs(pr-reviews): [#2560] record the third review round in the PR #2561 audit (Jose Celano) 9bc76fd docs(statistics): [#2560] say that counter_total reads a gauge as zero (Jose Celano) 12ffda0 docs(pr-reviews): [#2560] add the PR #2561 review audit (Jose Celano) db4b743 docs(issues): [#2560] keep the independent-reviewer-reports checkpoint open (Jose Celano) 88e7236 docs(issues): [#2560] make the Background say where the gauge cast stays (Jose Celano) aa62042 docs(issues): [#2560] set the spec in review and rerun the evidence on the final code (Jose Celano) 5ace658 refactor(statistics): [#2560] share the counter read behind a cast-free helper (Jose Celano) a2b1486 test(udp-server): [#2560] use a fixed timestamp for the exact-total test (Jose Celano) ceb0532 docs(issues): [#2560] record the verification evidence and the completed spec (Jose Celano) bab3b8d refactor(udp-server): [#2560] read counter totals through sum_counter (Jose Celano) b3bb319 refactor(udp-core): [#2560] read counter totals through sum_counter (Jose Celano) eec2ee2 refactor(http-core): [#2560] read counter totals through sum_counter (Jose Celano) 1049977 chore(deps): [#2560] bump torrust-metrics to 0.2.0 (Jose Celano) aa71bc1 docs(issues): [#2560] add the spec to adopt the exact counter sum from torrust-metrics 0.2.0 (Jose Celano) Pull request description: ## Summary Upgrades `torrust-metrics` to 0.2.0 in every package that uses it and reads integer counter totals through its new exact `SumCounter::sum_counter`, so the tracker no longer converts a `u64` counter through `f64` and back. This finishes the follow-up that #2244 left for the upstream release (torrust/torrust-metrics#1, implemented in torrust/torrust-metrics#2). Closes #2560 ## What changed - Bumped `torrust-metrics` from 0.1.0 to 0.2.0 in all eight manifests (`axum-rest-api-server`, `http-core`, `rest-api-protocol`, `rest-api-runtime-adapter`, `swarm-coordination-registry`, `tracker-core`, `udp-core`, `udp-server`). They have to move together: Cargo treats 0.1.0 and 0.2.0 as incompatible, and `MetricCollection` appears in structs that cross package boundaries. `Cargo.lock` changes only the `torrust-metrics` entry (version and checksum) and still resolves a single copy. - HTTP core and UDP core: replaced the `whole_total` helpers and their `#[expect]` attributes with a private cast-free `counter_total` helper, whose body is the only `sum_counter` call in the file. Every counter accessor calls it. - UDP server: the 17 counter accessors use the same `counter_total` helper, and the `whole_total` helper is gone. The banned-IP total is a gauge, because `BanService::reset_bans` lowers it, and `sum_counter` returns `None` for a gauge. Its accessor keeps the one remaining `f64`-to-`u64` cast under a permanent `#[expect]` that says so. The draft spec proposed a renamed gauge helper, but with a single call site the cast belongs in the accessor, and the spec records this deviation. A review finding asked for one place for the repeated counter read, which is why `counter_total` exists. - One test per package that a total of 2^53 + 1, the first whole number an `f64` rounds, is returned exactly. Each fails against the old path with 9007199254740992 and passes after the change. HTTP core's red run used the mutate-then-restore method. - Added the issue spec, the manual verification evidence, and its artifacts under `docs/issues/open/2560-adopt-torrust-metrics-sum-counter/`. ## What changes for large totals, and what does not Overflow behavior is unchanged. The old path summed the samples as `u64` in `Metric<Counter>::sum` before converting to `f64`, and `sum_counter` returns that same `u64` sum, so both panic on overflow in a debug build. The old final saturating cast never protected that earlier addition. A throwaway test with two samples at `u64::MAX` shows both paths panicking, and it is saved as `evidence/overflow-check.txt`. What the change removes is the rounding of a total above 2^53 to the nearest `f64`. A counter would need about 580,000 years at one million requests per second to reach `u64::MAX`, so overflow cannot occur in practice. ## Files and packages touched - `Cargo.lock` and the eight `packages/*/Cargo.toml` manifests. - `packages/http-core`, `packages/udp-core`, and `packages/udp-server`: `src/statistics/metrics.rs` only. - `docs/issues/open/2560-adopt-torrust-metrics-sum-counter/`. ## Validation - `cargo clippy -p torrust-tracker-http-core -p torrust-tracker-udp-core -p torrust-tracker-udp-server --all-targets --all-features -- -D warnings` is clean. `#[expect]` makes Clippy fail when a lint stops firing, so a stale exception would show. - The tests of the three packages and of the two consumers of the UDP server statistics pass. HTTP core has 33, UDP core 97, and UDP server 225, one new test each. - `cargo machete --with-metadata` finds nothing, and the pre-commit hook (`linter all` among others) passed on every commit. `pre-push.sh` passed (nightly format, check and docs, all tests). - Manual verification against a local release tracker: 1,000 `tracker_client udp announce` invocations read exactly 1,000 connects and 1,000 announces (2,000 requests and responses), and 50 HTTP announces read exactly 50. Six UDP announces with an invalid connection ID banned the IP, and `udp_banned_ips_total` read 1 through the retained gauge cast, matching the Prometheus gauge. The commands and output are in `manual-verification-evidence.md`. ## Not in this PR The averaged accessors and the moving-average code still read gauges and keep their documented casts. The archived #2158 inventory and the archived #2244 spec are historical records, so I left them untouched. ACKs for top commit: josecelano: ACK dc461b1 Tree-SHA512: dfd5998b9b0eed4c991a6cc8c4afc7598b08fe78615c8db03c490d09db5254781fc6f4340f00f8e1d2ed4ff46dad1c3b22fb56da2d0dab6cba318781840c7b85

josecelano and others added 2 commits October 17, 2026 13:27

feat: add SumCounter for exact u64 counter sums

8e68a43

Add a counter-only aggregate on MetricCollection that returns the exact u64 sum instead of round-tripping through f64. Sum and Avg are unchanged. Refs #1 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

style: fix clippy assert_is_empty in tests

aaa87d4

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI balanced review requested due to automatic review settings October 17, 2026 12:27

Copilot started reviewing on behalf of josecelano October 17, 2026 12:28

View session

Copilot AI reviewed Oct 17, 2026

View reviewed changes

josecelano self-assigned this Oct 17, 2026

josecelano requested a review from da2ce7 October 17, 2026 12:31

josecelano merged commit f0facdd into main Oct 19, 2026
6 checks passed

josecelano deleted the 1-add-exact-u64-counter-sum branch October 19, 2026 10:07

josecelano mentioned this pull request Oct 19, 2026

chore: release v0.2.0 #3

Merged

da2ce7 reviewed Oct 19, 2026

View reviewed changes

This was referenced Oct 19, 2026

Adopt the exact counter sum from torrust-metrics 0.2.0 torrust/torrust-tracker#2560

Closed

refactor(statistics): [#2560] adopt the exact counter sum from torrust-metrics 0.2.0 torrust/torrust-tracker#2561

Merged

Navigation Menu

Repository navigation

feat: add SumCounter for exact u64 counter sums - #2

feat: add SumCounter for exact u64 counter sums#2
josecelano merged 2 commits into
mainfrom
1-add-exact-u64-counter-sum

josecelano commented Oct 17, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

josecelano commented Oct 19, 2026

Uh oh!

Uh oh!

da2ce7 left a comment

Uh oh!

da2ce7 Oct 19, 2026

Uh oh!

da2ce7 Oct 19, 2026

Uh oh!

da2ce7 Oct 19, 2026

Uh oh!

da2ce7 Oct 19, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

josecelano commented Oct 17, 2026

Design choices

Tests

Checks

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

josecelano commented Oct 19, 2026

Uh oh!

Uh oh!

da2ce7 left a comment

Choose a reason for hiding this comment

Checks

Uh oh!

da2ce7 Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

da2ce7 Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

da2ce7 Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

da2ce7 Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants