moq-net: track announcement byte usage in stats#1953
Conversation
Add an `announced_bytes` counter alongside the existing `announced` / `announced_closed` event counts, so the per-node stats broadcast reports how many wire bytes a broadcast spends on announce/unannounce control messages, not just how many times it announced. Kept separate from the `bytes` payload counter and surfaced per (broadcast, tier, role) like the other counters. Measurement is taken at the encode/decode points: - `Writer::encode`/`encode_message` now return the bytes written. - `Reader` gains `decode_sized`/`decode_maybe_sized` returning the bytes consumed. Both are internal (the `coding` module is private) and non-breaking for existing `.await?;` call sites. Wired on both protocols and directions: lite ANNOUNCE Active/Ended (send + receive) and IETF PUBLISH_NAMESPACE / PUBLISH / NAMESPACE. `Counters` is `#[non_exhaustive]` and the new field is additive, so this is a non-breaking change targeting main. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @kixelated, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
Switch announced_bytes to sum the broadcast name length per announce / unannounce instead of the full encoded message size. This avoids charging a broadcast for transport overhead it doesn't control (size prefixes, message-type ids, hop chains). Because the name is available right at each announce site, this reverts the Writer::encode / encode_message and Reader byte-count plumbing back to their original signatures, and drops the announce_bytes parameter threading through the IETF subscriber (start_announce computes it from the absolute path). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughA new cumulative counter, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rs/moq-net/src/ietf/subscriber.rs`:
- Line 172: `SubscriberStats::bytes()` is being undercounted because
`start_announce`/`start_publish` currently receive only the payload size from
`handle_stream` and the decoder path, not the full wire message length. Update
the message dispatch flow in `subscriber.rs` so the total consumed encoded
length (including type-id and length prefix) is threaded through and recorded by
`start_announce` and `start_publish` for `Namespace`, `PublishNamespace`, and
`Publish` instead of using `size as u64` or `data.len() as u64`.
In `@rs/moq-net/src/lite/publisher.rs`:
- Around line 318-321: The unannounce byte count is only being recorded when a
live guard exists in publisher::publish/End handling, so paths that skipped
Active still spend bytes without updating metrics. Update the stats flow around
the `stream.writer.encode(&msg)` path and the `stats_guards.remove(...)` lookup
so the bytes are always recorded against a path-keyed stats entry first, then
remove and use the guard only if it exists. Keep the fix centered on the
`stats_guards` map and the `guard.bytes(...)` accounting so `announced_bytes`
reflects all lite control traffic.
In `@rs/moq-net/src/lite/subscriber.rs`:
- Around line 188-197: The announce byte accounting in `subscriber.rs` is only
happening after `start_announce` succeeds or when a guard exists, so dropped
`Active` announces and unmatched `Ended` messages are missing from
`announced_bytes`. Update the
`stream.reader.decode_maybe_sized::<lite::Announce>()` handling to credit `n`
immediately for each decoded announce using a byte-only stats path keyed by
`abs`, independent of `start_announce` and `stats_guards`, and keep the existing
guard-based tracking only for accepted broadcast lifetime in
`subscriber`/`stats.broadcast`.
In `@rs/moq-net/src/stats.rs`:
- Around line 151-154: The `announced_bytes` metric in `stats.rs` is currently
undercounting Lite01/02 because `AnnounceInit` traffic is handled in
`lite/publisher.rs` and `lite/subscriber.rs` without going through
`guard.bytes(...)`. Update the Lite publisher/subscriber paths to record the
initial announce control bytes in `announced_bytes`, or if that is not intended,
tighten the `announced_bytes` documentation and usage to explicitly cover only
Lite03+ so the metric’s scope matches the implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e7681a1-786b-4410-aacb-dcbeda91854f
📒 Files selected for processing (8)
doc/bin/relay/config.mdrs/moq-net/src/coding/reader.rsrs/moq-net/src/coding/writer.rsrs/moq-net/src/ietf/publisher.rsrs/moq-net/src/ietf/subscriber.rsrs/moq-net/src/lite/publisher.rsrs/moq-net/src/lite/subscriber.rsrs/moq-net/src/stats.rs
Record the broadcast name length keyed by broadcast path via new
BroadcastStats::{publisher,subscriber}_announced_bytes, instead of through
the lifetime guard. This captures announce/unannounce control messages
whose guard was skipped (hop/loop filtering), reflected, or already
dropped, which the guard-coupled version under-reported.
Also count the Lite01/02 AnnounceInit initial active set on both the
publisher and subscriber sides, so announced_bytes is no longer
protocol-version dependent.
Addresses CodeRabbit review feedback on #1953.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rs/moq-net/src/ietf/publisher.rs (1)
523-561: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCount
PUBLISH_NAMESPACEbytes before waiting for the OK response.Lines 547-561 only bump
announced_bytesafterPublishNamespaceOk/RequestOk, but Lines 527-535 have already writtenPUBLISH_NAMESPACEto the wire. A rejected namespace therefore consumes announce control bytes without ever surfacing in stats, which under-reports the new metric’s documented “independent of the lifetime guard” behavior.Suggested fix
let request_id = self.control.next_request_id().await?; let mut stream = Stream::open(&self.session, self.version).await?; // Count the broadcast name length, not the encoded message size, so // stats don't penalize the broadcast for framing overhead. let announce_bytes = absolute.as_str().len() as u64; + let bs = self.stats.broadcast(&absolute); // Write the PublishNamespace message stream.writer.encode(&ietf::PublishNamespace::ID).await?; stream .writer @@ .encode(&ietf::PublishNamespace { request_id, track_namespace: suffix.as_path(), }) .await?; + bs.publisher_announced_bytes(announce_bytes); // Read response from stream.reader let type_id: u64 = stream.reader.decode().await?; @@ (Version::Draft14, ietf::PublishNamespaceOk::ID) => { let msg = ietf::PublishNamespaceOk::decode_msg(&mut data, self.version)?; tracing::debug!(message = ?msg, "publish namespace ok"); - let bs = self.stats.broadcast(&absolute); - bs.publisher_announced_bytes(announce_bytes); namespace_streams.insert(suffix, (request_id, stream, bs.publisher())); } @@ (_, ietf::RequestOk::ID) => { let msg = ietf::RequestOk::decode_msg(&mut data, self.version)?; tracing::debug!(message = ?msg, "publish namespace ok"); - let bs = self.stats.broadcast(&absolute); - bs.publisher_announced_bytes(announce_bytes); namespace_streams.insert(suffix, (request_id, stream, bs.publisher())); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-net/src/ietf/publisher.rs` around lines 523 - 561, The `publish namespace` flow in `publisher.rs` only updates `publisher_announced_bytes` after `PublishNamespaceOk`/`RequestOk`, so rejected namespaces never count the `PUBLISH_NAMESPACE` control bytes that were already sent. Move the announcement byte accounting in the `publish`/`PublishNamespace` handling path so it is recorded immediately after writing the `ietf::PublishNamespace` request (before waiting on `stream.reader.decode()`), and keep the existing success handling in place for the namespace stream bookkeeping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rs/moq-net/src/ietf/subscriber.rs`:
- Around line 455-466: The subscriber-side announced-bytes tracking in
start_announce only records the announce path and misses the matching IETF
unannounce flow. Update the subscriber stats handling around start_announce,
stop_announce, run_subscribe_namespace(), and run_publish_namespace_stream() so
the same subscriber_announced_bytes(...) counter is also incremented when
NamespaceDone or PublishNamespaceDone is converted into stop_announce(path),
keeping the announce/unannounce metric symmetric.
---
Outside diff comments:
In `@rs/moq-net/src/ietf/publisher.rs`:
- Around line 523-561: The `publish namespace` flow in `publisher.rs` only
updates `publisher_announced_bytes` after `PublishNamespaceOk`/`RequestOk`, so
rejected namespaces never count the `PUBLISH_NAMESPACE` control bytes that were
already sent. Move the announcement byte accounting in the
`publish`/`PublishNamespace` handling path so it is recorded immediately after
writing the `ietf::PublishNamespace` request (before waiting on
`stream.reader.decode()`), and keep the existing success handling in place for
the namespace stream bookkeeping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2de92e35-5cce-4a04-b6b2-8e73e8175860
📒 Files selected for processing (6)
doc/bin/relay/config.mdrs/moq-net/src/ietf/publisher.rsrs/moq-net/src/ietf/subscriber.rsrs/moq-net/src/lite/publisher.rsrs/moq-net/src/lite/subscriber.rsrs/moq-net/src/stats.rs
✅ Files skipped from review due to trivial changes (1)
- doc/bin/relay/config.md
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rs/moq-net/src/ietf/publisher.rs (1)
523-561: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCount
PUBLISH_NAMESPACEbytes before waiting for the OK response.Lines 547-561 only bump
announced_bytesafterPublishNamespaceOk/RequestOk, but Lines 527-535 have already writtenPUBLISH_NAMESPACEto the wire. A rejected namespace therefore consumes announce control bytes without ever surfacing in stats, which under-reports the new metric’s documented “independent of the lifetime guard” behavior.Suggested fix
let request_id = self.control.next_request_id().await?; let mut stream = Stream::open(&self.session, self.version).await?; // Count the broadcast name length, not the encoded message size, so // stats don't penalize the broadcast for framing overhead. let announce_bytes = absolute.as_str().len() as u64; + let bs = self.stats.broadcast(&absolute); // Write the PublishNamespace message stream.writer.encode(&ietf::PublishNamespace::ID).await?; stream .writer @@ .encode(&ietf::PublishNamespace { request_id, track_namespace: suffix.as_path(), }) .await?; + bs.publisher_announced_bytes(announce_bytes); // Read response from stream.reader let type_id: u64 = stream.reader.decode().await?; @@ (Version::Draft14, ietf::PublishNamespaceOk::ID) => { let msg = ietf::PublishNamespaceOk::decode_msg(&mut data, self.version)?; tracing::debug!(message = ?msg, "publish namespace ok"); - let bs = self.stats.broadcast(&absolute); - bs.publisher_announced_bytes(announce_bytes); namespace_streams.insert(suffix, (request_id, stream, bs.publisher())); } @@ (_, ietf::RequestOk::ID) => { let msg = ietf::RequestOk::decode_msg(&mut data, self.version)?; tracing::debug!(message = ?msg, "publish namespace ok"); - let bs = self.stats.broadcast(&absolute); - bs.publisher_announced_bytes(announce_bytes); namespace_streams.insert(suffix, (request_id, stream, bs.publisher())); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-net/src/ietf/publisher.rs` around lines 523 - 561, The `publish namespace` flow in `publisher.rs` only updates `publisher_announced_bytes` after `PublishNamespaceOk`/`RequestOk`, so rejected namespaces never count the `PUBLISH_NAMESPACE` control bytes that were already sent. Move the announcement byte accounting in the `publish`/`PublishNamespace` handling path so it is recorded immediately after writing the `ietf::PublishNamespace` request (before waiting on `stream.reader.decode()`), and keep the existing success handling in place for the namespace stream bookkeeping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rs/moq-net/src/ietf/subscriber.rs`:
- Around line 455-466: The subscriber-side announced-bytes tracking in
start_announce only records the announce path and misses the matching IETF
unannounce flow. Update the subscriber stats handling around start_announce,
stop_announce, run_subscribe_namespace(), and run_publish_namespace_stream() so
the same subscriber_announced_bytes(...) counter is also incremented when
NamespaceDone or PublishNamespaceDone is converted into stop_announce(path),
keeping the announce/unannounce metric symmetric.
---
Outside diff comments:
In `@rs/moq-net/src/ietf/publisher.rs`:
- Around line 523-561: The `publish namespace` flow in `publisher.rs` only
updates `publisher_announced_bytes` after `PublishNamespaceOk`/`RequestOk`, so
rejected namespaces never count the `PUBLISH_NAMESPACE` control bytes that were
already sent. Move the announcement byte accounting in the
`publish`/`PublishNamespace` handling path so it is recorded immediately after
writing the `ietf::PublishNamespace` request (before waiting on
`stream.reader.decode()`), and keep the existing success handling in place for
the namespace stream bookkeeping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2de92e35-5cce-4a04-b6b2-8e73e8175860
📒 Files selected for processing (6)
doc/bin/relay/config.mdrs/moq-net/src/ietf/publisher.rsrs/moq-net/src/ietf/subscriber.rsrs/moq-net/src/lite/publisher.rsrs/moq-net/src/lite/subscriber.rsrs/moq-net/src/stats.rs
✅ Files skipped from review due to trivial changes (1)
- doc/bin/relay/config.md
🛑 Comments failed to post (1)
rs/moq-net/src/ietf/subscriber.rs (1)
455-466: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
announced_bytesis still missing IETF unannounce traffic on the subscriber side.Line 460 only credits the announce half of the flow.
run_subscribe_namespace()still processesNamespaceDoneviastop_announce(path), andrun_publish_namespace_stream()still turns Draft14-16PublishNamespaceDoneinto the same stop path, without any symmetricsubscriber_announced_bytes(...)bump. That leaves IETF subscriber stats under-counting the documented announce/unannounce metric.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-net/src/ietf/subscriber.rs` around lines 455 - 466, The subscriber-side announced-bytes tracking in start_announce only records the announce path and misses the matching IETF unannounce flow. Update the subscriber stats handling around start_announce, stop_announce, run_subscribe_namespace(), and run_publish_namespace_stream() so the same subscriber_announced_bytes(...) counter is also incremented when NamespaceDone or PublishNamespaceDone is converted into stop_announce(path), keeping the announce/unannounce metric symmetric.
Two gaps in the IETF announced_bytes accounting: - The publisher only recorded the name length after PUBLISH_NAMESPACE_OK, so a rejected namespace spent an announce we never counted. Record it as soon as the request is on the wire instead. - The subscriber counted the announce (start_announce) but not the matching unannounce. Record it in stop_announce, mirroring start_announce. Both sides now count the broadcast name length per announce and per unannounce operation, draft-agnostically (we measure the name, not the on-wire framing), keeping publisher and subscriber symmetric. Addresses CodeRabbit review feedback on #1953. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed both findings from the latest review (they came through as summary/outside-diff items, so no inline threads to reply on) in commit 789d6e3:
Both IETF sides now count the broadcast name length per announce and per unannounce operation, draft-agnostically (we measure the name, not the on-wire framing), keeping publisher and subscriber symmetric. 🤖 Addressed by Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rs/moq-net/src/ietf/subscriber.rs`:
- Around line 520-526: The byte accounting in `stop_announce` is currently
treating every cleanup path as an unannounce, which can inflate
`subscriber_announced_bytes` when `stop_announce` is used after failed
`write_ok` or `write_publish_ok`. Update the `stop_announce` flow in
`subscriber.rs` so the byte bump only happens for real unannounce or
stream-close control events, either by moving the accounting to the actual
unannounce call sites or by threading an explicit `count_unannounce_bytes` flag
through `stop_announce` and the related announce/rollback paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c0b45c1-3c6b-4cc6-bd04-c339c8f2442a
📒 Files selected for processing (4)
doc/bin/relay/config.mdrs/moq-net/src/ietf/publisher.rsrs/moq-net/src/ietf/subscriber.rsrs/moq-net/src/stats.rs
✅ Files skipped from review due to trivial changes (1)
- doc/bin/relay/config.md
🚧 Files skipped from review as they are similar to previous changes (1)
- rs/moq-net/src/stats.rs
stop_announce is also called on local teardown after a failed write_ok / write_publish_ok, where no unannounce/stream-close control event occurred. Thread an explicit count_bytes flag so only real unannounces (NamespaceDone, PublishNamespaceDone/stream close) bump announced_bytes; rollback paths pass false. Addresses CodeRabbit review feedback on #1953. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Summary
Adds an
announced_bytescounter to the relay stats so we track how much announcement data a broadcast uses, not just the count of announce events. It sits next to the existingannounced/announced_closedcounters and is surfaced per(broadcast, tier, role)on the stats broadcast, just likebytes/frames/groups. It's kept separate frombytes, which remains media payload.announced_bytessums the broadcast name length for each announce/unannounce message. It deliberately counts the name rather than the encoded message size, so a broadcast isn't charged for transport overhead it doesn't control (size prefixes, message-type ids, hop chains).What changed
rs/moq-net/src/stats.rs: newCounters.announced_bytesfield, threaded throughRawCounts,Snapshot,snapshot(), andprocess_slot. NewBroadcastStats::publisher_announced_bytes(n)/subscriber_announced_bytes(n)methods that record keyed by broadcast path, independent of the announce lifetime guard. Module docs updated. Two new tests.AnnounceInitinitial active set on both sides.start_announce).doc/bin/relay/config.md: addedannounced_bytesto the stats JSON example and field semantics (Cross-Package Sync:moq-relay config/behavior).API changes
moq-net::Counters— newpub announced_bytes: AtomicU64field. The struct is#[non_exhaustive], so this is additive and non-breaking.moq-net::BroadcastStats::publisher_announced_bytes(&self, n: u64)— new pub method.moq-net::BroadcastStats::subscriber_announced_bytes(&self, n: u64)— new pub method.announced_bytesfield.codingmodule'sWriter/Reader.Per the branch-targeting rules this is additive and non-breaking, so it targets main.
Notes for the reviewer
moq-transportv17+, an unannounce is just a stream close with no name on the wire, so it contributes nothing there (v14-16, which send PUBLISH_NAMESPACE_DONE, do).js/nethas no stats-aggregator counterpart (stats are relay-side only), so there's nothing to mirror there.Test plan
cargo test -p moq-net(incl. two newannounced_bytestests) greencargo checkacrossmoq-relay/moq-nativecargo clippy -p moq-net --all-targetsclean (via nix)cargo fmt(via nix)(Written by Claude Opus 4.8)