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

Make zero wait time the default #1626

Merged
merged 1 commit into from Nov 12, 2020
Merged

Conversation

cyberw
Copy link
Collaborator

@cyberw cyberw commented Nov 11, 2020

Make wait_time = constant(0) the default instead of forcing our users to specify it on every user class.

This makes a big difference in readability of small User classes and reduces the cognitive overhead for locust beginners. Fixes #1308

… to specify it. Makes a big difference in readability of small User classes and reduces the cognitive overhead for locust beginners. Fixes #1308
@codecov
Copy link

codecov bot commented Nov 11, 2020

Codecov Report

Merging #1626 (7676b53) into master (1ce7e9f) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1626      +/-   ##
==========================================
+ Coverage   82.49%   82.58%   +0.08%     
==========================================
  Files          28       28              
  Lines        2571     2572       +1     
  Branches      392      392              
==========================================
+ Hits         2121     2124       +3     
+ Misses        355      352       -3     
- Partials       95       96       +1     
Impacted Files Coverage Δ
locust/user/users.py 95.77% <100.00%> (+0.06%) ⬆️
locust/user/task.py 96.27% <0.00%> (ø)
locust/runners.py 85.96% <0.00%> (+0.38%) ⬆️

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 1ce7e9f...7676b53. Read the comment docs.


class QuickstartUser(HttpUser):
wait_time = between(1, 2)
Copy link
Member

@heyman heyman Nov 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removal of a wait time for this example doesn't really make sense (a typical web user - which I think is the most common usecase - doesn't have zero wait time between requests). It can also make users new to Locust DoS-attack themselves (either their web server, or the Locust client itself if the HTTP-server can keep up) if they simply run the example (case in point #1629).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont really know what is the issue in that ticket, but it seems he got 100% cpu even when specifying a wait time according to his latest post...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, the #1629 seems to be an unrelated issue, but my point still stands. Even if you don't agree with my statement about it making more sense with a wait time, don't you see an issue with having an example that'll hit 100% CPU and trigger the Locust CPU warning when started?


The class defines a ``wait_time`` that will make the simulated users wait between 1 and 2 seconds after each task (see below)
is executed. For more info see :ref:`wait-time`.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait time for Locust users its a really important concept when using Locust as originally intended (defining user behaviour in code, and then choosing how many simultaneous users using a website to simulate), and I definitely think it should exist in the quickstart documentation.

There are definitely use-cases for zero or super low wait time as well, but I don't think that's a reason to remove this short paragraph from the introduction docs.

Copy link
Collaborator Author

@cyberw cyberw Nov 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am aware of your arguments, and you are of mine so I wont repeat myself. The community voted in favour of changing it in #1308

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The community voted in favour of changing it in #1308

"The community" did not vote wether to include wait time in the quick start documentation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think having community votes to decide these things is a bad idea, and it's nothing that we've decided to have.

Out of curiosity - and since I didn't recognize the usernames of most of those who had "voted" - I checked the number of commits they had done to the Locust project. Among those who had voted 👍 , besides you, only one other user (though two were hidden behind "2 more" by the Github UI) had a single commit which was a typo fix. Among the 4 who had voted 👎 , besides me one had multiple (although old) commits and one had a single small fix commit.

Having "community votes" to decide design disagreements is a really bad idea in my opinion, and definitely not something that we've decided to have.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And out of the people who have taken their time to actually comment on the issue with arguments for the issue in question, none of them were arguing for defaulting the wait time to zero.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You haven't really been around for so long, so I did not expect you to care, and when I saw the large support for the issue I went ahead and implemented it.

I dont agree that the votes supporting your point of view are worth 3 times the ones supporting mine. You interpret the lack of comments as a lack of support, when it is just as likely that they just found that my argumentation was reasonable and they had nothing more to add. Would it have been better if I had phrased myself less convincingly so that other people would fill in the gaps, thus adding to the comment count?

I agree that community votes to decide design disagreements is not good, but as a last resort I think it is better than nothing when the two people with the biggest "claim" on the project cannot agree (and tbh, our energy is better spent almost anywhere else :)

If we are going to continue discussing this, can I suggest we appoint some kind of neutral arbiter? Someone we both think is a reasonable person (maybe some contributor that has not yet weighed in on this issue), and whose decision we can both accept.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You haven't really been around for so long, so I did not expect you to care

Well I do, and I have actively participated in arguing the issue in #1308.

I dont agree that the votes supporting your point of view are worth 3 times the ones supporting mine.

Like I said, I think having random votes decide design decisions is a really bad idea. I only commented on the vote because you brought it up. If I was to draw any conclusion from #1308, it would be that the only two people caring much about the issue is you and I.

You interpret the lack of comments as a lack of support, when it is just as likely that they just found that my argumentation was reasonable and they had nothing more to add

I really don't know how to interpret a bunch of thumbs up from random users, on a comment that only stated the arguments from one side of the issue. I would have liked for some counter-arguments to the arguments brought up by me. I think that would have been much more constructive.

and tbh, our energy is better spent almost anywhere else :)

