-
Notifications
You must be signed in to change notification settings - Fork 60
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
Filter PIDs to profile #761
Conversation
…ofiling only pids provided
I'll open two more issues and I'll link them to the code. It seems that flags --pid/-p doesn't work as intendent in pyperf and phpspy. Pyperf seems to ignore this parameter and phpspy is getting stuck with this flag |
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.
Looks good :) I gave some comments. Did not try it out locally yet, will do that after the next CR round.
…ove usage of extend
… and ProfilerState classes constructors kwargs-only
Two tests failed in CI/CD but I think that it isn't connected with these changes |
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.
Done reviewing
…or ProfilerState and GProfiler classes
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.
Awesome. Gave a few minor comments. I'll play with it a bit locally now and write my impression, currently no other comment besides those few and then it's good to merge.
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.
Done reviewing and trying it out. 2 UX comments and we can merge
gprofiler/main.py
Outdated
@@ -943,7 +980,8 @@ def main() -> None: | |||
logger.info( | |||
"Running gProfiler", version=__version__, commandline=" ".join(sys.argv[1:]), arguments=args.__dict__ | |||
) | |||
|
|||
if processes_to_profile is not None: | |||
logger.info("Target PIDs given by --pids: ", pids=[process.pid for process in processes_to_profile]) |
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.
This looks like:
[2023-04-12 15:56:36,138] INFO: gprofiler: Target PIDs given by --pids: (pids=[1])
No need for the :
and the space, since logging adds it anyway when formatting the extras.
Description
This PR:
Related Issue
#605