Skip to content

[ADDED] Add account-level connection stats #6967

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 20, 2025

Conversation

bruth
Copy link
Member

@bruth bruth commented Jun 11, 2025

This adds tracking of msgs and bytes for route, gateway, and leaf connections.

More tests need to be added, but I wanted to get a proposal of the idea prepared for initial review in case there are any glaring issues.

Signed-off-by: Byron Ruth [email protected]

@bruth
Copy link
Member Author

bruth commented Jun 11, 2025

I will fix the test.. it is an existing one that is doing a contains string match for the existing accstatz payload.

@bruth bruth force-pushed the byron/account-conn-stats branch from e0bc201 to d89f4e1 Compare June 18, 2025 11:00
@bruth bruth force-pushed the byron/account-conn-stats branch from e72e3b4 to a8d80b1 Compare June 18, 2025 15:11
@bruth bruth marked this pull request as ready for review June 18, 2025 16:24
@bruth bruth requested a review from a team as a code owner June 18, 2025 16:24
@bruth bruth force-pushed the byron/account-conn-stats branch 2 times, most recently from 79bc06d to 64f9af2 Compare June 18, 2025 17:27
server/client.go Outdated
if acc != nil {
atomic.AddInt64(&acc.inMsgs, int64(c.in.msgs))
atomic.AddInt64(&acc.inBytes, int64(c.in.bytes))
acc.smu.Lock()
Copy link
Member

Choose a reason for hiding this comment

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

Even though for larger more complex structs that could have multiple locks we use mu, for small simple structs you can leave it unamed and just do a lock direct on the struct. So if stats is always non-nil in acc, then you could do acc.stats.Lock()

Copy link
Member Author

Choose a reason for hiding this comment

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

There are four stats structs on the acc now. The existing one is embedded and the new ones, ln, gw, and rt. The choice for the smu was for a single lock when any/all of them need to be updated in one shot vs. individual locks.

The alternative is to refactor this to encapsulate the stats together as one unit (new struct) with a lock.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should do this.

Copy link
Member

Choose a reason for hiding this comment

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

Have refactored the struct shape a bit.

@@ -2732,8 +2732,12 @@ func (c *client) sendMsgToGateways(acc *Account, msg, subject, reply []byte, qgr
totalBytes -= dlvMsgs * int64(LEN_CR_LF)
}
if acc != nil {
atomic.AddInt64(&acc.outMsgs, dlvMsgs)
atomic.AddInt64(&acc.outBytes, totalBytes)
acc.smu.Lock()
Copy link
Member

Choose a reason for hiding this comment

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

Maybe acc needs updateStats(msgs, bytes, rmsgs, rbytes, gwmsgs, gwbytes, lnmsgs, lnbytes)?

Copy link
Member

Choose a reason for hiding this comment

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

I had a go at doing this, but it needs to be split over two functions for inbound vs outbound, and I couldn't come up with a shape that felt good at the callsites. For now I think the current approach is clearer.

This adds tracking of msgs and bytes for route, gateway,
and leaf connections.

Signed-off-by: Byron Ruth <[email protected]>
Signed-off-by: Neil Twigg <[email protected]>
@neilalexander neilalexander force-pushed the byron/account-conn-stats branch from 64f9af2 to a27cf43 Compare June 20, 2025 11:54
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM

@neilalexander neilalexander merged commit e292fbc into main Jun 20, 2025
69 of 70 checks passed
@neilalexander neilalexander deleted the byron/account-conn-stats branch June 20, 2025 16:26
@wallyqs wallyqs changed the title Add account-level connection stats [ADDED] Add account-level connection stats Jun 24, 2025
neilalexander added a commit that referenced this pull request Jun 24, 2025
Includes the following:

- #6990
- #6967
- #6995
- #6999
- #7001

Signed-off-by: Neil Twigg <[email protected]>
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.

3 participants