Changes custom_attribute virtual_attributes to support AREL/SQL #17615

NickLaMuro · 2018-06-20T00:26:31Z

Converts the virtual_column definitions in CustomAttributeMixin to support the arel attribute, and allow MiqReport#generate to take advantage of this, and avoid needing an extra include.

Other notable changes:

If all custom_attributes have arel backed virtual_attribute (aka virtual_column) methods, then the includes of :custom_attributes => {} will be removed from the Rbac query. A helper method was added to assist with this
The method defined for accessing the virtual_attribute from the instance will now check the attributes hash prior to executing the rest of the method, and return something if it has a key
Some shared values calculated in both the virtual_attribute method definition and would be in the custom_attribute_arel method have been extracted out so it is only calculated once (minor performance benefit).

Benchmarks

These benchmarks are currently a work in progress, and contain some incomplete data. Most of those inconsistencies are noted in below.

Tested on a local machine with the DB running on the same machine, so next to no network based latency for queries is observed.

Before

50 uniq query types executed. 212961 are just the N+1 fetching custom_attributes

ms	queries	query (ms)	rows
472820	223205	80092.4	419419

Raw Benchmark.measure numbers:

281.100000  13.840000 294.940000 (322.186379)
280.330000  12.210000 292.540000 (318.992034)
278.170000  11.730000 289.900000 (316.135889)

After

While that patch was present during the benchmarks, most of what is done in this patch negates it's usefulness, so almost everything seeing here is from the changes in this pull request.

NOTE: 10141 of the total queries and ~3580ms of the query (ms) here are INSERT statements into miq_report_result_details.

ms	queries	query (ms)	rows
41189	10220	5017.1	38435

Raw Benchmark.measure numbers:

 38.170000   1.270000  39.440000 ( 43.803676)
 34.660000   0.700000  35.360000 ( 38.079291)
 35.000000   0.730000  35.730000 ( 38.463884)

Links

https://bugzilla.redhat.com/show_bug.cgi?id=1592480
Related PR (now optional): [WIP] Use loaded custom_attributes in MiqReport#generate #17613

NickLaMuro · 2018-06-20T05:22:55Z

Test failures seem to be from these changes... but I will look at it tomorrow... it's late...

jrafanie · 2018-06-20T20:12:55Z

app/models/miq_report/generator.rb

@@ -307,6 +307,13 @@ def generate_basic_results(options = {})
    # TODO: add once only_cols is fixed
    # targets = targets.select(only_cols)
    where_clause = MiqExpression.merge_where_clauses(self.where_clause, options[:where_clause])
+
+    # Remove custom_attributes as part of the `inlcudes` if all of them exist


includes ☝️

jrafanie · 2018-06-20T20:14:12Z

app/models/miq_report/generator.rb

+
+    # Remove custom_attributes as part of the `inlcudes` if all of them exist
+    # in the select statement
+    if all_custom_attributes_in_select?


Maybe something like all_custom_attributes_are_virtual?

jrafanie · 2018-06-20T20:17:30Z

app/models/mixins/custom_attribute_mixin.rb

+      ca_sym            = custom_attribute.to_sym
+      without_prefix    = custom_attribute.sub(CUSTOM_ATTRIBUTES_PREFIX, "")
+      name_val, section = without_prefix.split(SECTION_SEPARATOR)
+      ca_db_col         = custom_attribute.tr('.', 'DOT').tr('/', 'BS').tr(':', 'CLN')


sanitized_column_alias maybe?

jrafanie · 2018-06-20T20:24:00Z

LGTM other than some nits. @kbrock can you approve of the arel changes? I don't have my arel dictionary with me.

jrafanie · 2018-06-20T20:27:01Z

@NickLaMuro was there a test covering this?

NickLaMuro · 2018-06-20T20:35:02Z

@jrafanie yeah, this gets exercised with some of the tests here:

manageiq/spec/models/miq_report_spec.rb

Lines 100 to 110 in 362b5ff

    
           let(:custom_column_key_1)  { 'kubernetes.io/hostname' } 
        
           let(:custom_column_key_2)  { 'manageiq.org' } 
        
           let(:custom_column_key_3)  { 'ATTR_Name_3' } 
        
           let(:custom_column_value)  { 'value1' } 
        
           let(:user)                 { FactoryGirl.create(:user_with_group) } 
        
           let(:ems)                  { FactoryGirl.create(:ems_vmware) } 
        
           let!(:vm_1)                { FactoryGirl.create(:vm_vmware) } 
        
           let!(:vm_2)                { FactoryGirl.create(:vm_vmware, :retired => false, :ext_management_system => ems) } 
        
           let(:virtual_column_key_1) { "#{CustomAttributeMixin::CUSTOM_ATTRIBUTES_PREFIX}kubernetes.io/hostname" } 
        
           let(:virtual_column_key_2) { "#{CustomAttributeMixin::CUSTOM_ATTRIBUTES_PREFIX}manageiq.org" } 
        
           let(:virtual_column_key_3) { "#{CustomAttributeMixin::CUSTOM_ATTRIBUTES_PREFIX}ATTR_Name_3" }

These were the tests that were failing when I mentioned them failing on CI here: #17615 (comment)

NickLaMuro · 2018-06-20T20:41:10Z

@kbrock mind weighing in on this PR?

NickLaMuro · 2018-06-20T22:09:00Z

Pretty sure these are sporadic test failures. Going to kick travis to confirm.

https://bugzilla.redhat.com/show_bug.cgi?id=1592480 This method is responsible for checking if the virtual_attributes in the Rbac query will contain all of the custom_attributes virtual_attributes. To be used to determine if we can safely remove: { :custom_attributes => {} } From the `include` hash on MiqReport, avoiding needing to load a lot of records.

https://bugzilla.redhat.com/show_bug.cgi?id=1592480 Conditionally removes `:custom_attributes => {}` from the includes clause if all of the `custom_attribute` columns happen to be in the select statement (the `@va_sql_cols` value). When the `custom_attribute` `virtual_attributes` can be converted to Arel, this will have some value, but for now this will mostly do nothing

https://bugzilla.redhat.com/show_bug.cgi?id=1592480 This change allows `custom_attributes` to be accessible via SQL. Most of the additions are found in the `custom_attribute_arel`, included by `CustomAttributeMixin`, and will simply generate the Arel equivalent necessary for determined the `virtual_attribute` value via SQL. Other notable changes: - The method defined for accessing the `virtual_attribute` from the instance will now check the attributes hash prior to executing the rest of the method, and return something if it has a key - Some shared values calculated in both the method definition and would be in the `custom_attribute_arel` method have been extracted out so it is only calculated once (minor performance benefit).

miq-bot · 2018-06-20T22:21:48Z

Checked commits NickLaMuro/manageiq@0ad75c2~...065405d with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🏆

kbrock

This is very sweet.

Just the 1 concern.

kbrock · 2018-06-21T17:20:54Z

app/models/mixins/custom_attribute_mixin.rb

-      define_method(custom_attribute.to_sym) do
-        custom_attribute_without_prefix           = custom_attribute.sub(CUSTOM_ATTRIBUTES_PREFIX, "")
-        custom_attribute_without_section, section = custom_attribute_without_prefix.split(SECTION_SEPARATOR)
+      virtual_column(ca_sym, :type => :string, :uses => :custom_attributes, :arel => ca_arel)


I think the trend is to always define the uses even with arel (as you have here)
Could we remove the :uses from the virtual_column and get rid of the remove_loading_relations_for_virtual_custom_attributes? ?

(speculation here)

No, we can't remove the remove_loading_relations_for_virtual_custom_attributes.

The remove_loading_relations_for_virtual_custom_attributes is making sure we don't have a :custom_attributes =>{} in the include of the report, which will get added to the .includes and .references in Rbac, causing the join bomb we were observing.

The :custom_attributes =>{} is also added via remove_loading_relations_for_virtual_custom_attributes's sister method, add_includes_for_virtual_custom_attributes, and that is only call as part of of the controller in the UI prior to saving the record.

The call to remove_loading_relations_for_virtual_custom_attributes is making sure we don't get that LEFT JOIN happening in the query, while the rest of the PR is avoiding not introducing an N+1 as a result.

Regarding :uses, unsure if keeping it has any value, though not really sure it hurts. Don't care either way though.

kbrock · 2018-06-21T17:21:43Z

app/models/mixins/custom_attribute_mixin.rb

+        return self[sanatized_column_alias] if has_attribute?(sanatized_column_alias)
+
+        where_args           = {}
+        where_args[:name]    = name_val


?

where_args = {:name => name_val}

Didn't write this originally, just updated the variable name. Just left it the same as it was.

kbrock · 2018-06-21T18:09:14Z

app/models/miq_report/generator.rb

+    # in the select statement
+    if all_custom_attributes_are_virtual_sql_attributes?
+      remove_loading_relations_for_virtual_custom_attributes
+    end


I want to understand this more:

def _generate_table if ... 4 generate_* methods else generate_basic_results # A end build_table # B end

This change adds remove_loading_relations_for_virtual_custom_attributes A (optionally).
build_table always calls remove_loading_relations_for_virtual_custom_attributes.

Do we need to consider using that method for other generate_* methods?

Do we need to make it optional in build_table?

1. Do we need to consider using that method for other generate_* methods?

