stats by daniel-noland · Pull Request #1774 · githedgehog/dataplane

daniel-noland · 2026-08-28T02:52:17Z

No description provided.

coderabbitai · 2026-08-28T02:52:24Z

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

_{Comment @coderabbitai help to get the list of available commands.}

codecov · 2026-08-28T02:58:32Z

Codecov Report

❌ Patch coverage is 91.79954% with 180 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
dataplane/src/packet_processor/fuzz.rs	93.50%	34 Missing and 12 partials ⚠️
stats/src/scrape.rs	56.86%	44 Missing ⚠️
stats/src/dpstats.rs	95.49%	26 Missing and 11 partials ⚠️
stats/src/rate.rs	94.79%	13 Missing and 9 partials ⚠️
interface-manager/src/interface/mod.rs	0.00%	13 Missing ⚠️
stats/src/vpc_stats.rs	93.40%	3 Missing and 3 partials ⚠️
nat/src/masquerade/fuzz.rs	37.50%	4 Missing and 1 partial ⚠️
mgmt/src/processor/proc.rs	0.00%	4 Missing ⚠️
nat/src/portfw/fuzz.rs	75.00%	2 Missing ⚠️
dataplane/src/drivers/kernel/worker.rs	0.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Smoothing clamps at zero. The 5-point kernel's edge coefficients are negative, so a sparse window fits a curve that dips below zero -- a sound trend estimate and a nonsensical count of bytes. It is the second of those that reaches an operator, which is where it was seen: a small negative rate in the CLI under low load. A step that rounds to zero microseconds is refused rather than divided by. It produced `Ok(NaN)`, a success value that is not a number and that every caller then propagates. One property already carried an `is_nan` guard stepping around it; that guard was this defect, and goes with it. `smooth` had no claim on its value anywhere in the tree, only that it returned `Ok`, which is how the negative rate reached a terminal before it reached a test. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The property asserted that a spec matching an interface is identical to that interface's requirement form. Comparing the two is deliberately lenient -- `mac` and `mtu` are documented as "None means the operating system picks" -- so it failed on precisely the case leniency exists for, an unset mtu against an observed one. Stated field by field rather than deferring to the comparison under test, since that comparison is what this is meant to pin down. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

A ring buffer read physically is only in time order until it wraps, and every Savitzky-Golay coefficient is position-dependent. Under a load that never varied the exported rate went wrong on four ticks in five, twice reporting zero. Absent destinations now read as zero rather than as their previous sample held forever, which is what a window of per-interval counts means; see `Dpstats::TIME_TICK`. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…igures The cli is only ever run when something is already wrong, so a lock-up there costs two things: the box, and the state that would have explained the original fault. `apalloc`'s Display already carries a hand-written defence against one such deadlock; nothing was watching for the next. The property discriminates a provider that panics from one that locks up. The doc comment records which hazard it still does not reach, and why. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Until a test could drive the clock, the only claim anything made about a rate was that computing one returned `Ok`. That is how a scrambled window and a negative smoothed count both reached production. The steady-load case is deliberately not the whole story: five identical samples read the same in any order, so it cannot see an ordering defect. The ramp can, and does -- both new properties fail against the `rate.rs` that read its window out of ring order. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…ight Twelve properties had a real oracle and built every window at ring position zero; twelve fuzzed ones wrapped the ring and only ever asserted a sign. The ring-order defect sat in that gap and shipped. `carried` closes it for the existing twelve -- all of them fail against the code that had it. Where a two-point difference and a five-point stencil are both exact they must agree exactly, so the disagreement is a proof rather than a hint. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

They were ten copies of one window: the `scan` closure read its state and never assigned to it. Ten windows ending at the same instant leave an instant no update can land in, and the counts arriving then were discarded silently -- one tick per vpc pair, missing from the cumulative counters for the life of the process. Two behaviour changes beyond the bug, both toward not losing counts. Traffic that lines up with no open batch now goes in the earliest open one instead of being dropped, which reverses what `apportion_no_overlap_records_nothing` asserted; that discard was a divide-by-zero guard, not a decision. And the duplicated apportionment in `update` is gone -- it now calls the helper the drop path already used, so there is one copy to keep right. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The fixed-load properties pin the arithmetic; this one goes looking. Drawing an update's window rather than always using the tick that just ended is the point: counts timed at an instant nothing is open for is the shape that lost a tick at startup, and it is what a stalled collector produces in the field. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Every generated-filter property drew from the one generator that existed, which builds a running total because that is what a finite difference needs. `Smooth` is fed per-interval counts instead. Measured over 2,595,038 full windows, the running-total generator produced no window that decreased, none containing a zero, and none that would smooth negative -- so the two properties guarding the negative clamp passed with the clamp deleted. They now count the windows that needed the clamp and refuse to pass without having built one, because a property about a rare state that does not check it reached the state has no way to tell you when it stops. `Derivative` and the EWMA have no caller outside this file; noted on the trait so its test count is not mistaken for coverage of the shipped path. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

