Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

pcp2graphite improvement: send all metrics at once in pickled mode #66

Merged
merged 4 commits into from Feb 9, 2016

Conversation

Projects
None yet
4 participants
Contributor

test-account-0 commented Feb 9, 2016

Before this change "pickled_input" variable was appended and metrics send over and over again.

Time to send metrics to graphite dropped for me from 6 minutes to 2 seconds.

test-account-0 added some commits Feb 9, 2016

pcp2graphite improvement: send all metrics at once in pickled mode
Before this change "pickled_input" variable was appended and metrics send over and over again.

Time to send metrics to graphite dropped for me from 6 minutes to 2 seconds.
pcp2graphite: cPickle instead of pickle
Further performance improvement.
pcp2graphite: option to specify pickle protocol
Another slight performance improvement with protocol 1 and 2.

fche commented on 139b0b0 Feb 9, 2016

good catch

fche commented on d4ec9b9 Feb 9, 2016

Can you comment about the portability of cPickle? How old is the version of python that started supporting it?

fche commented on 6731fe9 Feb 9, 2016

lgtm

Contributor

test-account-0 commented Feb 9, 2016

About cPickle. I think it was supported around Python 2.3 - https://www.python.org/dev/peps/pep-0307/

But in Python 3 - "The pickle module has an transparent optimizer (_pickle) written in C. It is used whenever available. Otherwise the pure Python implementation is used."

Contributor

test-account-0 commented Feb 9, 2016

I've added workaround for Python 3.

Contributor

test-account-0 commented Feb 9, 2016

Also the number of the metrics (self.metrics) printed at te beginning of execution is not really true. I think it should be len(miv_tuples) in the function execute.

Contributor

fche commented Feb 9, 2016

Yeah, but miv_tuples is a changing value within the main loop; self.metrics is a one-time value suitable for printing at program startup.

Contributor

dbrolley commented Feb 9, 2016

Would it be possible to create a qa test for this?

Contributor

natoscott commented Feb 9, 2016

@dbrolley qa/667 gives pretty good regression coverage for these changes, but maybe the newly added -r option would be worth adding a check for? (in that same test script I guess).

@dbrolley dbrolley merged commit 668793a into performancecopilot:master Feb 9, 2016

Contributor

dbrolley commented Feb 9, 2016

@natoscott, I went ahead and merged this. I took a look at qa/667, but couldn't figure out how to test if the -r option is working properly. Same output as the final test in the script?

Contributor

natoscott commented Feb 9, 2016

I went ahead and merged this

@dbrolley heh, there's no rush, take yer time. Yep, I think same output makes sense from a cursory glance. And just realised we also need a man page update here for the new option.

Contributor

natoscott commented Feb 9, 2016

@dbrolley I'll add the test case
@test-account-0 could you update the man page for the new option? Also, can you send through author details (name and email address) for these commits please? There's some concern about tracking who/where code is coming from, and we like to give commit log recognition to contributors too.

Thanks!

natoscott added a commit that referenced this pull request Feb 10, 2016

qa: update qa/667 to exercise latest pickle version via -r
Add regression testing for new pcp2graphite -r option from
#66
Contributor

dbrolley commented Feb 11, 2016

@test-account-0, Thanks again for contributing, but we really do need your name before can continue to merge your work, for copyright considerations and also to recognize your contribution.

Contributor

test-account-0 commented Feb 12, 2016

I think I'm not ready for it. Is it really such a big problem?

mkushnir pushed a commit to mkushnir/pcp that referenced this pull request Mar 17, 2016

qa: update qa/667 to exercise latest pickle version via -r
Add regression testing for new pcp2graphite -r option from
performancecopilot#66
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment