-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Fix ConnectionStatistics.*Rate() methods #5783
Comments
@sbordet do you really want The current implementation is just broken because it is comparing the total rate to the period of time since the last check. It should measure the change in the total over that time period. |
Signed-off-by: Lachlan Roberts <[email protected]>
Signed-off-by: Lachlan Roberts <[email protected]>
@lachlan-roberts there is a So I think it's fine to have just one timestamp, and if you want, you can call |
reset will reset everything, so perhaps just calc rate since the last call to rate. If you want the rate every 5s, then poll rate() every 5s. |
@sbordet that will also reset all the other statistics like total number of messages and bytes in/out, also total number connections and stats about the duration. |
@gregw I don't think calculating the rate since the last call to the rate methods is meaningful. What I really want from the user perspective is something since I last reset (which could be never). I open JMC and I see the right numbers immediately, calculated from startup. I would also consider snapshotting, so that applications can use snapshots and compare them. |
@sbordet I don't think that a rate since last reset is all that meaningful and it is too sensitive to exactly when sampled. It becomes only useful for automated solutions that will do a reset, run a test, then sample the rate - without any human delays. More over, having to reset the stats to obtain a meaningful rate means that you may lose lots of longer lived statistics. So at the minimum, we need a way to reset rate statistics without resetting all the other stats. Using the last get of the rate as the rate reset is a simple compromise that means the method is at least So I think this PR represents a good improvement within the current API. However, it is still a compromise and perhaps we need something more explicit as to what period the rate applies to, but that will need new API. |
PR #5789 has been merged and fixed the ConnectionStatistics rate calculation. |
Jetty version
9.4.x
Description
ConnectionStatistics.*Rate()
methods do agetAndSet()
on the timestamp that is obviously wrong (what was I thinking?!?).These methods should just do a
get()
of the startup timestamp and use it to calculate the elapsed time, and then do the rate calculations.Having a single startup timestamp will also get rid of the useless 4 timestamp fields we have now.
The text was updated successfully, but these errors were encountered: