Refactoring stats to handle custom percentiles #1477
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1477 +/- ##
==========================================
- Coverage 81.47% 81.23% -0.25%
==========================================
Files 27 27
Lines 2386 2398 +12
Branches 366 370 +4
==========================================
+ Hits 1944 1948 +4
- Misses 351 358 +7
- Partials 91 92 +1
Continue to review full report at Codecov.
|
|
Updated tests -> Passed. Moving to Open. |
|
Found issue with rounding: |
|
Fixed: |
|
Fixed console stats separator to any number of percentiles: |
|
Cool stuff! I think this may be better implemented as a command line switch, but I’m not wholly against it as is. It needs a proper test case (that actually modifies the setting and ensures it changes the output), and documentation. |
|
@heyman what do you think? Should this be a command line switch? I think we need to give some thought to how we manage settings in general (as the list of command line settings keeps growing) |
|
@cyberw Added a doc for customization of stats settings. Not sure about command-line options, but probably it's worth to add it to configuration file. |
locust/stats.py
Outdated
| '100%', | ||
| )) | ||
| console_logger.info("-" * (90 + STATS_NAME_WIDTH)) | ||
| headers = ('Type', 'Name', '# reqs') + tuple([f"{round(percentile*100, 4)}%" for percentile in PERCENTILES_TO_REPORT]) |
There was a problem hiding this comment.
Is a max number of decimal points assumed? Rounding like this can cause different percentiles to round to the same number. If a max number of decimal points is assumed, then there should be validation of the PERCENTILES_TO_REPORT.
There was a problem hiding this comment.
At the moment maximum percentile in default list is 0.99999, that matching to 3 digits after point (99.999%)
I've updated to 6 digits and believe that no-one ever will try to use percentiles with higher precision, so I think to do the additional calculation from percentile list will be redundant.
There was a problem hiding this comment.
That will probably do, but you should still verify the PERCENTILES_TO_REPORT so that the web interface does not break.
There was a problem hiding this comment.
These changes assume that a user will manually set a list of percentiles based on the default list, and this assumes that a user understands what does he do and why. Otherwise, we should handle all possible wrong or not relevant values that could be input ( >1, <0, not a number etc). There's no reason to set a percentile precision over 99.999999% in a performance testing world, and even if it will happen - it's just will cause a wrong header name in results stats.
So, imho it's a very little sense to add additional calculation in code to cover a non-breaking issue that unlikely could occur.
There was a problem hiding this comment.
In web.py there are assumptions about a few specific percentiles. I think it should be validated that those are present in the list. I agree that it is otherwise fine to assume that users will know to put relevant numbers in the list.
There was a problem hiding this comment.
That's a good point, I'll check web.py additionally
|
|
|
web.py has hardcoded percentiles in different places:
Not sure how median plays into this? |
|
This looks good to me, if nobody objects, I will merge it this weekend. |
|
|
Good point. I will hold off. |
|
Will check stats_history_csv_header and web.py additionally |
|
@cyberw @lhupfeldt Updated |
|
Nice. Let's give @heyman one last chance to comment before I merge :) |
|
Thanks! |
Goal
Currently, if we set a custom list of percentiles, we'll get a broken csv report & console stats for percentiles.
This PR is intended to:
How-to check:
In the locust file, set the list of percentiles you need to get:
Both csv report & console stats should be correct with a proper columns number and data.