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

Rename "request" to "sample", and refactor request_success/failure into one event #1724

Closed
cyberw opened this issue Mar 9, 2021 · 15 comments
Labels
feature request stale Issue had no activity. Might still be worth fixing, but dont expect someone else to fix it

Comments

@cyberw
Copy link
Collaborator

cyberw commented Mar 9, 2021

The word request makes no sense for a lot of non-http protocols. Users could be reporting things like the reception of async WebSocket messages, or the time to run a script. The word sample is more appropriate.

Having separate events for request_success and request_failure make no sense either, so I suggest we deprecate those (but support them for a long time) and just use a single one, sample(with a boolean parameter success).

Perhaps this should be done at the same time #1413

@aek
Copy link
Contributor

aek commented Mar 9, 2021

I don't like sample because for me sounds like something more related to perf tracing issues like with Jaeger
https://www.jaegertracing.io/docs/1.22/sampling/

I will prefer to use metric like more related to Prometheus
https://prometheus.io/docs/concepts/data_model/

@cyberw
Copy link
Collaborator Author

cyberw commented Mar 9, 2021

Hmm... I dont like metric because the word is often used for things not related to requests (like cpu usage, memory, etc).

Sample is used by JMeter so it is definitely not a very unique choice, but I guess it is not as obvious as I thought...

@aek
Copy link
Contributor

aek commented Mar 9, 2021

Yeah, exactly. I don't see why should be only related to requests. it's a metric, and stat entry value. With the specific meaning for the specific case, not precisely just for requests. Locust could be use to run and track more than just requests/response patterns

@cyberw
Copy link
Collaborator Author

cyberw commented Mar 9, 2021

I dont agree. In Locust, requests have at least a couple of unifying characteristics: pass/fail and response time.

Resource metrics like CPU utilization is something else entirely: they cannot logically be aggregated/averaged the same way requests/samples can, so it makes little sense forcing them into the same box.

While some sample types may not have a true response time, most will. It can be an obvious one when doing things like like running a script, or a calculated one, when doing things receiving a message (achieved by diffing the timestamp of an event creation against the current time).

If we were to implement "non-sample metrics" in locust some time in the future, it would have to be thru some other mechanism, separate from requests/samples.

@aek
Copy link
Contributor

aek commented Mar 10, 2021

A metric could represent anything. It's a matter of flexibility and how you consume the metrics/values.
I see a lot of manual code definition in Locust source code in regards of Stats usage without a proper/solid codebase API definition ready for extensions and integrations. This request stats scenario in one example that could be improved using a metric system that could allow it to manage any metric type.
I also like the idea to have prometheus and grafana as a sustitution of some components of Locust, like the locust exporter available for prometheus and the corresponding dashboard for grafana
You could see an example of this kind of integrations natively living in others opensource softwares like traefik
https://doc.traefik.io/traefik/observability/metrics/prometheus/
https://grafana.com/grafana/dashboards?category=webservers&dataSource=prometheus&search=traefik

@cyberw
Copy link
Collaborator Author

cyberw commented Mar 10, 2021

A metric could represent anything <- Exactly. This is why we should NOT call it a metric.

What you are suggesting is a major rewrite. Which might make sense, but what I'm talking about here is just renaming a couple of things.

As for Grafana, I too love it for visualiization - so much that I have abandoned the Locust GUI, and made this instead https://grafana.com/grafana/dashboards/10878 :) (on a side note, I use Timescale and log every request, because I've always felt that you need every request for analysis later on - particularly error messages etc)

@heyman
Copy link
Member

heyman commented Mar 10, 2021

I see your points, but I'm also afraid that choosing a more generic term would make it more abstract/unclear for most users.

The most common use-case for Locust is still HTTP (I'm guessing >90%), and for the other, non-HTTP cases I'm also guessing (please disagree if you think they my guesses are unreasonable) that most of them are "request-response-like" (i.e. a client/user triggers some action that generates load on the target system, and gets a response). Therefore, I think I prefer using a terminology that fits really well with the large majority (>98%) of use-cases, rather than switching to a more generic one.

@cyberw
Copy link
Collaborator Author

cyberw commented Mar 10, 2021

👍 You may be right. And I do have a vague recollection of feeling that "sample" was a bit overly scientific/abstract when I first started using Jmeter (about a hundred years ago)

What are your feelings about the other part, merging request_success/failure into a single event?

@cyberw
Copy link
Collaborator Author

cyberw commented Mar 10, 2021

Most of my event listeners tend to look like this:

    events.request_success.add_listener(self.request_success)
    events.request_failure.add_listener(self.request_failure)

...

def request_success(self, request_type, name, response_time, response_length, **_kwargs):
    self._log_request(request_type, name, response_time, response_length, True, None)

def request_failure(self, request_type, name, response_time, response_length, exception, **_kwargs):
    self._log_request(request_type, name, response_time, response_length, False, exception)

def _log_request(self, request_type, name, response_time, response_length, success, exception):
     # actual event handler

@heyman
Copy link
Member

heyman commented Mar 10, 2021

What are your feelings about the other part, merging request_success/failure into a single event?

I don't have a strong opinion about it. But if I were to choose today, I think I'd prefer a single event. So as long as we kept the old ones for backwards compatibility for a long time like you said, I think it's a good idea to change it.

@dchimeno
Copy link

In the case of join both request_success/failure, I would also think is a good idea to pass the entire request/response object, there are so many cases that it has meaningful info inside it, that would be great to have them instead of the 4 attributes (request_type, name, response_time, response_length)

@cyberw
Copy link
Collaborator Author

cyberw commented Apr 15, 2021

@DennisKrone is working on a PR to add optional metadata to the event (basically anything you like, things like customer id is a good example).

We've also talked about adding the User object, and I think request/response make a lot of sense as well. We just need a sensible behaviour for non-http / custom clients as well.

@dchimeno
Copy link

We've also talked about adding the User object, and I think request/response make a lot of sense as well. We just need a sensible behaviour for non-http / custom clients as well.

That's great, let me know if there is something I could help.

@github-actions
Copy link

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

@github-actions github-actions bot added the stale Issue had no activity. Might still be worth fixing, but dont expect someone else to fix it label Jun 15, 2021
@github-actions
Copy link

This issue was closed because it has been stalled for 10 days with no activity. This does not necessarily mean that the issue is bad, but it most likely means that nobody is willing to take the time to fix it. If you have found Locust useful, then consider contributing a fix yourself!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request stale Issue had no activity. Might still be worth fixing, but dont expect someone else to fix it
Projects
None yet
Development

No branches or pull requests

4 participants