Skip to content
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 the calculation of rates in ConnectionStatistics #5789

Merged

Conversation

lachlan-roberts
Copy link
Contributor

closes #5783

The calculation for the rate was previously done as getTotal() / elapsedTime.
When it should really be getIncreaseOverElapsedTime() / elapsedTime.

@lachlan-roberts
Copy link
Contributor Author

@sbordet another problem with this is that ConnectionStatistics doesn't collect live statistics from connections. It only updates when the connection closes, so its probably not going to give you great statistics for rates in websocket. You'll probably be using long lasting websocket connections so the rate values would probably be pretty useless.

@gregw
Copy link
Contributor

gregw commented Dec 10, 2020

Maybe have some kind of 'takeStats' method that updates all the totals and remembers the levels in each connection so it doesn't get double counted.
call takeStats only if somebody queries the stats... not sure if all the wiring necessary is there... but look

Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

I'm +0 as I don't really like the total being included in the RateCounter.
I can see how it makes the ConnectionStatistics a little neater, but it is kind of unrelated.

@lachlan-roberts lachlan-roberts merged commit 3d11d4e into jetty-9.4.x Dec 28, 2020
@lachlan-roberts lachlan-roberts deleted the jetty-9.4.x-5783-ConnectionStatisticsRates branch December 28, 2020 05:26
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.

2 participants