From 8198461c9b3bba4e5a7aba84b1cfced923c1b5d8 Mon Sep 17 00:00:00 2001
From: Case Nelson <case@metabase.com>
Date: Mon, 24 Jun 2024 10:21:34 -0600
Subject: [PATCH] perf: disable view log and card used updates during metadata
fetches (#44502)
* perf: disable view log and card used updates during metadata fetches
* Disable writes for view_count and last_used_at
* Disable view_count tests
* Disable last-used-at tests
* Disable view_count test
* Fix view count test
* Disable view_count e2e testing
---
e2e/test/scenarios/dashboard/tabs.cy.spec.js | 66 ++++++++++++-------
src/metabase/events/view_log.clj | 4 +-
src/metabase/query_processor/execute.clj | 3 +-
test/metabase/api/activity_test.clj | 8 +--
test/metabase/api/dashboard_test.clj | 2 +-
test/metabase/events/view_log_test.clj | 6 ++
.../middleware/update_used_cards_test.clj | 8 ++-
7 files changed, 67 insertions(+), 30 deletions(-)
diff --git a/e2e/test/scenarios/dashboard/tabs.cy.spec.js b/e2e/test/scenarios/dashboard/tabs.cy.spec.js
index d291e74a93d..307912a350c 100644
--- a/e2e/test/scenarios/dashboard/tabs.cy.spec.js
+++ b/e2e/test/scenarios/dashboard/tabs.cy.spec.js
@@ -455,10 +455,12 @@ describe("scenarios > dashboard > tabs", () => {
};
firstQuestion().then(r => {
- expect(r.view_count).to.equal(1);
+ // Disable view_count updates to handle perf issues (for now) (#44359)
+ expect(r.view_count).to.equal(0);
});
secondQuestion().then(r => {
- expect(r.view_count).to.equal(1);
+ // Disable view_count updates to handle perf issues (for now) (#44359)
+ expect(r.view_count).to.equal(0);
});
// Visit first tab and confirm only first card was queried
@@ -468,10 +470,12 @@ describe("scenarios > dashboard > tabs", () => {
cy.get("@secondTabQuerySpy").should("not.have.been.called");
cy.wait("@firstTabQuery").then(r => {
firstQuestion().then(r => {
- expect(r.view_count).to.equal(3); // 1 (previously) + 1 (firstQuestion) + 1 (firstTabQuery)
+ // Disable view_count updates to handle perf issues (for now) (#44359)
+ expect(r.view_count).to.equal(0); // 3 = 1 (previously) + 1 (firstQuestion) + 1 (firstTabQuery)
});
secondQuestion().then(r => {
- expect(r.view_count).to.equal(2); // 1 (previously) + 1 (secondQuestion)
+ // Disable view_count updates to handle perf issues (for now) (#44359)
+ expect(r.view_count).to.equal(0); // 2 = 1 (previously) + 1 (secondQuestion)
});
});
@@ -481,10 +485,12 @@ describe("scenarios > dashboard > tabs", () => {
cy.get("@firstTabQuerySpy").should("have.been.calledOnce");
cy.wait("@secondTabQuery").then(r => {
firstQuestion().then(r => {
- expect(r.view_count).to.equal(4); // 3 (previously) + 1 (firstQuestion)
+ // Disable view_count updates to handle perf issues (for now) (#44359)
+ expect(r.view_count).to.equal(0); // 4 = 3 (previously) + 1 (firstQuestion)
});
secondQuestion().then(r => {
- expect(r.view_count).to.equal(4); // 2 (previously) + 1 (secondQuestion) + 1 (secondTabQuery)
+ // Disable view_count updates to handle perf issues (for now) (#44359)
+ expect(r.view_count).to.equal(0); // 4 = 2 (previously) + 1 (secondQuestion) + 1 (secondTabQuery)
});
});
@@ -495,10 +501,12 @@ describe("scenarios > dashboard > tabs", () => {
cy.get("@secondTabQuerySpy").should("have.been.calledOnce");
firstQuestion().then(r => {
- expect(r.view_count).to.equal(5); // 4 (previously) + 1 (firstQuestion)
+ // Disable view_count updates to handle perf issues (for now) (#44359)
+ expect(r.view_count).to.equal(0); // 5 = 4 (previously) + 1 (firstQuestion)
});
secondQuestion().then(r => {
- expect(r.view_count).to.equal(5); // 4 (previously) + 1 (secondQuestion)
+ // Disable view_count updates to handle perf issues (for now) (#44359)
+ expect(r.view_count).to.equal(0); // 5 = 4 (previously) + 1 (secondQuestion)
});
// Go to public dashboard
@@ -528,10 +536,12 @@ describe("scenarios > dashboard > tabs", () => {
cy.get("@publicSecondTabQuerySpy").should("not.have.been.called");
cy.wait("@publicFirstTabQuery").then(r => {
firstQuestion().then(r => {
- expect(r.view_count).to.equal(7); // 5 (previously) + 1 (firstQuestion) + 1 (publicFirstTabQuery)
+ // Disable view_count updates to handle perf issues (for now) (#44359)
+ expect(r.view_count).to.equal(0); // 7 = 5 (previously) + 1 (firstQuestion) + 1 (publicFirstTabQuery)
});
secondQuestion().then(r => {
- expect(r.view_count).to.equal(6); // 5 (previously) + 1 (secondQuestion)
+ // Disable view_count updates to handle perf issues (for now) (#44359)
+ expect(r.view_count).to.equal(0); // 6 = 5 (previously) + 1 (secondQuestion)
});
});
@@ -541,10 +551,12 @@ describe("scenarios > dashboard > tabs", () => {
cy.get("@publicFirstTabQuerySpy").should("have.been.calledOnce");
cy.wait("@publicSecondTabQuery").then(r => {
firstQuestion().then(r => {
- expect(r.view_count).to.equal(8); // 7 (previously) + 1 (firstQuestion)
+ // Disable view_count updates to handle perf issues (for now) (#44359)
+ expect(r.view_count).to.equal(0); // 8 = 7 (previously) + 1 (firstQuestion)
});
secondQuestion().then(r => {
- expect(r.view_count).to.equal(8); // 6 (previously) + 1 (secondQuestion) + 1 (publicSecondTabQuery)
+ // Disable view_count updates to handle perf issues (for now) (#44359)
+ expect(r.view_count).to.equal(0); // 8 = 6 (previously) + 1 (secondQuestion) + 1 (publicSecondTabQuery)
});
});
@@ -554,10 +566,12 @@ describe("scenarios > dashboard > tabs", () => {
cy.get("@publicSecondTabQuerySpy").should("have.been.calledOnce");
cy.wait("@publicFirstTabQuery").then(r => {
firstQuestion().then(r => {
- expect(r.view_count).to.equal(10); // 8 (previously) + 1 (firstQuestion) + 1 (publicFirstTabQuery)
+ // Disable view_count updates to handle perf issues (for now) (#44359)
+ expect(r.view_count).to.equal(0); // 10 = 8 (previously) + 1 (firstQuestion) + 1 (publicFirstTabQuery)
});
secondQuestion().then(r => {
- expect(r.view_count).to.equal(9); // 8 (previously) + 1 (secondQuestion)
+ // Disable view_count updates to handle perf issues (for now) (#44359)
+ expect(r.view_count).to.equal(0); // 9 = 8 (previously) + 1 (secondQuestion)
});
});
});
@@ -593,10 +607,12 @@ describe("scenarios > dashboard > tabs", () => {
};
firstQuestion().then(r => {
- expect(r.view_count).to.equal(1); // 1 (firstQuestion)
+ // Disable view_count updates to handle perf issues (for now) (#44359)
+ expect(r.view_count).to.equal(0); // 1 (firstQuestion)
});
secondQuestion().then(r => {
- expect(r.view_count).to.equal(1); // 1 (secondQuestion)
+ // Disable view_count updates to handle perf issues (for now) (#44359)
+ expect(r.view_count).to.equal(0); // 1 (secondQuestion)
});
cy.intercept(
@@ -624,10 +640,12 @@ describe("scenarios > dashboard > tabs", () => {
cy.wait("@firstTabQuery").then(r => {
firstQuestion().then(r => {
- expect(r.view_count).to.equal(3); // 1 (previously) + 1 (firstQuestion) + 1 (first tab query)
+ // Disable view_count updates to handle perf issues (for now) (#44359)
+ expect(r.view_count).to.equal(0); // 3 = 1 (previously) + 1 (firstQuestion) + 1 (first tab query)
});
secondQuestion().then(r => {
- expect(r.view_count).to.equal(2); // 1 (previously) + 1 (secondQuestion)
+ // Disable view_count updates to handle perf issues (for now) (#44359)
+ expect(r.view_count).to.equal(0); // 2 = 1 (previously) + 1 (secondQuestion)
});
});
@@ -636,10 +654,12 @@ describe("scenarios > dashboard > tabs", () => {
cy.get("@firstTabQuerySpy").should("have.been.calledOnce");
cy.wait("@secondTabQuery").then(r => {
firstQuestion().then(r => {
- expect(r.view_count).to.equal(4); // 3 (previously) + 1 (firstQuestion)
+ // Disable view_count updates to handle perf issues (for now) (#44359)
+ expect(r.view_count).to.equal(0); // 4 = 3 (previously) + 1 (firstQuestion)
});
secondQuestion().then(r => {
- expect(r.view_count).to.equal(4); // 2 (previously) + 1 (secondQuestion) + 1 (second tab query)
+ // Disable view_count updates to handle perf issues (for now) (#44359)
+ expect(r.view_count).to.equal(0); // 4 = 2 (previously) + 1 (secondQuestion) + 1 (second tab query)
});
});
@@ -649,10 +669,12 @@ describe("scenarios > dashboard > tabs", () => {
cy.get("@secondTabQuerySpy").should("have.been.calledOnce");
cy.wait("@firstTabQuery").then(r => {
firstQuestion().then(r => {
- expect(r.view_count).to.equal(6); // 4 (previously) + 1 (firstQuestion) + 1 (first tab query)
+ // Disable view_count updates to handle perf issues (for now) (#44359)
+ expect(r.view_count).to.equal(0); // 6 = 4 (previously) + 1 (firstQuestion) + 1 (first tab query)
});
secondQuestion().then(r => {
- expect(r.view_count).to.equal(5); // 4 (previously) + 1 (secondQuestion)
+ // Disable view_count updates to handle perf issues (for now) (#44359)
+ expect(r.view_count).to.equal(0); // 5 = 4 (previously) + 1 (secondQuestion)
});
});
});
diff --git a/src/metabase/events/view_log.clj b/src/metabase/events/view_log.clj
index ef129e6e14e..287bdc81831 100644
--- a/src/metabase/events/view_log.clj
+++ b/src/metabase/events/view_log.clj
@@ -15,7 +15,9 @@
(defn increment-view-counts!
"Increments the view_count column for a model given a list of ids.
Assumes the model has one primary key `id`, and the column for the view count is named `view_count`"
- [model & ids]
+ [_model & _ids]
+ ;; Disable view_count updates to handle perf issues (for now) (#44359)
+ #_
(when (seq ids)
(t2/query {:update (t2/table-name model)
:set {:view_count [:+ :view_count [:inline 1]]}
diff --git a/src/metabase/query_processor/execute.clj b/src/metabase/query_processor/execute.clj
index eb0b2006f21..446aafd653e 100644
--- a/src/metabase/query_processor/execute.clj
+++ b/src/metabase/query_processor/execute.clj
@@ -4,6 +4,7 @@
[metabase.query-processor.middleware.cache :as cache]
[metabase.query-processor.middleware.enterprise :as qp.middleware.enterprise]
[metabase.query-processor.middleware.permissions :as qp.perms]
+ #_
[metabase.query-processor.middleware.update-used-cards :as update-used-cards]
[metabase.query-processor.pipeline :as qp.pipeline]
[metabase.query-processor.schema :as qp.schema]
@@ -41,7 +42,7 @@
e.g.
(f (f query rff)) -> (f query rff)"
- [#'update-used-cards/update-used-cards!
+ [#_#'update-used-cards/update-used-cards! ;; Disable report_card updates to handle perf issues (for now) (#44359)
#'add-native-form-to-result-metadata
#'add-preprocessed-query-to-result-metadata-for-userland-query
#'cache/maybe-return-cached-results
diff --git a/test/metabase/api/activity_test.clj b/test/metabase/api/activity_test.clj
index 37923c0eb04..ad63412ca21 100644
--- a/test/metabase/api/activity_test.clj
+++ b/test/metabase/api/activity_test.clj
@@ -46,7 +46,7 @@
{:topic :event/table-read :event {:object table-1}}]]
(events/publish-event! topic (assoc event :user-id (mt/user->id :crowberto))))
(testing "most_recently_viewed_dashboard endpoint shows the current user's most recently viewed dashboard."
- (is (= (assoc dash-3 :collection nil :view_count 1) #_dash-2 ;; TODO: this should be dash-2, because dash-3 is archived
+ (is (= (assoc dash-3 :collection nil :view_count 0) #_dash-2 ;; TODO: this should be dash-2, because dash-3 is archived
(mt/user-http-request :crowberto :get 200 "activity/most_recently_viewed_dashboard")))))
(mt/with-test-user :rasta
(testing "If nothing has been viewed, return a 204"
@@ -54,7 +54,7 @@
"activity/most_recently_viewed_dashboard"))))
(events/publish-event! :event/dashboard-read {:object-id (:id dash-1) :user-id (mt/user->id :rasta)})
(testing "Only the user's own views are returned."
- (is (= (assoc dash-1 :collection nil :view_count 2)
+ (is (= (assoc dash-1 :collection nil :view_count 0)
(mt/user-http-request :rasta :get 200
"activity/most_recently_viewed_dashboard"))))
(events/publish-event! :event/dashboard-read {:object-id (:id dash-1) :user-id (mt/user->id :rasta)})
@@ -72,13 +72,13 @@
(testing "view a dashboard in a personal collection"
(events/publish-event! :event/dashboard-read {:object-id (:id dash-1) :user-id (mt/user->id :crowberto)})
(let [crowberto-personal-coll (t2/select-one :model/Collection :personal_owner_id (mt/user->id :crowberto))]
- (is (= (assoc dash-1 :collection (assoc crowberto-personal-coll :is_personal true) :view_count 1)
+ (is (= (assoc dash-1 :collection (assoc crowberto-personal-coll :is_personal true) :view_count 0)
(mt/user-http-request :crowberto :get 200
"activity/most_recently_viewed_dashboard")))))
(testing "view a dashboard in a public collection"
(events/publish-event! :event/dashboard-read {:object-id (:id dash-2) :user-id (mt/user->id :crowberto)})
- (is (= (assoc dash-2 :collection (assoc coll :is_personal false) :view_count 1)
+ (is (= (assoc dash-2 :collection (assoc coll :is_personal false) :view_count 0)
(mt/user-http-request :crowberto :get 200
"activity/most_recently_viewed_dashboard"))))))))
diff --git a/test/metabase/api/dashboard_test.clj b/test/metabase/api/dashboard_test.clj
index b005f7aa93f..efa7b30bbe4 100644
--- a/test/metabase/api/dashboard_test.clj
+++ b/test/metabase/api/dashboard_test.clj
@@ -771,7 +771,7 @@
:creator_id (mt/user->id :rasta)
:collection false
:collection_id true
- :view_count 1})
+ :view_count 0})
(dashboard-response (t2/select-one Dashboard :id dashboard-id)))))
(testing "No-op PUT: Do not return 500"
diff --git a/test/metabase/events/view_log_test.clj b/test/metabase/events/view_log_test.clj
index ddc233354d3..3819e48eae9 100644
--- a/test/metabase/events/view_log_test.clj
+++ b/test/metabase/events/view_log_test.clj
@@ -100,6 +100,8 @@
(testing "Card read events are not recorded when viewing a dashboard"
(is (nil? (latest-view (u/id user) (u/id card))))))))))
+;; Disable view_count updates to handle perf issues (for now) (#44359)
+#_
(deftest card-read-view-count-test
(mt/with-temp [:model/User user {}
:model/Card card {:creator_id (u/id user)}]
@@ -111,6 +113,8 @@
(events/publish-event! :event/card-read {:object-id (:id card) :user-id (u/the-id user) :context :question})
(is (= 2 (t2/select-one-fn :view_count :model/Card (:id card)))))))
+;; Disable view_count updates to handle perf issues (for now) (#44359)
+#_
(deftest dashboard-read-view-count-test
(mt/with-temp [:model/User user {}
:model/Dashboard dashboard {:creator_id (u/id user)}
@@ -127,6 +131,8 @@
(events/publish-event! :event/dashboard-read {:object-id (:id dashboard) :user-id (u/the-id user)})
(is (= 2 (t2/select-one-fn :view_count :model/Dashboard (:id dashboard))))))))
+;; Disable view_count updates to handle perf issues (for now) (#44359)
+#_
(deftest table-read-view-count-test
(mt/with-temp [:model/User user {}
:model/Table table {}]
diff --git a/test/metabase/query_processor/middleware/update_used_cards_test.clj b/test/metabase/query_processor/middleware/update_used_cards_test.clj
index ec5dae055d2..d79ed0f1719 100644
--- a/test/metabase/query_processor/middleware/update_used_cards_test.clj
+++ b/test/metabase/query_processor/middleware/update_used_cards_test.clj
@@ -8,6 +8,7 @@
[metabase.query-processor.pipeline :as qp.pipeline]
[metabase.query-processor.util :as qp.util]
[metabase.test :as mt]
+ #_
[toucan2.core :as t2]))
(defmacro with-used-cards-setup
@@ -16,18 +17,23 @@
qp.util/*execute-async?* false]
~@body))
+#_
(defn- card-last-used-at
[card-id]
(t2/select-one-fn :last_used_at :model/Card card-id))
(defn do-test
"Check if `last_used_at` of `card-id` is nil, then execute `f`, then check that `last_used_at` is non nil."
- [card-id thunk]
+ [_card-id thunk]
(assert (fn? thunk))
(testing "last_used_at should be nil to start with"
+ ;; Disable last_used_at updates to handle perf issues (for now) (#44359)
+ #_
(is (nil? (card-last-used-at card-id))))
(thunk)
(testing "last_used_at should be updated to non nil"
+ ;; Disable last_used_at updates to handle perf issues (for now) (#44359)
+ #_
(is (some? (card-last-used-at card-id)))))
(deftest ^:parallel nested-cards-test
--
GitLab