-
Notifications
You must be signed in to change notification settings - Fork 70
Make NodeJS profiling attachable #451
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
Conversation
Jongy
left a comment
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 very good @kwisniewski98 !
I left some comments (did not complete reviewing, will continue this week).
Please add a test like test_nodejs that uses the new attach mode. Also, the target nodejs app should not be started with the jitdump flags like we do now - we can add another Dockerfile, or just control the extra flags passed to node via a parameter when starting the container (so the existing test_nodejs can pass the flags; and your new test won't pass them)
|
I ran the CI |
|
The attach causes this to be printed: Not too bad but if it can be avoided somehow, that's best. |
805a816 to
4f8382f
Compare
I didn't find any way inside nodeJS to hide that message, Seems like it is printed unconditionally ref |
4f8382f to
39993a2
Compare
Jongy
left a comment
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.
Finished 4th round
|
Reviewed again. Left 3 final comments. Please fix the lint errors as well, and the one comment from earlier that wasn't fixed yet as far as I can tell, and we're good to merge this :) |
Jongy
left a comment
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.
Lint errors are what I described in #517 (comment), so I ignore them. I will merge after tests pass.
|
@kwisniewski98 only the Dockerifle lint error seems relevant -
|
|
Also, you can merge from master, I fixed the flake8 issue |
I've added ignore for that, double quote seems to be not working. |
|
At last :) Congrats @kwisniewski98 ! |
Description
Adds
--nodejs-mode=attachable, which injects DSO into already running NodeJS processes.Related Issue
#418
Motivation and Context
At current state, resolving function addresses in NodeJS requires restarting each process, which might be very hard if not impossible in production. This features allows generating perf maps at runtime.
How Has This Been Tested?
Tested manually in container and as executable. NodeJS targets were tested in both same namespace as gprofiler and in separate namespace (docker container)
Checklist: