Merge pull request #22611 from kbrock/vps_for_ts

VimPerformanceState#vim_performance_state_for_ts optimizations

app/models/chargeback/consumption_with_rollups.rb

@@ -54,7 +54,7 @@ def chargeback_container_labels
 
     def container_tag_list_with_prefix
       if resource.kind_of?(ContainerImage)
-        state = resource.vim_performance_state_for_ts(timestamp.to_s)
+        state = resource.vim_performance_state_for_ts(timestamp)
         image_tag_name = "#{state.image_tag_names}|" if state
 
         image_tag_name.split("|")

app/models/metric/ci_mixin/capture.rb

@@ -89,6 +89,10 @@ def calculate_gap(interval_name, start_time)
     end
   end
 
+  # @param interval_name   ["realtime", "hourly", "historical"]
+  # @param start_time      [String|nil] start time for historical capture (nil for all other captures)
+  # @param end_time        [String|nil]   end time for historical capture (nil for all other captures)
+  # @param metrics_capture [MetricCapture]
   def just_perf_capture(interval_name, start_time, end_time, metrics_capture)
     log_header = "[#{interval_name}]"
     log_time = ''

app/models/metric/ci_mixin/state_finders.rb

@@ -1,25 +1,42 @@
 module Metric::CiMixin::StateFinders
+  # load from cache or create a VimPerformanceState for a given timestamp
+  #
+  # for a cache:
+  #   use preload to populate vim_performance_states associations
+  #     this loads all records
+  #     the cache is indexed by a Time object
+  #     used by Metric.rollup / rollup_hourly
+  #   preload_vim_performance_state_for_ts_iso8601 to populate @states_by_ts
+  #       contains a subset, typically top of the hour and the timestamp of interest
+  #       the cache is indexed by a String in iso8601 form
+  #       used by: CiMixin::Processing#perf_process
+  # @param ts [Time|String] beginning of hour timestamp (prefer Time)
   def vim_performance_state_for_ts(ts)
+    ts = Time.parse(ts).utc if ts.kind_of?(String)
+    ts_iso = ts.utc.iso8601
     return nil unless self.respond_to?(:vim_performance_states)
 
     @states_by_ts ||= {}
-    state = @states_by_ts[ts]
+    state = @states_by_ts[ts_iso]
     if state.nil?
-      # TODO: vim_performance_states.loaded? works only when doing resource.vim_performance_states.all, not when loading
-      # a subset based on available timestamps
+      # using preloaded vim_performance_states association
       if vim_performance_states.loaded?
         # Look for requested time in cache
-        t = ts.to_time(:utc)
-        state = vim_performance_states.detect { |s| s.timestamp == t }
+        state = vim_performance_states.detect { |s| s.timestamp == ts }
         if state.nil?
-          # Look for state for current hour in cache if still nil because the
-          #   capture will return a state for the current hour only.
-          t = Metric::Helper.nearest_hourly_timestamp(Time.now.utc).to_time(:utc)
+          # Look for state for current hour in cache
+          ts_iso_now = Metric::Helper.nearest_hourly_timestamp(Time.now.utc)
+          t = ts_iso_now.to_time(:utc)
           state = vim_performance_states.detect { |s| s.timestamp == t }
         end
+        # look for state from previous perf_capture_state call
+        state ||= @states_by_ts[ts_iso_now]
       else
-        state = @states_by_ts[Metric::Helper.nearest_hourly_timestamp(Time.now.utc)]
-        state ||= vim_performance_states.find_by(:timestamp => ts)
+        ts_iso_now = Metric::Helper.nearest_hourly_timestamp(Time.now.utc)
+        state = @states_by_ts[ts_iso_now]
+        unless ts_iso_now == ts_iso
+          state ||= vim_performance_states.find_by(:timestamp => ts)
+        end
       end
       state ||= perf_capture_state
       @states_by_ts[state.timestamp.utc.iso8601] = state

app/models/metric/finders.rb

@@ -1,9 +1,11 @@
 module Metric::Finders
+  # @param hour [String]
   def self.find_all_by_hour(resource, hour, interval_name)
     start_time, end_time = hour_to_range(hour)
     find_all_by_range(resource, start_time, end_time, interval_name)
   end
 
+  # @param day [String|Time] prefer Time
   def self.find_all_by_day(resource, day, interval_name, time_profile)
     start_time, end_time = day_to_range(day, time_profile)
     find_all_by_range(resource, start_time, end_time, interval_name)
