add optional ruby-profiling with --profile-ruby #4034
Conversation
dumps a large call graph into /var/chef/cache/graph_profile.out
|
I've been using this a lot lately. Seems it would be good to get it into PR shape and let other people play with it. @chef/client-core review? |
| @@ -53,6 +53,11 @@ | |||
| require 'chef/mixin/deprecation' | |||
| require 'ohai' | |||
| require 'rbconfig' | |||
| begin | |||
| require 'ruby-prof' | |||
There was a problem hiding this comment.
why not do this in start_profiling or profiling_prereqs
There was a problem hiding this comment.
6 of one, half dozen of the other? I certainly can...
|
I feel like maybe this shouldn't show in the |
|
👍 huge fan |
|
agree with @coderanger , given chef-client does not have dependency on ruby-prof, it feels strange to expose this as CLI flag. a config option should be good enough |
|
Are there any downsides to making it a dependency? I've had to walk people through profiling chef runs and having this just work would be awesome. I agree that it's not the most perfect feature, but it's still very useful. |
|
@jaym I would say it would fall under the kind of thing to include with the omnibus packages but not actually be a dependency (for those of us left that still install via gems). |
|
Its a dev dependency for that reason. 'gem install chef' won't pull it in, but 'bundle install' (not --without development) will, so it'll get shipped in omnibus. And whats the API to hide it from --help? |
|
There isn't a way to hide things from help in mixlib-cli (or else we would have done this with some other flags we added just for tests). I suppose you could detect whether the gem is available at file load time and then surround the |
|
If I omit the description it still shows up in --help. I really want it a CLI option because temporarily adding it to /etc/chef/client.rb is fussy, particularly if you have chef managing client.rb and have to edit it on every run. I avoided adding it as a config option, since I don't think people will want to have this option running on every run, particularly since the outfile gets truncated and its not a log (and that seems like a good way to DoS your disk space if we made it a log since the call graph is huge). |
|
yeah, i could |
|
I guess it depends on the purpose. If we just don't want to make a hard dep on the profiler gem, that'd be fine, if we want only expose this to users who know it's there, then it wouldn't help. |
|
I definitely agree this makes much more sense as a CLI flag than a config option. If you make with some typey-typey to make it really obvious what this does in the description I think we're good. |
|
I'm in agreement with @thommay's suggestion. |
|
Yeah, not worth a ton of work to add hidden CLI args, but something to warn off peoples would be nice. Also would be good to move the require I think so it doesn't add to the load time for the 99% of users that don't need it (but will have the gem thanks to omnibus et al). That's a minor issue though, a few more requires aren't going to be super noticeable. |
|
Made the description substantially more scary. I'll move the load in a sec, lazying the load time does marginally win in my mind over grouping the requires together. |
|
👍 |
| require 'ruby-prof' | ||
| rescue LoadError | ||
| raise "You must have the ruby-prof gem installed in order to use --profile-ruby" | ||
| end |
There was a problem hiding this comment.
as a truly trivial nitpick, the begin is redundant here no?
|
I'm 👍 regardless of my comment |
| @@ -52,6 +52,12 @@ class Chef::Application::Solo < Chef::Application | |||
| :boolean => true, | |||
| :default => false | |||
|
|
|||
| option :profile_ruby, | |||
| :long => "--[no-]profile-ruby", | |||
| :description => "Output ruby execution profile graph", | |||
There was a problem hiding this comment.
Might want to copy the scary here, too?
|
Probably want to update the description for the CLI option for solo and apply also. 👍 aside from that. |
add optional ruby-profiling with --profile-ruby
dumps a large call graph into /var/chef/cache/graph_profile.out