I agree.

If we are going to continue discussing this, can I suggest we appoint some kind of neutral arbiter? Someone we both think is a reasonable person (maybe some contributor that has not yet weighed in on this issue), and whose decision we can both accept.

I think it would be great to hear the opinion of more contributors who haven't weighed in. Ideally, one of us could be convinced of the other's perspective, though I realize that might be hoping on too much :).

In the meantime, would you be okay with at least including wait_time in the Quickstart documentation? I think it's a very important aspect of Locust, and I'm convinced it makes the documentation better. I get that you don't think so, but maybe you could live with a small piece of "unnecessary" info in the introduction docs?

Copy link
Collaborator Author

@cyberw cyberw Nov 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really don't know how to interpret a bunch of thumbs up from random users, on a comment that only stated the arguments from one side of the issue. I would have liked for some counter-arguments to the arguments brought up by me. I think that would have been much more constructive.

Maybe all users did not read and understand the implications of the issue, but I think it is more likely than not that that people did read the ticket, as well as the followup reasoning by you and me. At least that is how I interpret the the 6 upvotes for my followup argument & clarification (and Anuj's counter-argument that got 3 upvotes).

In the meantime, would you be okay with at least including wait_time in the Quickstart documentation? I think it's a very important aspect of Locust, and I'm convinced it makes the documentation better. I get that you don't think so, but maybe you could live with a small piece of "unnecessary" info in the introduction docs?

I think the simplicity of beginner documentation is one of the key reasons for not forcing people to be explicit about wait times. Particularly I feel that forcing new users to learn & think about wait times before thinking about tasks is bad for useability/pedagogy (remember that I wanted to move wait_time to the end of the class in the quickstart example? you did have a good point about keeping attributes before functions though...)

But if you feel this strongly about it then yes, for the sake of moving on, we can put it back into quickstart. Maybe one of us will turn around completely later on, who knows :)

Sorry if come across as a stubborn idiot :)


class MyUser(User):
wait_time = constant(1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, using zero wait time in examples - especially if they're printing stuff - gives a horrible user experience for new users who copy & paste examples while familiarizing with Locust.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I tried to leave it in on those examples but missed this one. Fixed now in 81d9707

@HeyHugo
Copy link
Member

HeyHugo commented Nov 17, 2020

Hi!

I think this is a strange default. I've used locust for a couple of projects and it has mostly been with a wait time set to simulate real user load. I can see that there are use cases for this, yet it feels more like a special case rather than the norm. I think we should optimize the examples towards new users of locust.

wait_time = constant(0)

Having an enforced wait_time makes the API explicit and the intention of the locust file clear. I don't think that the extra verbosity would scare away people more than the lack of information about waiting/throttle.

Apart from the docs confusion I think we should expect that people will not always think about what service they target or how many users they try with. Having a decent wait time in examples will prevent some unintended DOS.

I did not voice my opinion on this before because it didn't seem like you agreed and I thought it wasn't going to be merged :)

Maybe there should be a dedicated example locust file for load testing without wait. Also the MissingWaitTimeError could suggest setting the wait_time to constant(0) if no throttling is intended.

If you don't like the extra import for constant one possibility could be to set wait_time to a NOT_SET value in the base User class for the warning check and make None equate to constant(0). But I still think it should be explicitly set: wait_time = None. It feels logical that if you explicitly set it to None it would not wait.

