Skip to content

Aggregate data rates at vhost level for all protocols#1699

Open
kickster97 wants to merge 3 commits intomainfrom
vhost-byte-rates
Open

Aggregate data rates at vhost level for all protocols#1699
kickster97 wants to merge 3 commits intomainfrom
vhost-byte-rates

Conversation

@kickster97
Copy link
Member

@kickster97 kickster97 commented Feb 12, 2026

WHAT is this pull request doing?

Track data rates at vhost level across all protocols

Previously, byte rates in the overview were aggregated from per-connection stats, which meant HTTP API traffic was invisible in the data rates graph since HTTP has no persistent connections. This adds recv_oct and send_oct counters to the vhost, fed by all three protocols: AMQP and MQTT clients bubble up their per-connection byte counts to the vhost alongside existing connection-level tracking, and HTTP reports bytes directly to the vhost. The overview endpoint now reads byte rates from the vhost instead of summing connections, giving a complete picture of all traffic.

Based off discussion in #1682

HOW can this pull request be tested?

specs to come

kickster97 and others added 2 commits February 12, 2026 13:53
AMQP and MQTT clients now report bytes to vhost counters
alongside their per-connection tracking. The overview endpoint
reads byte rates from vhost stats instead of summing connections,
ensuring HTTP traffic is included.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@kickster97 kickster97 changed the title Vhost byte rates Aggregate data rates at vhost level for all protocols Feb 12, 2026
@kickster97 kickster97 self-assigned this Feb 12, 2026
@kickster97 kickster97 marked this pull request as ready for review February 12, 2026 14:36
@kickster97 kickster97 requested a review from a team as a code owner February 12, 2026 14:36
Copy link
Member

@carlhoerberg carlhoerberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did it have any performance impact?

@carlhoerberg
Copy link
Member

HTTP revc/send doesn't include the whole request/response size, but i guess that's ok.

@jage
Copy link
Member

jage commented Feb 14, 2026

Can we add specs? Ideally specs that break if we add additional protocols and miss this.

@claude
Copy link

claude bot commented Mar 2, 2026

PR Review: Vhost-level byte rate tracking

1. Test logic flaw in "all protocols should aggregate byte counts to vhost level" (spec/vhost_stats_spec.cr)

The send_and_receive_over_protocol method for AMQP explicitly waits for connections to be removed:

wait_for { vhost.connections.none?(LavinMQ::AMQP::Client) }

And the MQTT path's with_client_io blocks close connections when they exit. By the time the test reaches:

conn_recv_sum = vhost.connections.sum(&.recv_oct_count)
conn_send_sum = vhost.connections.sum(&.send_oct_count)

vhost.recv_oct_count.should be >= conn_recv_sum
vhost.send_oct_count.should be >= conn_send_sum

vhost.connections is empty (or nearly so), making both sums 0. The >= assertions pass trivially and do not actually verify that vhost-level byte counts aggregate per-connection counts. The wait_for condition reinforces this since it succeeds immediately when vhost.connections.empty? due to the ||:

wait_for { vhost.connections.empty? || vhost.connections.all? { |c| c.recv_oct_count > 0 } }

The final > 0 assertions are the only ones that verify anything meaningful. Consider either keeping connections alive during the comparison, or removing the misleading >= conn_sum assertions.

Everything else looks correct

  • The atomic add calls with :relaxed ordering in add_recv_bytes/add_send_bytes are consistent with existing per-connection tracking and appropriate for the hot path (single atomic add instruction, no new allocations).
  • The rate_stats macro correctly generates @recv_oct_count/@send_oct_count atomics and the corresponding rate computation in update_rates, which is already called periodically via Server#update_stats_rates.
  • The overview endpoint change from summing per-connection rates to using vhost-level rates is correct since the vhost now tracks bytes directly.
  • HTTP API publish/get byte tracking (body-only, no protocol framing) is a reasonable design choice for non-AMQP paths.
  • All AMQP client send paths (single frame, deliver method+header+body frames) and the receive path (process_frame) are covered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants