thanks @filipecosta90
LGTM. (failed test is timing issue of a new test i added, fix in #7604)

@redis/core-team this PR adds another (small) dependency, solely for redis-benchmark. I don't like to bloat the project just for the benchmark, but on the other hand the benchmark has little value if it's not accurate or doesn't provide enough info, so it should either be extracted to a different repo, or improved inside redis.
in anyway, while improving it, there's no sense to write everything from scratch if there's something that fits just what we need and doesn't include much more extra.

the other concern is backwards compatibility of the output, but i guess that's not a big concern.
if someone has a script that relies on the output, he'll need to adjust.
An alternative would be to keep supporting the old output and add some switch, i suppose we can do that using the new implementation which will just emit a format similar to the old one, but i'm not sure it worth it.

please share your thoughts.

I don't think redis-benchmark is generally considered to be on the same level as something like memtier, which is a much better benchmarking tool. I would not be inclined to accept this if we intend to continue using redis-benchmark as just an easy adhoc tool that comes out of the box. If we want to start using it more for validating the performance of Redis, I think something like this would be justified.

Also, anecdotally I have known users who have "latency goals", and so they care less about the percentile response you are proposing and more of what percentage of requests will hit them. So I think we should definitely include a backwards compatible version too.

I don't think redis-benchmark is generally considered to be on the same level as something like memtier, which is a much better benchmarking tool. I would not be inclined to accept this if we intend to continue using redis-benchmark as just an easy adhoc tool that comes out of the box. If we want to start using it more for validating the performance of Redis, I think something like this would be justified.

@madolson I believe both tools have different strong points. IMHO redis-benchmark gains for it's easness of usage, out of the box tests for more than sets/gets, and of course being available when users clone redis. Memtier is more a full blown benchmarking tool but I believe they can both complement each other. That said I believe they should both present extended metrics on latency.

Also, anecdotally I have known users who have "latency goals", and so they care less about the percentile response you are proposing and more of what percentage of requests will hit them. So I think we should definitely include a backwards compatible version too.

Regarding the latency goals, I think it's a valid point that they rely on it and that is basically the inverse of percentile (CDF), and enables answering questions like "What is the percentage of requests served up to X ms". I would keep the both:

• "Cumulative distribution of latencies" and
• "Latency by percentile distribution"
  WDYT?

@filipecosta90 I think that makes sense. So it would be great if you can update the PR to be able to produce both options. I'll take a pass through and leave some comments. Would like to hear other thoughts from @redis/core-team as well.

I generally agree there's good justification for both tools. Ideally I think memtier_benchmark should be the superset: support everything that redis-benchmark can do and be the target for all complex/powerful stuff.

For example, redis-benchmark currently doesn't handle TLS/SSL. Last time I had a look at it, it seemed a pretty tedious task to do so I'm OK with avoiding it.

As for this PR, I think the main justification to accept it is the fact that memtier_benchmark is not yet a full superset and users still need to resort to redis-benchmark.

@filipecosta90 I think that makes sense. So it would be great if you can update the PR to be able to produce both options. I'll take a pass through and leave some comments. Would like to hear other thoughts from @redis/core-team as well.

@madolson wdyt about the following output? mimicking the previous behavior on the cumulative distribution of latencies we iterate by a step of 0.1ms up to 2ms and by a step of 1ms afterwards:

====== SET ======                                                     
  100000 requests completed in 0.99 seconds
  50 parallel clients
  3 bytes payload
  keep alive: 1
  host configuration "save": 
  host configuration "appendonly": no
  multi-thread: no

Latency by percentile distribution:
0.000% <= 0.111 milliseconds (cumulative count 1)
50.000% <= 0.303 milliseconds (cumulative count 51727)
75.000% <= 0.359 milliseconds (cumulative count 75435)
87.500% <= 0.415 milliseconds (cumulative count 87716)
93.750% <= 0.471 milliseconds (cumulative count 94099)
96.875% <= 0.527 milliseconds (cumulative count 96875)
98.438% <= 0.615 milliseconds (cumulative count 98464)
99.219% <= 0.799 milliseconds (cumulative count 99223)
99.609% <= 1.319 milliseconds (cumulative count 99614)
99.805% <= 1.911 milliseconds (cumulative count 99807)
99.902% <= 2.375 milliseconds (cumulative count 99903)
99.951% <= 2.983 milliseconds (cumulative count 99952)
99.976% <= 3.535 milliseconds (cumulative count 99976)
99.988% <= 3.823 milliseconds (cumulative count 99988)
99.994% <= 3.999 milliseconds (cumulative count 99994)
99.997% <= 4.087 milliseconds (cumulative count 99997)
99.998% <= 4.215 milliseconds (cumulative count 99999)
99.999% <= 4.327 milliseconds (cumulative count 100000)
100.000% <= 4.327 milliseconds (cumulative count 100000)

Cumulative distribution of latencies:
0.000% <= 0.103 milliseconds (cumulative count 0)
3.355% <= 0.207 milliseconds (cumulative count 3355)
51.727% <= 0.303 milliseconds (cumulative count 51727)
86.394% <= 0.407 milliseconds (cumulative count 86394)
95.979% <= 0.503 milliseconds (cumulative count 95979)
98.373% <= 0.607 milliseconds (cumulative count 98373)
98.944% <= 0.703 milliseconds (cumulative count 98944)
99.237% <= 0.807 milliseconds (cumulative count 99237)
99.366% <= 0.903 milliseconds (cumulative count 99366)
99.452% <= 1.007 milliseconds (cumulative count 99452)
99.512% <= 1.103 milliseconds (cumulative count 99512)
99.546% <= 1.207 milliseconds (cumulative count 99546)
99.599% <= 1.303 milliseconds (cumulative count 99599)
99.666% <= 1.407 milliseconds (cumulative count 99666)
99.695% <= 1.503 milliseconds (cumulative count 99695)
99.726% <= 1.607 milliseconds (cumulative count 99726)
99.751% <= 1.703 milliseconds (cumulative count 99751)
99.776% <= 1.807 milliseconds (cumulative count 99776)
99.802% <= 1.903 milliseconds (cumulative count 99802)
99.843% <= 2.007 milliseconds (cumulative count 99843)
99.868% <= 2.103 milliseconds (cumulative count 99868)
99.958% <= 3.103 milliseconds (cumulative count 99958)
99.997% <= 4.103 milliseconds (cumulative count 99997)
100.000% <= 5.103 milliseconds (cumulative count 100000)

Summary:
  throughput summary: 101214.58 requests per second
  latency summary: min=0.104 msec, q50=0.303 msec, q95=0.487 msec, q99=0.719 msec, max=4.327 msec, avg=0.327 msec

Let's try to move ahead with merging this - my only "discomfort" is the INFO like latency summary line which sticks like a sore thumb in the overall stdout format. Maybe I'm just too old - WDUT @filipecosta90 (about the format, not my age)?

Bump @madolson - any additional comments before I approve this?

WDUT @filipecosta90 (about the format, not my age)?

@itamarhaber do you think that a multi-line summary would fit best the summary? like the following:

(...)
Summary:
  throughput summary: 101214.58 requests per second
  latency summary: 
    - min: 0.104 msec
    - p50: 0.303 msec
    - p95: 0.487 msec
    - p99: 0.719 msec
    - max: 4.327 msec
    - avg: 0.327 msec

WDYT?

@filipecosta90 @itamarhaber Not that I want to turn this into a huge issue... but to me a good compromise would be columns:

  latency summary (msec):
	min	q50	q95	q99	max	avg
	0.104	0.303	0.487	0.99	4.327	0.327

At least to me it looks UNIX-ly familiar and as a bonus it's easier to parse.

My comment was to enhance readability and parse-ability (for automation) - I'm good with either option (rows or columns).

@filipecosta90 @itamarhaber Not that I want to turn this into a huge issue... but to me a good compromise would be columns:

  latency summary (msec):
	min	q50	q95	q99	max	avg
	0.104	0.303	0.487	0.99	4.327	0.327

At least to me it looks UNIX-ly familiar and as a bonus it's easier to parse.

@filipecosta90 @itamarhaber Not that I want to turn this into a huge issue... but to me a good compromise would be columns:

  latency summary (msec):
	min	q50	q95	q99	max	avg
	0.104	0.303	0.487	0.99	4.327	0.327

At least to me it looks UNIX-ly familiar and as a bonus it's easier to parse.

will push the changes with the column format :)

@yossigo and @itamarhaber added changes per the PR review:

Summary:
  throughput summary: 132802.12 requests per second
  latency summary (msec):
          avg       min       p50       p95       p99       max
        0.195     0.112     0.175     0.343     0.631     3.095