Skip to content

Commit

Permalink
Run a single query for pre-loading VimPerformanceStates
Browse files Browse the repository at this point in the history
This bug was introduced by ManageIQ#22579

I misread this code and thought it was fetching all records a single time.
Turns out it is running this query N+1 times.

So adding the extra resource_id values (and a large count at that) to each query,
means we still run N+1, we fetch the same number of records, but our query
is much larger

After
=====

We are running a single query to fetch all VimPerformanceStates

Details
=======

One fix would have been to add a `load`, but that depends upon
our preloader monkey patch: ManageIQ#22594

The number of queries are the same, the index usage is the same, the number
of rows returned is the same for unchanged, load, and not specifying the resource

But the PR in question does double the size of the query string.
Which is a problem when rolling up a container image from a bunch of containers.

I opted to change the code rather than rollback because I liked the extra comments
around the block of code.

In the future, we want to consolidate the caching of vim_performance_states
as we currently have 2 methods of caching those values:
- preload that association
- store in an @ variable.


Numbers
=======

|       ms |query |  qry ms |     rows |` comments`
|      ---:|  ---:|     ---:|      ---:|  ---
|  4,808.1 |   19 | 2,736.4 |    1,334 |` 340-2023-06-22 20:00:00Z-long-1#before`
|    130.2 |    1 |   130.2 |          |`. SELECT vim_performance_states.* -- [30991]`
|      ---:|  ---:|     ---:|      ---:|  ---
|  4,431.2 |   19 | 2,500.3 |    1,334 |` 340-2023-06-22 20:00:00Z-short-1#after`
|     82.6 |    1 |    82.5 |          |`. SELECT vim_performance_states.* -- [15036]`
|      ---:|  ---:|     ---:|      ---:|  ---
|  4,659.0 |   19 | 2,690.4 |    1,334 |` 340-2023-06-22 20:00:00Z-load-1#alt`
|    107.9 |    1 |   107.9 |          |`. SELECT vim_performance_states.* -- [15036]`

alt being to use load instead of removing resource from the query
  • Loading branch information
kbrock committed Aug 10, 2023
1 parent 5ab0513 commit 4888bd6
Showing 1 changed file with 1 addition and 3 deletions.
4 changes: 1 addition & 3 deletions app/models/metric/rollup.rb
Original file line number Diff line number Diff line change
Expand Up @@ -295,9 +295,7 @@ def self.rollup_child_metrics(obj, timestamp, interval_name, assoc)
# For infra, if that record is not found, use the current performance_state (can't capture in the past)
if recs.present?
rec_states = VimPerformanceState.where(
:timestamp => [ts, Metric::Helper.nearest_hourly_timestamp(Time.now.utc)],
:resource_type => recs.first.class.base_class.name,
:resource_id => recs.map(&:id)
:timestamp => [ts, Metric::Helper.nearest_hourly_timestamp(Time.now.utc)]
)
MiqPreloader.preload(recs, :vim_performance_states, rec_states)
end
Expand Down

0 comments on commit 4888bd6

Please sign in to comment.