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

Specify host in web ui #523

Merged
merged 22 commits into from
Nov 7, 2019
Merged

Conversation

PayscaleNateW
Copy link
Contributor

Add support for specifying the hostname to test in the web ui, as opposed to in the command line.

@PayscaleNateW
Copy link
Contributor Author

Hmm getting closer - now just python 2.6 is failing

@PayscaleNateW
Copy link
Contributor Author

Having a lot of trouble debugging that 2.6 issue... I get different issues in each environment I try. So far have run vagrant up to try to use the virtual environment, in which case running export TOXENV=py26; tox gets me:

making hard links in locustio-0.8a2...
hard linking MANIFEST.in -> locustio-0.8a2
error: Operation not permitted

ERROR: FAIL could not package project - v = InvocationError('/usr/bin/python /vagrant/setup.py sdist --formats=zip --dist-dir /vagrant/.tox/dist (see /vagrant/.tox/log/tox-0.log)', 1)

Also, on my mac, I get:

'import site' failed; use -v for traceback
Traceback (most recent call last):
  File "/usr/local/lib/python3.5/site-packages/virtualenv.py", line 16, in <module>
    import base64
  File "/usr/local/lib/python2.6/base64.py", line 9, in <module>
    import struct
  File "/usr/local/lib/python2.6/struct.py", line 1, in <module>
    from _struct import *
ImportError: dlopen(/usr/local/lib/python2.6/lib-dynload/_struct.so, 2): Symbol not found: _PyExc_DeprecationWarning

@cgoldberg
Copy link
Member

there is an open PR to drop 2.6 support

@cgoldberg
Copy link
Member

@PayscaleNateW don't worry about Python 2.6. I just merged a PR that drops support for it.
(make sure to rebase your branch and fix any conflicts)

@PayscaleNateW
Copy link
Contributor Author

Awesome, thanks so much @cgoldberg !

@PayscaleNateW
Copy link
Contributor Author

Now tests are actually passing; conflicts resolved. Hopefully the pr looks ok

@sgerhardt
Copy link

@PayscaleNateW I pulled your changes, and I'm wondering, does it make sense to default in any values for the host text box? Maybe by parsing the locust file, and/or by reading a --host flag?

For example, it's now an extra step to run basic.py in the examples. It used to default to the value in the file, but now we'd have to explicitly enter it into the text box.

Otherwise, I like the idea of making this configurable from the command line.

@PayscaleNateW
Copy link
Contributor Author

Thanks for checking it out! Good call on including the default value. Just added a basic default. In terms of getting it from the locustfile, what would be the recommended way? Is it to set runners.locust_runner.host to locust_classes[0].host or something like that? That feels a bit off to me - what if you have multiple classes with different hosts? Then that would override the other classes' hosts when it ran. I know a similar problem emerges for displaying the host at the top in the ui. Seems like storing the host on individual classes creates some difficulties.

@sgerhardt
Copy link

Fair point, trying to parse the files wouldn't work. I think "Set host input default to command line value" is a good enough improvement.

@PayscaleNateW
Copy link
Contributor Author

Cool. Maybe I should remove the host attribute from the examples?

@paravz
Copy link

paravz commented Nov 14, 2017

Any help needed for this feature to merge? This feature would help install master+slaves once and POST different target hosts via web ui. Thanks @PayscaleNateW

@PayscaleNateW
Copy link
Contributor Author

Hi @paravz - definitely open to help! I sort of lost track of this PR and looks like it now conflicts. So I don't know how it would play with the code now. But definitely feel free to pick it up from here.

@mbeacom
Copy link
Member

mbeacom commented Nov 15, 2017

@PayscaleNateW Conflicts resolved. What other changes are outstanding here?

@cgoldberg
Copy link
Member

ALTERNATIVE_HOST = 'http://localhost'
SWARM_DATA_WITH_HOST = {'locust_count': 5, 'hatch_rate': 5, 'host': ALTERNATIVE_HOST}
SWARM_DATA_WITH_NO_HOST = {'locust_count': 5, 'hatch_rate': 5}

these might be better declared within the tests since the data isn't shared globally

@Acanthostega
Copy link

Hi,
it seems great. Nobody wants to merge it ?

