in reviewhumanPR #95Other
metrics: fix delivery_rate/success_rate exceeding 1.0 on broadcast-heavy scenarios
Fixes delivery_rate (and its success_rate alias) returning a value greater than 1.0 on any scenario that uses broadcasts. A delivery "rate" above 1.0 is impossible and misreports every broadcast-heavy run.
Author
@SwasthikaDev
github profile →- Status
- In review
- Opened on
- Jul 8
- Branch
- hackathon/swasthika-delivery-rate-broadcast
Description
The pitch.
## Summary
Fixes `delivery_rate` (and its `success_rate` alias) returning a value **greater than 1.0** on any scenario that uses broadcasts. A delivery "rate" above 1.0 is impossible and misreports every broadcast-heavy run.
## Where — exact location
- **File:** `packages/nest-core/nest_core/metrics.py`
- **Function:** `_delivery_rate` at **line 57**, surfaced as the `delivery_rate` metric and via the alias `_success_rate = _delivery_rate` at **line 92** (the `success_rate` metric name).
## Root cause
The simulator logs a one-to-one message as a `send` record (sender) plus a `receive` record (recipient). A **broadcast** is logged differently in `packages/nest-core/nest_core/sim/simulator.py`: one `broadcast` record on the sender and one `receive` record per recipient — **no `send` records at all**.
The old `_delivery_rate` used `send` events as the denominator and `receive` events as the numerator:
```python
# BEFORE (metrics.py:57)
sends = 0
receives = 0
for ev in events:
kind = ev.get("kind", "")
if kind == "send":
sends += 1
elif kind == "receive":
receives += 1
if sends == 0:
return 0.0
return receives / sends
```
Every broadcast adds N to `receives` and 0 to `sends`, so the ratio climbs above 1.0.
## Reproduction — a shipped scenario
`scenarios_builtin/reputation.py`'s observer calls `ctx.broadcast(...)`:
```
$ uv run nest run reputation
# trace event kinds: {'send': 280, 'receive': 340, 'broadcast': 3, ...}
# delivery_rate = 340 / 280 = 1.2143 <-- impossible
```
## The fix
Count **delivery attempts** for the denominator. Every attempt the simulator makes ends as exactly one record — a `receive` (delivered) or a `dropped` (lost) — whether it came from a `send` or a `broadcast`:
```python
# AFTER (metrics.py:57)
receives = 0
dropped = 0
for ev in events:
kind = ev.get("kind", "")
if kind == "receive":
receives += 1
elif kind == "dropped":
dropped += 1
attempts = receives + dropped
if attempts == 0:
return 0.0
return receives / attempts
```
**Why this is safe for existing traces:** for unicast, every `send` yields exactly one `receive` or one `dropped`, so `sends == receives + dropped` and the value is *identical*. It only changes the broadcast case, and it can never exceed 1.0. After the fix, `reputation` reports `1.0`.
## Tests
- **Unchanged, still pass:** `test_delivery_rate` (`test_metrics.py:35`) and `test_success_rate_backward_compat` (`test_metrics.py:41`) — a unicast trace with 3 sends / 2 receives / 1 dropped, still `2/3`.
- **Added:** `test_delivery_rate_with_broadcast_stays_within_one` at **`packages/nest-core/tests/test_metrics.py:48`** — a trace with two broadcasts (one fully delivered, one with a dropped fan-out). Asserts the rate is `<= 1.0` and equals `3/4` (3 delivered of 4 attempts). This test **fails** on the old `receives / sends` formula.
## Files changed (2 files, +19 / -7 and +26)
| File | +/- | What |
|---|---|---|
| `packages/nest-core/nest_core/metrics.py` | +19 / -7 | Rewrote `_delivery_rate` denominator (line 57) |
| `packages/nest-core/tests/test_metrics.py` | +26 | Broadcast regression test (line 48) |
## Verification — full CI, branch merged with current `main`
`uv run ruff check .` clean · `uv run ruff format --check .` clean (172 files) · `uv run pyright` strict **0 errors** · `uv run pytest` **792 passed**, 1 skipped. Scoped to the one metric plus its test; no other metric or behavior is touched.
Try it
Open PR on GitHubView diffCheckout locally
git fetch origin pull/95/head:pr-95
git checkout pr-95