-
Notifications
You must be signed in to change notification settings - Fork 513
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
/profile endpoint using https://github.com/bdarnell/plop profiler #46
base: master
Are you sure you want to change the base?
Conversation
The only concern I have about this is making sure it doesn't get abused, since it could be easily used to take down a production server. Maybe we can protect it somehow?. Like putting it behind some static auth, configured in settings. Or even passing a token to the url (also configured in settings or command line arg). |
I don't think there is much overhead here. Plop is a sampling profiler - it basically saves info about current frame (increments a counter for it) 100 times a second (this is customizable, default interval is 0.01s). This is nothing like cProfile overhead which increments a counter for every frame changed. It is true that /profile takes a persistent connection to the client (for 30s by default), but so do render.* endpoints if remote host is show to respond. As measured by CPU monitor, single /debug call uses more CPU time that leaving /profile ON for 30s (in fact, on my machine I can't notice /profile call in 0.01s resolution, while /debug call increases total CPU time for 0.04s). It is way easier to abuse splash by submitting render requests :) |
return NOT_DONE_YET | ||
|
||
def finishProfile(self, request): | ||
self.collector.stop() |
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.
Should we stop the collector on an addBoth
callback?. Otherwise, if the request fails (connection closed or similar) it may never be called and leave the collector running.
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.
you may found request.notifyFinish
method useful, it is used with the same goal in https://github.com/scrapy/scrapy/blob/master/scrapy/tests/mockserver.py#L48
Thanks for the clarification @kmike, I'm no longer concerned now :). I added another comment. |
Thanks for comments! Code was indeed messy. I have another concern about this PR: it exposes paths and line numbers of code that is executed, and this could be seen as a security risk, because based on this information offender can figure out directory structure + Python, twisted, PyQT and splash versions. |
I believe this PR would be quite useful. About the security concerns maybe we can make it disabled by default (with a setting) and then a Splash admin can enable it if it needs to do some profiling. |
Agreed, I don't have any objections to merge it. As for the security concern, the simplest would be to add a debug token (configured in settings) that you'd need to pass to the /profile url (in an argument) in order to work. Can we add that and merge? |
I've added token support. TODO:
I don't want to add default token to debian package because most people would leave this insecure default as-is. It looks like a better idea to get the token from environment variable. Is this ok? |
Yeah, environment variable or make the settings overrideable with a |
Just in case :)