@mbeacom
Copy link
Member

mbeacom commented Apr 18, 2019

@Acanthostega If you'd like to resolve the conflicts and turn around the requested changes/feedback, we're definitely open to reviewing the PR.

@cyberw
Copy link
Collaborator

cyberw commented Oct 20, 2019

@Acanthostega I'm so sorry that this PR has not gotten the attention it deserves. If you just resolve the conflicts I will test it out and merge it.

@cyberw
Copy link
Collaborator

cyberw commented Oct 31, 2019

@heyman I would like to merge this, but I cant seem to fix the conflicts directly on GH (I think I can do it by adding a commit to master with one line to remove that diff completely).

@codecov
Copy link

codecov bot commented Oct 31, 2019

Codecov Report

Merging #523 into master will increase coverage by 0.74%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #523      +/-   ##
=========================================
+ Coverage   74.76%   75.5%   +0.74%     
=========================================
  Files          18      18              
  Lines        1807    1817      +10     
  Branches      271     276       +5     
=========================================
+ Hits         1351    1372      +21     
+ Misses        389     382       -7     
+ Partials       67      63       -4
Impacted Files Coverage Δ
locust/web.py 88.37% <100%> (+6.4%) ⬆️
locust/runners.py 65.02% <0%> (+0.54%) ⬆️
locust/stats.py 82.67% <0%> (+0.62%) ⬆️

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 bcaff5c...c8a4acb. Read the comment docs.

@heyman
Copy link
Member

heyman commented Oct 31, 2019

I've merged master into this branch and resolved the conflicts. However the web UI needs to be updated before we can merge it. I also think it should have the label Target Host (optional) and be the third input box instead of first (since it's optional).

This is what it looks like currently:

image

@cyberw
Copy link
Collaborator

cyberw commented Nov 7, 2019

I can fix the UI ordering once it is merged. Having this really old PR open is annoying to me :P

@heyman Is it ok if I make number of users to simulate and hatch rate default to 1 at the same time? I dont use the web ui much, but having to fill out that number just to be able to run is annoying to me :)

@heyman
Copy link
Member

heyman commented Nov 7, 2019

@cyberw You can merge it into a separate branch and make the changes there. Or make the changes directly to this PR (the PayscaleNateW:specify-host-in-web-ui branch).

The rendering bug (see the upper left corner below the logo in the screenshot from my previous message) needs to be fixed as well. It 's probably caused by changes to the DOM/CSS that has been made in master after this PR was created.

Is it ok if I make number of users to simulate and hatch rate default to 1 at the same time?

I think it should be much more common that one wants to spawn >1 users than exactly 1 user, so I'd rather leave them empty I think.

@cyberw
Copy link
Collaborator

cyberw commented Nov 7, 2019

I'll try to make the changes directly to the PR.

I can make it default to 1 and auto select the value when you click the field so there is no downside, even if you want to run >1 users...

@cyberw
Copy link
Collaborator

cyberw commented Nov 7, 2019

Hmm.. actually, if there needs to be fixes to the DOM/CSS then I'm probably not the right person to fix it :)

@heyman
Copy link
Member

heyman commented Nov 7, 2019

Hmm.. actually, if there needs to be fixes to the DOM/CSS then I'm probably not the right person to fix it :)

I can take a look at it!

@heyman
Copy link
Member

heyman commented Nov 7, 2019

I've fixed the issues, but while doing so I found another issue that we need to fix before merging (looking back in the thread, I can also see that @PayscaleNateW has already mentioned this issue):

When running a test with two (or more) different Locust classes, with different host attributes specified (an example could be a WebUser class that would use http://example.com as host and a ApiUser class that would use http://api.example.com as host), both classes' host attribute would get overridden by the value specified in the form.

I think the solution to this would be to only pre-fill the value picked up from the Locust classes if all of them have the same host attribute, while also adding some kind of caveat note that specifying a host through the Web UI will override the host for all locust classes.

@heyman
Copy link
Member

heyman commented Nov 7, 2019

I think this should be mergeable now.

@cyberw
Copy link
Collaborator

cyberw commented Nov 7, 2019

LGTM & tested it

@cyberw cyberw merged commit 95016a1 into locustio:master Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants