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

New API for specifying wait time #1118

Merged
merged 15 commits into from Nov 13, 2019

Conversation

@heyman
Copy link
Member

heyman commented Oct 22, 2019

This is a proposal for changing the API for how the Locust and TaskSet wait time is specified.

Here's an example of the current API:

from locust import Locust
class User(Locust):
    min_wait = 1000
    max_wait = 25000

This would be the equivalent example with the proposed API:

from locust import Locust, between
class User(Locust):
    wait_time = between(1, 25)

I've also created two other wait_time functions besides between:

  • constant - Always wait the specified (constant) time
  • constant_pacing - Try to sleep the amount of time required to make the time between task executions constant (so if a task takes 2 seconds to execute, the wait time will be 2 seconds lower). This is a requested feature in #646.

Some things to note

  • These changes are backwards compatible, so specifying min_wait and max_wait still works in this PR (though currently not wait_function but that could be fixed), so we could add deprecation warnings and keep that functionality for another version or two.
  • The wait time is specified in seconds instead of milliseconds. That's what builtin python functions such as time.sleep expects, and I think it's more common that the wait time is multiple seconds rather than fractions of a second, so I think that change makes sense to do if we're changing the API.
  • I have only updated the most important documentation pages, since I'd like some feedback before spending too much time on updating the docs.
  • I haven't updated all existing tests but if we were to do this change we would have to update the old tests to the new API, and add some new tests that checks the backwards compatibility of min_wait and max_wait.

The reason I started working on this, was because I was working on fixing the bug in #891 and realised that the fix would result in a somewhat messy solution (master...fix-891) that would involve taking a bound method on the Locust instance, and re-binding it on the TaskSet instance. So besides giving us a (IMO) nicer API, it would also help avoid that messy solution :).

What do you think? Is there something I haven't thought of that would break by this?

@codecov

This comment has been minimized.

Copy link

codecov bot commented Oct 22, 2019

Codecov Report

Merging #1118 into master will increase coverage by 0.39%.
The diff coverage is 94.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1118      +/-   ##
==========================================
+ Coverage   75.46%   75.85%   +0.39%     
==========================================
  Files          18       20       +2     
  Lines        1818     1864      +46     
  Branches      276      286      +10     
==========================================
+ Hits         1372     1414      +42     
- Misses        382      384       +2     
- Partials       64       66       +2
Impacted Files Coverage Δ
locust/main.py 35.24% <ø> (ø) ⬆️
locust/exception.py 100% <100%> (ø) ⬆️
locust/wait_time.py 100% <100%> (ø)
locust/core.py 85.23% <100%> (+0.01%) ⬆️
locust/util/deprecation.py 85% <85%> (ø)
locust/runners.py 65.02% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1af7de...bb3ae02. Read the comment docs.

@cyberw

This comment has been minimized.

Copy link
Contributor

cyberw commented Oct 23, 2019

Nice! Personally I favor pacing the total throughput for all running locusts, instead of individually (like I do in the TaskSetRPS class at https://github.com/SvenskaSpel/locust-plugins/blob/master/locust_plugins/tasksets.py), but maybe we could support both?

While we are on the subject: I think we should change the default wait times to zero. A lot of new users get confused and immediately ask "Why is my test so slow, is Locust broken?". This would be a change impacting some old tests, but hopefully not many people will be relying on the default wait times...

@cyberw

This comment has been minimized.

Copy link
Contributor

cyberw commented Oct 23, 2019

Tbh, I think we should only have one mechanism for setting wait times. I'm +1 for at least deprecating (log a warning) if someone is using min_wait / max_wait.

@heyman

This comment has been minimized.

Copy link
Member Author

heyman commented Oct 23, 2019

Personally I favor pacing the total throughput for all running locusts, instead of individually

I can see that there might be use-cases for it, but I don't think it fits very well with the idea of defining User behaviour and then simulating those users (if the wait time of a user is dependent on the number of - and actions of - other users). This PR shouldn't break TaskSetRPS in any way though.

I think we should change the default wait times to zero.

In this PR I've changed so that a MissingWaitTimeError exception is raised if no wait_time is specified (or min_wait + max_wait or wait_function). I considered letting no specified wait_time result in 0, but I think it's better to explicitly raise an exception. For example if someone forget to specify the wait time, I think it's better that they get an error instead of risk not noticing it.

Tbh, I think we should only have one mechanism for setting wait times. I'm +1 for at least deprecating (log a warning) if someone is using min_wait / max_wait.

Yep, that's the goal. But I do think that we should aim for backwards compatibility with deprecation warnings (for awhile) if possible.

@heyman

This comment has been minimized.

Copy link
Member Author

heyman commented Oct 23, 2019

I've now also fixed backwards compatibility for wait_function, and added DeprecationWarnings when any of min_wait/max_wait or wait_function is used.

I've also updated all the tests and added som new tests for the old API.

This PR should actually be mergeable pretty soon.

@heyman heyman force-pushed the wait_time branch from 1570d1e to af92687 Oct 24, 2019
heyman added 8 commits Oct 22, 2019
Add deprecation warnings for usage of min_wait, max_wait, or wait_function.
Updated tests to use new wait API.
Added tests that tests the old wait API.
… warnings.simplefilter(“always”) from tests (should hopefully fix the failing Python 3.7 tests on Travic CI)
@heyman heyman force-pushed the wait_time branch from 36d6341 to 4095119 Oct 24, 2019
@heyman heyman removed the proposal label Oct 25, 2019
@HeyHugo

This comment has been minimized.

Copy link
Contributor

HeyHugo commented Oct 25, 2019

Sorry if this is coming too late, but an alternative api could be to try using python primitives instead, that way you could skip importing any extra functions:

# Instead of constant(1) interpret any numeric type as constant
wait_time = 1

# Instead of between(1, 10) interpret tuples as between
wait_time = (1, 10)

# Instead of constant_pacing(5) interpret a function taking first positional arg as time spent on last request
wait_time = lambda t: 5 - t

Edit:
Ah the constant_pacing() function makes sense. Didn't look at it at first but now I see you store the timestamp on the instance each run. That's nice to have a prepared function for.

@heyman

This comment has been minimized.

Copy link
Member Author

heyman commented Oct 26, 2019

Sorry if this is coming too late, but an alternative api could be to try using python primitives instead, that way you could skip importing any extra functions

I think the proposed API is more clear, and less "magical", and it also allow users to define their own wait_time functions (that have access to the Locust instance). The third of your examples feels very unintuitive to me.

@heyman

This comment has been minimized.

Copy link
Member Author

heyman commented Oct 26, 2019

We could possibly change so that just specifying a number as wait_time would result in constant() and specifying a tuple would result in between(), though I'm not sure I think that it's worth it (making the code harder to understand to save an import).

@cyberw

This comment has been minimized.

Copy link
Contributor

cyberw commented Oct 30, 2019

Shouldnt the wait functions be in their own package? Maybe, also maybe "between" as a distribution description is a little vague? It could be "uniform" instead...

@cyberw

This comment has been minimized.

Copy link
Contributor

cyberw commented Oct 30, 2019

I still think we could use zero as the default time (it is the default in every other load testing tool I've used :)

The main reason I think this is important is to lower the threshold for people just getting started with locust.

@heyman

This comment has been minimized.

Copy link
Member Author

heyman commented Oct 30, 2019

Maybe, also maybe "between" as a distribution description is a little vague? It could be "uniform" instead...

I agree that uniform would perhaps be a more scientifically "correct" naming. However I think between reads better ("Wait time between x, y"), and I find it unlikely that anyone would assume any other distribution than uniform for a wait function named between.

I still think we could use zero as the default time (it is the default in every other load testing tool I've used :)

This PR currently raises as MissingWaitTimeError if wait time is missing. I could see us defaulting to zero instead. However, I think it's quite rare that one actually want to have 0 as the wait_time which makes it a strange default. Many other load tests are just about cramming out as many reqs/s as possible, while Locust's main advantage is the ability to create realistic load by defining user behaviour in code.

@heyman

This comment has been minimized.

Copy link
Member Author

heyman commented Oct 30, 2019

Shouldnt the wait functions be in their own package?

Do you mean a totally separate python package? I think it make sense to include them (at least between and constant) in Locust.

They are located in a separate module (locust.wait_time) within Locust and I was really going back and fourth between importing them into the main locust namespace. The python programmer in me wanted people to have to import them from locust.wait_time but at the same time I realise that Locust has some users who are not super familiar with python code, and figured it was worth to pollute the locust.* namespace with some default wait_time functions, in order to save people from having to do an additional import.

@cyberw

This comment has been minimized.

Copy link
Contributor

cyberw commented Oct 30, 2019

Shouldnt the wait functions be in their own package?

Do you mean a totally separate python package? I think it make sense to include them (at least between and constant) in Locust.

They are located in a separate module (locust.wait_time) within Locust and I was really going back and fourth between importing them into the main locust namespace. The python programmer in me wanted people to have to import them from locust.wait_time but at the same time I realise that Locust has some users who are not super familiar with python code, and figured it was worth to pollute the locust.* namespace with some default wait_time functions, in order to save people from having to do an additional import.

Yea, I meant separate module/namespace, not package, sorry. Sure, another import is not so nice, but finding the available wait_time functions would be easier if they were in a separate namespace. the main locust namespace is kind of polluted as is.

@cyberw

This comment has been minimized.

Copy link
Contributor

cyberw commented Oct 30, 2019

Maybe, also maybe "between" as a distribution description is a little vague? It could be "uniform" instead...

I agree that uniform would perhaps be a more scientifically "correct" naming. However I think between reads better ("Wait time between x, y"), and I find it unlikely that anyone would assume any other distribution than uniform for a wait function named between.

I still think we could use zero as the default time (it is the default in every other load testing tool I've used :)

This PR currently raises as MissingWaitTimeError if wait time is missing. I could see us defaulting to zero instead. However, I think it's quite rare that one actually want to have 0 as the wait_time which makes it a strange default. Many other load tests are just about cramming out as many reqs/s as possible, while Locust's main advantage is the ability to create realistic load by defining user behaviour in code.

"between" is fine in isolation, but looks weird next to "constant" and "constant_pacing". I guess it is a matter of taste.

In my experience having zero wait times is not rare or strange.

@heyman

This comment has been minimized.

Copy link
Member Author

heyman commented Oct 30, 2019

Sure, another import is not so nice, but finding the available wait_time functions would be easier if they were in a separate namespace. the main locust namespace is kind of polluted as is.

It's still possible to import the wait_time functions from locust.wait_time. I don't feel strongly about this one, but I think I'm leaning more towards including it, for convenience.

In my experience having zero wait times is not rare or strange.

I don't feel strongly about making wait_time mandatory VS having a default of zero, either. If you feel like it's much better, I'd be fine with changing it.

Does anyone else want to chime in?

@skivis

This comment has been minimized.

Copy link
Contributor

skivis commented Oct 30, 2019

Regarding the wait_time default zero vs. mandatory, I think either way is fine.
Might be worth mentioning though that defaulting to zero might surprise some locust users that up until now have been relying on the current 1 second default when not defining min_wait & max_wait (don't know how common that is though...).
Edit:
I just realized that the "concern" above was already mentioned by @cyberw in his first comment, my bad.

That being said, I guess defaulting to zero seems a tad more natural.

As for the importing from what namespace, I'd say leave it as is now, for convenience :)

@cyberw

This comment has been minimized.

Copy link
Contributor

cyberw commented Oct 31, 2019

Does this remove the need for #1005 ?

@heyman

This comment has been minimized.

Copy link
Member Author

heyman commented Oct 31, 2019

Does this remove the need for #1005 ?

Yes, it does.

@heyman heyman merged commit 795b5a1 into master Nov 13, 2019
4 checks passed
4 checks passed
codecov/patch 94.33% of diff hit (target 75.46%)
Details
codecov/project 75.85% (+0.39%) compared to b1af7de
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@cyberw cyberw mentioned this pull request Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.