valkey-benchmark: fix non-interactive session output#2801
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #2801 +/- ##
============================================
+ Coverage 72.30% 72.44% +0.14%
============================================
Files 128 128
Lines 70211 70253 +42
============================================
+ Hits 50763 50898 +135
+ Misses 19448 19355 -93
🚀 New features to boost your workflow:
|
| hdr_mean(config.current_sec_latency_histogram) / 1000.0f, hdr_mean(config.latency_histogram) / 1000.0f); | ||
| config.last_printed_bytes = printed_bytes; | ||
| } else { | ||
| printf("%s: rps=%.1f (overall: %.1f) avg_msec=%.3f (overall: %.3f)\n", config.title, instantaneous_rps, rps, |
There was a problem hiding this comment.
I have not forgotten this PR. :) The code changes are small, but I wanted to test it locally to see the behaviour. I've done that now.
So instead of overwriting the progress indicator, we print it many times in multiple lines. This is what I get locally when piping the output to cat. We get many lines like this:
$ ./src/valkey-benchmark -n 1000000 -t get -q | cat
GET: rps=0.0 (overall: nan) avg_msec=nan (overall: nan)
GET: rps=284031.9 (overall: 284031.9) avg_msec=0.121 (overall: 0.121)
GET: rps=192940.0 (overall: 238576.9) avg_msec=0.162 (overall: 0.137)
GET: rps=295292.0 (overall: 257456.7) avg_msec=0.122 (overall: 0.131)
GET: rps=244816.0 (overall: 254299.7) avg_msec=0.130 (overall: 0.131)
GET: rps=288032.0 (overall: 261040.8) avg_msec=0.119 (overall: 0.128)
GET: rps=269064.0 (overall: 262377.1) avg_msec=0.127 (overall: 0.128)
GET: rps=295576.0 (overall: 267117.1) avg_msec=0.124 (overall: 0.127)
GET: rps=227880.5 (overall: 262197.8) avg_msec=0.150 (overall: 0.130)
GET: rps=226408.0 (overall: 258224.7) avg_msec=0.142 (overall: 0.131)
GET: rps=256704.0 (overall: 258072.7) avg_msec=0.131 (overall: 0.131)
GET: rps=294444.0 (overall: 261376.8) avg_msec=0.120 (overall: 0.130)
GET: rps=266188.0 (overall: 261777.5) avg_msec=0.133 (overall: 0.130)
GET: rps=162352.0 (overall: 254134.1) avg_msec=0.185 (overall: 0.133)
GET: rps=220988.0 (overall: 251767.8) avg_msec=0.150 (overall: 0.134)
GET: rps=236768.9 (overall: 250764.7) avg_msec=0.141 (overall: 0.134)
GET: 252844.50 requests per second, p50=0.127 msec
I don't think this progress indicator is meaningful in non-interactive mode. I prefer that we don't print it at all. Maybe I'm missing something. Do you have some use case for a non-interactive progress indicator that I'm not aware of?
There was a problem hiding this comment.
Anecdotally, I personally use it in scripts to get time series data from valkey-benchmark. I pasted some graphs below for fun. ;) What you describe though is kind of what --csv does. Do you feel there's a need for different behavior?
Above I noticed that throughput was jumping in an odd way over time when there were io threads due to context switching. (top half is time series, bottom half is histogram)
Above I was testing with and without sleep states and variable/boost frequency.
Above I was testing different cpu pinning strategies.
There was a problem hiding this comment.
Wow! Beautiful, especially the text version! So you are parsing this output...
What you describe though is kind of what --csv does. Do you feel there's a need for different behavior?
My hypothetical use case is that automated benchmarks can be collected with the same outout as when running interactivly, for example to be posted on github by a bot or so. It's just hypothetical though. I didn't see the time series use case coming.
For machine consumption, wouldn't it be nice to get time series in CSV format? Or not---I suppose the text version is just as easy to parse using a regex so no real need to invent a new format.
Isn't it best to just keep the old tty-unaware code and just let people handle the \r in the output, like you did?
There was a problem hiding this comment.
Yeah, I'm happy enough to leave it. It's not difficult to programatically parse the \r, and AIs and humans seem to be able to deal with it, and there's always --csv. I think it could be convenient (but not essential) to add a --json parameter.
I'll cancel the PR then. :)
Signed-off-by: Rain Valentine <rsg000@gmail.com>
1f4cb21 to
9e10c35
Compare
This fixes output in valkey-benchmark for non-interactive sessions to avoid \r character. This issue was noticed by @zuiderkwast in #2581