-
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
Run locust programmatically #805
Conversation
CI will pass after #804 has been merged. |
Codecov Report
@@ Coverage Diff @@
## master #805 +/- ##
==========================================
+ Coverage 66.55% 69.01% +2.46%
==========================================
Files 14 14
Lines 1438 1488 +50
Branches 226 236 +10
==========================================
+ Hits 957 1027 +70
+ Misses 430 394 -36
- Partials 51 67 +16
Continue to review full report at Codecov.
|
Hi, could someone tell me what is delaying this PR or give a detailed breakdown of exactly what I need to do to get this merged? @heyman |
So I merged this into my own branch, and I think there's a few things that need to be changed before this should be merged. This PR does a great job in providing an alternative interface to launching locust, but I think there is still some work to be done in accessing the results programatically. Inovkust shows a way of allowing access to the results in process rather than having that be controlled through an option in the command line. I'll look into it more to see if there's a good way to combine the benefits of |
@vamega Ok let me know how you get on and if you need any help. Keen to have this functionality merged. |
This allows programmatic usage to collect stats from objects after the locust run has completed.
Would love to see this get merged! @ps-george would you mind fixing the conflicts and contact the project owner(s) to have it merged? |
@joaonc Me too, conflicts resolved. It's an important but also quite big change, I'm not sure the project owners have the time to properly review. I'm sure if we get some thumbs up and support for this PR, they will be more willing to look into it and help us merge this. |
|
||
|
||
def create_options( | ||
locustfile='locustfile', |
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 wonder if there's a more reader friendly way to do this - or if the general use case will be ok with a massive list of kwargs here.
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.
Unsure, open to any suggestions.
Essentially, with the CLI tool, you're basically doing this same thing - loads of default arguments but the ability to change all of them via CLI arguments. But have put defaults in so unless the user who's calling this code only needs to put in the ones they're changing.
@@ -210,7 +210,7 @@ def parse_options(): | |||
action='store_true', | |||
dest='only_summary', | |||
default=False, | |||
help='Only print the summary stats' | |||
help="Only print the summary stats" |
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.
super nitpicky but why this change?
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.
Looking back, I have no idea. I think it was to be consist with some code elsewhere.
max_wait = 5000 | ||
task_set = UserTasks | ||
|
||
options = create_options(locust_classes = [WebsiteUser]) |
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'd add a comment here saying that all command line options are also available as kwargs to this function
print("Locust %s" % (version,)) | ||
version_text = "Locust %s" % (version,) | ||
print(version_text) | ||
if not cli_mode: |
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 trying to think of a valid use case for this type of function call from a non-cli mode - is there a reason you'd ever intentionally hit this codepath programmatically?
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 way I was finding use from this was being able to import locust
in Python and being able to interact with all the statistics directly on the class, rather than reading from the CLI interface.
.. _running-locust-programmatically: | ||
|
||
=============================== | ||
Running Locust programmatically |
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.
Nit: you could link to the example here
This looks pretty good to me, hopefully a few other folks can chime in - most of my comments are pretty minor and documentation related. I think this is a great add to Locust. There's still likely opportunities to ensure that the results can be reported meaningfully but this is a good start (I think you could just access them directly at |
list_commands=False): | ||
"""Create options objects for passing to `run_locust` when running Locust programmatically. | ||
|
||
Keyword Arguments: |
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 like how all of the options are listed here, but could the most important ones be isolated out and set as kwargs in run_locust call with appropriate defaults. Other options could be grouped by their functionality like no-web options, distributed options, etc..
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.
Good comment; can probably apply some functional programming concepts here to reduce the complexity.
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.
Ok, will group these up with comments explaining the grouping.
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.
Looks good, I want the options to be grouped by functionality so each parameters purpose is more explicit. Functionality groupings could look something like:
Undistributed options,
distributed options,
no-web options,
etc.
One other thing to note, if locust is run pragmatically by the multiprocessing module the response times are not accurate (they are largely inflated) - when you run them in the subprocess module the response times appear to be accurate - if this could be noted explicitly that would help avoid a lot of confusion. |
@SpencerPinegar RE response times, you think issue #930 would take care of it? |
While I cannot say definitively, I think that this fix works great for all clients using http requests (and it seems it would be the most accurate way to benchmark) but this wont work for non http-clients so using it as the way to monitor all response times isn't the options (unless I am missing something - kinda hope I am because it seems accurate). That being said for most http clients it would probably fix the multiprocessing issue. |
@ps-george Hi! sorry for the lack of "official" response on this PR. @heyman what is your opinion on this? The diff is reasonably small at least... |
I think it's a good idea to be able to run Locust programatically (and be able to use it more as a a lib). I'm thinking that the best way to do this would be to expose a larger part of Locust's internal code and classes (runners, stats, etc.) as part of the public API. However, this would definitely require some refactoring/re-work of some of Locust's code. Both to make sure we expose a good API, and also because once we add something to the public API, we want to try our best to not do breaking changes. I've been thinking about this quite a bit, but haven't taken the time to concretize the ideas into a Github issue yet. For this reason, I'm a bit hesitant to merge this pull request. I understand that this PR aims to introduce this feature with the least obtrusive changes as possible. That is usually a great goal for PRs to have, but in this case I think it might be better to take a step back and do some re-factoring of the existing code instead. |
@heyman Ok to close/decline? |
Fixes #222, fewer changes than #799