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

#331: Use rounded_response_time for min/max/total response times #558

Merged
merged 3 commits into from
Oct 1, 2017

Conversation

jude
Copy link
Contributor

@jude jude commented Mar 6, 2017

I was going to open a new ticket for these changes, but saw #331 already existed with almost the same changes that ended up working for me. (I also use the rounded_response_time when computing the total response time)

I also added a few unit tests to cover the rounding behavior a little, and added a test_suite line to setup.py, so that 'python setup.py test' will find and run the tests.

@heyman
Copy link
Member

heyman commented Sep 18, 2017

I agree that it's confusing that you might get 99 percentile that is larger than 100% (as discussed in #331).

However, I don't think we should round the total_response_time as it's used to calculate the average response time. Rounding the min and max should be okay, but I'm also thinking that maybe it's even better to change so that we use self.get_response_time_percentile() for the 100% value as well (at this row: https://github.com/jude/locust/blob/a65de0fddc9bb6f4ca027325cbf510cfdbef7839/locust/stats.py#L358), and not rounding the min/max.

The rounding of the response times are a necessary evil that drastically shrinks the amount of data that needs to be sent from the worker nodes to the master. But that's not really needed for min and max.

@jude
Copy link
Contributor Author

jude commented Oct 1, 2017

Cool. I've updated the PR as per the comment above.

I also updated the Vagrantfile to use xenial instead of precise, since it looks like precise has EOL'd since the the PR was opened.

@heyman heyman merged commit 71e4864 into locustio:master Oct 1, 2017
@heyman
Copy link
Member

heyman commented Oct 1, 2017

Great, thanks!

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.

2 participants