Closed finnefloyd closed 3 years ago
Thanks for the contribution! I'm going to be offline for ~2 weeks and may not be able to do a thorough review before I return.
I believe this would close https://github.com/tag1consulting/goose/issues/65
Thank you for taking this into consideration. Yes I believe I'll close the issues 65
I ran a performance comparison to ensure no regressions with these changes, and instead measured a 4-5% speedup. Before each test I reload the database and restart all services. The load test is between two VMs on the same host. I performed the following command (running a 10 minute load test 3 times with a short pause between each):
cargo run --release --example drupal_loadtest -- --host http://apache -u10 -v -s5 -t10m && sleep 30 && cargo run --release --example drupal_loadtest -- --host http://apache -u10 -v -s5 -t10m && sleep 30 && cargo run --release --example drupal_loadtest -- --host http://apache -u10 -v -s5 -t10m
This PR increased Goose's overall ability to make and analyze requests with the same load test plan and configuration on average from 8,000 requests per second to 8,344 requests per second, a 4.13% increase.
The high level results can be compared here: https://docs.google.com/spreadsheets/d/1eMC5IgUCUrvKSnraIwxx7h_riYeEHOh9Ei45XlfwPU8/edit?usp=sharing
The full Goose command line output can be viewed here: https://docs.google.com/document/d/1Oc_IbAOVfeEaTzOAZIZGhOnGFh_3eu1FkuAGL2STXOw/edit?usp=sharing
A lot of nice cleanups and simplifications here, and a handy new feature -- thanks! Once we address a few little nits, I plan to merge -- this will bump the Goose version to 0.14 as well as it's an API change.
If you are comfortable adding something to CHANGELOG.txt documenting at a high level what's changed, please do. Otherwise I'll do so in a followup.
If you don't mind waiting a bit longer for the release, there is a few other PR I'd like to submit to you.
If you don't mind waiting a bit longer for the release, there is a few other PR I'd like to submit to you.
Definitely! I'm eager to see whatever else you contribute! And as we're going to be bumping the API version I'll want to review if there are other changes we can/should make at this time.
Definitely! I'm eager to see whatever else you contribute! And as we're going to be bumping the API version I'll want to review if there are other changes we can/should make at this time.
Great, I'll open them once this one is merged otherwise I'll have conflicts with myself.
I'm on the road today, I hope to do a final review and merge tomorrow morning. Thanks again!
I was finally able to sit down and do a full read-through of your PR -- I'm still very happy with it, thanks again! There's some nice cleanup in here, in addition to adding session support.
If you can please rename a few functions for general consistency per my comments, and address a few other final nits, then once addressed this is ready to merge.
Thank you for the review.
I have addressed all your remarks.
Fantastic contribution, thanks!
The goal of this PR is to add the possibility to set session data on each GooseUser running.
This feature is useful for multiple use cases but I'm using it for authenticated users.
Changes made: