Dy/pm 15994 query optimization #294

davidyeeprocore1 · 2022-07-26T19:28:09Z

Currently, blueprinter does not attempt to eager load associations for collections of activerecords (ActiveRecordRelation). This results in n+1 queries being performed. The way this is resolved at the moment is the client (application that ingests blueprinter) must perform preloading. This PR aims to solve the n+1 query issue from within blueprinter rather than placing the burden on the client.

…ind.

…iable to take ld flag instead

…ready found an association to load for the current field

…ecks for nil directly instead

philipqnguyen · 2022-08-11T17:12:42Z

lib/blueprinter/helpers/base_helpers.rb

-            object_to_hash(obj,
-                           view_name: view_name,
-                           local_options: local_options)
+          if object.respond_to?(:reflect_on_all_associations) && preload_associations?


Would you flip this comparison so that it is
if preload_associations? && object.respond_to?(:reflect_on_all_associations)
While overall impact is small, we should put the less performant method after the && as it is not always needed to execute.

philipqnguyen · 2022-08-11T17:35:33Z

lib/blueprinter/helpers/association_helpers.rb

+    # Example: object_relation is an Account that has_one :user and has_many :subscriptions.
+    # The hash returned would be {:user => :user, :subscriptions => :subscriptions,
+    # :subscription_ids => :subscriptions}. Then the calling method would preload
+    # :subscriptions and :user


We've been using yardoc meta tags to generate our documentation on https://rubydoc.info/gems/blueprinter/0.25.3/Blueprinter/Base. To keep it consistent, would you change these docs to use yardoc tags. And same where you have the other method docs as well.

philipqnguyen · 2022-08-11T17:36:24Z

lib/blueprinter/helpers/association_helpers.rb

+      collection_associated_hash
+    end
+  end
+end


missing newline

philipqnguyen · 2022-08-11T17:41:27Z

lib/blueprinter/helpers/association_helpers.rb

+    # The hash returned would be {:user => :user, :subscriptions => :subscriptions,
+    # :subscription_ids => :subscriptions}. Then the calling method would preload
+    # :subscriptions and :user
+    def get_field_to_association_hash(object_relation)


instead of get_field_to_association_hash it should be field_to_association_hash or field_to_association. Ruby methods typically are nouns representing the return value.

philipqnguyen · 2022-08-11T18:08:02Z

lib/blueprinter/helpers/base_helpers.rb

@@ -6,6 +6,8 @@ def self.included(base)

    module SingletonMethods
      include TypeHelpers
+      include AssociationHelpers
+      @@should_preload_associations = false


Two things:

Instead of should_preload_associations, it should just be preload_associations. Otherwise it seems unsure and somewhat like a test file.

This should be in configurations.rb https://github.com/procore/blueprinter/blob/60b8f94616e18aa0346fa3c697b9e30c66c4ec8c/lib/blueprinter/configuration.rb#L3-L18 so that users can configure their options like everything else.

I see what you are saying here @philipqnguyen . One thing to note is that the preload_associations variable you are referencing is not meant to be a permanent field. It would only exist until we are done testing this feature. After that time, blueprinter will try to eager load associations that would otherwise not be eager loaded. My main concerns are:

I will need to introduce more code into the client.

Developers in the client may start using this option which will could result in blueprinter continuing to user eager loading even if we turn the flag off. This would be a problem if a bug arises and we want to roll back the eager loading changes in blueprinter. We will not be able to do so without a redeploy

Blueprinter is a public gem and changes to Blueprinter should be ready for use by the greater Ruby community.
If the goal in this PR is just to test this feature, I suggest 2 things:
a) Either have the monolith's Gemfile reference this branch
b) Or directly override the monolith's Blueprinter with these changes (basically a monkey patch).

When you have completed the test in the monolith, then we can introduce the change into Blueprinter itself.

I like the idea of pointing the Gemfile to this branch. I would also like to be able to toggle this feature with a feature flag, as opposed to a config that would allow developers to use the feature in other places as we test. The idea being that we would only scope the change to a narrow part of the code (extended view of tasks controller).

I would propose:

Pointing the client repo to this local branch via the Gemfile as you suggested.

Using the feature flag as is currently implemented on this branch

Remove the feature flag once we're done testing then merge into master

This approach would decrease the likelihood of needing a redeploy in case something goes wrong because we would be able to toggle the feature flag off.

Please let me know what you think. Thank you

philipqnguyen · 2022-08-11T19:03:53Z

lib/blueprinter/helpers/base_helpers.rb

