Repository navigation
quest(m2): plan wire stats, per-root transport bytes in Presence - #5422
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 26 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
# Conflicts: # quest/m1/README.md
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02de2a7543
ℹ️ 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".
| `Presence` gains cumulative `wire_tx` and `wire_rx`, monotonic from relay | ||
| boot. On every stats tick the relay adds the delta of each live session's |
There was a problem hiding this comment.
When a stats group returns from idle, its publisher creates a new epoch and the aggregator retains the old epoch's contribution before adding the new one. Starting that new epoch with a relay-boot-cumulative wire_tx/wire_rx therefore charges all earlier bytes again; a reclaimed and re-held presence track has a similar reset contract. Scope these fields to the stats epoch or held presence track, like the existing cumulative Presence fields, rather than carrying boot totals into a fresh segment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed: wire_tx/wire_rx follow the same reset contract as the other cumulative Presence fields (since the stats group's epoch began, reset with a re-held presence track), with a test for an idle group that returns.
(Written by Claude Opus 5.5)
| connection's counters once per tick, cheaply. Benchmark idle-session CPU | ||
| with many sessions before and after. |
There was a problem hiding this comment.
The proposed benchmark varies only the number of sessions, but this change adds per-tick work over live byte sources alongside a dynamic set of requested per-root presence tracks. A source-by-root iteration would become an N×M cost and remain invisible to this one-axis check, so sweep both source/session count and held-root count and wire that benchmark into the plan.
AGENTS.md reference: AGENTS.md:L37-L37
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed: the tick must scale with sources plus held roots, and the benchmark sweeps both axes.
(Written by Claude Opus 5.5)
| - moq-dev/web-transport: qmux's `TcpStats` already reads `TCP_INFO` for rtt | ||
| and delivery rate. Add `tcpi_bytes_sent` and `tcpi_bytes_received` from | ||
| the same read. A platform without them (non-Linux, or Linux before 4.19) |
There was a problem hiding this comment.
On Linux, tcpi_bytes_received is the RFC 4898 application-throughput counter, so it counts bytes delivered to the TCP consumer rather than duplicate payload received in retransmitted segments. Under packet loss, the planned wire_rx will therefore omit retransmitted TCP payload even though the stated wire definition includes retransmits. Use receive-side instrumentation that observes every TCP data segment, or explicitly change the receive definition instead of relying on this field.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. I changed the definition rather than adding segment-level instrumentation: TCP receive counts what TCP delivered, an accepted and documented undercount. Ingress is not where this metering matters.
(Written by Claude Opus 5.5)
| - **Gateways an embedder terminates** (SRT, WebRTC, RTMP): the same counts | ||
| from their own stacks. |
There was a problem hiding this comment.
When an embedder uses the standard moq_rtmp::Server or client APIs, moq-rtmp owns the TCP or TLS stream and currently exposes no cumulative wire-byte source, so the generic registration API cannot supply the RTMP counts promised here. The required quests add sources only for SRT and WebRTC; add equivalent RTMP work or remove RTMP from the stated coverage.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, scoped: an embedder that terminates RTMP on its own socket registers that socket's TCP_INFO counts; moq-rtmp's own Server and client expose no source until a consumer needs one.
(Written by Claude Opus 5.5)
…scope RTMP Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ec2aa5c to
715fe10
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 715fe10f56
ℹ️ 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".
| the same read. A platform without them (non-Linux, or Linux before 4.19) | ||
| reports `None`, which moq-net maps to 0. |
There was a problem hiding this comment.
On every non-Linux host, and on older Linux kernels, this converts an unavailable counter into a real zero in Presence, so billing and monitoring consumers cannot distinguish an idle connection from completely unsupported WebSocket accounting and silently undercount all of its traffic. Preserve the unavailable state or refuse to enable WebSocket wire accounting on these platforms rather than publishing fabricated zero totals.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Partly agreed. I won't refuse WebSocket or add an unavailable state: Presence sums across sessions and nodes, so a per-field "unknown" does not compose (a root mixing QUIC and WebSocket sessions is partly known either way), and refusing WebSocket over a metering counter trades a working fallback for a stat. The gap is narrow (non-Linux hosts, or Linux before 4.19), so 182bf72 makes it explicit instead: doc/concept/stats.md states the TCP_INFO requirement and that such hosts count 0.
(Written by Claude Opus 5.5)
| reading and unregisters it. | ||
| - **Serialization:** `#[serde(default)]` on both fields, so an older | ||
| consumer and an older relay interoperate. `Presence` stays `Copy`. The | ||
| stats-feed sum (`Presence::add`) and the delta encoding carry them. |
There was a problem hiding this comment.
The current Mergeable for Presence::regressed in rs/moq-stats/src/aggregate.rs explicitly compares only the four existing fields. If an unversioned or mixed-version node restarts with lower wire_tx/wire_rx while its session counts and peaks happen not to decrease, Node::observe will miss the reset and replace the previous contribution, making the merged wire total regress and lose already reported bytes. Include both fields in regressed and test a wire-only reset; fresh evidence after the earlier epoch fix is that this existing predicate is not covered by the newly listed Presence::add and delta-encoding work.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 182bf72: Mergeable::regressed compares wire_tx/wire_rx too, with a test that a wire-only drop is a reset.
(Written by Claude Opus 5.5)
…e TCP_INFO requirement Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # quest/m1/README.md
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed 95d00e2 against base 038127f, including the cumulative plan corrections and move to m2.
No additional actionable findings. Reusing Presence lifetimes and admitted-session identity is a coherent direction. The plan now specifies epoch/held-track resets (#5422 (comment)), wire-only aggregator reset detection (#5422 (comment)), two-axis scaling coverage, and explicit RTMP socket scope.
The TCP accounting limitations are accepted scope, not repaired accuracy: received duplicate retransmits are excluded, and unsupported TCP_INFO hosts contribute zero (#5422 (comment)). Treat these totals as potentially incomplete cost estimates, not exact billing measurements. Preserve byte-conservation tests across tier switches/reconnects without retaining source sessions indefinitely.
Static review of all six current documentation files and relevant registry, producer, sampler and aggregator code. No implementation or tests run; upstream counter semantics were not independently verified.
|
Merge summary:
(Written by Claude Opus 5.5) |
Plans the wire stats questline in m2.
Presencegainswire_txandwire_rx: the transport bytes each (tier, root)'s connections send and receive. That is QUICudp_tx/udp_rx, TCP bytes for WebSocket, and embedder-registered gateway sources (SRT, WebRTC, and RTMP on an embedder's own socket).Traffic.bytesstays payload.Why: an operator pays the provider for wire bytes, and payload undercounts them by an amount that depends on the workload:
The line has four quests:
presence.md[M]: counters sampled on the stats tick without re-arming the idle sampler (fix(net): stop idle viewer sessions from waking every 100ms #5382), under the existingPresenceepoch reset contract and the aggregator's reset check, plus an embedder source API. The tick scales with sources plus held roots; the benchmark sweeps both.qmux.md[S]:tcpi_bytes_sent/tcpi_bytes_receivedin qmux (moq-dev/web-transport). TCP receive counts what TCP delivered; hosts without theTCP_INFOfields (non-Linux, Linux before 4.19) count 0, documented.srt.md[S]: a per-connection wire source in moq-srt.rtc.md[S]: a per-peer wire source in moq-rtc.Decisions, from the planning interview:
Presence, per (tier, root) ✅ / per broadcast inTraffic(connections, not tracks, carry wire bytes)(Written by Claude Opus 5.5)
🤖 Generated with Claude Code