-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Pass multiple Locustfiles and allow selecting User and Shape class from the WebUI #2137
Conversation
examples/use_as_lib.py
Outdated
@@ -25,7 +26,8 @@ def task_404(self): | |||
env.create_local_runner() | |||
|
|||
# start a WebUI instance | |||
env.create_web_ui("127.0.0.1", 8089) | |||
options = parse_options() # TODO: Confirm this is accurate | |||
env.create_web_ui(options, "127.0.0.1", 8089) |
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.
Cant we make this a non-breaking change by just putting the parameter last and defaulting to use parse_options() inside in create_web_ui() if nothing is provided?
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.
Sounds good to me, I'll give it a shot
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.
After reading your other comment, I realized that my approach here was probably very unnecessary. I'm already passing in show_locustfiles
, which I can use for the same check in /swarm
to see if the locustfile in the payload needs to be loaded.
Nice! Couple comments, and I’ll need to do a full review when not on mobile :) |
Hmm. When I think about it - could this be solved by allowing multiple parameters for -f / —locusfile instead? It seems a more obvious and user friendly approach (if you want a whole directory you would just do |
Ah nice, I think this is a great idea! |
Can you change it so that it works that way? I guess using Also, maybe we could make it so that you select Users in the GUI instead of locustfiles (and default to run all users). That would make a lot of sense, I think. |
Yup! I've already got that working locally, I just need to fix and add tests 😄 However, for the use case of passing in a directory, I add another flag
I think before I push up, however, I'm going to see if I can try to not use the flag by adding a check in
Great idea! I think this would be easier to read in the UI as well. I actually got started on the PR from an old thread I saw in the Issues where someone got that working so some degree, but never opened it up as a PR. I'm busy this week, so I might not have any commits up until Monday (just fyi). |
@cyberw I just pushed up a few commits, but this isn't ready for re-review yet. I still have to update the docs and fix some failing tests (I'm on an M1 and it seems like some tests fail no matter what locally, but are fine in Github Actions. Some due to slowness) |
6faeacf
to
31ef8c7
Compare
I've nothing constructive to add other than to say thank you for working on this pull request. I've been thinking about this feature for about a week so decided to take a look today to see if it had already been discussed .. and found this :) |
Merge with master for a fix for the build (flask made an api change in 2.2) |
31ef8c7
to
0f67c06
Compare
Thanks! I'm glad others have use for this as well 🤘 |
@cyberw This is ready for re-review. I've updated with your requested changes, most notably that the UI allows you to pick from UserClasses instead of Locustfiles (which is a much better so thanks for the suggestion!). I also added in a ShapeClass picker which only accepts one value and defaults to the normal use-case and only appears when the UserClass picker is active. Additionally, there is no need for any additional command-line arguments/flags.
|
locust/main.py
Outdated
|
||
# parse all command line options | ||
options = parse_options() | ||
|
||
if userclass_picker_is_active: |
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.
Is it not possible to make headless work with multiple files?
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.
(Defaulting to run all of them, but also having the ability to specify a subset of Users, as with a single file)
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.
Ah so if --headless
and userclass_picker_is_active==True
, then use all UserClasses in the specified locustfiles? That makes sense! But, I'm just wondering how to handle ShapeClasses in the Locustfiles, specifically if there is more than one?
Defaulting to run all of them, but also having the ability to specify a subset of Users, as with a single file
I think defaulting to running them all is the only option here, as there wouldn't be a way to specify which UserClasses to use since the UI would be disabled. Unless I'm totally missing something ??? (likely since I'm still a Locust noob)
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.
Any trailing arguments are used to select from users (e.g. locust … User1 User2
)
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 there is more than one shape class, then we can just crash
locust/web.py
Outdated
} | ||
|
||
def update_environment(self, user_classes, shape_class_name, parsed_options): |
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.
Is this even needed now? Arent we just loading all the files and only changing Users and shape class at run time?
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.
I'm pretty confident that it is, because if multiple locustfiles are used, then main.py
doesn't initiate the Environment with the UserClasses (see line 72 of main.py
). So update_environment
needs to update the user_classes
and shape_class
properties. The other ones can probably be removed, I forgot to test that out, but will do so now.
Additionally, the application breaks without this line environment.runner._users_dispatcher = None
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 multiple locustfiles are used, then main.py doesn't initiate the Environment with the UserClasses (see line 72 of main.py).
but why cant main.py initiate it? (Now that we’re not selecting which locustfile to run after startup)
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.
That's a good point. I modified main.py
to:
if userclass_picker_is_active:
# iterate through each locustfile and 'load' them
else:
docstring, user_classes, shape_class = load_locustfile(locustfile)
I think the # iterate through each locustfile and 'load' them
block would work for both scenarios. I'll update that.
But i think update_environment
would still be needed to be called when the user selects a UserClass or Swarm class in the UI. So that the environment is updated with the UserClass/ShapeClass selection and also so that the following is called:
environment.runner.stop()
environment.runner._users_dispatcher = None
which causes this user dispatcher to be recreated with the selected UserClasses: https://github.com/locustio/locust/blob/master/locust/runners.py#L491
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.
But i think update_environment would still be needed to be called when the user selects a UserClass or Swarm class in the UI. So that the environment is updated with the UserClass/ShapeClass selection and also so that the following is called
Yes, that makes sense. I’m unsure about having user_picker_is_active as an implied effect of having multiple files specified though? Its a very UI-specific thing but impacts behaviour deep inside main. Maybe have it as a separate setting? Or just always have it on but hide the User selection more in the UI?
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.
I think the other change will resolve half of this comment. main
will essentially only use userclass_picker_is_active
to toggle the UI to show the UserClass & ShapeClass select fields. The other changes I made using that variable in main
won't be necessary.
In terms of having user_picker_is_active
as an implied effect of multiple files, I think the best alternative is to just have it as a CLI Option (which seems like that's what you might be suggesting)
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 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.
@cyberw I've made all the requested updates:
- Added
--enable-userclass-picker
option to explicitly trigger the UI selections. This cleans up the issues you had withuserclass_picker_is_enabled
behavior inmain
. There are no more conditional checks sinceEnvironment
is always createduser_classes
andshape_class
.- Now
options.enable_userclass_picker
is only used inmain
to enable the UI selections (line 262) and to log that the UserClass picker is enabled (line 445)
- Now
- The new workflow is that
/swarm
will update the environment only if the Userclass Picker is enabled AND those values are in the payload. This also lead to removingupdate_environment
for a more simplified approach
There is a breaking change that was implemented with allowing multiple values to be passed into -f/--locustfile
. I only just now caught it. Specifying User classes in the CLI cannot be done after the -f
arguments, since it expects the values to be locustfiles.
Good:
locust -f locustfiles --csv test_csv QuickstartUser1
- `locust QuickstartUser1 -f locustfiles
Bad:
locust -f locustfiles QuickstartUser1
- Error:
Invalid file 'locustfiles'. File should have '.py' extension
- Error:
I didn't update the docs on this yet as I wanted to see if I should keep it as is (maybe at a note in the error message), or perhaps add a flag to specify the User classes.
This is starting to look really good. Anything else before we merge? Can you squash the commits that didnt really relate to the final product? Or I can squash everything into one commit |
Yeah, actually. Incase you didn't see my other comment: Good:
Bad:
I didn't update the docs on this yet as I wanted to see if I should keep it as is (maybe at a note in the error message), or perhaps add a flag to specify the User classes. I'm thinking that the best move is to keep it as is and update the docs & error message, since that will affect the least amount of users. |
Oh, right. Yea we probably dont want to do that. Maybe use a comma separated list? Argparse doesnt seem to support it out of the box so SO recommends something like https://stackoverflow.com/questions/52132076/argparse-action-or-type-for-comma-separated-list |
6d39a50
to
5b2a247
Compare
Done. Excellent suggestion 🙏 . I also squashed all of the commits. |
Minor details:
|
Btw, its kind of a ”user AND shape class” selector, isnt it? Maybe it should be named something different? |
That's how I originally had it, but
You're referring to the UI select box, right? |
Yup! I couldn't think of a better name 😐 . Open to suggestions |
5b2a247
to
fb831f0
Compare
Not sure if it works, but maybe —class-selector or —class-picker? if that doesnt work then maybe —select-class or —pick-class |
…User classes and shape class from WebUi. -f accepts a directory and multiple comma-separated files.
fb831f0
to
fe3ad97
Compare
@cyberw
|
Looks good now! Can you just update the PR description? |
Done 👍 |
Thx! |
My team has a use case where we'd like to be able to pick User classes after spinning up the WebUI. The goal of the PR is to:
The
-f
argument has been updated to allow for multiple locust files or a directory. All User classes in the specified locustfiles will be used by default.The newly added
--class-picker
option enables the user to select the desired User classes & shape class in the Web Ui. If no selection is made for User classes, all User classes are used. The shape class defaults to the regular shape.Other noteworthy changes:
/swarm
will use all available UserClassesload_locustfile
,is_shape_class
,is_user_class
were moved frommain.py
toutil/load_locustfile.py
to avoid a circular import inweb.py
main.py
Example:
Running
locust -f locustfiles --class-picker
with the following file structure: