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 ConnectionStatistics.*Rate() methods #5783

Closed
sbordet opened this issue Dec 10, 2020 · 8 comments
Closed

Fix ConnectionStatistics.*Rate() methods #5783

sbordet opened this issue Dec 10, 2020 · 8 comments

Comments

@sbordet
Copy link
Contributor

sbordet commented Dec 10, 2020

Jetty version
9.4.x

Description
ConnectionStatistics.*Rate() methods do a getAndSet() 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.

@lachlan-roberts
Copy link
Contributor

@sbordet do you really want getRate() methods which give you the rate since the start of the server, I think it needs to just be over a shorter period of time for it to be meaningful. If you had a very long period of time with no traffic, then you get a high burst of traffic, it will barely register on the rate statistic.

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.

lachlan-roberts added a commit that referenced this issue Dec 10, 2020
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Dec 10, 2020
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@sbordet
Copy link
Contributor Author

sbordet commented Dec 10, 2020

@lachlan-roberts there is a reset() method if you want to reset the stats and look at the numbers over shorter time, or perhaps past an initial transient state that can skew the stats.

So I think it's fine to have just one timestamp, and if you want, you can call reset().

@gregw
Copy link
Contributor

gregw commented Dec 10, 2020

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.
Or have a rate(boolean reset) method as an option?

@lachlan-roberts
Copy link
Contributor

@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.

@sbordet
Copy link
Contributor Author

sbordet commented Dec 19, 2020

@gregw I don't think calculating the rate since the last call to the rate methods is meaningful.
If you poll every 5 s, you poll on the total and then you derive the rate based on the 5 s period.

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.
Let's say that I then run some benchmarks or I know the system went through spikes and such but now it's at steady state.
Hit reset(), run the benchmark or leave the system running, then refresh the values and I have the numbers I want.
Makes sense?

I would also consider snapshotting, so that applications can use snapshots and compare them.
I.e. have a Data collect() method that takes a snapshot of all the current values (not necessarily atomically).
Then applications can compare 2 Data objects.

@gregw
Copy link
Contributor

gregw commented Dec 19, 2020

@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
feasible for a human to sample the rate in a meaningful way and still works for automated systems (which probably just call the rate getter once at the end of the test).

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.

@lachlan-roberts
Copy link
Contributor

@gregw @sbordet I could add an option in ConnectionStatistics to not reset the rates in the getRate methods, and add a new method called resetRates() which resets the rates but not the totals.

@lachlan-roberts
Copy link
Contributor

PR #5789 has been merged and fixed the ConnectionStatistics rate calculation.

This was referenced Mar 10, 2021
This was referenced Mar 10, 2021
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

No branches or pull requests

3 participants