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

pmcollectl in replay mode sleeps unnecessarily #550

Closed
fche opened this issue Aug 10, 2018 · 18 comments
Closed

pmcollectl in replay mode sleeps unnecessarily #550

fche opened this issue Aug 10, 2018 · 18 comments

Comments

@fche
Copy link
Contributor

fche commented Aug 10, 2018

The following patch appears to be needed to make pmcollectl -p/-a (archive-replay) mode run at full speed.

k.txt

@goodwinos
Copy link
Contributor

@fche, we have a roadmap item to rewrite pmcollectl as pcp-collectl; it will be a post-processing front-end for pmrep, with various pmrep configs similar to those already in pmrep.conf. See what we have so far with the following config entries :
[collectl-sc] [collectl-sm] [collectl-sd] [collectl-sD] [collectl-dm-sD] [collectl-sn]

The front-end will aim to be cmdline compatible with collectl, i.e. more or less a drop in replacement.

https://pcp.io/roadmap/ seems to be hanging at the moment, so I can't point it out to you. But in any case, please don't spend time on pmcollectl.

Regards

@fche
Copy link
Contributor Author

fche commented Aug 10, 2018

Thanks for the note. I don't plan to work on this any more, but a real user asked for help and that two-liner patch helped today.

myllynen pushed a commit that referenced this issue Dec 17, 2018
@myllynen
Copy link
Contributor

I've now applied this patch, the collectl QA passing before/after (and QA runtime dropped from 3:50 to 18s). Next time, please provide a proper pull request and also try to run the related QA tests. Thanks.

@fche
Copy link
Contributor Author

fche commented Dec 17, 2018

Next time, please provide a proper pull request

I reported a problem, and provided a plausible patch for consultation & consideration. That should be enough.

and also try to run the related QA tests

Why are you assuming I didn't -- and why should that be the burden of a bug reporter anyway?

@myllynen
Copy link
Contributor

That should be enough.

Many projects require patch submitters to create proper PRs to have them even evaluated in order not to put too much overhead on maintainers. While I'm unaware of such hard requirement for PCP, I'm sure you are well aware that proper PRs would make life easier for those checking your patches (and open PRs show up much more prominently than a patch attached to an issue buried among 150+ open issues).

Why are you assuming I didn't

I assumed so because you didn't say you did. I would certainly have mentioned it myself to remove any doubts.

why should that be the burden of a bug reporter anyway?

How would you otherwise know your patch doesn't break anything if you did not test it? Again, while not a hard requirement (AFAIK) it also makes life easier for others and increase chances of your patches being applied in a timely manner.

@fche
Copy link
Contributor Author

fche commented Dec 17, 2018

why should that be the burden of a bug reporter anyway?
How would you otherwise know your patch doesn't break anything if you did not test it?

"did not test it"? The patch came with a statement that indicated it works for that particular situation, so it was obviously tested to some extent. Beyond that, it was provided for your consideration, not presuming that it was ready for merging. If every hypothetical patch were formed into a pull request, then the low-volume PR queue would become a high-volume one.

Pl.ease do not throw any unnecessary hurdles in the way of people who report bugs and then make an additional effort in providing a possible fix.

@fche
Copy link
Contributor Author

fche commented Dec 17, 2018

By the way, one way of marking issues that have hypothetical patches associated with them, even if no PRs, would be to add a github label to them. They can stand out with pretty colours.

@myllynen
Copy link
Contributor

The patch came with a statement that indicated it works for that particular situation

That's one of the reasons to have automated QA, to try to make sure patches do not introduce unexpected regressions elsewhere. Anyway, I'm not sure what's the issue here, personally I consider PRs being polite towards maintainers as PRs are most often easier to process than attachments to issues (thus you'll find that no matter how small patches I've submitted to various projects at GitHub I always create PRs, it's insignificant additional effort for the submitter). Having N open PRs instead of N issues (labeled or not) with patches attached to them would be much more preferable situation, IMHO.

@fche
Copy link
Contributor Author

fche commented Dec 17, 2018

it's insignificant additional effort for the submitter

For those submitters who do not deem it insignificant, an agreeable maintainer could undertake this insignificant additional effort on their behalf.

@myllynen
Copy link
Contributor

That's what seems to have happened here. However, since the effort of going thru issues for patches, copypasting patches to files, and applying them locally with correct attribution is certainly more effort than merely merging an open PR, I'm still warmly recommending creating a PR next time. Thanks.

@lberk
Copy link
Member

lberk commented Dec 17, 2018

Hey Frank, Thanks for sending the patch along (and marko for getting it all merged)!. I noticed on your github profile, you've sent pull request's to various projects in the past. Is there a technical difference with PCP's github pull request workflow that we could change to make it easier for you to contribute them in the future?

@fche
Copy link
Contributor Author

fche commented Dec 17, 2018

But already in this thread, I was told to do more than just send a PR. Procedural obstacles to bug reports & contributions should be torn down rather than built up.

@myllynen
Copy link
Contributor

I was told to do more than just send a PR

You were asked to try running QA. It would have required dnf install pcp-testsuite ; cd /var/lib/pcp/testsuite ; ./check -g collectl ; on Fedora in this case - if that's not possible for you, that's ok. The main point I tried to communicate is that PRs are easier to track and process, I found this patch today by accident when closing some other completely unrelated issues and happened to open this ticket to see what this is about.

@myllynen
Copy link
Contributor

Perhaps it should be considered documenting the preferred steps properly, for example systemd and Ansible have these kinds of guidelines:

https://github.com/systemd/systemd/blob/master/docs/CONTRIBUTING.md
https://docs.ansible.com/ansible/latest/community/development_process.html

They also document other aspects (of which all may not be relevant to PCP):

https://github.com/systemd/systemd
https://docs.ansible.com/ansible/latest/community/

@fche
Copy link
Contributor Author

fche commented Dec 17, 2018

If the intended result is to preclude plain reporting of a bug and a sharing of a tentative patch, then I submit these would be counter-productive procedural barriers. It is a good idea to accommodate people who write patches reluctantly, such as if their bug reports are not attended to, by reducing rather than increasing their burdens.

@lberk
Copy link
Member

lberk commented Dec 17, 2018

But already in this thread, I was told to do more than just send a PR. Procedural obstacles to bug reports & contributions should be torn down rather than built up.

Agreed with respect to trying to tear down obstacles! I'd like to re-ask my specific question though; "...you've sent pull request's to various projects in the past. Is there a technical difference with PCP's github pull request workflow that we could change to make it easier for you to contribute them in the future?" I'm not discussing anything else other than, is there a technical limitation to how PCP's github pull request feature compared to others you've submitted in the past.

If the intended result is to preclude plain reporting of a bug and a sharing of a tentative patch, then I submit these would be counter-productive procedural barriers. It is a good idea to accommodate people who write patches reluctantly, such as if their bug reports are not attended to, by reducing rather than increasing their burdens.

I absolutely agree, we have some automated QA currently, however, currently, requires maintainers to run the testsuite themselves. We're working on adding more automated QA via pull requests. This would decrease your burden by adding assurance -- via testsuite confirmation -- that the patch in question works as expected and doesn't cause any regressions. Again, specifically, given that you've submitted PR's in the past to other projects on github, is there any technical difference or limitation to PCP's github pull request process that would stop you from doing so in the future?

@myllynen
Copy link
Contributor

If the intended result is to preclude plain reporting of a bug and a sharing of a tentative patch

That is not the intended result. The suggestion was that if you happen to have time to write a patch then it should not add significant overhead to provide possible patches as PRs (of which benefits I've listed above).

if their bug reports are not attended to

I've also got several bug reports open for quite some time, it might make sense to have a bug-fixes-only week or so at some point to try reduce the amount of open issues.

@fche
Copy link
Contributor Author

fche commented Dec 18, 2018

Again, specifically, given that you've submitted PR's in the past to other projects on github, is there any technical difference or limitation to PCP's github pull request process

That sounds like a reasonable question. Yes, the process is about the same. But no, the argument is similar to the "sunk cost" fallacy, whereby previous efforts are used to justify future effort, regardless of the cost & benefit of those future efforts.

Selling the extra effort of formal PRs as a benefit to the contributor is the right approach here, so good job making that case. Now let's make & keep that real.

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

No branches or pull requests

4 participants