A VPC's exported series are named after the VPC; its identity is its discriminant. Nothing reconciled the two, so a rename orphaned every series under the old name -- no discriminant left the map, so the removal path never fired -- while the next update started a live set under the new one. Both then carried the rate. The removal path was itself computing the wrong set: it zeroed the series pairing a departed VPC with a *survivor*, so deleting both ends of a peering cleared nothing. Nothing in the crate installed a `metrics::Recorder`, so every gauge write in the suite went to the no-op and none of this was observable from a test. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…fabric Registering a series is configuration work; it was being redone on every update, at 8N + 8N^2 gauges for N VPCs. Publishing a rate for a peering that has never carried a packet is not work at all; it was creating the store entry that made the prune, the snapshot and the CLI's pair listing quadratic too. Both matter because the collector reads a bounded channel and the losing arm is silent: `try_send` returns `Ok(false)`, a batch of deltas is dropped, and the only trace is a `warn!`. The cost was worst exactly when the numbers are being watched. Measured at 64 VPCs carrying one live pair, before: 33280 registrations per update, 4096 pairs held. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

What travels this channel is a delta, so the old `Ok(false) => warn!()` arm did not skip a reading -- it subtracted those packets from the cumulative counters permanently, with no metric saying so and only a log line to find it by. `try_send_option` leaves the batch in hand, and the batch is keyed by VPC pair, so holding it costs no more memory the longer it is held. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

A stage whose work is timed can only notice the time when `process` is called, and the reader skipped the pipeline entirely on an empty read and on its watchdog tick. An interface that stopped carrying traffic therefore held its last stats batch until traffic resumed -- for ever, if it did not -- and then delivered it stamped with the whole idle gap, which the collector spreads over every window it spans. The watchdog tick already runs at 2s, so this costs one empty `process` per interface per tick and no new wake-up. The stats half of the contract has a test; the reader's select loop has none, and this was reviewed by reading it. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Recycling a discriminant is expected and the counter reset that goes with it is the point, but it only happened when the discriminant was absent from the map for a moment -- that is what `prune_to_vpcs` keys on. Swap tenants in one configuration change, which is the ordinary way, and only the name changes: the incoming tenant's first scrape reported every packet the outgoing one ever sent, under the incoming tenant's name. Registration also had two sites that disagreed about the base label set, so a series could carry `from` twice, which Prometheus cannot represent. Which set was live depended on which site had run more recently; making registration a configuration-time job is what made the constructor's version the one that showed. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Resetting the store leaves a dozen seconds of the outgoing tenant's traffic elsewhere in the pipeline -- outstanding batches, the smoothing window, and the stats stage's own batch -- all keyed by discriminant, which is the one thing a handover does not change. It arrives after the swap and is credited to whoever holds the VNI then. Both directions have to be forgotten. A recycled VNI is as often the far end of somebody else's traffic as the near end, and that is filed under the other VPC's source entry. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Three things were quadratic in the VPC count and none of them had to be: the prune ran on every update over every pair the store held, the rate loop visited every peering of every VPC, and the store held a pair for each because visiting one creates its entry. Measured at 64 VPCs carrying a single live pair -- one concluded batch, which is once a second: 159ms before, 1.6ms after, and one pair held rather than 4096. Also reorders the store writes. A reader takes the names and the counters as separate snapshots -- `handle_get_dataplane_status` does -- so a read landing between them pairs one with stale data from the other. Counters are dropped before the new names are published, which makes the worst such read an old name against no counters rather than a new tenant's name against the old tenant's traffic. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

A pair series belongs to both ends and rightly stops when either leaves. A VPC's total is its own: it sent those packets whatever became of where they went. Both were gated on the destination being alive, so deleting a VPC reached backwards and subtracted the whole in-flight window from every surviving peer's cumulative counter, permanently. Also separates "the map is empty" from "the map could not be read", which `snapshot_vpc_pairs` collapsed into the same empty `Vec`. Reading a failure as a deletion of everything prunes the store, zeroes every series and drops what is in flight, none of it recoverable. Only reachable at shutdown today, since the writer lives for the process -- a guard against the class, not a live defect. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

