Client: Add the ability to disable stats #67
Conversation
These stats are fairly lightweight, but do incur some memory and computational overhead. This change allows them to be disabled on a per-connection basis either as a setting change on the `SocketExecutor`, or on the `Client` itself.
…dScheduler instance This is to allow stats to be collected on the underline scheduler if desired. In addition SingleThreadSocketExecutor was transitioned to manually constructing a single thread (rather than a pool), to reduce overhead. It also will do a `join` on the stop to ensure that the stop has completed before returning. ThreadedSocketExecutor removed unnecssary variable
| perConnectionStatsEnabled = enabled; | ||
| } | ||
|
|
||
| protected void recordReadStatst(int size) { |
There was a problem hiding this comment.
I assume this is supposed to be recordReadStats
There was a problem hiding this comment.
Thanks for pointing out, fixed now
| } | ||
|
|
||
| protected void addReadAmount(int size) { | ||
| public void setPerConnectionStatsEnabled(boolean enabled) { |
There was a problem hiding this comment.
Not sure what you think about this, but it seems like we should loop through all clients and enable or disable them when this changes.
There was a problem hiding this comment.
I considered looping through, it would require locking to be 100%, so I was thinking that this is probably good enough. Let me know your thoughts
There was a problem hiding this comment.
You could do it with schedule on the selector threads, not sure if its worth it or not, just an idea.
There was a problem hiding this comment.
For now I say we leave it, I can't imagine changing this while clients are active is likely
|
@lwahlmeier Any ideas on how we can get other stats from the selector? For example any ideas on how we could get signal about possible network congestion, or other conditions outside of pool execution time? |
Yeah there are some options, keeping track of total bytes pending write VS amount written over a period of time, it is a little tricky to know if its backed up or not. Maybe just how many/what % of clients are waiting for Write to the socket. I also feel like getting stats on the selector thread loop time it takes the selector to process clients could be another indication. |
…Bytes This metric helps you know across all clients how backed up you are either consuming data into the application (likely cpu or otherwise bound by the application), or write bound (network or remote side being the bottleneck).
|
@lwahlmeier Yeah, I was originally thinking something like "set write interests, then time how long till we can write". I am not sure if that would really help or not though. So instead, for now I kept it simple, adding to the SocketExecutor a way to total up the pending bytes across all the Clients. What do you think of this API? Docs seem accurate? I wonder if these pending byte stats should be part of the stats object they already return. I played with that, I think API wise it's a little cleaner since all the stats are more located together. However code structure wise is slightly not as pretty. Let me know your preference if you like this way I can go ahead and merge, if you want stats more co-located...I can deprecate Thanks for taking a look, I want to make sure you are happy with this and I believe these stat improvements will be useful to me. |
|
Hum, moving the get read/write size to the stats could make sense, it does feel a little weird to me to have those as changing stats when the SimpleByteStats object is returned, but it is no different then the read/write rates/totals. If you think it make sense I say go for it. Client stats for write round trip times could be pretty nice. It could also show congestion or other issues, though I dont like the idea of hitting the clock that much in high load situations so it would have to be disableable. I am good with this the way it is for now, but if you want to do the above let me know and I will look through the changes. |
|
I filed issue #68 so we can consider the API changes later for 5.0. I would like to make the API changes, but am frustrated by not being able to keep things as efficient as they are with the current API. Feel free to drop thoughts on that issue, but going to just roll with this for now. |
These stats are fairly lightweight, but do incur some memory and computational overhead.
This change allows them to be disabled on a per-connection basis either as a setting change on the
SocketExecutor, or on theClientitself.I would in the future like to add additional stats, but have not arrived at what would be good to include. Pool based stats might make sense, but those can be passed in when constructing the SocketExecutor. Maybe stats around delays around notifying the selector and the selector waking up? However I was unsure precisely how that would be useful. @lwahlmeier let me know if you have thoughts around that, or this PR in general.