Allow LoadTestShapes to reuse run-time, spawn-rate and users parameters

[noirbizarre]

Currently, it is not possible for a custom LoadTestShape to reuse the built-in run_time, num_users and spawn_rate parameters, even if they need/use it (they are rather classical load testing parameters).
There must be an undocumented reason somewhere for this choice.

This pull request allows custom LoadTestShape to reuse those command line parameters if explicitly told with the reuse_parameters attribute.

So with this change, the following code will work:

class MyCustomShape(LoadTestShape):

    reuse_parameters = True

    def tick(self):
        expected_run_time = self.runner.environment.parsed_options.run_time
        # Do something with this expected run time
        ...
        return None

This change is tested, documented and backward compatible. The WebUI swarm form behavior has been adapted to it (user_count and spawn_rate fields are not disabled with reuse_parameters = True).

Note: build is failing on github while succeeded in tox locally (for all Python version). I added some output to the test in case of failure to understand why it fails on github.

[cyberw]

I thought I commented on this but cant seem to find it now. I think this makes sense, but can be done without the extra parameter and just make it the default behavior. Maybe just log an info level message about the fact that run-time etc will not be automatically picked up by the shape.

[noirbizarre]

Oh, I chose this path because I thought you wanted to keep the actual behavior but making it the default is even better !
I'll rebase this PR and update it, making it the default behavior 👌🏼

[cyberw]

yea, I kind of changed my mind. hmm... I guess there is no way of having it both ways... (getting a clear error message when e.g. run time was specified but not actually read/respected by load shape, without having to add the new parameter)

Hmm... let me think about it a bit more.

[cyberw]

Hmm. I had forgotten about the web ui, where it becomes really strange to show the parameters by default.

[cyberw]

Yea. I think my first idea was the most appropriate

We had that discussion in an issue, right? Link it from the PR by adding it in the PR description please. So I dont forget stuff :)

"reuse-parameters" is a strange parameter name though. Maybe something else, that relates to shapes..

[noirbizarre]

You might be mistaking me for someone else about the in-PR discussion. But it means there is already a discussion about it (I will try to find it back).

So which way is it ? Default with proper logging or opt-in like it is already ?

For the reuse-parameters, I would take any better proposal. Just use-parameters ?

(In the meantime, PR rebased against the current master)

[cyberw]

Uhm. I completely misread your description and thought you were introducing a command line flag.

[cyberw]

This change looks good (I dont know what happened when I read it the first time, maybe reviewing too many PR:s at once:)

Two minor things:

• The name of the "is_shape" parameter in web.py doesnt make sense any more after the updates this PR introduces. Does it make sense to call it something more like "hide_run_parameters"?
• update the error message "It makes no sense to combine --run-time and LoadShapes. Bailing out." to something that mentions the ability to use --run-time in the shape using reuse_parameters

[cyberw]

--run-time, --users and --spawn-rate dont really have a name as a group, but we could call them "common arguments" (they are called "common options" in the help message, and it differentiates them somewhat from "custom arguments" which is handled differently in the UI and other places).

Perhaps we should use "hide_common_arguments" in web.py and "reuse_common_arguments" as class variable name?

Edit: I do like the word "option" better than "argument" though. How do you feel about hide_common_options / reuse_common_options?

[noirbizarre]

I agree, options is more concise.
I am working on it, I'll choose between hide/reuse when I get the big picture. If opted for reuse for backward compatibility, but keeping those 3 common or base parameters by default and opting-out when not needed make sense to me. Web UI and cli are 2 different cases, I'll see what makes more sense for the 2

(I am on it, PR should be updated soon)

[cyberw]

If opted for reuse for backward compatibility, but keeping those 3 common or base parameters by default and opting-out when not needed make sense to me.

Not 100% sure I understand what you mean here, but looking forward to your updated PR :)

[noirbizarre]

PR updated with:

• chosen wording (use_common_options/hide_common_options)
• better error message with deep link to the doc

[cyberw]

If we're also logging a warning on line 218 in main.py for this case then it doesnt really need to be said twice, right?

[noirbizarre]

I'll keep only one of them and add a link to the documentation too.
Which one do you prefer to keep ? The main one or the runner one (I prefer the main one as it seems more logical to group parameters validation in)?

[cyberw]

Looking really good now. I just made a single comment in the code.

[noirbizarre]

Last question because we have different behavior for --run-time vs --users and --spawn-rate and there is a contradictory statement:

• Ignoring parameters https://github.com/noirbizarre/locust/blob/shape/parameters/locust/main.py#L217-L220
• Then bailing out if headless and runtime specified: https://github.com/noirbizarre/locust/blob/shape/parameters/locust/main.py#L217-L220

I think this is the same pattern and we should have a warning for the 3, choose to bail or ignore but do the same in both case.

Are you OK for:

• log.warning for the 3 (instead of sys.stderr.write)
• ignoring instead of bailing out for the 3
• do it once in early parameter validation (given the WebUI also ignore those parameters if provided with use-common-options)

[noirbizarre]

PR updated with those changes.

Sample log output (with a custom logger I use, but the message is the same):

On terminal supporting hyperlinks, the link to the doc is clickable.

[cyberw]

I agree with the change regarding warning instead of failing. But we should adjust the first two log lines accordingly.

How about merging them into one line:
"Note: --run-time, --users or --spawn-rate have no impact on LoadShapes unless the shape class explicitly reads them. See: https://docs.locust.io/en/stable/custom-load-shape.html#use-common-options"

[noirbizarre]

I splitted them on 2 lines to avoid having a log line spanning on multiple lines but yes, I can group the first 2 log statements into a single. However I would not add the Note: prefix given it's a warning statement and it will be presented as:

WARNING: --run-time, --users or --spawn-rate have no impact on LoadShapes unless the shape class explicitly reads them. See: docs.locust.io/en/stable/custom-load-shape.html#use-common-options

PR updated accordingly

[cyberw]

Excellent!