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

Introduce SimpleTaskSet, a way to make super simple locust files without having to define a Locust class #1274

Closed
wants to merge 9 commits into from

Conversation

cyberw
Copy link
Collaborator

@cyberw cyberw commented Mar 2, 2020

This allows you to define a locust plan like this:

from locust import SimpleTaskSet

class MyTask(SimpleTaskSet):
    @task
    def task1(self):
        self.client.get("/1")

I havent updated documentation yet, but I think this can be the default in the documentation, because it is 100 times less fuss & unnecessary settings for a new user :)

You could argue that SimpleTaskSet should default to FastHttpLocust instead of HttpLocust, if that is "the future"

I spent a lot of time implementing this but in the end the change is so small, I almost feel bad :)

This relates to #1264

@cyberw cyberw changed the title Introduce @simple_task, a way that allows super simple locust files Introduce @simple_task, a way to make super simple locust files Mar 2, 2020
@codecov
Copy link

codecov bot commented Mar 2, 2020

Codecov Report

Merging #1274 into master will increase coverage by 0.30%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1274      +/-   ##
==========================================
+ Coverage   77.52%   77.82%   +0.30%     
==========================================
  Files          20       20              
  Lines        1984     2002      +18     
  Branches      313      316       +3     
==========================================
+ Hits         1538     1558      +20     
+ Misses        368      364       -4     
- Partials       78       80       +2     
Impacted Files Coverage Δ
locust/runners.py 72.62% <0.00%> (ø) ⬆️
locust/core.py 94.14% <0.00%> (+2.05%) ⬆️

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 ed3b263...b90cfa3. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 2, 2020

Codecov Report

Merging #1274 into master will decrease coverage by 0.11%.
The diff coverage is 42.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1274      +/-   ##
==========================================
- Coverage   80.16%   80.04%   -0.12%     
==========================================
  Files          23       23              
  Lines        2067     2080      +13     
  Branches      319      323       +4     
==========================================
+ Hits         1657     1665       +8     
- Misses        332      336       +4     
- Partials       78       79       +1
Impacted Files Coverage Δ
locust/main.py 23.49% <20%> (-0.1%) ⬇️
locust/core.py 89.91% <55.55%> (-1.34%) ⬇️
locust/runners.py 77.83% <0%> (ø) ⬆️
locust/rpc/zmqrpc.py 92.1% <0%> (+7.89%) ⬆️

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 40c7558...5a97470. Read the comment docs.

@cyberw cyberw requested a review from heyman March 2, 2020 12:14
@heyman
Copy link
Member

heyman commented Mar 2, 2020

Interesting! I like that this can be done with pretty few changes. Here are some initial thoughts:

  • Couldn't this be done without changing main.py if one just imports the SimpleHttpLocust in the locustfile.py?

  • Maybe the code could live in a separate module (something like locust.contrib.simple) (I think we should also move out HttpLocust from core.py, but that's another issue)?

  • Here's an alternative API we should consider:

    from locust.contrib.simple import SimpleHttpLocust
    
    @SimpleHttpLocust.task
    def my_task(t):
        pass

    that would also make this possible

    from locust.contrib.simple import SimpleHttpLocust, constant
    
    SimpleHttpLocust.wait_time = constant(3)
    
    @SimpleHttpLocust.task
    def my_task(t):
      pass

I think this can be the default in the documentation, because it is 100 times less fuss & unnecessary settings for a new user :)

I'm not sure if I think it should be the default in the docs. Locust's power does not come from very small Apache Bench-like load tests, but rather from the ability to create realistic load by simulating real user behaviour. I think that this simplified approach is worse at communicating that. Especially since there's an implicit wait_time of 0. The typical real Locust test isn't 10, 20 or even a 100 lines of code. Therefore I'd say that the current "overhead" is almost negligible, and I think it can be negative if the typical real use-case is very different from the ones presented in examples to new users.

@cyberw
Copy link
Collaborator Author

cyberw commented Mar 2, 2020

Interesting! I like that this can be done with pretty few changes. Here are some initial thoughts:

  • Couldn't this be done without changing main.py if one just imports the SimpleHttpLocust in the locustfile.py?

Yes, that sounds like a very good plan, I will do that!

  • Maybe the code could live in a separate module (something like locust.contrib.simple) (I think we should also move out HttpLocust from core.py, but that's another issue)?
  • Here's an alternative API we should consider:
    from locust.contrib.simple import SimpleHttpLocust
    
    @SimpleHttpLocust.task
    def my_task(t):
        pass

Sure, but if I manage to convince you (see below) that it should be the first introduction in the documentation I think it should be part of locust module.

I think this can be the default in the documentation, because it is 100 times less fuss & unnecessary settings for a new user :)

I'm not sure if I think it should be the default in the docs. Locust's power does not come from very small Apache Bench-like load tests, but rather from the ability to create realistic load by simulating real user behaviour. I think that this simplified approach is worse at communicating that. Especially since there's an implicit wait_time of 0. The typical real Locust test isn't 10, 20 or even a 100 lines of code. Therefore I'd say that the current "overhead" is almost negligible, and I think it can be negative if the typical real use-case is very different from the ones presented in examples to new users.

I think most people start by making a very small tests and then expands from there (either because they thought the AB-like test would be enough and then change their minds, or just because that is a natural first step in developing a scenario).

I think Locust needs to be documented primarily with the beginner in mind (because most of our users are not professional load testers), but allow for growth. I think the documentation can start out with simple_task but move on to the more complex cases later on. The good thing about the simple_task implementation is that it allows for this growth, without creating special cases.

