-
Notifications
You must be signed in to change notification settings - Fork 13
feat(sub): add socket-wide stats including dropped messages #88
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
Signed-off-by: 7suyash7 <[email protected]>
mempirate
left a comment
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.
Thanks for the PR! I've got a small suggestion
msg-socket/src/sub/stats.rs
Outdated
| } | ||
|
|
||
| #[derive(Debug, Default)] | ||
| pub struct SocketWideStats { |
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.
Agreed with having socket-agnostic stats for shared metrics, but I would like to see it implemented differently, something like the following:
struct SocketStats<S> {
// ... socket-wide stats, and
substats: S,
}Where S = a stats struct specific to the socket implementation, for example SubStats or PubStats.
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.
Hi, @mempirate yeah a unified structure makes sense. Just want to clarify though:
Are you thinking of a struct like maybe SocketStats<SpecificStats> (eg. SocketStats) where specific SpecificStats (like SubStats) hold the implementation specific details like the session map and the new counters like dropped_messages_total for Sub? And for this specific PR should I only refactor the SubSocket stats into this new pattern, leaving the refactor of Pub/Rep/Req stats for future work ? I'm happy to do a big refactor if thats preferred!
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.
Yeah that's right! And would be great if you'd want to do this across the different socket types so we don't get any inconsistencies
Signed-off-by: 7suyash7 <[email protected]>
Signed-off-by: 7suyash7 <[email protected]>
Signed-off-by: 7suyash7 <[email protected]>
Signed-off-by: 7suyash7 <[email protected]>
Signed-off-by: 7suyash7 <[email protected]>
Signed-off-by: 7suyash7 <[email protected]>
|
Hi! @mempirate, can you please take a look |
mempirate
left a comment
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! Thanks for the contribution :)
Implement statistics tracking for the SubSocket, #37
Add a new
SocketWideStatsstruct to hold metrics related tothe overall socket operation, distinct from per-publisher stats. Includes counters for: