-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Fix csv stats precision #1503
Fix csv stats precision #1503
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1503 +/- ##
=======================================
Coverage 81.19% 81.19%
=======================================
Files 27 27
Lines 2388 2388
Branches 367 367
=======================================
Hits 1939 1939
Misses 355 355
Partials 94 94
Continue to review full report at Codecov.
|
s.avg_content_length, | ||
s.total_rps, | ||
s.total_fail_per_sec, | ||
round(s.median_response_time) if s.median_response_time else s.median_response_time, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can define a function above such as _adjust_precision(stat)
instead of fixing the precision at this scope. Moreover, having a separate function allows customization using different precision, if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are not intended to bring some new feature, but just to return an old view of csv report after regression.
Previously, data format was done also at this scope and was broken with an injection of csv_writer in 1.x.x version:
https://github.com/locustio/locust/pull/1428/files#diff-5d5f310549d6d596beaa43a1282ec49eL822
If we'll create a new function, it will be a complicated because of different data types in stats entries (time metrics, content metric, rate metrics). So it should be a some reason to add a new tangled function that probably will be used only once for this specific case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm, thanks
Fix for #1501 issue