refactor(stats): retain exact finite timing samples #98

2026-08-15T03:41:59Z

Lochnair commented

2026-08-15 03:41:59 +00:00

(Migrated from github.com)

Summary

SampleMode::Exact now retains exact samples for send call, timer error, and server processing, which previously always built their TimeMetric with retention disabled.
All eleven public timing metrics now follow the same Exact/RunningOnly retention contract, derived from one retain_exact decision in CoreStats::new.
No new capability API was needed. TimeStats::median_ns: Option<f64> already distinguishes a retained median from an absent one; the missing behavior was retention, not representation.

Behavior

Finite summaries gain real medians for the send call, timer error, and server processing rows when samples exist, instead of -.
Continuous mode remains bounded and medians remain unavailable for every timing metric; the existing continuous-mode note stays correct.
Absent optional server-processing measurements remain absent — a reply without a processing measurement adds no sample, and no zero/default value is synthesized.
Event semantics are unchanged: send call and timer error still come only from ClientEvent::EchoSent, and server processing only from replies that carry one.
LateReplyMode is untouched. A CountOnly matched late reply still contributes no timing measurements, so it does not enter server-processing exact retention.

Retention estimate

The estimator stays in irtt-stats. EXACT_SAMPLE_VECS_PER_REPLY = 8 becomes EXACT_SAMPLES_PER_PROBE = 11, since exact storage now includes send-side metrics associated with probes rather than replies.
Finite estimate on this target rises from 668 to 764 bytes per probe, reflecting real additional retention. The running-only ceiling is unchanged at ~1.6 MiB.
The estimator keeps PR #97's philosophy: approximate, conservative, size_of-grounded, deterministic, saturating, optional measurements assumed present for an upper bound, running-only bounded, time-based rolling excluded as documented.
exact_mode_retains_one_sample_per_counted_metric counts the public timing metrics a fully measured probe actually retains and asserts it equals EXACT_SAMPLES_PER_PROBE, so changing retention without revisiting the estimate fails in irtt-stats.
The CLI still calls StatsConfig::estimated_retained_bytes(...) and consumed the new figure automatically — no CLI test needed a hardcoded adjustment, and no frontend memory heuristic was added.

Scope

No changes to:

the median algorithm or sample variance;
the rolling API or rolling retention semantics;
LateReplyMode;
IPDV tracking or continuous memory bounds;
irtt-client, irtt-server, or irtt-proto behavior;
CLI summary columns, row ordering, units, formatting vocabulary, or verbosity rules;
TUI layout. The TUI timing panel does not render a median column, so it is unaffected. PR #93's timing-panel wrapping observation is deliberately not addressed here.

Tests

irtt-stats: exact medians for send call (odd count), timer error (even count), and server processing; absent server processing retains no samples; running-only still counts those three but reports no median; a regression test asserting every timing metric with samples reports a median in exact mode; a CountOnly late-reply assertion that server processing gains no retained median.
irtt-stats retention: the estimate covers every retained timing sample, documented as an implementation test rather than a stable public constant. Existing linearity, saturation, running-only bound, rolling, and time-based-exclusion tests are unchanged and still pass.
irtt-cli: a finite summary test proving real median cells for the three rows using spread values whose median differs from min, mean, and max, plus a continuous counterpart asserting those cells stay -.

Verification

All run on this branch, all passing:

cargo test -p irtt-stats --all-features --locked
cargo test -p irtt-cli --all-features --locked
cargo fmt --all -- --check
cargo check --workspace --all-targets --all-features --locked
cargo test --workspace --all-targets --all-features --locked
cargo clippy --workspace --all-targets --all-features --locked -- -D warnings
RUSTDOCFLAGS="-D warnings" cargo doc --workspace --all-features --no-deps --locked
cargo check -p irtt-cli --no-default-features --features client|tui|server --all-targets --locked
git diff --check

The client and tui feature-only builds emit the pre-existing cmd::format dead-code warnings (unused format_percent_ratio, format_count, format_optional_count, format_optional_ns_f64, format_signed_duration). They are unrelated to this change, are not errors under any required command, and were left alone.

## Summary - `SampleMode::Exact` now retains exact samples for send call, timer error, and server processing, which previously always built their `TimeMetric` with retention disabled. - All eleven public timing metrics now follow the same Exact/RunningOnly retention contract, derived from one `retain_exact` decision in `CoreStats::new`. - No new capability API was needed. `TimeStats::median_ns: Option<f64>` already distinguishes a retained median from an absent one; the missing behavior was retention, not representation. ## Behavior - Finite summaries gain real medians for the `send call`, `timer error`, and `server processing` rows when samples exist, instead of `-`. - Continuous mode remains bounded and medians remain unavailable for every timing metric; the existing continuous-mode note stays correct. - Absent optional server-processing measurements remain absent — a reply without a processing measurement adds no sample, and no zero/default value is synthesized. - Event semantics are unchanged: send call and timer error still come only from `ClientEvent::EchoSent`, and server processing only from replies that carry one. - `LateReplyMode` is untouched. A `CountOnly` matched late reply still contributes no timing measurements, so it does not enter server-processing exact retention. ## Retention estimate - The estimator stays in `irtt-stats`. `EXACT_SAMPLE_VECS_PER_REPLY = 8` becomes `EXACT_SAMPLES_PER_PROBE = 11`, since exact storage now includes send-side metrics associated with probes rather than replies. - Finite estimate on this target rises from 668 to 764 bytes per probe, reflecting real additional retention. The running-only ceiling is unchanged at ~1.6 MiB. - The estimator keeps PR #97's philosophy: approximate, conservative, `size_of`-grounded, deterministic, saturating, optional measurements assumed present for an upper bound, running-only bounded, time-based rolling excluded as documented. - `exact_mode_retains_one_sample_per_counted_metric` counts the public timing metrics a fully measured probe actually retains and asserts it equals `EXACT_SAMPLES_PER_PROBE`, so changing retention without revisiting the estimate fails in `irtt-stats`. - The CLI still calls `StatsConfig::estimated_retained_bytes(...)` and consumed the new figure automatically — no CLI test needed a hardcoded adjustment, and no frontend memory heuristic was added. ## Scope No changes to: - the median algorithm or sample variance; - the rolling API or rolling retention semantics; - `LateReplyMode`; - IPDV tracking or continuous memory bounds; - `irtt-client`, `irtt-server`, or `irtt-proto` behavior; - CLI summary columns, row ordering, units, formatting vocabulary, or verbosity rules; - TUI layout. The TUI timing panel does not render a median column, so it is unaffected. PR #93's timing-panel wrapping observation is deliberately not addressed here. ## Tests - `irtt-stats`: exact medians for send call (odd count), timer error (even count), and server processing; absent server processing retains no samples; running-only still counts those three but reports no median; a regression test asserting every timing metric with samples reports a median in exact mode; a `CountOnly` late-reply assertion that server processing gains no retained median. - `irtt-stats` retention: the estimate covers every retained timing sample, documented as an implementation test rather than a stable public constant. Existing linearity, saturation, running-only bound, rolling, and time-based-exclusion tests are unchanged and still pass. - `irtt-cli`: a finite summary test proving real median cells for the three rows using spread values whose median differs from min, mean, and max, plus a continuous counterpart asserting those cells stay `-`. ## Verification All run on this branch, all passing: - `cargo test -p irtt-stats --all-features --locked` - `cargo test -p irtt-cli --all-features --locked` - `cargo fmt --all -- --check` - `cargo check --workspace --all-targets --all-features --locked` - `cargo test --workspace --all-targets --all-features --locked` - `cargo clippy --workspace --all-targets --all-features --locked -- -D warnings` - `RUSTDOCFLAGS="-D warnings" cargo doc --workspace --all-features --no-deps --locked` - `cargo check -p irtt-cli --no-default-features --features client|tui|server --all-targets --locked` - `git diff --check` The `client` and `tui` feature-only builds emit the pre-existing `cmd::format` dead-code warnings (unused `format_percent_ratio`, `format_count`, `format_optional_count`, `format_optional_ns_f64`, `format_signed_duration`). They are unrelated to this change, are not errors under any required command, and were left alone.

