From ece16b8a27f663038e3ba4e91550d88f668df204 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Fri, 9 Jun 2017 19:23:49 -0400 Subject: [PATCH 1/3] perf capture inject use select and each_with_object for filtering. Return directly instead of using temp variables --- app/models/metric/capture.rb | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/app/models/metric/capture.rb b/app/models/metric/capture.rb index 0bdefa3ca8a..5ee22067333 100644 --- a/app/models/metric/capture.rb +++ b/app/models/metric/capture.rb @@ -128,32 +128,33 @@ def self.perf_capture_health_check(zone) end private_class_method :perf_capture_health_check + # Collect realtime targets and group them by their rollup parent + # + # 1. Only display rollups for Hosts + # 2. Since perf_target_to_interval_name(Host) == "realtime", no need to filtering on interval_name == "realtime" + # 3. Some Hosts have an EmsCluster as a parent, others have none. + # + # @returns Hash> + # e.g.: {"EmsCluster:4"=>[Host:4], "EmsCluster:5"=>[Host:1, Host:2]} def self.calc_targets_by_rollup_parent(targets) - # Collect realtime targets and group them by their rollup parent, e.g. {"EmsCluster:4"=>[Host:4], "EmsCluster:5"=>[Host:1, Host:2]} - targets_by_rollup_parent = targets.inject({}) do |h, target| - next(h) unless target.kind_of?(Host) && perf_capture_now?(target) - - interval_name = perf_target_to_interval_name(target) - next unless interval_name == "realtime" - - target.perf_rollup_parents(interval_name).to_a.compact.each do |parent| + targets.select { |target| target.kind_of?(Host) && perf_capture_now?(target) }.each_with_object({}) do |target, h| + target.perf_rollup_parents("realtime").to_a.compact.each do |parent| pkey = "#{parent.class}:#{parent.id}" h[pkey] ||= [] h[pkey] << "#{target.class}:#{target.id}" end - - h end - targets_by_rollup_parent end private_class_method :calc_targets_by_rollup_parent + # Create task for ems host cluster rollups + # {"EmsCluster:4" => Task{1}, "EmsCluster:5" => Task{2} } def self.calc_tasks_by_rollup_parent(targets_by_rollup_parent) task_end_time = Time.now.utc.iso8601 default_task_start_time = 1.hour.ago.utc.iso8601 # Create a new task for each rollup parent - tasks_by_rollup_parent = targets_by_rollup_parent.keys.inject({}) do |h, pkey| + targets_by_rollup_parent.keys.each_with_object({}) do |pkey, h| name = "Performance rollup for #{pkey}" prev_task = MiqTask.where(:identifier => pkey).order("id DESC").first task_start_time = prev_task ? prev_task.context_data[:end] : default_task_start_time @@ -175,10 +176,7 @@ def self.calc_tasks_by_rollup_parent(targets_by_rollup_parent) ) _log.info "Created task id: [#{task.id}] for: [#{pkey}] with targets: #{targets_by_rollup_parent[pkey].inspect} for time range: [#{task_start_time} - #{task_end_time}]" h[pkey] = task - h end - - tasks_by_rollup_parent end private_class_method :calc_tasks_by_rollup_parent @@ -192,7 +190,6 @@ def self.calc_target_options(zone, targets, targets_by_rollup_parent, tasks_by_r pkey = "#{parent.class}:#{parent.id}" tkey = "#{target.class}:#{target.id}" if targets_by_rollup_parent[pkey].include?(tkey) - # FIXME: check that this is still correct options[:task_id] = tasks_by_rollup_parent[pkey].id options[:force] = true # Force collection since we've already verified that capture should be done now end From a193cb8aecd5750148843001f04f5c5414f3c7b2 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Thu, 15 Jun 2017 11:51:11 -0400 Subject: [PATCH 2/3] merge perf_capture target_options rollup_parents Was producing a hash with a target parents then correlating that with targets. Easer to just produce the correlated hash in one go. now storing the object in the hash. It was storing the object at some times and the key at others. easier to just consistently store the object --- app/models/metric/capture.rb | 58 ++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 33 deletions(-) diff --git a/app/models/metric/capture.rb b/app/models/metric/capture.rb index 5ee22067333..5ed80729e63 100644 --- a/app/models/metric/capture.rb +++ b/app/models/metric/capture.rb @@ -46,8 +46,7 @@ def self.perf_capture_timer(zone = nil) targets = Metric::Targets.capture_targets(zone) targets_by_rollup_parent = calc_targets_by_rollup_parent(targets) - tasks_by_rollup_parent = calc_tasks_by_rollup_parent(targets_by_rollup_parent) - target_options = calc_target_options(zone, targets, targets_by_rollup_parent, tasks_by_rollup_parent) + target_options = calc_target_options(zone, targets_by_rollup_parent) targets = filter_perf_capture_now(targets, target_options) queue_captures(targets, target_options) @@ -130,31 +129,38 @@ def self.perf_capture_health_check(zone) # Collect realtime targets and group them by their rollup parent # - # 1. Only display rollups for Hosts - # 2. Since perf_target_to_interval_name(Host) == "realtime", no need to filtering on interval_name == "realtime" - # 3. Some Hosts have an EmsCluster as a parent, others have none. + # 1. Only calculate rollups for Hosts + # 2. Some Hosts have an EmsCluster as a parent, others have none. + # 3. Only Hosts with a parent are rolled up. # # @returns Hash> # e.g.: {"EmsCluster:4"=>[Host:4], "EmsCluster:5"=>[Host:1, Host:2]} def self.calc_targets_by_rollup_parent(targets) - targets.select { |target| target.kind_of?(Host) && perf_capture_now?(target) }.each_with_object({}) do |target, h| + realtime_targets = targets.select do |target| + target.kind_of?(Host) && + perf_target_to_interval_name(target) == "realtime" && + perf_capture_now?(target) + end + realtime_targets.each_with_object({}) do |target, h| target.perf_rollup_parents("realtime").to_a.compact.each do |parent| pkey = "#{parent.class}:#{parent.id}" - h[pkey] ||= [] - h[pkey] << "#{target.class}:#{target.id}" + (h[pkey] ||= []) << target end end end private_class_method :calc_targets_by_rollup_parent - # Create task for ems host cluster rollups - # {"EmsCluster:4" => Task{1}, "EmsCluster:5" => Task{2} } - def self.calc_tasks_by_rollup_parent(targets_by_rollup_parent) + # Determine queue options for each target + # Is only generating options for Vmware Hosts, which have a task for rollups. + # The rest just set the zone + def self.calc_target_options(zone, targets_by_rollup_parent) task_end_time = Time.now.utc.iso8601 default_task_start_time = 1.hour.ago.utc.iso8601 + target_options = Hash.new { |h, k| h[k] = {:zone => zone} } # Create a new task for each rollup parent - targets_by_rollup_parent.keys.each_with_object({}) do |pkey, h| + # mark each target with the rollup parent + targets_by_rollup_parent.keys.each_with_object(target_options) do |pkey, h| name = "Performance rollup for #{pkey}" prev_task = MiqTask.where(:identifier => pkey).order("id DESC").first task_start_time = prev_task ? prev_task.context_data[:end] : default_task_start_time @@ -169,33 +175,19 @@ def self.calc_tasks_by_rollup_parent(targets_by_rollup_parent) :start => task_start_time, :end => task_end_time, :parent => pkey, - :targets => targets_by_rollup_parent[pkey], + :targets => targets_by_rollup_parent[pkey].map { |target| "#{target.class}:#{target.id}" }, :complete => [], :interval => "realtime" } ) _log.info "Created task id: [#{task.id}] for: [#{pkey}] with targets: #{targets_by_rollup_parent[pkey].inspect} for time range: [#{task_start_time} - #{task_end_time}]" - h[pkey] = task - end - end - private_class_method :calc_tasks_by_rollup_parent - - def self.calc_target_options(zone, targets, targets_by_rollup_parent, tasks_by_rollup_parent) - targets.each_with_object({}) do |target, all_options| - interval_name = perf_target_to_interval_name(target) - - options = {:zone => zone} - target.perf_rollup_parents(interval_name).to_a.compact.each do |parent| - if tasks_by_rollup_parent.key?("#{parent.class}:#{parent.id}") - pkey = "#{parent.class}:#{parent.id}" - tkey = "#{target.class}:#{target.id}" - if targets_by_rollup_parent[pkey].include?(tkey) - options[:task_id] = tasks_by_rollup_parent[pkey].id - options[:force] = true # Force collection since we've already verified that capture should be done now - end - end + targets_by_rollup_parent[pkey].each do |target| + h[target] = { + :task_id => task.id, + :force => true, # Force collection since we've already verified that capture should be done now + :zone => zone, + } end - all_options[target] = options end end private_class_method :calc_target_options From 69b974c40e5f3f6765cd419f19899314ae6a2822 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Sat, 29 Jul 2017 11:24:20 -0400 Subject: [PATCH 3/3] perf_capture remove keys interation (rubocop / PR feedback) --- app/models/metric/capture.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/models/metric/capture.rb b/app/models/metric/capture.rb index 5ed80729e63..0743773773e 100644 --- a/app/models/metric/capture.rb +++ b/app/models/metric/capture.rb @@ -138,8 +138,8 @@ def self.perf_capture_health_check(zone) def self.calc_targets_by_rollup_parent(targets) realtime_targets = targets.select do |target| target.kind_of?(Host) && - perf_target_to_interval_name(target) == "realtime" && - perf_capture_now?(target) + perf_target_to_interval_name(target) == "realtime" && + perf_capture_now?(target) end realtime_targets.each_with_object({}) do |target, h| target.perf_rollup_parents("realtime").to_a.compact.each do |parent| @@ -160,7 +160,7 @@ def self.calc_target_options(zone, targets_by_rollup_parent) target_options = Hash.new { |h, k| h[k] = {:zone => zone} } # Create a new task for each rollup parent # mark each target with the rollup parent - targets_by_rollup_parent.keys.each_with_object(target_options) do |pkey, h| + targets_by_rollup_parent.each_with_object(target_options) do |(pkey, targets), h| name = "Performance rollup for #{pkey}" prev_task = MiqTask.where(:identifier => pkey).order("id DESC").first task_start_time = prev_task ? prev_task.context_data[:end] : default_task_start_time @@ -175,13 +175,13 @@ def self.calc_target_options(zone, targets_by_rollup_parent) :start => task_start_time, :end => task_end_time, :parent => pkey, - :targets => targets_by_rollup_parent[pkey].map { |target| "#{target.class}:#{target.id}" }, + :targets => targets.map { |target| "#{target.class}:#{target.id}" }, :complete => [], :interval => "realtime" } ) _log.info "Created task id: [#{task.id}] for: [#{pkey}] with targets: #{targets_by_rollup_parent[pkey].inspect} for time range: [#{task_start_time} - #{task_end_time}]" - targets_by_rollup_parent[pkey].each do |target| + targets.each do |target| h[target] = { :task_id => task.id, :force => true, # Force collection since we've already verified that capture should be done now