They live behind three locks and were read one at a time, so a name came from one instant and its counters from another. During a handover that pairs a tenant's name with a different tenant's traffic, and no ordering fixes it: whichever of the two reads happens first is the one that can be stale, and the writer's order only decides which of the two mispairings a reader gets. Observed directly against a race that names each tenant after what it sends -- tenant 7's name exported carrying tenant 8's traffic. `snapshot` holds all three read guards. Safe because every site in the file acquires them in the same order, and `vpc_names` is the synchronous one so it is taken last and nothing awaits while it is held. `hand_over` replaces `forget_vpcs` plus a separately-ordered name write, so the constraint that made the handover safe now lives in one function rather than in a comment on two call sites. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`advance` returns as soon as time has moved; the timer tasks it wakes still have to be polled, and each takes more than one poll to retire because dropping the last `Arc` to a flow table cascades into dropping every flow in it. The four yields this had were not enough, and nothing checked. Measured on `distinct_published_tuples_reach_distinct_targets`, memory per case: 620KB with no yield, 76KB with four, nothing measurable once it waits for `num_alive_tasks() == 0`. That is out of memory by case 2312 versus 47MB flat at case 5104, with no loss of throughput -- so the target went from dying four minutes into a run to being able to finish one. Waiting is bounded and a timeout panics rather than passing quietly: a case whose tasks will not retire is the defect this exists to prevent. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The timer tasks are released only when the runtime is dropped, and a runtime hoisted out of `for_each` is dropped once, at the end of the campaign. Measured on `stress_test_concurrency_model`: 175KB a case, out of memory by roughly case 12,700, and 3.7x slower for the queue it was dragging. The third property in this stack to need the same repair; the two before it were in the packet processor and in nat. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Giving `FlowKey` one address pair instead of two changed `FlowKey::new` and updated this file, but not the `#[cfg(feature = "shuttle")]` module inside it, so `--features shuttle` has not compiled since 2026-08-21. Nothing local builds that cfg: the `concurrency` CI job that would have caught it is `ci-gate`d to push and merge queue, so a branch that never enters the queue gets no signal. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`flow_filter::context::tables::table_name` carries the account; this counter had the same fault and never got the same treatment. Surfaced as `An ACL context named 'acl_v4_209' already exists` from `packet_processor::fuzz::model`. The unused-import warning that would have pointed here does not appear until the std arm is the only one holding those imports, which is why the sibling's fix did not draw attention to this one. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Seven of the eleven took one hand-written input each, so they explored schedules over a single shape. They now draw flow counts, host and port choices, the public address count, and -- where the participants do not have to agree on it -- how many times the thing under test happens while traffic rides over it. Both backends gain: the `::plain` leaves are what `just test sanitize=thread` runs. `APPLIES` is left pinned in the two barrier tests. The tenant, the reader and the applier rendezvous on one `Barrier::new(3)` per round, so the round count is a contract between them rather than a parameter of any one; drawing it parks every thread at 0.1% CPU with no panic to name. `without_unwinding` does not help, being `Ok(body())` under shuttle by design. Only one of the seven is known to discriminate a real defect: `two_workers_are_not_given_the_same_public_tuple` still fails on sight against the load-then-store lost update in `PortBlockList::pick_available_block`. The other six carry no such evidence yet. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

A fresh flow's genid is zero, so a drawn genid of zero made "the partner does not hold this value" true of an untouched partner, and the property fired on correct behaviour. It was the empty input that found it -- the first sweep of this crate, which the 34-target sweeps had never covered. Holding the partner's prior reading is the claim that was meant and has teeth for every draw rather than all but one. 11.4M runs clean. Signed-off-by: Daniel Noland <daniel@githedgehog.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

`check/debug` denies these, so the whole PR fails to build: - two doc comments outlived the `const CHURN`/`const PER_ROUND` they described, which became values the generator draws; - a `match` that binds one pattern and returns on the other is a `let ... else`; - `const CASES` sat below the runtime `let`s in three properties, which reads as though it came into scope there rather than at the top of the body. Signed-off-by: Daniel Noland <daniel@githedgehog.com>

… rules `stats` is production code with a real `concurrency` dependency, so its primitives go through the facade. `acl-filter`'s sequence counter is the same deliberate case as `flow-filter`'s -- an instrumented counter is not process-unique -- and takes the same suppression, which its `static` already carried and its `fetch_add` did not. Signed-off-by: Daniel Noland <daniel@githedgehog.com>

daniel-noland added the ci:+merge-ready Run all checks which will be run in the merge queue regardless of label status label Aug 28, 2026

daniel-noland force-pushed the pr/daniel-noland/driven-clock branch from fa0e8f2 to b164a0f Compare August 28, 2026 03:05

daniel-noland force-pushed the pr/daniel-noland/stats-collector branch 2 times, most recently from 80e27f5 to c14f713 Compare August 28, 2026 03:34

daniel-noland force-pushed the pr/daniel-noland/driven-clock branch from b164a0f to 3cd5687 Compare August 28, 2026 03:34

daniel-noland changed the title ~~fix(stats): the collector's rate, its windows, and what happens when a VNI changes hands~~ stats Aug 28, 2026

daniel-noland force-pushed the pr/daniel-noland/stats-collector branch from c14f713 to 42a24de Compare August 28, 2026 04:22

daniel-noland force-pushed the pr/daniel-noland/driven-clock branch 2 times, most recently from 1ec0be0 to de8e45f Compare August 28, 2026 05:11

daniel-noland force-pushed the pr/daniel-noland/stats-collector branch from 42a24de to 39a7f24 Compare August 28, 2026 05:11

daniel-noland force-pushed the pr/daniel-noland/driven-clock branch from de8e45f to bea6549 Compare August 28, 2026 05:32

daniel-noland force-pushed the pr/daniel-noland/stats-collector branch from 39a7f24 to 05f602f Compare August 28, 2026 05:32

daniel-noland force-pushed the pr/daniel-noland/driven-clock branch from bea6549 to f850a96 Compare August 28, 2026 05:48

daniel-noland force-pushed the pr/daniel-noland/stats-collector branch from 05f602f to 7823471 Compare August 28, 2026 05:48

daniel-noland force-pushed the pr/daniel-noland/driven-clock branch from f850a96 to 9398527 Compare August 28, 2026 06:21

daniel-noland force-pushed the pr/daniel-noland/stats-collector branch from 7823471 to b6054e3 Compare August 28, 2026 06:23

daniel-noland force-pushed the pr/daniel-noland/driven-clock branch from 9398527 to d497a39 Compare August 28, 2026 06:40

daniel-noland force-pushed the pr/daniel-noland/stats-collector branch from b6054e3 to 4d2a746 Compare August 28, 2026 06:40

daniel-noland force-pushed the pr/daniel-noland/driven-clock branch from d497a39 to 3e43aea Compare August 28, 2026 07:08

daniel-noland force-pushed the pr/daniel-noland/stats-collector branch from 4d2a746 to a68c341 Compare August 28, 2026 07:08

daniel-noland force-pushed the pr/daniel-noland/driven-clock branch from 3e43aea to 610dcf1 Compare August 28, 2026 07:31

daniel-noland force-pushed the pr/daniel-noland/stats-collector branch from a68c341 to 2064af0 Compare August 28, 2026 07:31

daniel-noland force-pushed the pr/daniel-noland/driven-clock branch from 610dcf1 to 698f663 Compare August 28, 2026 07:43

daniel-noland force-pushed the pr/daniel-noland/stats-collector branch from 2064af0 to c620950 Compare August 28, 2026 07:44

daniel-noland force-pushed the pr/daniel-noland/driven-clock branch from 698f663 to 26b967b Compare August 28, 2026 09:14

daniel-noland force-pushed the pr/daniel-noland/stats-collector branch from c620950 to 21dc552 Compare August 28, 2026 09:14

daniel-noland and others added 2 commits August 28, 2026 11:14

daniel-noland and others added 24 commits August 28, 2026 11:14

daniel-noland force-pushed the pr/daniel-noland/driven-clock branch from 26b967b to 8968292 Compare August 28, 2026 17:16

daniel-noland force-pushed the pr/daniel-noland/stats-collector branch from 21dc552 to 7d29a55 Compare August 28, 2026 17:16

Navigation Menu

Uh oh!

stats - #1774

stats#1774
daniel-noland wants to merge 26 commits into
pr/daniel-noland/driven-clockfrom
pr/daniel-noland/stats-collector

daniel-noland commented Aug 28, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Aug 28, 2026

Draft PR not reviewed

Uh oh!

codecov Bot commented Aug 28, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

daniel-noland commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Aug 28, 2026

Draft PR not reviewed

Uh oh!

codecov Bot commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

daniel-noland commented Aug 28, 2026 •

edited

Loading

codecov Bot commented Aug 28, 2026 •

edited

Loading