Fixes #36715 - Speed up host fact retrieval when you only need a subset of facts

[jeremylenz]

A user with about 3k hosts has a report that runs host.facts on each host multiple times. This was causing the report generation time to be over an hour, since host.facts runs a few includes which result in quite a few SQL queries:

fact_values.includes(:fact_name).collect do |fact|
        hash[fact.fact_name.name] = fact.value
      end

This PR adds a new argument to host.facts so you can pass specific fact names. It also changes collect to each in the above code because we don't use the resulting arrays, we just iterate and build the final hash.

The report template can look something like this:

<%- load_hosts(search: input('Hosts filter')).each_record do |host| -%>
<%- host_facts = host.facts(['net::interface::eth0::ipv4_address', 'network::hostname']) -%>
<%-   report_row(
        'Hostname': host_facts['network::hostname'],
        'IP': host_facts['net::interface::eth0::ipv4_address']
    ) -%>
<%- end -%>

<%= report_render -%>

[theforeman-bot]

Issues: #36715

[jeremylenz]

For some reason the report renderer splats the first argument, turning

host.facts_with_names(['fact1', 'fact2'])

into

host.facts_with_names('fact1', 'fact2')

resulting in an ArgumentError if I don't handle it here.

[parthaa]

I 'd recommend this approach instead of facts_with_names. In my benchmark tests it was way faster.

diff --git a/app/models/host/base.rb b/app/models/host/base.rb
index 36fbe72..4f1d980 100644
--- a/app/models/host/base.rb
+++ b/app/models/host/base.rb
@@ -221,13 +221,16 @@ module Host
       example '@host.facts["uptime"] # => "30 days"', desc: 'Getting specific fact value, +uptime+ in this case'
       aliases :facts
     end
-    def facts_hash
+    def facts_hash(fact_names = [])
       hash = {}
-      fact_values.includes(:fact_name).collect do |fact|
+      query = fact_values.includes(:fact_name)
+      query = query.where(fact_names: {name: fact_names}) if fact_names.present?
+      query.collect do |fact|
         hash[fact.fact_name.name] = fact.value
       end
       hash
     end

For example with my change

fact_names = [
    'ansible_local::gls_ansible_ctd_posture::_ctd',
    'ansible_local::gls_ansible_timestemp::_timestemp',
    'ansible_local::gls_ansible_model::_model',
    'ansible_local::gls_ansible_mac::_mac_address',
    'ansible_local::gls_ansible_ipaddress::_gls_ansible_ipaddress',
    'ansible_local::gls_ansible_owner::_owner',
    'ansible_local::gls_ansible_lrt::_lab_lrt',
    'ansible_local::gls_ansible_hostname::_gls_ansible_hostname'
  ]

hashes = []
Benchmark.measure do 
  hashes = Host.all.map {|host| host.facts(fact_names)  }
end
=> #<Benchmark::Tms:0x000055fc793fe1b8 @label="", @real=4.964442664990202, @cstime=0.0, @cutime=0.0, @stime=0.068133, @utime=3.543512999999999, @total=3.611645999999999>

About 3.6 seconds for 2851 Hosts.

vs

hashes1 = []
Benchmark.measure do 
  hashes1 = Host.limit(500).map {|host| host.facts_with_names(fact_names) }
end
=> #<Benchmark::Tms:0x000055fcf3c900b8 @label="", @real=39.416546951048076, @cstime=0.0, @cutime=0.0, @stime=0.7837529999999999, @utime=30.203799, @total=30.987552>

30 seconds for 500 host records

[jeremylenz]

@parthaa updated 👍

[parthaa]

This seems to work well for me. APJ

[jeremylenz]

In our tests on a system with ~2900 hosts, this new approach has improved report generation time from over 1 hour to about 11-13 seconds.

[ekohl]

@ofedoren is this also correct documentation for *args parameters, or is there a specific tag for that?

[ofedoren]

Agh, my bad for not noticing this earlier :/

Well, it's not as wrong. It's still quite optional :) But for *args params, I'd use one of list/splat/rest instead of optional.

[ekohl]

Could you submit a follow up PR?

[ofedoren]

Sure: #9850

[ekohl]

It'd be interested to see some benchmarks of the new code in the latest iteration.

[jeremylenz]

I ran it again and report generation time (on our system with ~2900 hosts) is down to 9 seconds, not bad!

[jeremylenz]

Squashed

[ekohl]

[test katello]

[ekohl]

I like the improvement (looks like join & pluck shaved off another few seconds), but I wonder if there is test coverage for this new functionality. Can we also assert that it only performs a single DB query?

[jeremylenz]

test coverage for this new functionality. Can we also assert that it only performs a single DB query?

Did a bit of research and it looks like there is a "bullet" gem that helps with this, but I didn't want to add any new gems. So I added a helper method in test_helper; see what you think.

Turns out .pluck was adding an additional SQL query being on a separate line, so I fixed that as well.

I also improved facts to cache the results for a single host. This is because a lot of reports call host.facts[xxx] repeatedly, and not all users may immediately switch to the new facts(['fact_name1', 'fact_name2']) syntax:

'CTD': host.facts['ansible_local::gls_ansible_ctd_posture::_ctd'],
 'TimeStemp': host.facts['ansible_local::gls_ansible_timestemp::_timestemp'],
 'Model': host.facts['ansible_local::gls_ansible_model::_model'],

Added tests to cover all the new functionality.

[evgeni]

shouldn't this also catch UPDATE and DROP and INSERT statements?

(but probably out of scope right now)

[jeremylenz]

It could be argued that those are SQL "statements" and not "queries" :)

[evgeni]

Given SELECT can change data (hello triggers!), I see no difference. But as said, this is probably more than was asked in the original comment by Ewoud :)

[evgeni]

I had promised you a review when I am back, but it seems Ewoud did beat me, so we're good :)

[parthaa]

Suggested change

	def assert_sql_queries(num_of_queries)
	def assert_sql_queries(num_of_queries, match = /SELECT/)

followed by

 queries << payload[:sql] if payload[:sql] =~ match

[jeremylenz]

Good idea. Updated 👍

[jeremylenz]

Seems this will need another approving review since I added those tests.

[jeremylenz]

@evgeni @ekohl any more comments?