In my experience test plans start out small but can grow pretty big (although I'd say I average around 100 loc, and I still have never used nested tasksets and only rarely more than one Locust class in the same test :) But of course, the benefit of simple_task is not decreasing the lines of code but decreasing the complexity and cognitive load on the new locust user.

@heyman
Copy link
Member

heyman commented Mar 2, 2020

Hmm, another alternative (which I believe you've suggested before), would be to allow task declarations directly on the Locust user class:

class User(Locust):
    @task
    def t1(self):
        pass

What I like about that approach is that it is similar to the default way of declaring tasks, compared to introducing a new approach (with a Locust.task classmethod that can be used as a decorator to create tasks).

@cyberw
Copy link
Collaborator Author

cyberw commented Mar 2, 2020

Hmm, another alternative (which I believe you've suggested before), would be to allow task declarations directly on the Locust user class:

class User(Locust):
    @task
    def t1(self):
        pass

What I like about that approach is that it feels more similar to the default way of declaring tasks, compared to introducing an additional approach (with a Locust.task classmethod that can be used as a decorator to create tasks).

Hmm.. but I thought you didnt like that? :) I guess that could work, but I think it would require more changes under the hood. But I guess it could be done. It would have to be a different decorator than the usual @task though. This one would have to check if there is a TaskSet defined on the locust, create if it isnt there and then add itself there.

But what would the self reference point to? The task set instance? That would be very weird (because you are defining it on the locust) The locust? That would be ok, but very different from the normal behaviour. And could be hard to implement.

Interesting! I like that this can be done with pretty few changes. Here are some initial thoughts:
Couldn't this be done without changing main.py if one just imports the SimpleHttpLocust in the locustfile.py?

Yes, that sounds like a very good plan, I will do that!

Hmm... I tried that but couldnt get it to work. The tasks need to be defined before calling TaskSet.__new__ due to the magic meta-class stuff. I can of course do it using a different decorator (like @simple_task), but that is almost as magical as doing it on module level (also, people might get confused on when to call @simple_task and when to call @task).

@cyberw
Copy link
Collaborator Author

cyberw commented Mar 2, 2020

Interesting! I like that this can be done with pretty few changes. Here are some initial thoughts:
Couldn't this be done without changing main.py if one just imports the SimpleHttpLocust in the locustfile.py?

Yes, that sounds like a very good plan, I will do that!

Hmm... I tried that but couldnt get it to work. The tasks need to be defined before calling TaskSet.__new__ due to the magic meta-class stuff. I can of course do it using a different decorator (like @simple_task), but that is almost as magical as doing it on module level (also, people might get confused on when to call @simple_task and when to call @task).

I guess I would be ok with this though, if you think it is better, and worthy of making it the "default" :) (we can always just throw a descriptive exception if someone mixes the two)

@heyman
Copy link
Member

heyman commented Mar 2, 2020

Hmm.. but I thought you didnt like that?

I'm not sure that I do, though I still might prefer it to introducing an additional approach for declaring tasks. The more I think about it, the more I feel skeptical about having two different ways of declaring tasks, and allowing @task on the Locust class would make the hello world example simpler, while still diverging less from the default way of declaring tasks.

But what would the self reference point to? The task set instance? That would be very weird (because you are defining it on the locust) The locust? That would be ok, but very different from the normal behaviour. And could be hard to implement.

Yes, that's all things that we would need to consider, and it's very much part of the reasons why I'm not sure if it's a good idea.

I think that the only logical thing would be to have self point to the Locust instance. I'm not sure how much it would complicate the implementation.

But I guess it could be done. It would have to be a different decorator than the usual @task though.

I think that it should be possible to do it with the same decorator.

…et added to SimpleTaskSet.tasks, which is a little weird. But I havent found a way to know inside the decorator whether we are adding tasks to SimpleTaskSet or some other TaskSet
@cyberw
Copy link
Collaborator Author

cyberw commented Mar 3, 2020

hmm... that last commit broke stuff... will try to fix it...

…ly subtask SimpleTaskSet, and it will automatically set SimpleHttpLocust to be the locust class, and set SimpleHttpLocust.task_set to use the correct TaskSet.
@cyberw cyberw changed the title Introduce @simple_task, a way to make super simple locust files Introduce SimpleTaskSet, a way to make super simple locust files Mar 3, 2020
@cyberw
Copy link
Collaborator Author

cyberw commented Mar 3, 2020

So I'm quite happy with the change now. What could make it even easier for the user is if we just made this the default (so that it happens even when subclassing TaskSet, instead of SimpleTaskSet)

If we had just made constant(0) the default sleep time when it was introduced it would have been one less hacky thing to do in main.py :)

@cyberw cyberw changed the title Introduce SimpleTaskSet, a way to make super simple locust files Introduce SimpleTaskSet, a way to make super simple locust files without having to define a Locust class Mar 5, 2020
@cyberw
Copy link
Collaborator Author

cyberw commented Mar 10, 2020

I think this would be a great simplification for new users of locust and I want it in as part of 1.0 (I'll update the docs too of course)

Maybe @max-rocket-internet or @timran1 want to weigh in?

@cyberw
Copy link
Collaborator Author

cyberw commented Mar 21, 2020

@heyman Pretty please with sugar on top? :P I really think this change will do a lot of good for Locust's usability (for myself personally it doesnt matter much, I'm fine with having a couple of lines of extra code, and I know what it is used for, but I dont want to have to keep explaining to new users why they need to define a Locust class that in most basic cases doesnt really do anything)

@cyberw
Copy link
Collaborator Author

cyberw commented Apr 1, 2020

Closing this in favour of #1304

@cyberw cyberw closed this Apr 1, 2020
@cyberw cyberw deleted the introduce-simple_task branch November 2, 2020 12:25
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.

2 participants