@@ -37,12 +39,14 @@ def self.find_all_by_range(resource, start_time, end_time, interval_name)
   # Helper methods
   #
 
+  # @param hour [String]
   def self.hour_to_range(hour)
     start_time = hour
     end_time = "#{hour[0..13]}59:59Z"
     return start_time, end_time
   end
 
+  # @param [String|Time] day prefer Time
   def self.day_to_range(day, time_profile)
     day = Time.parse(day) if day.kind_of?(String)
     day = day.in_time_zone(time_profile.tz_or_default)

app/models/metric/rollup.rb

@@ -266,8 +266,13 @@ def self.rollup_realtime_perfs(obj, rt_perfs, new_perf = {})
     new_perf
   end
 
+  # @param obj parent object
+  # @param timestamp [Time] for hourly, time is at the beginning of the hour
+  # @param interval_name  ["realtime", "hourly", "historical"]
+  # @param assoc [Symbol] name of the association to rollup
   def self.rollup_child_metrics(obj, timestamp, interval_name, assoc)
-    ts = timestamp.kind_of?(Time) ? timestamp.utc.iso8601 : timestamp
+    timestamp = Time.parse(timestamp).utc if timestamp.kind_of?(String)
+    ts = timestamp.utc.iso8601
     recs = obj.vim_performance_state_association(timestamp, assoc).to_a
 
     result = {}

app/models/vim_performance_state.rb

@@ -46,6 +46,8 @@ class VimPerformanceState < ApplicationRecord
   # => host_sockets     (derive from assoc_ids)
 
   # TODO: do a single query for the finds
+  # @param objs [Array[Object]|Object] MetricsCapture#target - all should be the same object class
+  # @returns [Array[VimPerformanceState]|VimPerformanceState] return an array if an array was passed in
   # NOTE: a few calls (with a single object) do use the return and expect a single result
   def self.capture(objs)
     ts = Time.now.utc

spec/models/metric/ci_mixin/state_finders_spec.rb

@@ -10,8 +10,8 @@
   let(:storage1) { FactoryBot.create(:storage) }
   let(:storage2) { FactoryBot.create(:storage) }
 
-  let(:ts_now) { Time.now.utc.beginning_of_hour.to_s }
-  let(:timestamp) { 2.hours.ago.utc.beginning_of_hour.to_s }
+  let(:ts_now) { Time.now.utc.beginning_of_hour }
+  let(:timestamp) { 2.hours.ago.utc.beginning_of_hour }
 
   describe "#vim_performance_state_association" do
     let(:c_vps_now) { create_vps(image, ts_now, :containers => [container1, container2]) }
@@ -41,7 +41,7 @@
 
       expect do
         expect(region.vim_performance_state_association(timestamp, :ext_management_systems)).to eq([ems1])
-      end.to make_database_queries(:count => 2) # TODO: vps caching (another PR) will change to 1
+      end.to make_database_queries(:count => 1)
     end
 
     it "fetches a second timestamp" do
@@ -93,6 +93,15 @@
           expect(image.vim_performance_state_for_ts(timestamp)).to eq(vps_now)
         end.not_to make_database_queries
       end
+
+      it "doesn't search db for now since perf_capture_state will do that" do
+        expect(image).to receive(:perf_capture_state).once.and_return(vps_now)
+
+        expect do
+          expect(image.vim_performance_state_for_ts(ts_now)).to eq(vps_now)
+        end.to make_database_queries(:count => 0)
+        expect { image.vim_performance_state_for_ts(ts_now) }.not_to make_database_queries
+      end
     end
 
     # ci_mixin/processing.rb uses this
@@ -104,8 +113,8 @@
         expect(image).to receive(:perf_capture_state).never
 
         expect do
-          expect(image.vim_performance_state_for_ts(timestamp)).to eq(vps_now) # TODO: should be vps
-        end.to make_database_queries(:count => 0)
+          expect(image.vim_performance_state_for_ts(timestamp)).to eq(vps)
+        end.not_to make_database_queries
       end
 
       it "falls back to cached now" do
@@ -159,7 +168,7 @@
       it "creates (and caches) a value when now isn't cached" do
         rec_states = VimPerformanceState.where(:timestamp => [ts_now, timestamp])
         MiqPreloader.preload(image, :vim_performance_states, rec_states)
-        expect(image).to receive(:perf_capture_state).twice.and_return(vps_now) # fix
+        expect(image).to receive(:perf_capture_state).once.and_return(vps_now)
 
         expect do
           expect(image.vim_performance_state_for_ts(timestamp)).to eq(vps_now)