Skip to content
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

Report Widget #14285

Merged
merged 2 commits into from
Mar 15, 2017
Merged

Report Widget #14285

merged 2 commits into from
Mar 15, 2017

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Mar 10, 2017

High level

Widgets are reports that are pre-run for users.
They are updated every hour and run in the user's group context and timezone.
In summary, each report is run many times (# groups x # timezones x # times per day).
So any performance issue will be felt.

A customer complained that three reports were taking too long to run.

Before

After

  • Tables for SQL-friendly virtual attributes are no longer downloaded. This is done by removing them from the includes list.
  • Vm#provisioned_storage is now SQL-friendly so we can take advantage of the above includes optimization.

numbers

ms bytes objects queries query (ms) rows comments
6,119.8 103,342,161* 7,534,105 1,194 1,497.9 6,768 before
4,958.1 121,689,623* 5,639,577 1,190 1,197.1 1,148 after
19.0% n/a 25% 0.3% 20.1% 83% diff

* Memory usage does not reflect 7,169,463 freed objects.
* Memory usage does not reflect 2,051,300 freed objects.
GC cleaned up too many objects to truly reflect the memory improvement

related to:


reproduction details

profile_methods("MiqWidget::ContentGenerator#generate_content", "MiqWidget#generate_one_content_for_group", "MiqWidget#generate_one_content_for_user")
opts = ["MiqGroup", "EMS_OPS_BU", nil, ["Pacific Time (US & Canada)", "UTC"]] # w.group_options.last
w = MiqWidget.find_by(description: 'report_top_storage_consumers')
w.reload ; bookend("master", gc: true) { w.send(:content_generator).generate(w, *opts) }
ms bytes objects queries query (ms) rows comments
11,311.3 120,445,839* 16,006,917 1,194 2,529.8 6,768 master-1
6,180.1 7,873,235* 7,534,025 1,194 1,418.4 6,768 master-1
6,195.6 104,498,675* 7,516,979 1,194 1,528.1 6,768 master-1
6,119.8 103,342,161* 7,534,105 1,194 1,497.9 6,768 master-1

* Memory usage does not reflect 7,169,463 freed objects.
* Memory usage does not reflect 3,134,060 freed objects.
* Memory usage does not reflect 3,117,013 freed objects.
* Memory usage does not reflect 3,134,141 freed objects.

ms bytes objects queries query(ms) rows comments
8,915.0 44,824,308* 12,650,749 1,190 2,322.9 1,148 widget-1
4,912.0 124,986,592* 5,598,635 1,190 1,230.9 1,148 widget-1
5,074.4 9,322,759* 5,426,925 1,190 1,381.1 1,148 widget-1
4,958.1 121,689,623* 5,639,577 1,190 1,197.1 1,148 widget-1

* Memory usage does not reflect 5,280,780 freed objects.
* Memory usage does not reflect 2,010,357 freed objects.
* Memory usage does not reflect 1,838,645 freed objects.
* Memory usage does not reflect 2,051,300 freed objects.

@kbrock
Copy link
Member Author

kbrock commented Mar 11, 2017

/cc @NickLaMuro

@@ -97,7 +97,11 @@ def include_as_hash(includes = include, klass = nil)
if klass.nil?
klass = db_class
result = {}
cols.each { |c| result[c.to_sym] = {} if klass.virtual_attribute?(c) } if cols
if cols && respond_to?(:virtual_attribute?)
Copy link
Member

@gtanzillo gtanzillo Mar 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be klass.respond_to?(:virtual_attribute?)

kbrock added 2 commits March 15, 2017 10:18
It was embedding sql directly into the sql
yet requesting the tables be downloaded anyway
@miq-bot
Copy link
Member

miq-bot commented Mar 15, 2017

Checked commits kbrock/manageiq@4357b8d~...26de928 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks good. 🍪

Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

@gtanzillo gtanzillo added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 15, 2017
@gtanzillo gtanzillo merged commit 832c5a3 into ManageIQ:master Mar 15, 2017
@kbrock kbrock deleted the widget_2 branch March 16, 2017 12:55
@simaishi
Copy link
Contributor

@kbrock Please resolve conflict and create an Euwe-specific PR (referencing this one) or suggest other PRs to backport.

$ git diff
diff --cc app/models/miq_report/generator.rb
index b118111,f340ead..0000000
--- a/app/models/miq_report/generator.rb
+++ b/app/models/miq_report/generator.rb
@@@ -85,7 -97,11 +85,15 @@@ module MiqReport::Generato
      if klass.nil?
        klass = db_class
        result = {}
++<<<<<<< HEAD
 +      cols.each { |c| result.merge!(c.to_sym => {}) if klass.virtual_attribute?(c) || klass == LiveMetric } if cols
++=======
+       if cols && klass.respond_to?(:virtual_attribute?)
+         cols.each do |c|
+           result[c.to_sym] = {} if klass.virtual_attribute?(c) && !klass.attribute_supported_by_sql?(c)
+         end
+       end
++>>>>>>> 832c5a3... Merge pull request #14285 from kbrock/widget_2
      end
  
      if includes.kind_of?(Hash)
@@@ -99,12 -115,16 +107,20 @@@
            assoc_klass = assoc_reflection.nil? ? nil : (assoc_reflection.options[:polymorphic] ? k : assoc_reflection.klass)
  
            if v.nil? || v["include"].blank?
 -            result[k] = {}
 -          elsif assoc_klass
 -            result[k] = include_as_hash(v["include"], assoc_klass)
 +            result.merge!(k => {})
 +          else
 +            result.merge!(k => get_include_for_find(v["include"], assoc_klass)) if assoc_klass
            end
  
++<<<<<<< HEAD
 +          v["columns"].each { |c| result[k].merge!(c.to_sym => {}) if assoc_klass.virtual_attribute?(c) } if assoc_klass && assoc_klass.respond_to?(:virtual_attribute?) && v["columns"]
++=======
+           if assoc_klass && assoc_klass.respond_to?(:virtual_attribute?) && v["columns"]
+             v["columns"].each do |c|
+               result[k][c.to_sym] = {} if assoc_klass.virtual_attribute?(c) && !assoc_klass.attribute_supported_by_sql?(c)
+             end
+           end
++>>>>>>> 832c5a3... Merge pull request #14285 from kbrock/widget_2
          end
        end
      elsif includes.kind_of?(Array)

@kbrock
Copy link
Member Author

kbrock commented Mar 22, 2017

Thanks @simaishi - removing euwe label

kbrock added a commit to kbrock/manageiq that referenced this pull request Mar 23, 2017
Introduced by ManageIQ#14285
Addresses ManageIQ#14393
A null hardware record was returning null provisioned_storage
it is now returning 0

This was causing issues for automate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants