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

Max concurrent RPS is mis-reported as RPS value #200

Closed
mud2monarch opened this issue Nov 17, 2024 · 1 comment
Closed

Max concurrent RPS is mis-reported as RPS value #200

mud2monarch opened this issue Nov 17, 2024 · 1 comment
Labels
bug Something isn't working

Comments

@mud2monarch
Copy link
Contributor

Version
0.3.2-11-gc3a2f1d

Platform
M1 MBP on Sonoma 14.6.1
output of uname -a: Darwin CORN-Zach-913.local 23.6.0 Darwin Kernel Version 23.6.0: Mon Jul 29 21:14:30 PDT 2024; root:xnu-10063.141.2~1/RELEASE_ARM64_T6000 arm64

Description
It appears that a user's max concurrent requests setting is mis-assigned to the user's requests-per-second setting.

I tried this code:
Image
Image

I expected max concurrent requests to be set to 10 in both cases, but it was set to whatever value I passed to --requests-per-second.

I believe the error code is in source::parse_source().

It currently reads

max_concurrent_requests: args.requests_per_second.map(|x| x as u64)

but should be changed to

max_concurrent_requests: args.max_concurrent_requests.map(|x| x as u64)

I'll open a PR. Should I add tests? This is my first time contributing to anything so appreciate guidance.

@mud2monarch mud2monarch added the bug Something isn't working label Nov 17, 2024
@mud2monarch mud2monarch changed the title Max concurrent RPS is mis-assigned to RPS Max concurrent RPS is mis-reported as RPS value Nov 17, 2024
@mud2monarch
Copy link
Contributor Author

Edit: now see that it's just the CLI output that is mis-assigned? Concurrency is properly set in lines 50-57.

mud2monarch added a commit to mud2monarch/cryo that referenced this issue Nov 17, 2024
sslivkoff added a commit that referenced this issue Nov 24, 2024
* Ensure output value for max_concurrent_requests is reported accurately

Fixes issue #200

* small correction. removing map since it's already an Option<u64>

* Added test for PR.

* Update source.rs

* Update Cargo.toml

* Update Cargo.lock

---------

Co-authored-by: Zach Wong <[email protected]>
Co-authored-by: sslivkoff <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants