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 ability to write csv stats files #612

Merged
merged 3 commits into from
Sep 7, 2017

Conversation

aldenpeterson-wf
Copy link
Contributor

I started reviewing #445 but found that it had enough problems that it'd be just easier to rewrite the entire PR.

This:

  • Ability to write csv files for both the requests and distribution csv as a command line flag
  • Refactored the logic in the web handler to reuse this csv generation for better portability

I also fixed #598 because that issue has been annoying me for a while.

@mambusskruj
Copy link

mambusskruj commented Jun 27, 2017

Hi! As I understand one row will write in csv per 2 seconds? Why not per one second? It will be great for some TSDB like InfluxDB.

@aldenpeterson-wf
Copy link
Contributor Author

@mambusskruj I copied the interval here: https://github.com/locustio/locust/blob/master/locust/stats.py#L519

I'd actually be more inclined to decrease the frequency given that it's blocking IO for locust and on slower systems that might affect your loadtest performance.

@mambusskruj
Copy link

mambusskruj commented Jun 27, 2017

@aldenpeterson-wf well, if locust used in master-slave mode, this is not a big issue I think.

@aldenpeterson-wf
Copy link
Contributor Author

@mambusskruj I could make it a user defined option but I'm not sure that it's worth the added complexity.

I'd be curious what other people think. I could also define the time at the module level so anyone who really wanted to could set the timeframe for both the printing/csv save.

locust/web.py Outdated
@@ -15,7 +15,8 @@
from . import runners
from .cache import memoize
from .runners import MasterLocustRunner
from locust.stats import median_from_dict
from .stats import sort_stats
Copy link
Member

Choose a reason for hiding this comment

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

why the 2 different imports from .stats and then locust.stats?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.. no reason, will fix this

@cgoldberg
Copy link
Member

I could make it a user defined option but I'm not sure that it's worth the added complexity.

I think 2 secs for a writing interval sounds good for the initial implementation. Once we gauge impact of this change on performance, we can revisit or make it configurable.

First pass at a review LGTM, but I haven't tested it. Also, can something be added to the docs or README that describes stats output?

@aldenpeterson-wf
Copy link
Contributor Author

@cgoldberg will update the docs.

I also might just make the sleep time to be constants in that file, so someone can change them it if they want since.. python.

@aldenpeterson-wf
Copy link
Contributor Author

I'm not 100% sure where to put this in the docs. There's not really a good section detailing all the flags but I added them, given activity on GH it seems an option like this would be very commonly used.

Also @mambusskruj I added the ability to easily customize how frequently the stats CSV is written.

@aldenpeterson-wf
Copy link
Contributor Author

@cgoldberg this is updated and ready for review

@aldenpeterson-wf
Copy link
Contributor Author

@cgoldberg bump :)

@mbeacom mbeacom self-requested a review September 7, 2017 19:47
Copy link
Member

@mbeacom mbeacom left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@mbeacom mbeacom merged commit 0fd5c5c into locustio:master Sep 7, 2017
@mbeacom mbeacom mentioned this pull request Sep 8, 2017
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.

bump gevent version
4 participants