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

Locust Improvements of various sorts #278

Closed
wants to merge 35 commits into from

Conversation

SandyChapman
Copy link

We've been developing a fork of Locust for the last couple months and have added various features. Some highlights:

  • HTTPS compatibility via SSL_KEY and SSL_CERT environment variables to specify the SSL/TLS cert and signing key. Useful for securing web communication. There's potential for PII to passed the web interface and the monitoring user (via endpoints being listed in the HTML tables).
  • Added web auth on site via LOCUST_USERNAME and LOCUST_PASSWORD environment variables.
  • Unique IDs assigned to locusts. Each locust of a given type will be assigned a unique id. This can enable differentiating behaviour between other locusts (i.e. querying credentials from a file based on the locust id).
  • Added a --run-time flag to support headless testing (such as via CI build system) where a predefined run-time can be specified.
  • Added a --slave-count flag to support headless testing in distributed mode. Once slave_count slaves connect, distributed locusts will begin hatching. Again, this is useful for CI build system testing.
  • Some bug fixes:
    -- adding **kwargs to some event handlers.
    -- don't reset api stats on hatching complete - reset on new test instead.
    -- num_clients flag now actually used.

Please offer and suggestions / comments / improvements / criticisms.

Sandy Chapman and others added 20 commits March 25, 2015 12:16
…nique id.

Not sure how this will affect changing the user count after a test has started.
This can be done by specifying both the --slave-count and the --run-time flags to set test start and end conditions.
@SandyChapman
Copy link
Author

We'll need to address this test failures before merging. It's on our todo list.

@SandyChapman
Copy link
Author

@HeyHugo : We're looking to see if this set of changes is desired by the locust team. I realize there's a lot here to review, but we'd like to contribute our changes back upstream. Let us know how to proceed with this? I.e. is it needed to be broken up into smaller chunks? Are there changes / features you'd like excluded from the PR?

@HeyHugo
Copy link
Member

HeyHugo commented May 19, 2015

Cool, thanks for the contributions! However let me redirect the questions to @heyman who is more active in this project.

A note on resetting the stats on hatch complete - I think this is the desired behavior, otherwise the statistics data will be mixed with both higher and lower load.

@SandyChapman
Copy link
Author

@HeyHugo Thanks for the info. I thought resetting on hatch would be your desired behaviour. For us, what was happening was that we were slowly ramping up our load via a low hatch_rate. When we reached the user count, the stats were reset causing that data to be removed. We actually wanted the mixed low and high load data as we considered that data valuable. Notably, we were doing authentication calls as the first task and by waiting until all locusts were hatched, we were throwing out all the authentication endpoint tracking.

I think other users may want the mixed load results to be maintained as well, so maybe this could be set as a flag given to the locust command?

@heyman Any thoughts on this?

@heyman
Copy link
Member

heyman commented May 19, 2015

Hey!

Sorry for not replying to this earlier. I've been very busy, and simply haven't had the time to review the whole thing.

Overall, I'm very positive to the changes, though I think it might be better, if possible, to separate a few of these features into separate issues/pull requests.

Here are a few thoughts so far:

HTTPS compatibility via SSL_KEY and SSL_CERT environment variables to specify the SSL/TLS cert and signing key

Sounds great! Should be noted in the documentation.

Added web auth on site via LOCUST_USERNAME and LOCUST_PASSWORD environment variables.

My main issue here would be that during all development of Locust so far, no way of interfacing with it has been intended to be "public", and the way to secure Locust has been to simply restrict access to the machine/ports altogether (e.g. by using firewall rules). Therefore, providing an authentication feature for the web UI, might give users the impression that it's safe to expose Locust to the public, which is currently not the case for the master/slave communication ports. This could perhaps be solved by a clear warning in the docs. We would also need tests that covers this.

Unique IDs assigned to locusts. Each locust of a given type will be assigned a unique id. This can enable differentiating behaviour between other locusts (i.e. querying credentials from a file based on the locust id).

I like the feature, but have only glanced over the implementation. It's not obvious to me how it currently works. How does it handle when parts of the user instances are killed, and when new are spawned? Some tests covering this feature would be nice, and perhaps a few more comments in the code around the LocustIdTracker.

Added a --run-time flag to support headless testing (such as via CI build system) where a predefined run-time can be specified.

Great! This is something I've intended to implement.

Added a --slave-count flag to support headless testing in distributed mode. Once slave_count slaves connect, distributed locusts will begin hatching. Again, this is useful for CI build system testing.

This is also great!

I think other users may want the mixed load results to be maintained as well, so maybe this could be set as a flag given to the locust command?

Yes, I think the best would be to have this configurable. There's an open issue for it here: #205

Some other things:

Perhaps it would be useful to be able to specify a directory path for where the CSV files go. The presence of such path could then control if files should be created or not. Also, maybe it would be easier to - instead of incrementing numbers - just use the current epoch time in the filenames?

What's the use for send_relay_msg? I couldn't see that it was actually used anywhere (thought I might have missed something). Do you use it for some kind of communication between the slave nodes from within your test scripts?

I'm not very fond of whitespace changes, since it clutters diffs. Also - even though I know PEP8 says differently - I prefer to keep the indentation level for empty lines within blocks. My main reason is that one can then copy and paste functions and methods into the console if needed.

Anyway, these are my initial thougths. I think it looks very promising. Thanks!

@SandyChapman
Copy link
Author

@heyman Thanks for the great feedback. We'll look to address your comments over the next week or so as time permits. I think I agree on pretty much every point you made. The send_relay_msg is for what you suspect, node-to-node communication. We actually dropped the use of this feature in favour for running a thread concurrent to (and owned) by an existing locust (and thus I'll likely remove the change from this PR since they're even untested on our end). You may have noticed the new die method passed from the runner down to the locust and ultimately to the taskset. This die method is where we actually clean up this thread since it can exist independently of the locust (and thus be orphaned) once created.

I'll come up with some documentation / examples that will hopefully answer your questions shortly.

Thanks again!

@SandyChapman
Copy link
Author

@heyman : I've broken up this commit into a bunch of smaller pieces and will close this PR. Please review the individual commits when you get a chance. I'll address any additional issues you raise.

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.

5 participants