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.

Sign up for GitHub

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

Improve N+1 detection for simple hydration methods #45848

Merged
merged 2 commits into from
Jul 24, 2024
Merged

Improve N+1 detection for simple hydration methods #45848

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1d91a99
Improve N+1 detection for simple hydration methods
johnswanson Jul 19, 2024
5dce66c
Add a test for n+1 detection
johnswanson Jul 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Improve N+1 detection for simple hydration methods 
We have N+1 detection for simple hydration methods that checks to see
whether the hydration method made any DB queries at all. This makes
sense, because a simple hydration method is called on each instance
individually - if we make database calls *each time*, we're in trouble.

Where this runs into issues is with cached data, where the simple
hydration method is populating the cache - e.g. `:can_read` or
`:can_write` dereferencing `api/*current-user-permissions-set*`.
Currently we specifically exclude those hydration methods from the
check.

But after running into this again for a different simple hydration
method, I think there's a more robust approach:

- first, run the hydration method without counting DB queries

- then, run it again and make sure we don't make any additional DB
queries.

There is a potential downside here: if someone wrote a simple hydration
method with side effects, they'd potentially be very confused (in
development - this doesn't run in prod!) when those side effects
happened twice for a single hydration. But I *think* that's fairly
unlikely.
  • Loading branch information
@johnswanson
johnswanson committed Jul 19, 2024
commit 1d91a9909117eff45ee2625d0203814814431b72
35 changes: 20 additions & 15 deletions dev/src/dev.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,26 +309,31 @@
(qp.compile/compile built-query))]
(into [query] params)))

(defn- maybe-realize
"Realize a lazy sequence if it's a lazy sequence. Otherwise, return the value as is."
[x]
(if (instance? clojure.lang.LazySeq x)
(doall x)
x))

(methodical/defmethod t2.hydrate/hydrate-with-strategy :around ::t2.hydrate/multimethod-simple
"Throws an error if do simple hydrations that make DB call on a sequence."
[model strategy k instances]
(if (or config/is-prod?
(< (count instances) 2)
;; we skip checking these keys because most of the times its call count
;; are from deferencing metabase.api.common/*current-user-permissions-set*
(#{:can_write :can_read} k))
(< (count instances) 2))
(next-method model strategy k instances)
(t2/with-call-count [call-count]
(let [res (next-method model strategy k instances)
;; if it's a lazy-seq then we need to realize it so call-count is counted
res (if (instance? clojure.lang.LazySeq res)
(doall res)
res)]
;; only throws an exception if the simple hydration makes a DB call
(when (pos-int? (call-count))
(throw (ex-info (format "N+1 hydration detected!!! Model %s, key %s]" (pr-str model) k)
{:model model :strategy strategy :k k :items-count (count instances) :db-calls (call-count)})))
res))))
(do
;; prevent things like dereferencing metabase.api.common/*current-user-permissions-set* from triggering the check
;; by calling `next-method` *twice*. To reduce the performance impact, just call it with the first instance.
(maybe-realize (next-method model strategy k [(first instances)]))
;; Now we can actually run the hydration with the full set of instances and make sure no more DB calls happened.
(t2/with-call-count [call-count]
(let [res (maybe-realize (next-method model strategy k instances))]
;; only throws an exception if the simple hydration makes a DB call
(when (pos-int? (call-count))
(throw (ex-info (format "N+1 hydration detected!!! Model %s, key %s]" (pr-str model) k)
{:model model :strategy strategy :k k :items-count (count instances) :db-calls (call-count)})))
res)))))

(defn app-db-as-data-warehouse
"Add the application database as a Database. Currently only works if your app DB uses broken-out details!"
Expand Down
Loading