Added a stats-success-breakdown flag for more detailed status code specific response statistics

[wjhoward]

Edit: This comment is now out of date, see additional comments below

Added a command line flag success-stats-only which when set includes only successful (2xx responses) when measuring the Response time histogram and distribution statistics (as per issue: #212 )

The below example was generated using a backend server which returns slow 5xx responses. The left terminal is running with the default options, and the right terminal has the new flag set. As you can see, the slow 5xx responses are excluded from the right terminal:

The same flag is applied to the JSON equivalent fields.

I did investigate adding a test for this:

• Adding a new warp handler to return non 2xx responses for every other request
• Adding a test which runs an oha command with 2 requests, asserting that when the success flag is set only a single response would appear in the output (2nd request would be a 5xx)

Although I stopped as it was a little tricky, and I wanted to see what the initial response to the PR was first.
Can proceed with adding the test if the changes here make sense.

[hatoo]

Thanks!

It looks Success rate: is broken, I think those shouldn't be 100% even if --success-stats are presented.

I think it's better to print response times for all of

1. All responses
2. Only succeeding responses
3. Only non-succeeding responses

Yes, it's verbose but --success-stats flag should be a rare case so it's not required to be simple.

[hatoo]

For testing, there are no tests for outputs.
We can proceed it without tests for now.

[wjhoward]

Thanks @hatoo , so just to clarify you are suggesting that when the flag is set the summary would include 3 sections for both the Response time histogram and Response time distribution sections, e.g:

Response time histogram:
  // all requests
  0.016 [1]  |
  ...

Response time histogram (2xx only):
  0.016 [1]  |
...

Response time histogram (non 2xx):
  0.016 [1]  |

In addition the JSON would also need to include the 3 sections.

I think we would probably then want to change the flag name, e.g. something like stats-success-breakdown

[hatoo]

@wjhoward
Exactly.

I think we would probably then want to change the flag name, e.g. something like stats-success-breakdown.

Yeah, the flag name should be changed and stats-success-breakdown seems good 👍

[wjhoward]

@hatoo Regarding the broken Success rate: % I believe that's broken in the existing code (I've not actually changed that) - I can raise an issue and we can track fixing that separately?

EDIT: Actually I can just fix that in this PR - may be easier 😄

[hatoo]

@wjhoward
Sorry, I misunderstood, the Success rate is a number for lower levels of protocol stack e.g. socket error or something, so 100% is OK.
So It should be untouched.

[wjhoward]

With the recent changes the summary output looks like this:

The JSON output also includes the new fields only if the flag is set:

Successful status codes include 2xx only.
Not successful status codes includes 4xx and 5xx only.

Need to test this more. Does this match your thinking @hatoo ?

[hatoo]

LGTM 👍 .
Please proceed.

[wjhoward]

@hatoo I've tested this and it works as I would expect. Ideally it would be nice to include output tests in the test suite for validating this, but understand that it's non trivial.
I've also updated the CHANGELOG.md file.
Let me know if you want me to update anything else?

[hatoo]

Thank you!
Merged. It's perfect.