👍 1

chatgpt-codex-connector[bot] (Migrated from github.com) reviewed

2026-08-15 03:46:08 +00:00

chatgpt-codex-connector[bot] (Migrated from github.com) left a comment

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: db9bd515bb

ℹ️ 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".

### 💡 Codex Review Here are some automated review suggestions for this pull request. **Reviewed commit:** `db9bd515bb` <details> <summary>ℹ️ About Codex in GitHub</summary> [Your team has set up Codex to review pull requests in this repo](https://chatgpt.com/codex/cloud/settings/general). 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". </details>

crates/irtt-stats/src/lib.rs

chatgpt-codex-connector[bot] (Migrated from github.com) commented

2026-08-15 03:46:08 +00:00

Qualify the exact-mode promise for rolling snapshots

When a caller enables rolling_count or rolling_time on an exact configuration, rolling_count() and rolling_time() still recompute their snapshots with CoreStats::new(SampleMode::RunningOnly, ...) in rolling.rs:62, so populated timing metrics return median_ns: None. The new promise that Exact makes every timing metric report a median is therefore false for the crate's public rolling snapshots; either preserve the configured sample mode during rolling recomputation or explicitly limit this contract to the cumulative snapshot.

AGENTS.md reference: AGENTS.md:L53-L53

Useful? React with 👍 / 👎.

**![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat) Qualify the exact-mode promise for rolling snapshots** When a caller enables `rolling_count` or `rolling_time` on an exact configuration, `rolling_count()` and `rolling_time()` still recompute their snapshots with `CoreStats::new(SampleMode::RunningOnly, ...)` in `rolling.rs:62`, so populated timing metrics return `median_ns: None`. The new promise that `Exact` makes every timing metric report a median is therefore false for the crate's public rolling snapshots; either preserve the configured sample mode during rolling recomputation or explicitly limit this contract to the cumulative snapshot. AGENTS.md reference: [AGENTS.md:L53-L53](https://github.com/Lochnair/irtt-rs/blob/db9bd515bbbf2aae9da274abf85359f3fa2d54cf/AGENTS.md#L53-L53) Useful? React with 👍 / 👎.

Lochnair (Migrated from github.com) reviewed

2026-08-15 03:48:55 +00:00

crates/irtt-stats/src/lib.rs

Lochnair (Migrated from github.com) commented

2026-08-15 03:48:55 +00:00

Correct — rolling snapshots recompute with RunningOnly in rolling.rs:62 regardless of config, so the new wording over-promised. Rolling semantics are out of scope for this PR, so I scoped the contract instead: SampleMode/finite()/samples now say the exact-median promise applies to the cumulative snapshot, rolling_count()/rolling_time() state plainly that they use running statistics only and report no median, and rolling_snapshots_report_no_medians_even_in_exact_mode pins both halves so the wording cannot go stale (71b5819).

🤖 Addressed by Claude Code

Correct — rolling snapshots recompute with `RunningOnly` in `rolling.rs:62` regardless of config, so the new wording over-promised. Rolling semantics are out of scope for this PR, so I scoped the contract instead: `SampleMode`/`finite()`/`samples` now say the exact-median promise applies to the cumulative snapshot, `rolling_count()`/`rolling_time()` state plainly that they use running statistics only and report no median, and `rolling_snapshots_report_no_medians_even_in_exact_mode` pins both halves so the wording cannot go stale (71b5819). _🤖 Addressed by [Claude Code](https://claude.com/claude-code)_

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: Lochnair-Labs/irtt-rs#98