-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[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
Conversation
I will fix the test.. it is an existing one that is doing a contains string match for the existing accstatz payload. |
e0bc201
to
d89f4e1
Compare
e72e3b4
to
a8d80b1
Compare
79bc06d
to
64f9af2
Compare
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() |
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
server/gateway.go
Outdated
@@ -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() |
There was a problem hiding this comment.
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)
?
There was a problem hiding this comment.
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]>
64f9af2
to
a27cf43
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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]