Skip to content
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

rewrk fails to process arguments when used via some external program #30

Closed
ishtms opened this issue Jul 24, 2022 · 1 comment · Fixed by #31
Closed

rewrk fails to process arguments when used via some external program #30

ishtms opened this issue Jul 24, 2022 · 1 comment · Fixed by #31

Comments

@ishtms
Copy link
Contributor

ishtms commented Jul 24, 2022

Hi, I am currently building a framework benchmark tool using rust, and just switched from using wrk to rewrk. Loving it so far as it doesn't has inconsistencies like wrk.

There are some issues with clap when it comes to parsing through another program, for example rust's own std::process::Command utility. I am also submitting a PR to get rid all these issues.

Problem

Here's a demo code that my benchmark program is using (using absolute path just to debug it further)

code

The exact same arguments seem to work okay with wrk, and any other programs. However, because of this clap issue, rewrk expects us to either specify arguments using t=2 threads=2 or t2.

Solution

Threads

// It's trying to parse a `usize` when the value is like ` 3` - with a space, which fails
// clap doesn't removes that space
let threads: usize = match args.value_of("threads").unwrap_or("1").parse()

connections

// Same issue as above
let conns: usize = match args.value_of("connections").unwrap_or("1").parse()

rounds

// Same issue as above
let rounds: usize = args  .value_of("rounds") .unwrap_or("1").parse()

host/url

// again the `uri` fails to parse since it has a space ` http://localhost:3000`
let user_input = UserInput::new(bench_type, uri_string).await?;

There's a quick fix - trimming values and that will do it for all the use cases. I'll be submitting a PR that fixes all these.

@ChillFish8
Copy link
Collaborator

Sounds reasonable to me, probably worth upgrading to clap 3 and using the derived version of the options instead in the future 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants