Stats and UI Support for Multiple Hosts

[andrewbaldwin44]

Fixes Issue #2389

Proposal

• Stat Entries should be grouped by name and host
• Add Host to response from /stats/requests and update the UI accordingly
  • Changes in the UI will only exist in the modern-ui, the old UI will be left unchanged
  • If multiple hosts are present, an option to group tables by "host" may now be present in the "Statistics" and "Failures" tab. If only one host is present, no changes will occur
  • Two new query parameters may now be passed to the HTML report (only from the web UI): group_stats_by and group_failures_by. This will allow the tables in the HTML report to be grouped as specified
  • Two new query parameters may now be passed to the CSV report (only from the web UI): group_stats_by and group_failures_by. This will allow the tables in the CSV report to be grouped as specified
  • All hosts will be shown in the header of the UI
• Strictly speaking there is no change to the CLI output, however you will now see two entries if multiple hosts are being used and both hosts have requests to the same path
• If only one host is active, there will be no change to the UI , HTML, CLI, or CSV output

Multiple Hosts

Single Host

(no change)

[cyberw]

Hmm... This is quite a significant change for a very rare use case.. I'd like to wait until after we rework the UI. The html template files are unreadable enough as it is, and more parametrization makes them even uglier.

[andrewbaldwin44]

I understand the hesitation 👍 I tried to keep the changes as minimal as possible but we need to pass the host in a lot of different places, and consequently update a lot of tests.

I think the changes are quite safe, as the tests pass and I have added further tests for the cases with multiple hosts. We could omit the changes to the template files for the moment, but it probably makes more sense to wait so that we can keep the UI consistent with the downloaded reports

Although this is a very rare use case, I think it is something that we should support, considering that we do support setting multiple hosts, and considering that returning None anytime we have multiple hosts doesn't make much sense

I will focus on the UI updates in that case and then hopefully we can re-visit this

[andrewbaldwin44]

@cyberw I have updated this now so that the changes to the UI will only affect the modern-ui, and once again, no changes will occur unless multiple hosts are present. Now that I have some more flexibility with the UI, I have also proposed using separated tables in the UI and CSV files for better visibility of the data.

Currently the CLI still uses one table, with an additional column for indicating the host. I think in the CLI it becomes a bit too difficult to read if we have several tables per report, but let me know if you prefer this to be changed

[cyberw]

Cool stuff! (I'll remember to squash the changes this time :)

Is this change backwards compatible? I noticed that it isn't compatible between different versions on master-worker, but that is... not terrible.

[andrewbaldwin44]

I am perhaps not familiar enough with the release process, what do you mean that this wouldn't be compatible between different versions on master-worker?

[cyberw]

it changes the protocol between master and worker, so a worker without this change cannot send stats to a master with this change. There is a version check between worker and master when they connect, but usually they just log a warning if they are not the same minor version (only major versions are considered breaking by default)

[cyberw]

https://github.com/locustio/locust/blob/master/locust/runners.py#L1011

[andrewbaldwin44]

Ah interesting, are there really cases where users would want to update the master worker but not the workers themselves?

I think a work-around could be possible, where we ensure the host is defined in log_request or log_error, and then basically decide between storing the entry with or without the host ([(name, method)] vs [(name, method, host)]).

Otherwise we save this for a major version change?

[cyberw]

Its not really a supported use case across minor versions, but people often forget to upgrade one or the other :)

Its completely fine to adjust that code to refuse connections from workers with version <=2.17.0

[andrewbaldwin44]

@cyberw I performed some tests using worker versions that did not have this change, reporting to a master worker that would have this change. I was able to then make this backwards compatible with minimal changes

When no host can be found in the stats, the UI shows as it would for when only a single host is present (tested all tabs and all reports)

Finally, I also realized the "Aggregated" column was not quite placed correctly if we are separating the tables per host. I have proposed showing the aggregated values as their own table and updated the PR description accordingly

[cyberw]

sorry, that was a bug in the example...

[andrewbaldwin44]

My locust file looks like this:

from locust import TaskSet, task


class ExampleTest(TaskSet):
    @task
    def get_products(self):
        self.client.get("/products/example-products")
        self.client.get("/some/example-products")
        self.client.get("/other/example-products")
        # raise ValueError("some exception")



class Example(HttpUser):
    wait_time = between(5, 15)
    tasks = [ExampleTest]
    host = "http://0.0.0.0:10"

class ExampleTwo(HttpUser):
    wait_time = between(5, 15)
    tasks = [ExampleTest]
    # host = "http://0.0.0.0:10"
    host = "http://another-host.com"

[cyberw]

Hmm... I'm not sure this will work. Users frequently access other domains/hosts than the one in self.host, and this change groups them in a very weird way if they do:

from locust import TaskSet, task, HttpUser, constant


class ExampleTest(TaskSet):
    @task
    def get_products(self):
        self.client.get("/stuff")
        self.client.get("http://another-host.com/another_host")


class Example(HttpUser):
    wait_time = constant(1)
    tasks = [ExampleTest]
    host = "http://0.0.0.0:10"


class ExampleTwo(HttpUser):
    wait_time = constant(1)
    tasks = [ExampleTest]
    host = "http://another-host.com"

output:

Type     Host                      Name                                                                          # reqs      # fails |    Avg     Min     Max    Med |   req/s  failures/s
--------|-------------------------|----------------------------------------------------------------------------|-------|-------------|-------|-------|-------|-------|--------|-----------
GET      http://0.0.0.0:10         /another_host                                                                      7   7(100.00%) |      2       2       3      3 |    1.15        1.15
GET      http://another-host.com   /another_host                                                                      6   6(100.00%) |      2       1       3      2 |    0.99        0.99
GET      http://0.0.0.0:10         /stuff                                                                             7   7(100.00%) |      4       2       7      4 |    1.15        1.15
GET      http://another-host.com   /stuff                                                                             6   6(100.00%) |      5       3       6      5 |    0.99        0.99
--------|-------------------------|----------------------------------------------------------------------------|-------|-------------|-------|-------|-------|-------|--------|-----------
                                   Aggregated                                                                        26  26(100.00%) |      3       1       7      3 |    4.28        4.28

[andrewbaldwin44]

@cyberw So what would you prefer as an output in that case? To separate the tables per host as in the UI / CSV? I was thinking this might make it a bit difficult to read but I think it could be possible with minimal changes. Or we could change the sort_stats function so that the stats are ordered by host. Or do you prefer that the host is not shown in the CLI output?

[cyberw]

Just to be clear, the problem isnt the presentation being unclear, the problem is that the output is a lie: no requests have been made to http://0.0.0.0:10/another_host

I guess what we'd have to do is determine what host was actually used instead of relying on the host property. Not sure it is can be done in a reasonable way though.

[andrewbaldwin44]

Ah my mistake, I think that's actually a very easy fix:

# locust/clients.py:147

        parsed_url = urlparse(url)
        actual_host = f"{parsed_url.scheme}://{parsed_url.netloc}"

        # store meta data that is used when reporting the request to locust's statistics
        request_meta = {
            "request_type": method,
            "response_time": response_time,
            "name": name,
            "context": context,
            "response": response,
            "exception": None,
            "start_time": start_time,
            "url": url,
            "host": actual_host,
        }

Output:

Type     Host                      Name                                                                          # reqs      # fails |    Avg     Min     Max    Med |   req/s  failures/s
--------|-------------------------|----------------------------------------------------------------------------|-------|-------------|-------|-------|-------|-------|--------|-----------
GET      http://another-host.com   /another_host                                                                     13  13(100.00%) |      3       1      26      2 |    2.15        2.15
GET      http://0.0.0.0:10         /stuff                                                                             7   7(100.00%) |      2       1       5      2 |    1.16        1.16
GET      http://another-host.com   /stuff                                                                             6   6(100.00%) |      2       1       4      3 |    0.99        0.99
--------|-------------------------|----------------------------------------------------------------------------|-------|-------------|-------|-------|-------|-------|--------|-----------
                                   Aggregated                                                                        26  26(100.00%) |      3       1      26      2 |    4.29        4.29

And because of _build_url on line 131, we are guaranteed an absolute URL and we can follow the same logic in fasthttp.py

[cyberw]

👍 Would it be possible/good to parse this on the "receiving" end instead? I mean in RequestStats.log_request()... that way we wouldnt have to pass host around everywhere (if host is more related to the URL than User.host it kind of makes sense not to calculate it so early)

or maybe in on_request https://github.com/locustio/locust/blob/feature%2F2389/locust/runners.py#L133

[andrewbaldwin44]

@cyberw To me, I think it makes the most sense that the HTTPSession (or FastHttpSession) be the one that interprets the host. The class already has context that absolute urls can exist and everything related to the request is stored and passed via the request_meta. Plus all other URL parsing is being handled here as well (if we handle the parsing of the host elsewhere, I would wonder why we don't do the same with the url path (name).

We also wouldn't be saving ourselves from passing variables by handling this logic in the RequestStats.log_request() and RequestStats.log_error() because neither function receives the URL. The principle of this function is also to simply log the request it is given, and if we are to follow the single responsibility rule, it doesn't make much sense for this function to handle URL parsing. I would argue the same for the on_request of the runner, I'm not sure it makes sense for the runner to need the context that it should parse the request url it is given.

[cyberw]

I agree, having it in the User/client makes sense when I think about it.

What I’m not so sure about is whether it is worth the added complexity in the stats library. And perhaps there is a use case when you want requests against two different hosts to be grouped as one metric? Perhaps a rare use case, but one made impossible by this change I think?

Perhaps it could be done more easily with a switch on the User that changes the name to use the whole URL instead of just the path? That way the name is still all you need to know/control in order to do grouping, and reports are not made extra complicated but your use case is still supported.

[andrewbaldwin44]

To be honest, I'm not even sure I understand the use case where someone would want to both create a request to an absolute url and track the metrics of this request. For example, I can see use cases existing where you need to query some 3rd party to get some data to pass into the next request to your own client. But then the metrics for this 3rd party host wouldn't really be your concern.

I do understand what you mean, however, that we may prefer to group stats by the User, rather than the Host. However I'm not sure how easily this could be done again without adding too much complexity to the stats library. I imagine for this to be done, each StatsEntry would need to track which User the request came from rather than the Host. Changing the name on the User wouldn't really solve the problem, because when the list of Stats gets sent to the frontend, we still have no context as to which Stat belongs to which user

[andrewbaldwin44]

We do receive the user in the request method of the HttpSession, so if we prefer to group by User, it could be done by switching from tracking the host in stats.py 1:1 with tracking the user. Although it won't reduce much complexity to the stats library, I can agree with you that it could make more sense than grouping by host. It would also make the UI a bit cleaner by displaying the User, rather than the hostname, and I think this would also allow us to do away with the has_multiple_hosts.

The one issue is that this would mean we start to group stats by user in the UI, regardless if they are using different hosts or not. Either this or we would have to track by both host and user

[cyberw]

I do understand what you mean, however, that we may prefer to group stats by the User, rather than the Host. However I'm not sure how easily this could be done again without adding too much complexity to the stats library. I imagine for this to be done, each StatsEntry would need to track which User the request came from rather than the Host. Changing the name on the User wouldn't really solve the problem, because when the list of Stats gets sent to the frontend, we still have no context as to which Stat belongs to which user

That's not what I meant, I meant an option to do something like this:

class MyUser(HttpUser):
log_full_url = True
host = "http://somewhere.com"
@task
def t(self):
self.client.get("/foo")

... in order to make the name of the request be http://somewhere.com/foo, instead of just /foo.

[andrewbaldwin44]

You mean log_full_url as opposed to tracking the host in stats.py? Sure it would work but I don't think it's as easy to read as having the stats grouped by host / user, especially when you have a lot of requests

Is there not a use case where it would be useful to allow filtering stats / failures by user?

[cyberw]

Yes, that is what I mean. Here's an example of the problem this introduces (with no real workaround available):

class TestUser(HttpUser):
    host = "http://google.com"

    @task
    def task(self):
        self.client.get("/")
        self.client.get("http://www.this_is_a_long_domain_name.com/somewhere", name="/somewhere")

Old output (slightly confusing, I'll give you that, but at least it can be managed):

With this PR:

[andrewbaldwin44]

Yes but what doesn't work is this:

class ExampleTest(TaskSet):
    @task
    def get_products(self):
        self.client.get("/stuff")
        self.client.get("/another_host")


class Example(FastHttpUser):
    wait_time = constant(1)
    tasks = [ExampleTest]
    host = "http://0.0.0.0:10"


class ExampleTwo(FastHttpUser):
    wait_time = constant(1)
    tasks = [ExampleTest]
    host = "http://another-host.com"

Output:

Type     Name                                                                          # reqs      # fails |    Avg     Min     Max    Med |   req/s  failures/s
--------|----------------------------------------------------------------------------|-------|-------------|-------|-------|-------|-------|--------|-----------
GET      /another_host                                                                      6   6(100.00%) |      0       0       1      0 |    2.84        2.84
GET      /stuff                                                                             6   6(100.00%) |     16       0      97      1 |    2.84        2.84
--------|----------------------------------------------------------------------------|-------|-------------|-------|-------|-------|-------|--------|-----------
         Aggregated                                                                        12  12(100.00%) |      8       0      97      1 |    5.68        5.68

If two hosts hit the same path, they will be recorded as the same stat, but it isn't correct. That's why I suggest to additionally group by host.

I agree the horizontal space is limited, that's why I would suggest multiple tables. If you prefer to have a single table in the CLI or CSV however I could suggest this:

CLI / CSV Output (we simply don't show the host as before):

Type     Name                                                                          # reqs      # fails |    Avg     Min     Max    Med |   req/s  failures/s
--------|----------------------------------------------------------------------------|-------|-------------|-------|-------|-------|-------|--------|-----------
GET      /another_host                                                                      6   6(100.00%) |      0       0       1      0 |    2.84        2.84
GET      /stuff                                                                             6   6(100.00%) |     16       0      97      1 |    2.84        2.84
GET      /another_host                                                                      6   6(100.00%) |      0       0       1      0 |    2.84        2.84
GET      /stuff                                                                             6   6(100.00%) |     16       0      97      1 |    2.84        2.84
--------|----------------------------------------------------------------------------|-------|-------------|-------|-------|-------|-------|--------|-----------
         Aggregated                                                                        12  12(100.00%) |      8       0      97      1 |    5.68        5.68

Then in the web UI we could allow the user to view the table as they wish

[cyberw]

I havent forgotten about this, but I cant really decide whether this is a good idea or not, or put too much time into making an alternative. Rewriting names to be full URLs ("log_full_url") would be a much more low-impact change that doesnt complicate statistics tracking and presentation. So I kind of prefer that.

[andrewbaldwin44]

Yes but in my opinion it doesn't really solve the issues (and as you showed, with a long host name, it doesn't really solve the problem of presentation either). The issue is that we are grouping stats together when they should not be grouped. I think it doesn't make much sense that the user then has to have the knowledge to set this "log_full_url" in order for the stats to be "properly" grouped (just as you showed above, if the user class has a host property but uses an absolute URL, we wouldn't expect the user to have to use "log_full_url" in order for absolute URLs to be tracked correctly right?)

[andrewbaldwin44]

I have a new proposal for this. I think it can simplify things a lot. Let me know what you think:

RequestStats has a Dict entries which is simply { "someUniqueKey": StatsEntry }, or in this specific case { [name, method]: StatsEntry }. Strictly speaking, we actually don't care at all what the key is to the RequestStats, the key gets thrown away during sort_stats (we use the key for sorting but we could also sort without it if we wanted to). So rather than complicating the code and having to change every test in the codebase by adding a third value to the Tuple, we can simply update the name in the Tuple to include the host (e.g. /some-path -> https://some-host/some-path). This is in effect the same idea as your "log_full_url", but without affecting presentation, and without putting the expectation on the user.

From there:

• We can add a property host and/or user to StatsEntry
• We could update the presentation in the CSV and CLI output, or we could also decide not to (if not, the user would just see two entries of /some-path, which, could be clearer but is perhaps still manageable)
• The web UI could then have an updated presentation as was proposed with multiple tables, or again we could decide to leave the presentation as is. We also have a little more flexibility with the presentation in the web UI so we could also provide some display buttons for the user to choose how they wish the data to be displayed

[cyberw]

Hmm... If it simplifies the code then that sounds reasonable!

To me, showing the host name does make sense in the web ui (maybe making it optional, like you showed in the screenshot) and splitting but not showing the host column makes sense in command line (if we want to be really ambitious we can show it if the terminal width exceeds a certain value, or add a setting for it).

[andrewbaldwin44]

Ok so I've simplified the code as best as I can:

• No changes at all to the CLI needed now
• Removed the environment property "has_multiple_hosts" in favor of a request argument "group_by"
• the UI will display as one table by default, with the option to group by host. The CSV and downloaded report will equally be grouped as one table by default, and will only change if the user has chosen to group the tables in the UI

I was thinking there could be a way to make this change without having to change so many tests, but unfortunately it doesn't really work. The idea was to change the way we record the stats, but any changes to recording the stats would equally change the way we look up the stats (and thus we would need to update the same amount of tests).

The nice part about this change is that the "group_by" property is built to be generic, so in the future we could quite easily add more group options if we wished :)

I actually wanted to have the option to group by user, and technically we're already quite close to being able to do it. The _kwargs here represent everything from the request_meta. All we would need to do is pass this down to log_request

def on_request(request_type, name, host, response_time, response_length, exception=None, **_kwargs):
            self.stats.log_request(request_type, name, host, response_time, response_length)

The problem is log_request does not instantiate a StatEntry in a "conventional" way, it relies on the magic __missing__ dunder, which wouldn't have access to all the props from log_request. There could potentially be some ways around this such as with if statements to determine if the entry exists, or defining a variable for the instance and then updating it:

stat_entry_instance = self.entries[(name, method, host)]
for key, value in log_request_kwargs.items():
            if key not in some_valid_keys:
                continue

            setattr(stat_entry_instance, key, value)
stat_entry_instance.log(response_time, content_length)

But to me it doesn't feel super clean and I understand that the decision to use the dunder was performance based and I didn't want to be responsible for reverting the performance increase :) so for now we can group by host and if it's interesting to you perhaps we can look into other grouping options in a future ticket

[github-actions]

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 10 days.

[andrewbaldwin44]

Hey @cyberw, I am still of the opinion that two hosts being recorded as the same stat is a bug and should be fixed.

I understand there is some hesitation with how many tests this change affects, but I am of the opinion that these tests should be updated. Since #2410, the get method is no longer used in Locust, so why keep it and why test it?

Is there anything that could be changed for you to consider this? :)