Possibly, though not the priority for this BZ. Should function still without it at the moment, and I think the way we gather results for reports is vastly different in each of those methods, so I think waiting to do that makes sense for this PR.

2. Do we need to make it optional in build_table?

No, this should be idempotent to remove the value from the include hash. It is not saving the record with it removed anyway, so we are just removing it earlier than originally planed.

kbrock · 2018-06-21T18:11:44Z

app/models/mixins/custom_attribute_mixin.rb

        where_args[:section] = section if section

        custom_attributes.find_by(where_args).try(:value)
      end
    end
+
+    def self.custom_attribute_arel(name_val, section, column_alias)


nicely done

NickLaMuro · 2018-06-21T22:12:50Z

@miq-bot add_labels fine/yes, gaprindashvili/yes

simaishi · 2018-06-26T12:44:28Z

Backported to Gaprindashvili via #17629

simaishi · 2018-06-26T13:30:45Z

Backported #17629 to Fine branch.

NickLaMuro force-pushed the custom_attributes_as_arel_virtual_attributes branch 2 times, most recently from 8941a96 to 90a1505 Compare June 20, 2018 18:26

jrafanie reviewed Jun 20, 2018

View reviewed changes

NickLaMuro force-pushed the custom_attributes_as_arel_virtual_attributes branch from 90a1505 to 41d0e2a Compare June 20, 2018 20:33

NickLaMuro mentioned this pull request Jun 20, 2018

[WIP] Use loaded custom_attributes in MiqReport#generate #17613

Closed

2 tasks

NickLaMuro added 3 commits June 20, 2018 17:10

NickLaMuro force-pushed the custom_attributes_as_arel_virtual_attributes branch from 41d0e2a to 065405d Compare June 20, 2018 22:16

jrafanie added blocker core enhancement labels Jun 21, 2018

kbrock reviewed Jun 21, 2018

View reviewed changes

kbrock self-assigned this Jun 21, 2018

kbrock added this to the Sprint 89 Ending Jul 2, 2018 milestone Jun 21, 2018

kbrock merged commit 762abcf into ManageIQ:master Jun 21, 2018

miq-bot added fine/yes labels Jun 21, 2018

This was referenced Jun 21, 2018

[GAPRINDASHVILI] Changes custom_attribute virtual_attributes to support AREL/SQL #17626

Closed

[GAPRINDASHVILI] Use AREL for custom_attributes virtual_attributes in MiqReport ONLY #17628

Closed

NickLaMuro mentioned this pull request Jun 22, 2018

[GAPRINDASHVILI] Use AREL for custom_attributes virtual_attributes in MiqReport ONLY #17629

Merged

simaishi added the gaprindashvili/conflict label Jun 25, 2018

simaishi added gaprindashvili/backported and removed gaprindashvili/conflict labels Jun 26, 2018

simaishi added fine/backported and removed fine/yes labels Jun 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes custom_attribute virtual_attributes to support AREL/SQL #17615

Changes custom_attribute virtual_attributes to support AREL/SQL #17615

NickLaMuro commented Jun 20, 2018 •

edited

Loading

NickLaMuro commented Jun 20, 2018

jrafanie Jun 20, 2018

jrafanie Jun 20, 2018

jrafanie Jun 20, 2018

jrafanie commented Jun 20, 2018

jrafanie commented Jun 20, 2018

NickLaMuro commented Jun 20, 2018

NickLaMuro commented Jun 20, 2018

NickLaMuro commented Jun 20, 2018

miq-bot commented Jun 20, 2018

kbrock left a comment

kbrock Jun 21, 2018

NickLaMuro Jun 21, 2018

NickLaMuro Jun 21, 2018

kbrock Jun 21, 2018

NickLaMuro Jun 21, 2018

kbrock Jun 21, 2018

NickLaMuro Jun 21, 2018

kbrock Jun 21, 2018

NickLaMuro Jun 21, 2018

NickLaMuro commented Jun 21, 2018

simaishi commented Jun 26, 2018

simaishi commented Jun 26, 2018

Changes custom_attribute virtual_attributes to support AREL/SQL #17615

Changes custom_attribute virtual_attributes to support AREL/SQL #17615

Conversation

NickLaMuro commented Jun 20, 2018 • edited Loading

Benchmarks

Links

NickLaMuro commented Jun 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie commented Jun 20, 2018

jrafanie commented Jun 20, 2018

NickLaMuro commented Jun 20, 2018

NickLaMuro commented Jun 20, 2018

NickLaMuro commented Jun 20, 2018

miq-bot commented Jun 20, 2018

kbrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickLaMuro commented Jun 21, 2018

simaishi commented Jun 26, 2018

simaishi commented Jun 26, 2018

NickLaMuro commented Jun 20, 2018 •

edited

Loading