-
Notifications
You must be signed in to change notification settings - Fork 897
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
Add memory usage to worker status in rake evm:status and status_full #15375
Add memory usage to worker status in rake evm:status and status_full #15375
Conversation
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.
Pedantic suggestion only, and not really needed. You probably should just 🐑 🇮🇹 before I get any other bad ideas...
lib/tasks/evm_application.rb
Outdated
@@ -102,11 +102,12 @@ def self.output_workers_status(servers) | |||
w.miq_server_id, | |||
w.queue_name || w.uri, | |||
w.started_on && w.started_on.iso8601, | |||
w.last_heartbeat && w.last_heartbeat.iso8601] | |||
w.last_heartbeat && w.last_heartbeat.iso8601, | |||
(w.proportional_set_size || w.memory_usage || 0) / 1.megabyte] |
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.
I know I had this really gross ternary operator for you in a PM as an alternative:
(mem = (w.proportional_set_size || w.memory_usage)).nil? ? 0 : mem / 1.megabyte
While what you have effectively does the exact same thing without requiring some TUMS™ afterwords, the advantage of the ternary is instead of returning a zero, we could return a "N/A"
, which probably is more accurate. But this is being a bit pedantic.
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.
yeah, shrug. Is it pedantic? I thought the same thing but couldn't convince myself to make it more complicated.
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.
Is it pedantic?
Well, I said it was, so it is!
But really, my rational is that seeing a 0 Mb
process in the status might be confusing to a user, and might give the impression of "the worker is just super efficient memory!" to someone who doesn't know what is going on under the hood. But again, I do think I am being pedantic, and problem more (questionably correct) info is better than none.
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.
Yeah, I agree with you. Maybe blank would be better than N/A. Let me look at that and clean it up.
98f41f3
to
e5f2f50
Compare
Ok, updated as per @NickLaMuro suggestion of not showing 0 and @Fryguy's suggestion of using the number helper. |
lib/tasks/evm_application.rb
Outdated
@@ -102,11 +102,12 @@ def self.output_workers_status(servers) | |||
w.miq_server_id, | |||
w.queue_name || w.uri, | |||
w.started_on && w.started_on.iso8601, | |||
w.last_heartbeat && w.last_heartbeat.iso8601] | |||
w.last_heartbeat && w.last_heartbeat.iso8601, | |||
ActionView::Base.new.number_to_human_size(w.proportional_set_size || w.memory_usage) || ""] |
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.
Prefer .to_s(:human_size)
over calling out to ActionView directly. http://edgeguides.rubyonrails.org/active_support_core_extensions.html#formatting
memory_size = w.proportional_set_size || w.memory_usage
memory_size ? memory_size.to_s(:human_size) : ""
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.
@Fryguy Not sure that will work with an empty string:
> "".to_s(:to_human)
ArgumentError: wrong number of arguments (given 1, expected 0)
from (irb):3:in `to_s'
from (irb):3
from /Users/nicklamuro/.gem/ruby/2.3.3/gems/railties-5.0.3/lib/rails/commands/console.rb:65:in `start'
from /Users/nicklamuro/.gem/ruby/2.3.3/gems/railties-5.0.3/lib/rails/commands/console_helper.rb:9:in `start'
from /Users/nicklamuro/.gem/ruby/2.3.3/gems/railties-5.0.3/lib/rails/commands/commands_tasks.rb:78:in `console'
from /Users/nicklamuro/.gem/ruby/2.3.3/gems/railties-5.0.3/lib/rails/commands/commands_tasks.rb:49:in `run_command!'
from /Users/nicklamuro/.gem/ruby/2.3.3/gems/railties-5.0.3/lib/rails/commands.rb:18:in `<top (required)>'
from bin/rails:4:in `require'
from bin/rails:4:in `<main>'
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.
But yes, some form of that would look a bit nicer.
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.
yeah, I edited my response probably as you were typing yours.
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.
@Fryguy Ah, so we are back to the really gross ternary I suggested here: #15375 (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.
I don't even care what color the shed is 🚲 🏠
e5f2f50
to
5be3b45
Compare
@@ -102,11 +102,12 @@ def self.output_workers_status(servers) | |||
w.miq_server_id, | |||
w.queue_name || w.uri, | |||
w.started_on && w.started_on.iso8601, | |||
w.last_heartbeat && w.last_heartbeat.iso8601] | |||
w.last_heartbeat && w.last_heartbeat.iso8601, | |||
(mem = (w.proportional_set_size || w.memory_usage)).nil? ? "" : mem / 1.megabyte] |
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.
I feel like I've seen this before...
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.
(☞゚ヮ゚)☞
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.
I would use (mem = (w.proportional_set_size || w.memory_usage)).nil? ? "" : mem.to_s(:human_size)]
, but that's just splitting hairs
LGTM 👍
Ready to go @NickLaMuro @Fryguy |
Checked commit jrafanie@5be3b45 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
I approved already... but since you added my "lovely ternary", that gets you 👍 👍 way up! |
I was using this locally for measuring worker sizes and thought it might be helpful to others.
Note, any worker with a nil value will show up as an empty string.
Note, this also shows up when you call
evm:status_full
to see multiple servers and the workers on those servers.