-            object_to_hash(obj,
-                           view_name: view_name,
-                           local_options: local_options)
+          if object.respond_to?(:reflect_on_all_associations) && preload_associations?


What happens if the object given is already eager loaded?

Good call. I'll add a check for this

philipqnguyen · 2022-08-18T03:21:16Z

lib/blueprinter/helpers/association_helpers.rb

+        collection_associated_hash[association_name_as_symbol] = association_name_as_symbol
+      end
+      # do our best to determine the autogenerated method name. Does not account for custom names
+      collection_ids = association_name_as_symbol.to_s[0..-2] + '_ids'


This took me some time to understand why you were doing [0..-2]. You can instead use association_name_as_symbol.to_s.singularize, which is part of ActiveSupport in Rails. This section of the code should only be executing inside Rails, so you should have access to that method.

philipqnguyen · 2022-08-18T03:40:46Z

lib/blueprinter/helpers/association_helpers.rb

+      collection_associated_hash = Hash.new
+      if object_relation[0].respond_to?(association_name_as_symbol)
+        collection_associated_hash[association_name_as_symbol] = association_name_as_symbol
+      end


You might be able to DRY up line 56 - 59 because these are identical to line 39 - 43 in the singular_associated_fields_hash method above

philipqnguyen · 2022-08-18T03:56:33Z

lib/blueprinter/helpers/base_helpers.rb

+            list_of_associations_to_preload << field_to_association_hash[field.name]
+          end
+        end
+        object_relation_with_associations_loaded = object.preload(list_of_associations_to_preload)


Does this preload nested associations?
For example, User.preload(posts: :comments)?

No, it does not

lessthanjacob · 2023-08-25T02:08:29Z

Thanks for opening this PR @davidyeeprocore1, and apologies for the extended delay here.

Looking through the proposed changes, while there's certainly benefit here, my fear is that it over-extends the responsibilities of Blueprinter, which is primarily focused on the serialization of data and less about how the data is aggregated beforehand. Moving forward, ensuring that Blueprinter isn't directly tied to any persistence layer will help improve maintainability and enable more use cases for applications that do not use ActiveRecord.

From my perspective, to ensure clear, enforceable boundaries in application code, any required preloading should be handled prior to invoking Blueprinter.

ritikesh · 2023-09-14T10:03:17Z

@njbbaer are you considering taking this proposal forward? I agree with @lessthanjacob's take that this definitely extends the responsibilities for a serializer. Also, when you consider supporting things like Arrays, hashes and other objects like Mongo docs, this could mean adding additional overhead just for handling one type of data. ActiveRecord's internal API can change with versions and we have internally burnt our fingers trying to conditional eager_load outside of core AR. It gets really tricky to manage this across rails versions etc. So I would definitely recommend to let end-users optimise for stuff like eager-loading instead of the serializer trying to do it magically.

njbbaer · 2023-09-14T16:13:11Z

@ritikesh I agree with you and @lessthanjacob, this expands Blueprinter's scope probably more than we want. I reached out to @davidyeeprocore1 directly to see if he wants to make a case for it, but if not I'll close this PR shortly.

jhollinger · 2023-09-19T14:57:51Z

I can understand why this isn't being merged, but Blueprinter should at least expose the introspection capabilities necessary for other tools do this. Otherwise it's just an "N+1 query generator". Would you all be open to accepting a PR like that?

ritikesh · 2023-09-19T15:11:58Z

@jhollinger - would something like Bullet be of help?

jhollinger · 2023-09-19T15:24:31Z

@ritikesh No, never found Bullet very useful. Dev/staging data is never complete enough to find all N+1's outside of production. Blueprint already has the info necessary to do eager loading (with a little help from AR reflections maybe). If Blueprint just exposed its definitions for other tools to hook into, it would be so much more powerful (e.g. a blueprinter-activerecord gem for AR eager loading).

davidyeeprocore1 · 2023-09-19T16:19:22Z

Hello! Just catching up on this. I like the approach proposed by @jhollinger regarding a blueprinter-activerecord gem. Are you suggesting that we make a separate gem? That could work nicely and reduce the risk of breaking existing code that depends on blueprinter. I completely agree that blueprinter is capable of handling eager loading and should remove this responsibility from the client

Perhaps we can use this PR as inspiration/reference for building the blueprinter-activerecord gem

David Yee added 10 commits May 26, 2022 10:25

CI Tests: All. Preloads fields that are marked as eager load

1534c65

CI Tests: All. Now eager loading necessary associations that we can f…

b8875eb

…ind.

CI Tests: All. Introduces private method visibility

2ca5b7b

CI Tests: All. Adds unit tests for association helpers

adc1b5b

CI Tests: All. Adds unit tests for active records with custom names

18c585e

CI Tests: All. Adds comment headers to methods

e314d41

[test*]. Successfully connecting to launch darkly

a7d7446

[test*]. removes integration with launch darkly. now using static var…

1c4ff04

…iable to take ld flag instead

[test*]. removes dead code and continues loop in the event that we al…

3743c00

…ready found an association to load for the current field

[test*]. Removes &. operator because ruby 2.2 does not support it. Ch…

a3d6a52

…ecks for nil directly instead

davidyeeprocore1 requested a review from grahamleslie July 29, 2022 16:21

philipqnguyen suggested changes Aug 11, 2022

View reviewed changes

philipqnguyen reviewed Aug 11, 2022

View reviewed changes

[test*]. adds check to not preload again

96e2087

philipqnguyen reviewed Aug 18, 2022

View reviewed changes

davidyeeprocore1 requested a review from philipqnguyen September 12, 2022 22:49

lessthanjacob added enhancement New feature or request needs code review labels Jul 11, 2023

jmeridth assigned njbbaer Sep 5, 2023

njbbaer closed this Sep 13, 2023

njbbaer reopened this Sep 13, 2023

njbbaer requested a review from a team as a code owner September 13, 2023 23:00

njbbaer closed this Oct 2, 2023

jhollinger mentioned this pull request Oct 3, 2023

Discussion of a stable reflection API #341

Closed

ritikesh deleted the dy/PM-15994-query-optimization branch November 20, 2023 08:42

ritikesh mentioned this pull request Nov 20, 2023

lazy load for association #359

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dy/pm 15994 query optimization #294

Dy/pm 15994 query optimization #294

davidyeeprocore1 commented Jul 26, 2022 •

edited by lessthanjacob

Loading

philipqnguyen Aug 11, 2022

philipqnguyen Aug 11, 2022 •

edited

Loading

philipqnguyen Aug 11, 2022

philipqnguyen Aug 11, 2022 •

edited

Loading

philipqnguyen Aug 11, 2022

davidyeeprocore1 Aug 15, 2022 •

edited

Loading

philipqnguyen Aug 15, 2022

davidyeeprocore1 Aug 19, 2022 •

edited

Loading

philipqnguyen Aug 11, 2022

davidyeeprocore1 Aug 15, 2022

philipqnguyen Aug 18, 2022 •

edited

Loading

philipqnguyen Aug 18, 2022

philipqnguyen Aug 18, 2022

davidyeeprocore1 Aug 19, 2022

lessthanjacob commented Aug 25, 2023

ritikesh commented Sep 14, 2023 •

edited

Loading

njbbaer commented Sep 14, 2023 •

edited

Loading

jhollinger commented Sep 19, 2023

ritikesh commented Sep 19, 2023

jhollinger commented Sep 19, 2023 •

edited

Loading

davidyeeprocore1 commented Sep 19, 2023 •

edited

Loading

Dy/pm 15994 query optimization #294

Dy/pm 15994 query optimization #294

Conversation

davidyeeprocore1 commented Jul 26, 2022 • edited by lessthanjacob Loading

Choose a reason for hiding this comment

philipqnguyen Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipqnguyen Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidyeeprocore1 Aug 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidyeeprocore1 Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipqnguyen Aug 18, 2022 • edited Loading

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

lessthanjacob commented Aug 25, 2023

ritikesh commented Sep 14, 2023 • edited Loading

njbbaer commented Sep 14, 2023 • edited Loading

jhollinger commented Sep 19, 2023

ritikesh commented Sep 19, 2023

jhollinger commented Sep 19, 2023 • edited Loading

davidyeeprocore1 commented Sep 19, 2023 • edited Loading

davidyeeprocore1 commented Jul 26, 2022 •

edited by lessthanjacob

Loading

philipqnguyen Aug 11, 2022 •

edited

Loading

philipqnguyen Aug 11, 2022 •

edited

Loading

davidyeeprocore1 Aug 15, 2022 •

edited

Loading

davidyeeprocore1 Aug 19, 2022 •

edited

Loading

philipqnguyen Aug 18, 2022 •

edited

Loading

ritikesh commented Sep 14, 2023 •

edited

Loading

njbbaer commented Sep 14, 2023 •

edited

Loading

jhollinger commented Sep 19, 2023 •

edited

Loading

davidyeeprocore1 commented Sep 19, 2023 •

edited

Loading