-
Notifications
You must be signed in to change notification settings - Fork 35
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
Progress bar for benchmark tests #196
Conversation
@jeffbean, thanks for your PR! By analyzing the history of the files in this pull request, we identified @prashantv, @billf and @kriskowal to be potential reviewers. |
benchmark.go
Outdated
@@ -100,6 +104,7 @@ func runBenchmark(out output, allOpts Options, m benchmarkMethod) { | |||
if opts.RPS > 0 && opts.MaxDuration > 0 { | |||
// The RPS * duration in seconds may cap opts.MaxRequests. | |||
rpsMax := int(float64(opts.RPS) * opts.MaxDuration.Seconds()) | |||
// FIXME: if we are using small time like 500ms the calculation is still 0 |
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.
hmm why is that? Seconds
is a float, so it should be a positive integer
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.
if RPS and MaxDuration are both low enough, the product of multiplying them together will be a fraction less than 1
. e.g. 10 * time.Millisecond.Seconds() = 0.01
and the cast will truncate int(0.01) = 0
benchmark.go
Outdated
@@ -132,6 +137,9 @@ func runBenchmark(out output, allOpts Options, m benchmarkMethod) { | |||
states[i] = newBenchmarkState(statter) | |||
} | |||
|
|||
progressBar := pb.StartNew(opts.MaxRequests) |
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.
There's 2 main types of benchmarks:
- by total number of requests
- by total duration
we're only taking total number of requests into account but we should also think about duration.
we could progress bars for both if both are specified?
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.
Thanks for this. Found a race testing this with Duration. After playing around with the library I forked it and might try to upstream more fixes. Seems small enough and active that this can wait for the upstream to take my fix or we can use my fork and improve the library for our needs as well.
@@ -16,6 +16,7 @@ import: | |||
version: ^1 | |||
- package: github.com/uber/jaeger-client-go | |||
version: ^1 | |||
- package: gopkg.in/cheggaaa/pb.v1 |
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.
the github repo supports semver, maybe better to go directly with that and ^1
benchmark.go
Outdated
@@ -100,6 +104,7 @@ func runBenchmark(out output, allOpts Options, m benchmarkMethod) { | |||
if opts.RPS > 0 && opts.MaxDuration > 0 { | |||
// The RPS * duration in seconds may cap opts.MaxRequests. | |||
rpsMax := int(float64(opts.RPS) * opts.MaxDuration.Seconds()) | |||
// FIXME: if we are using small time like 500ms the calculation is still 0 |
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.
if RPS and MaxDuration are both low enough, the product of multiplying them together will be a fraction less than 1
. e.g. 10 * time.Millisecond.Seconds() = 0.01
and the cast will truncate int(0.01) = 0
…ing asser tin tests
…m data race from the maintainer
@jeffbean what is the status on this? |
This lib keeps introducing data races. i will come back to this when I fix more upstream issues. |
Fixes #57
Prints a Progress bar to the console based on request count. Using library that does atomic increments. Other notes is it uses the terminal width and we are not setting it manually.
Output:
yab moe --health --rps 100 -d 10s
Test without finishing total requests
yab moe --health --rps 20000 -d 10s