-
-
Notifications
You must be signed in to change notification settings - Fork 329
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
List percentiles in output data, fixes #138 #144
Conversation
I think so, yes. This would require an update also on the format for the programmatic API, as the percentiles are not included in result.requests and the others. These are important data. For all, I think we should add: 2.75% 50% 97.5% percentiles. See #143 (comment). Something like: Stat | P2.75 | P50 | P97.5 | P99 | Avg | Stdev | Max Alternatively: Stat | P2.75 | P50 | P97.5 | P99 @AndreasMadsen what do you think? |
For sure the standard deviation is pretty useless for humans. You properly want the std error. The P2.75 and P97.5 are fine too, and properly prefered because the distribution is likely to be screwed (details below). For the same reason, it is a good idea to include P50 (median). – However, the standard deviation/variance and number of observations is the only reasonable summaries if you want to calculate a statistical significance later. Math details: Time (latency) is very often gamma distributed, this is a screwed distribution which means that the "mean" is unlikely to be close to the "median". One is not more correct than the other (I don't know why so many believe this), they are just different ways of summarizing something. As with all summaries, something will be lost. The different is just in what will be lost. The book "How to lie with statistics" is pretty good at explaining this in layman's terms. Regarding the difference between latency, and Req/Sec or Bytes/Sec. The only difference is that those should have a reciprocal gamma distribution. This turns out to be the same as the inverse-gamma distribution (proof). In all honesty, there is not (edit) much difference in terms of how well they summarize using typical statistics, so whatever you argue for latency should apply for Req/Sec and Bytes/Sec (this is where you can prove me wrong). |
lib/run.js
Outdated
@@ -13,6 +13,10 @@ const reInterval = require('reinterval') | |||
const histAsObj = histUtil.histAsObj | |||
const addPercentiles = histUtil.addPercentiles | |||
|
|||
// TODO upstream to hdr-histogram-percentiles-obj? | |||
histUtil.percentiles.push(2.5, 97.5) | |||
histUtil.percentiles.sort((a, b) => a - b) |
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.
I think those needs to be upstreamed.
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.
Aye: GlenTiki/hdr-histogram-percentiles-obj#2 May want to change the 2.5% name (as mentioned in the PR)
latency: addPercentiles(latencies, histAsObj(latencies)), | ||
throughput: histAsObj(throughput, totalBytes), | ||
throughput: addPercentiles(throughput, histAsObj(throughput, totalBytes)), |
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.
Can you please add some tests that verify that those are there?
Per discussion in mcollina/autocannon#138 and mcollina/autocannon#144 The 2.5 percentile is stored as `p25` which is quite confusing. maybe we can change it to use underscores `p2_5`, `p99_999`, would be a breaking change. We could also zero pad numbers <10 to `p025`, `p99999` which would not be breaking because there were no sub 10 percentiles previously.
lib/progressTracker.js
Outdated
// Percentiles | ||
Object.keys(stat).filter(k => /^p\d+$/.test(k)).forEach((k) => { | ||
result[k] = prettyBytes(stat[k]) | ||
}) |
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.
This is not matching 97_5 and 2_5 percentiles.
lib/progressTracker.js
Outdated
}) | ||
|
||
logToStream(out) | ||
})) | ||
|
||
if (opts.renderLatencyTable) { |
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.
This will need an update as well, in conjunction with GlenTiki/hdr-histogram-percentiles-obj#2 I get:
$ node autocannon.js -c 100 -d 5 -l localhost:3000
Running 5s test @ http://localhost:3000
100 connections
Stat 2.5% 50% 97.5% 99% Avg Stdev Max
Latency (ms) 3 4 8 11 4.66 2.19 93.68
Req/Sec 17503 19823 20623 20623 19384 1105.74 20619
Bytes/Sec 2883583 3.28 MB 3407871 3.41 MB 3.16 MB 196 kB 3.38 MB
Percentile Latency (ms)
2.5
50 4
75 5
90 6
97.5
99 11
99.9
99.99
99.999
97k requests in 5s, 15.9 MB read
(CI failures are because the old hdr-histogram-percentiles-obj is still used) |
Can you please update the docs as well? |
I would like to point out that we want Latency to be low but Req/Sec and Bytes/Sec to be high. An symmetric summary thus doesn't make sense. Right now, you are including max which makes sense for latency. But properly we are mostly interested in the min for Req/sec and Bytes/sec. Similarly, you are including the 99% quantile. However, the 1% quantile is properly more interesting for Req/sec and Bytes/sec. |
What’s your recommendation @AndreasMadsen? |
Does something along these lines make more sense?
e; + adding a "sampled N times per second" note below? |
@goto-bus-stop Statistically it makes sense. Although, I'm not sure how easy it is to read like that. |
yeah. could also just add all of them but then you have to filter them when reading. in a terminal the headers have colours which helps a little. i'll play with it to get at least that info on the screen then. |
This changes the Latency output to Avg / 90% / 99% / Max. The Req/Sec and Bytes/Sec is unchanged. ``` Running 10s test @ https://wlk.yt 10 connections Stat Avg 90% 99% Max Latency (ms) 255.41 337 715 987.17 Stat Avg Stdev Max Req/Sec 38.4 5.97 46 Bytes/Sec 6.14 MB 954 kB 7.37 MB 384 requests in 10s, 61.5 MB read ``` I'm no stats expert but from mcollina#138 my understanding is that stdev is fine for the other two metrics so I just split the tables. Maybe latency could use a little cell margin to make it easier to read the percentile figures. Lmk if the percentiles should also be used for the req and bytes/sec
Just the ones used by the CLI here. Need to do `t.type()` instead of `ok()` because a lot of them are too fast to be not-0 :)
Needs a version bump for hdr-histogram-percentiles-obj before it'll work :)
``` 10 connections Stat 2.5% 50% 97.5% 99% Avg Stdev Max Latency 17 ms 29 ms 204 ms 334 ms 45.32 ms 64.1 ms 954.08 ms Stat 1% 2.5% 50% 97.5% Avg Stdev Min Req/Sec 91 91 274 288 217.67 89.75 91 Bytes/Sec 688 kB 688 kB 2.03 MB 2.1 MB 1.58 MB 642 kB 657 kB Req/Bytes counts sampled once per second. 653 requests in 3s, 4.71 MB read ```
1c2ed00
to
50e2fbb
Compare
@goto-bus-stop I think the last UI snippet is really good! |
Code looks good as well |
looks like the appveyor failures were fixed by increasing the timeout! |
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
|
||
logToStream(out) | ||
logToStream(table([ | ||
asColor(chalk.cyan, ['Stat', '2.5%', '50%', '97.5%', '99%', 'Avg', 'Stdev', 'Max']), |
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.
As a suggestion: rename 50%
to Median
in both cases.
There is just one thing I miss: a description how to read this properly. That is likely out of scope for this PR but I suggest to create a small static webpage that describes everything in more detail - also how to read the statistical data and to add a link to that webpage next to the output (e.g., |
@BridgeAR I would prefer if we had that description in the README of the project. It's the easiest page where people would expect that stuff to be. As for a website.. that's a completely different discussion (outside of this PR). |
I just fear most people do not read the README ;-) But it's still a good point to add a description to it! |
@goto-bus-stop could you add that to the README? |
This was merged as part of #145 (I merged this into that branch to make tests pass reliably). I'll try write something about how to read it this week :) |
This changes the Latency output to Avg / 90% / 99% / Max. The Req/Sec
and Bytes/Sec is unchanged.
I'm no stats expert but from #138 my understanding is that stdev is fine
for the other two metrics so I just split the tables. (Maybe latency
could use a little cell margin to make it easier to read the percentile
figures.)
Lmk if the percentiles should also be used for the req and bytes/sec
Fixes #138
Fixes #143