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

Add failure per seconds as a series in the chart #1140

Merged
merged 10 commits into from Nov 12, 2019

Conversation

alercunha
Copy link
Contributor

@alercunha alercunha commented Nov 12, 2019

While using locust it's very useful to know the % of requests failing. However when pushing heavy loads against a GCP service it's common the auto-scaling can't cope with demand. Having the failures being represented in the chart as a series is very valuable to understand how the scaling overall is working.

image

@codecov
Copy link

codecov bot commented Nov 12, 2019

Codecov Report

Merging #1140 into master will increase coverage by 0.09%.
The diff coverage is 78.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1140      +/-   ##
==========================================
+ Coverage   75.46%   75.55%   +0.09%     
==========================================
  Files          18       18              
  Lines        1818     1833      +15     
  Branches      276      279       +3     
==========================================
+ Hits         1372     1385      +13     
- Misses        382      384       +2     
  Partials       64       64
Impacted Files Coverage Δ
locust/web.py 88.37% <ø> (ø) ⬆️
locust/stats.py 82.81% <78.94%> (+0.14%) ⬆️

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 b1af7de...42fc692. Read the comment docs.

@heyman
Copy link
Member

heyman commented Nov 12, 2019

Thanks for the PR! I'm not sure that I like the FPS acronym though. Maybe we could call it failures/s in the UI, and current_fail_per_sec in the JSON-blob?

locust/stats.py Outdated
total_reqs = 0
total_failures = 0
for key in sorted(six.iterkeys(stats)):
r = stats[key]
total_rps += r.current_rps
total_fps += r.current_fps
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable doesn't seem to be used, so it should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to include it in the print_stats table. I'll do that instead.

@alercunha
Copy link
Contributor Author

Thanks for the PR! I'm not sure that I like the FPS acronym though. Maybe we could call it failures/s in the UI, and current_fail_per_sec in the JSON-blob?

I changed the UI to show 'Failures/s' as you suggested.

Regarding changing current_fps to current_fail_per_sec, if that's the case, then all other variables/methods named _fps should be changed as well. Do you really think that's the case?

Sometimes I like using an acronym that makes my code easier to read and nicer to the eyes. But it's a matter of taste I guess.

@heyman
Copy link
Member

heyman commented Nov 12, 2019

Regarding changing current_fps to current_fail_per_sec, if that's the case, then all other variables/methods named _fps should be changed as well. Do you really think that's the case?

Hmm, I'm not aware of any existing use of the fps acronym in the existing code?

I'm not against acronyms per se, but I don't really associate FPS with "failures per second", as opposed to RPS which I think is more commonly used for "requests per second" (even though I think one could make the argument that "requests/s" would be better as well).

@alercunha
Copy link
Contributor Author

Hmm, I'm not aware of any existing use of the fps acronym in the existing code?

I meant the new variables and methods introduced as part of this PR. It's not just the current_fps variable. In general I agree that FPS doesn't immediately translates to 'failures per second' but in the context of the code it sounded fair to me.

I have no problem in renaming all variables/methods, just wanted to confirm.

@heyman
Copy link
Member

heyman commented Nov 12, 2019

I meant the new variables and methods introduced as part of this PR. It's not just the current_fps variable.

Ah, ok, got you! The reason I mentioned the JSON-blob was because even if not intended, it'll become part of a somewhat semi-public API, since it's not uncommon for people to request the stats endpoint "manually". Though, I do think it's better to avoid fps in variable names in the other code as well (though I think it's perfectly fine in CSS classes).

@alercunha
Copy link
Contributor Author

@heyman done.

@heyman heyman merged commit bcdc8ed into locustio:master Nov 12, 2019
@heyman
Copy link
Member

heyman commented Nov 12, 2019

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.

None yet

2 participants