@cyberw
Copy link
Collaborator Author

cyberw commented Nov 17, 2020

Removing the need for an extra import makes it a little bit better, but kind of increases the complexity, and most importantly doesnt help with the "first time user" impact.

Just reintroducing it to the quick start example as Jonathan suggested is better I think. Imho, it is still bad from a pedagogy stand point, and I still argue all the things I've said before are true - it is the obvious expected default behaviour, it is this way in almost every other tool (literally, I can't think of any other tool that forces you to specify a wait time), etc - but I can live with having it in one example (even if it is the most important one)

We have other default behaviours that are much more arbitrary (ramp up 1 user/s in headless, no connection timeouts, no stop timeout, ignore proxy settings, etc)

@heyman
Copy link
Member

heyman commented Nov 18, 2020

it is the obvious expected default behaviour, it is this way in almost every other tool (literally, I can't think of any other tool that forces you to specify a wait time), etc - but I can live with having it in one example (even if it is the most important one)

In this first section, there's a large risk that I just repeat stuff I've already said to you in chat @cyberw - sorry if that's the case, but it might as well live on github as well:

I have no problem what so ever doing some things different than what other tools do - I'd argue that's what have made Locust successful in the first place. When we created Locust one of the main novel ideas was to generate realistic load by defining the behaviour of typical users. We intentionally did not take a thread or RPS-centric approach, but rather a PSU or user-centric approach. Having zero wait time definitely makes sense for tools that don't have a user centric approach.

I think that's maybe the core of our disagreement. Whether the default Locust use-case is to have a wait time > 0.

We have other default behaviours that are much more arbitrary (ramp up 1 user/s in headless, no connection timeouts, no stop timeout, ignore proxy settings, etc)

All of those defaults are things that people would expect to be able to change because they are familiar concepts. The approach that Locust takes to simulate user behaviour is something that most other tools don't, and therefore wait time might not be a familiar concept, which is the exact reason why I think it's important to show it in an introduction to Locust (and why I keep arguing for having it mandatory).

If you don't like the extra import for constant one possibility could be to set wait_time to a NOT_SET value in the base User class for the warning check and make None equate to constant(0). But I still think it should be explicitly set: wait_time = None. It feels logical that if you explicitly set it to None it would not wait.

If we were to change it, I think that's a good idea.

@heyman
Copy link
Member

heyman commented Nov 18, 2020

If we are to keep it, we could add an info message to the log. Something like this:

INFO/locust.main: No wait_time specified for WebUser, defaulting to constant(0)

@cyberw
Copy link
Collaborator Author

cyberw commented Nov 18, 2020

I think that's maybe the core of our disagreement. Whether the default Locust use-case is to have a wait time > 0.

Agreed. And as a precursor to that, whether locust's main strength is being a general purpose, programmable/hackable load testing tool or if its strength is in having a different mental model than other tools.

@heyman
Copy link
Member

heyman commented Nov 18, 2020

And as a precursor to that, whether locust's main strength is being a general purpose, programmable/hackable load testing tool or if its strength is in having a different mental model than other tools.

Of course I also think it's great that Locust is a general purpose, programmable/hackable load testing tool. Whether wait_time is mandatory or not won't change that.

Borrowing some of your wording, maybe this could be a boiled down, neutral description of the disagreement: Whether it's a strength or weakness to have a different default mental model than other tools

@cyberw
Copy link
Collaborator Author

cyberw commented Nov 18, 2020

And as a precursor to that, whether locust's main strength is being a general purpose, programmable/hackable load testing tool or if its strength is in having a different mental model than other tools.

Of course I also think it's great that Locust is a general purpose, programmable/hackable load testing tool. Whether wait_time is mandatory or not won't change that.

Borrowing some of your wording, maybe this could be a boiled down, neutral description of the disagreement: Whether it's a strength or weakness to have a different default mental model than other tools

Yes. Or at least kind of. Whether it is a strength or weakness to have a different mental model, and that mental model is significantly weakened/obscured by having a default wait time of zero.

@cyberw cyberw deleted the make-wait_time-default-to-zero branch January 19, 2021 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make wait_time default to zero (vote up/down for this ticket please :)
3 participants