From 2767fce7adff7bf4a18fb1229e02abb137698172 Mon Sep 17 00:00:00 2001
From: Ngoc Khuat <qn.khuat@gmail.com>
Date: Tue, 16 Jul 2024 17:38:50 +0700
Subject: [PATCH] Batch processing usage metadata (#45007)
---
.clj-kondo/config.edn | 3 +
deps.edn | 1 +
e2e/test/scenarios/dashboard/tabs.cy.spec.js | 78 +++++-----
.../middleware/update_used_cards_test.clj | 19 +--
src/metabase/events/view_log.clj | 53 +++++--
src/metabase/lib/expression.cljc | 3 +-
src/metabase/metabot.clj | 3 +-
src/metabase/models/data_permissions.clj | 2 +-
src/metabase/query_processor/execute.clj | 4 +-
.../middleware/process_userland_query.clj | 18 ++-
.../middleware/update_used_cards.clj | 42 ++++--
src/metabase/util.cljc | 20 ++-
src/metabase/util/grouper.clj | 51 +++++++
.../xrays/automagic_dashboards/comparison.clj | 50 +++----
test/metabase/api/activity_test.clj | 114 ++++++++-------
test/metabase/api/dashboard_test.clj | 138 +++++++++---------
test/metabase/events/view_log_test.clj | 109 ++++++++------
.../process_userland_query_test.clj | 42 +++---
.../middleware/update_used_cards_test.clj | 94 +++++++-----
test/metabase/test/util/async.clj | 4 +-
test/metabase/util/grouper_test.clj | 19 +++
21 files changed, 527 insertions(+), 340 deletions(-)
create mode 100644 src/metabase/util/grouper.clj
create mode 100644 test/metabase/util/grouper_test.clj
diff --git a/.clj-kondo/config.edn b/.clj-kondo/config.edn
index f67ee94adcc..d4d4eecd728 100644
--- a/.clj-kondo/config.edn
+++ b/.clj-kondo/config.edn
@@ -115,6 +115,7 @@
:discouraged-namespace
{camel-snake-kebab.core {:message "CSK is not Turkish-safe, use the versions in metabase.util instead."}
clojure.tools.logging {:message "Use metabase.util.log instead of clojure.tools.logging directly"}
+ grouper.core {:message "Use metabase.util.grouper instead of grouper.core"}
honeysql.core {:message "Use Honey SQL 2. Honey SQL 1 is removed as a dependency in Metabase 49+."}
honeysql.format {:message "Use Honey SQL 2. Honey SQL 1 is removed as a dependency in Metabase 49+."}
honeysql.helpers {:message "Use Honey SQL 2. Honey SQL 1 is removed as a dependency in Metabase 49+."}
@@ -579,6 +580,7 @@
metabase.util.encryption encryption
metabase.util.encryption-test encryption-test
metabase.util.files u.files
+ metabase.util.grouper grouper
metabase.util.honey-sql-2 h2x
metabase.util.i18n i18n
metabase.util.i18n.impl i18n.impl
@@ -657,6 +659,7 @@
metabase.test/with-temp-empty-app-db clojure.core/let
metabase.test/with-temp-file clojure.core/let
metabase.test/with-user-in-groups clojure.core/let
+ metabase.test/with-grouper-batches! clojure.core/fn
metabase.upload-test/with-upload-table! clojure.core/let
metabase.util.files/with-open-path-to-resource clojure.core/let
metabase.util.malli.defn/defn schema.core/defn
diff --git a/deps.edn b/deps.edn
index f9fbb07473c..57656e1cd1c 100644
--- a/deps.edn
+++ b/deps.edn
@@ -71,6 +71,7 @@
hiccup/hiccup {:mvn/version "1.0.5"} ; HTML templating
inflections/inflections {:mvn/version "0.14.1"} ; Clojure/Script library used for prularizing words
instaparse/instaparse {:mvn/version "1.4.12"} ; Make your own parser
+ junegunn/grouper {:mvn/version "0.1.1"} ; Batch processing helper
clj-commons/clj-yaml {:mvn/version "1.0.27"} ; Clojure wrapper for YAML library SnakeYAML
io.github.camsaul/toucan2 {:mvn/version "1.0.538"}
io.github.eerohele/pp {:git/tag "2024-01-04.60" ; super fast pretty-printing library
diff --git a/e2e/test/scenarios/dashboard/tabs.cy.spec.js b/e2e/test/scenarios/dashboard/tabs.cy.spec.js
index 307912a350c..52e76e1e02f 100644
--- a/e2e/test/scenarios/dashboard/tabs.cy.spec.js
+++ b/e2e/test/scenarios/dashboard/tabs.cy.spec.js
@@ -91,10 +91,22 @@ const TAB_2 = {
name: "Tab 2",
};
+const changeSynchronousBatchUpdateSetting = value => {
+ cy.request("PUT", "/api/setting/synchronous-batch-updates", {
+ value: value,
+ });
+};
+
describe("scenarios > dashboard > tabs", () => {
beforeEach(() => {
restore();
cy.signInAsAdmin();
+ changeSynchronousBatchUpdateSetting(true);
+ });
+
+ afterEach(() => {
+ cy.signInAsAdmin();
+ changeSynchronousBatchUpdateSetting(false);
});
it("should only display cards on the selected tab", () => {
@@ -455,12 +467,10 @@ describe("scenarios > dashboard > tabs", () => {
};
firstQuestion().then(r => {
- // Disable view_count updates to handle perf issues (for now) (#44359)
- expect(r.view_count).to.equal(0);
+ expect(r.view_count).to.equal(1);
});
secondQuestion().then(r => {
- // Disable view_count updates to handle perf issues (for now) (#44359)
- expect(r.view_count).to.equal(0);
+ expect(r.view_count).to.equal(1);
});
// Visit first tab and confirm only first card was queried
@@ -470,12 +480,10 @@ describe("scenarios > dashboard > tabs", () => {
cy.get("@secondTabQuerySpy").should("not.have.been.called");
cy.wait("@firstTabQuery").then(r => {
firstQuestion().then(r => {
- // 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)
+ expect(r.view_count).to.equal(3); // 1 (previously) + 1 (firstQuestion) + 1 (firstTabQuery)
});
secondQuestion().then(r => {
- // Disable view_count updates to handle perf issues (for now) (#44359)
- expect(r.view_count).to.equal(0); // 2 = 1 (previously) + 1 (secondQuestion)
+ expect(r.view_count).to.equal(2); // 1 (previously) + 1 (secondQuestion)
});
});
@@ -485,12 +493,10 @@ describe("scenarios > dashboard > tabs", () => {
cy.get("@firstTabQuerySpy").should("have.been.calledOnce");
cy.wait("@secondTabQuery").then(r => {
firstQuestion().then(r => {
- // Disable view_count updates to handle perf issues (for now) (#44359)
- expect(r.view_count).to.equal(0); // 4 = 3 (previously) + 1 (firstQuestion)
+ expect(r.view_count).to.equal(4); // 3 (previously) + 1 (firstQuestion)
});
secondQuestion().then(r => {
- // 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)
+ expect(r.view_count).to.equal(4); // 2 (previously) + 1 (secondQuestion) + 1 (secondTabQuery)
});
});
@@ -501,12 +507,10 @@ describe("scenarios > dashboard > tabs", () => {
cy.get("@secondTabQuerySpy").should("have.been.calledOnce");
firstQuestion().then(r => {
- // Disable view_count updates to handle perf issues (for now) (#44359)
- expect(r.view_count).to.equal(0); // 5 = 4 (previously) + 1 (firstQuestion)
+ expect(r.view_count).to.equal(5); // 4 (previously) + 1 (firstQuestion)
});
secondQuestion().then(r => {
- // Disable view_count updates to handle perf issues (for now) (#44359)
- expect(r.view_count).to.equal(0); // 5 = 4 (previously) + 1 (secondQuestion)
+ expect(r.view_count).to.equal(5); // 4 (previously) + 1 (secondQuestion)
});
// Go to public dashboard
@@ -536,12 +540,10 @@ describe("scenarios > dashboard > tabs", () => {
cy.get("@publicSecondTabQuerySpy").should("not.have.been.called");
cy.wait("@publicFirstTabQuery").then(r => {
firstQuestion().then(r => {
- // 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)
+ expect(r.view_count).to.equal(7); // 5 (previously) + 1 (firstQuestion) + 1 (publicFirstTabQuery)
});
secondQuestion().then(r => {
- // Disable view_count updates to handle perf issues (for now) (#44359)
- expect(r.view_count).to.equal(0); // 6 = 5 (previously) + 1 (secondQuestion)
+ expect(r.view_count).to.equal(6); // 5 (previously) + 1 (secondQuestion)
});
});
@@ -551,12 +553,10 @@ describe("scenarios > dashboard > tabs", () => {
cy.get("@publicFirstTabQuerySpy").should("have.been.calledOnce");
cy.wait("@publicSecondTabQuery").then(r => {
firstQuestion().then(r => {
- // Disable view_count updates to handle perf issues (for now) (#44359)
- expect(r.view_count).to.equal(0); // 8 = 7 (previously) + 1 (firstQuestion)
+ expect(r.view_count).to.equal(8); // 7 (previously) + 1 (firstQuestion)
});
secondQuestion().then(r => {
- // 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)
+ expect(r.view_count).to.equal(8); // 6 (previously) + 1 (secondQuestion) + 1 (publicSecondTabQuery)
});
});
@@ -566,12 +566,10 @@ describe("scenarios > dashboard > tabs", () => {
cy.get("@publicSecondTabQuerySpy").should("have.been.calledOnce");
cy.wait("@publicFirstTabQuery").then(r => {
firstQuestion().then(r => {
- // 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)
+ expect(r.view_count).to.equal(10); // 8 (previously) + 1 (firstQuestion) + 1 (publicFirstTabQuery)
});
secondQuestion().then(r => {
- // Disable view_count updates to handle perf issues (for now) (#44359)
- expect(r.view_count).to.equal(0); // 9 = 8 (previously) + 1 (secondQuestion)
+ expect(r.view_count).to.equal(9); // 8 (previously) + 1 (secondQuestion)
});
});
});
@@ -607,12 +605,10 @@ describe("scenarios > dashboard > tabs", () => {
};
firstQuestion().then(r => {
- // Disable view_count updates to handle perf issues (for now) (#44359)
- expect(r.view_count).to.equal(0); // 1 (firstQuestion)
+ expect(r.view_count).to.equal(1); // 1 (firstQuestion)
});
secondQuestion().then(r => {
- // Disable view_count updates to handle perf issues (for now) (#44359)
- expect(r.view_count).to.equal(0); // 1 (secondQuestion)
+ expect(r.view_count).to.equal(1); // 1 (secondQuestion)
});
cy.intercept(
@@ -640,12 +636,10 @@ describe("scenarios > dashboard > tabs", () => {
cy.wait("@firstTabQuery").then(r => {
firstQuestion().then(r => {
- // 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)
+ expect(r.view_count).to.equal(3); // 1 (previously) + 1 (firstQuestion) + 1 (first tab query)
});
secondQuestion().then(r => {
- // Disable view_count updates to handle perf issues (for now) (#44359)
- expect(r.view_count).to.equal(0); // 2 = 1 (previously) + 1 (secondQuestion)
+ expect(r.view_count).to.equal(2); // 1 (previously) + 1 (secondQuestion)
});
});
@@ -654,12 +648,10 @@ describe("scenarios > dashboard > tabs", () => {
cy.get("@firstTabQuerySpy").should("have.been.calledOnce");
cy.wait("@secondTabQuery").then(r => {
firstQuestion().then(r => {
- // Disable view_count updates to handle perf issues (for now) (#44359)
- expect(r.view_count).to.equal(0); // 4 = 3 (previously) + 1 (firstQuestion)
+ expect(r.view_count).to.equal(4); // 3 (previously) + 1 (firstQuestion)
});
secondQuestion().then(r => {
- // 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)
+ expect(r.view_count).to.equal(4); // 2 (previously) + 1 (secondQuestion) + 1 (second tab query)
});
});
@@ -669,12 +661,10 @@ describe("scenarios > dashboard > tabs", () => {
cy.get("@secondTabQuerySpy").should("have.been.calledOnce");
cy.wait("@firstTabQuery").then(r => {
firstQuestion().then(r => {
- // 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)
+ expect(r.view_count).to.equal(6); // 4 (previously) + 1 (firstQuestion) + 1 (first tab query)
});
secondQuestion().then(r => {
- // Disable view_count updates to handle perf issues (for now) (#44359)
- expect(r.view_count).to.equal(0); // 5 = 4 (previously) + 1 (secondQuestion)
+ expect(r.view_count).to.equal(5); // 4 (previously) + 1 (secondQuestion)
});
});
});
diff --git a/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/update_used_cards_test.clj b/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/update_used_cards_test.clj
index 36b682bd504..18a7cbf9a0c 100644
--- a/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/update_used_cards_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/sandbox/query_processor/middleware/update_used_cards_test.clj
@@ -8,12 +8,13 @@
[toucan2.core :as t2]))
(deftest sandboxing-test
- (qp.updated-used-cards-test/with-used-cards-setup
- (met/with-gtaps! {:gtaps {:categories {:query (mt/mbql-query categories {:filter [:< $id 3]})}}}
- (let [gtap-card-id (:id (t2/query-one {:select [:c.id]
- :from [[:report_card :c]]
- :left-join [[:sandboxes :s] [:= :s.card_id :c.id]]
- :where [:= :s.group_id (:id &group)]}))]
- (qp.updated-used-cards-test/do-test
- gtap-card-id
- #(qp/process-query (mt/mbql-query categories)))))))
+ (mt/test-helpers-set-global-values!
+ (qp.updated-used-cards-test/with-used-cards-setup!
+ (met/with-gtaps! {:gtaps {:categories {:query (mt/mbql-query categories {:filter [:< $id 3]})}}}
+ (let [gtap-card-id (:id (t2/query-one {:select [:c.id]
+ :from [[:report_card :c]]
+ :left-join [[:sandboxes :s] [:= :s.card_id :c.id]]
+ :where [:= :s.group_id (:id &group)]}))]
+ (qp.updated-used-cards-test/do-test!
+ gtap-card-id
+ #(qp/process-query (mt/mbql-query categories))))))))
diff --git a/src/metabase/events/view_log.clj b/src/metabase/events/view_log.clj
index 287bdc81831..e03741270b9 100644
--- a/src/metabase/events/view_log.clj
+++ b/src/metabase/events/view_log.clj
@@ -7,21 +7,54 @@
[metabase.models.query.permissions :as query-perms]
[metabase.public-settings.premium-features :as premium-features]
[metabase.util :as u]
+ [metabase.util.grouper :as grouper]
[metabase.util.log :as log]
[methodical.core :as m]
[steffan-westcott.clj-otel.api.trace.span :as span]
[toucan2.core :as t2]))
-(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]
- ;; 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]]}
- :where [:in :id ids]})))
+(defn- group-by-frequency
+ "Given a list of items, returns a map of frequencies to items.
+ (group-by-frequency [:a :a :b :b :c :c :c])
+ ;; => {2 [:a :b] 3 [:c]}"
+ [items]
+ (reduce (fn [acc [item cnt]]
+ (update acc cnt u/conjv item))
+ {}
+ (frequencies items)))
+
+(defn- increment-view-counts!*
+ [items]
+ (log/debugf "Increment view counts of %d items" (count items))
+ (try
+ (let [model->ids (reduce (fn [acc {:keys [id model]}]
+ (update acc model conj id))
+ {}
+ items)]
+ (doseq [[model ids] model->ids]
+ (let [cnt->ids (group-by-frequency ids)]
+ (t2/query {:update (t2/table-name model)
+ :set {:view_count [:+ :view_count (into [:case]
+ (mapcat (fn [[cnt ids]]
+ [[:in :id ids] cnt])
+ cnt->ids))]}
+ :where [:in :id (apply concat (vals cnt->ids))]}))))
+ (catch Exception e
+ (log/error e "Failed to increment view counts"))))
+
+(def ^:private increment-view-count-interval-seconds 20)
+
+(defonce ^:private
+ increase-view-count-queue
+ (delay (grouper/start!
+ increment-view-counts!*
+ :capacity 500
+ :interval (* increment-view-count-interval-seconds 1000))))
+
+(defn- increment-view-counts!
+ "Increment the view count of the given `model` and `model-id`."
+ [model model-id]
+ (grouper/submit! @increase-view-count-queue {:model model :id model-id}))
(defn- record-views!
"Simple base function for recording a view of a given `model` and `model-id` by a certain `user`."
diff --git a/src/metabase/lib/expression.cljc b/src/metabase/lib/expression.cljc
index 24a1baaf360..a62a1277415 100644
--- a/src/metabase/lib/expression.cljc
+++ b/src/metabase/lib/expression.cljc
@@ -21,6 +21,7 @@
[metabase.lib.util.match :as lib.util.match]
[metabase.shared.util.i18n :as i18n]
[metabase.types :as types]
+ [metabase.util :as u]
[metabase.util.malli :as mu]
[metabase.util.malli.registry :as mr]))
@@ -201,7 +202,7 @@
[stage
expression
{:keys [add-to-fields?], :or {add-to-fields? true}, :as _options}]
- (cond-> (update stage :expressions (fnil conj []) expression)
+ (cond-> (update stage :expressions u/conjv expression)
;; if there are explicit fields selected, add the expression to them
(and (vector? (:fields stage))
add-to-fields?)
diff --git a/src/metabase/metabot.clj b/src/metabase/metabot.clj
index a64cb1ea06a..65bf6f17c14 100644
--- a/src/metabase/metabot.clj
+++ b/src/metabase/metabot.clj
@@ -9,6 +9,7 @@
[metabase.metabot.settings :as metabot-settings]
[metabase.metabot.util :as metabot-util]
[metabase.models :refer [Table]]
+ [metabase.util :as u]
[metabase.util.log :as log]
[potemkin :as p]
[toucan2.core :as t2]))
@@ -117,7 +118,7 @@
database-id user_prompt best-model-id)
(update model
:prompt_template_versions
- (fnil conj [])
+ u/conjv
(format "%s:%s" prompt_template version)))
(log/infof "No model inferred for database '%s' with prompt '%s'." database-id user_prompt)))
(log/warn "Metabot is not enabled")))
diff --git a/src/metabase/models/data_permissions.clj b/src/metabase/models/data_permissions.clj
index 89f64ca69ec..8a0fc5b72f1 100644
--- a/src/metabase/models/data_permissions.clj
+++ b/src/metabase/models/data_permissions.clj
@@ -108,7 +108,7 @@
[:data_permissions :p] [:= :p.group_id :pg.id]]
:where [:= :pgm.user_id user-id]})
(reduce (fn [m {:keys [user_id perm_type db_id] :as row}]
- (update-in m [user_id perm_type db_id] (fnil conj []) row))
+ (update-in m [user_id perm_type db_id] u/conjv row))
{})))
(defn- relevant-permissions-for-user-perm-and-db
diff --git a/src/metabase/query_processor/execute.clj b/src/metabase/query_processor/execute.clj
index c683835c2bc..38b0eaf8d25 100644
--- a/src/metabase/query_processor/execute.clj
+++ b/src/metabase/query_processor/execute.clj
@@ -1,10 +1,10 @@
(ns metabase.query-processor.execute
(:require
- #_[metabase.query-processor.middleware.update-used-cards :as update-used-cards]
[metabase.lib.schema.id :as lib.schema.id]
[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]
[metabase.query-processor.setup :as qp.setup]
@@ -43,7 +43,7 @@
e.g.
(f (f query rff)) -> (f query rff)"
- [#_#'update-used-cards/update-used-cards! ;; Disable report_card updates to handle perf issues (for now) (#44359)
+ [#'update-used-cards/update-used-cards!
#'add-native-form-to-result-metadata
#'add-preprocessed-query-to-result-metadata-for-userland-query
#'cache/maybe-return-cached-results
diff --git a/src/metabase/query_processor/middleware/process_userland_query.clj b/src/metabase/query_processor/middleware/process_userland_query.clj
index ce7a0e732cd..435962026b8 100644
--- a/src/metabase/query_processor/middleware/process_userland_query.clj
+++ b/src/metabase/query_processor/middleware/process_userland_query.clj
@@ -20,6 +20,7 @@
#_
[metabase.query-processor.store :as qp.store]
[metabase.query-processor.util :as qp.util]
+ [metabase.util.grouper :as grouper]
[metabase.util.log :as log]
[metabase.util.malli :as mu]
#_{:clj-kondo/ignore [:discouraged-namespace]}
@@ -37,6 +38,19 @@
;;; | Save Query Execution |
;;; +----------------------------------------------------------------------------------------------------------------+
+(def ^:private field-usage-interval-seconds 20)
+
+(defonce ^:private
+ field-usages-queue
+ (delay (grouper/start!
+ (fn [field-usages]
+ (try
+ (t2/insert! :model/FieldUsage field-usages)
+ (catch Throwable e
+ (log/error e "Error saving field usages"))))
+ :capacity 500
+ :interval (* field-usage-interval-seconds 1000))))
+
;; TODO - I'm not sure whether this should happen async as is currently the case, or should happen synchronously e.g.
;; in the completing arity of the rf
;;
@@ -51,7 +65,9 @@
(log/warn "Cannot save QueryExecution, missing :context")
(let [qe-id (t2/insert-returning-pk! QueryExecution (dissoc query-execution :json_query))]
(when (seq field-usages)
- (t2/insert! :model/FieldUsage (map #(assoc % :query_execution_id qe-id) field-usages))))))
+ (let [queue @field-usages-queue]
+ (doseq [field-usage field-usages]
+ (grouper/submit! queue (assoc field-usage :query_execution_id qe-id))))))))
(defn- save-execution-metadata!
"Save a `QueryExecution` row containing `execution-info`. Done asynchronously when a query is finished."
diff --git a/src/metabase/query_processor/middleware/update_used_cards.clj b/src/metabase/query_processor/middleware/update_used_cards.clj
index 2ff9d7d3812..9cef0aaec94 100644
--- a/src/metabase/query_processor/middleware/update_used_cards.clj
+++ b/src/metabase/query_processor/middleware/update_used_cards.clj
@@ -1,9 +1,10 @@
(ns metabase.query-processor.middleware.update-used-cards
(:require
+ [java-time.api :as t]
[metabase.lib.metadata :as lib.metadata]
[metabase.query-processor.schema :as qp.schema]
[metabase.query-processor.store :as qp.store]
- [metabase.query-processor.util :as qp.util]
+ [metabase.util.grouper :as grouper]
[metabase.util.log :as log]
[metabase.util.malli :as mu]
#_{:clj-kondo/ignore [:discouraged-namespace]}
@@ -11,15 +12,29 @@
(set! *warn-on-reflection* true)
+(def ^:private update-used-card-interval-seconds 20)
+
(defn- update-used-cards!*
- [used-card-ids]
- (when (seq used-card-ids)
- (qp.util/with-execute-async
- (fn []
- (try
- (t2/update! :model/Card :id [:in used-card-ids] {:last_used_at :%now})
- (catch Throwable e
- (log/error e "Error updating used cards")))))))
+ [card-id-timestamps]
+ (let [card-id->timestamp (update-vals (group-by :id card-id-timestamps)
+ (fn [xs] (apply t/max (map :timestamp xs))))]
+ (log/debugf "Update last_used_at of %d cards" (count card-id->timestamp))
+ (try
+ (t2/update! :model/Card :id [:in (keys card-id->timestamp)]
+ {:last_used_at (into [:case]
+ (mapcat (fn [[id timestamp]]
+ [[:= :id id] [:greatest [:coalesce :last_used_at (t/offset-date-time 0)] timestamp]])
+ card-id->timestamp))})
+ (catch Throwable e
+ (log/error e "Error updating used cards")))))
+
+(defonce ^:private
+ update-used-cards-queue
+ (delay
+ (grouper/start!
+ update-used-cards!*
+ :capacity 500
+ :interval (* update-used-card-interval-seconds 1000))))
(mu/defn update-used-cards! :- ::qp.schema/qp
"Middleware that get all card-ids that were used during a query execution and updates their `last_used_at`.
@@ -34,7 +49,10 @@
[qp :- ::qp.schema/qp]
(mu/fn [query :- ::qp.schema/query
rff :- ::qp.schema/rff]
- (letfn [(rff* [metadata]
- (update-used-cards!* (set (lib.metadata/invoked-ids (qp.store/metadata-provider) :metadata/card)))
- (rff metadata))]
+ (let [now (t/offset-date-time)
+ rff* (fn [metadata]
+ (doseq [card-id (distinct (lib.metadata/invoked-ids (qp.store/metadata-provider) :metadata/card))]
+ (grouper/submit! @update-used-cards-queue {:id card-id
+ :timestamp now}))
+ (rff metadata))]
(qp query rff*))))
diff --git a/src/metabase/util.cljc b/src/metabase/util.cljc
index d99b39efb63..3c9bcc0d836 100644
--- a/src/metabase/util.cljc
+++ b/src/metabase/util.cljc
@@ -1,14 +1,14 @@
(ns metabase.util
"Common utility functions useful throughout the codebase."
(:require
- #?@(:clj ([clojure.math.numeric-tower :as math]
- [me.flowthing.pp :as pp]
- [metabase.config :as config]
- #_{:clj-kondo/ignore [:discouraged-namespace]}
- [metabase.util.jvm :as u.jvm]
- [metabase.util.string :as u.str]
- [potemkin :as p]
- [ring.util.codec :as codec]))
+ #?@(:clj ([clojure.math.numeric-tower :as math]
+ [me.flowthing.pp :as pp]
+ [metabase.config :as config]
+ #_{:clj-kondo/ignore [:discouraged-namespace]}
+ [metabase.util.jvm :as u.jvm]
+ [metabase.util.string :as u.str]
+ [potemkin :as p]
+ [ring.util.codec :as codec]))
[camel-snake-kebab.internals.macros :as csk.macros]
[clojure.data :refer [diff]]
[clojure.pprint :as pprint]
@@ -1009,6 +1009,10 @@
m))]
(with-meta ret (meta m)))))
+(def conjv
+ "Like `conj` but returns a vector instead of a list"
+ (fnil conj []))
+
(defn string-byte-count
"Number of bytes in a string using UTF-8 encoding."
[s]
diff --git a/src/metabase/util/grouper.clj b/src/metabase/util/grouper.clj
new file mode 100644
index 00000000000..e94866b693a
--- /dev/null
+++ b/src/metabase/util/grouper.clj
@@ -0,0 +1,51 @@
+(ns metabase.util.grouper
+ "Our wrapper for grouper -- the batch processing utility.
+
+ Note:
+ - These utilities should only be used for scenarios where data consistency is not a requirement,
+ Execution is best effort and may not occur as the batched items are not persisted.
+ - Suitable for use cases that can tolerate lag time in processing. For example, updating
+ last_used_at of cards after a query execution. Things like recording view_log should not use
+ grouper since it's important to have the data immediately available.
+
+
+ Batch processing can be disabled by setting the environment variable `MB_DISABLE_GROUPER_BATCH_PROCESSING=true`."
+ (:require
+ #_{:clj-kondo/ignore [:discouraged-namespace]}
+ [grouper.core :as grouper]
+ [metabase.models.setting :refer [defsetting]]
+ [metabase.util.i18n :refer [deferred-tru]]
+ [potemkin :as p])
+ (:import
+ (grouper.core Grouper)))
+
+(set! *warn-on-reflection* true)
+
+(comment
+ p/keep-me
+ Grouper/keep-me)
+
+(defsetting synchronous-batch-updates
+ (deferred-tru "Process batches updates synchronously. If true, all `submit!` calls will be processed immediately. Default is false.")
+ ;; Should be used for testing purposes only, currently set by some e2e tests
+ :type :boolean
+ :default false
+ :export? true
+ ;; :admin instead of :internal because we want to change this during e2e testing
+ :visibility :admin)
+
+(p/import-vars
+ [grouper
+ start!
+ shutdown!])
+
+(defn submit!
+ "A wrapper of [[grouper.core/submit!]] that returns nil instead of a promise.
+ We use grouper for fire-and-forget scenarios, so we don't care about the result."
+ [& args]
+ (let [p (apply grouper/submit! args)]
+ (when (synchronous-batch-updates)
+ ;; wake up the group immediately and wait for it to finish
+ (.wakeUp ^Grouper (first args))
+ (deref p))
+ nil))
diff --git a/src/metabase/xrays/automagic_dashboards/comparison.clj b/src/metabase/xrays/automagic_dashboards/comparison.clj
index c94a14a4a79..7766da8eb77 100644
--- a/src/metabase/xrays/automagic_dashboards/comparison.clj
+++ b/src/metabase/xrays/automagic_dashboards/comparison.clj
@@ -92,15 +92,15 @@
(update :name #(format "%s (%s)" % (comparison-name right)))
vector)]
(update dashboard :dashcards conj (merge (populate/card-defaults)
- {:col 0
- :row row
- :size_x populate/grid-width
- :size_y height
- :card card
- :card_id (:id card)
- :series series
- :visualization_settings {:graph.y_axis.auto_split false
- :graph.series_labels [(:name card) (:name (first series))]}})))
+ {:col 0
+ :row row
+ :size_x populate/grid-width
+ :size_y height
+ :card card
+ :card_id (:id card)
+ :series series
+ :visualization_settings {:graph.y_axis.auto_split false
+ :graph.series_labels [(:name card) (:name (first series))]}})))
(let [width (/ populate/grid-width 2)
series-left (map clone-card (:series card-left))
series-right (map clone-card (:series card-right))
@@ -112,23 +112,23 @@
(assoc-in [:visualization_settings :graph.colors] [color-right]))]
(-> dashboard
(update :dashcards conj (merge (populate/card-defaults)
- {:col 0
- :row row
- :size_x width
- :size_y height
- :card card-left
- :card_id (:id card-left)
- :series series-left
- :visualization_settings {}}))
+ {:col 0
+ :row row
+ :size_x width
+ :size_y height
+ :card card-left
+ :card_id (:id card-left)
+ :series series-left
+ :visualization_settings {}}))
(update :dashcards conj (merge (populate/card-defaults)
- {:col width
- :row row
- :size_x width
- :size_y height
- :card card-right
- :card_id (:id card-right)
- :series series-right
- :visualization_settings {}}))))))
+ {:col width
+ :row row
+ :size_x width
+ :size_y height
+ :card card-right
+ :card_id (:id card-right)
+ :series series-right
+ :visualization_settings {}}))))))
(populate/add-text-card dashboard {:text (:text card)
:width (/ populate/grid-width 2)
diff --git a/test/metabase/api/activity_test.clj b/test/metabase/api/activity_test.clj
index 5bac8459909..51d6f9a2c83 100644
--- a/test/metabase/api/activity_test.clj
+++ b/test/metabase/api/activity_test.clj
@@ -24,63 +24,67 @@
(deftest most-recently-viewed-dashboard-views-test
(clear-recent-views-for-user :crowberto)
(clear-recent-views-for-user :rasta)
- (mt/with-temp [Card card-1 {:name "rand-name"
- :creator_id (mt/user->id :crowberto)
- :display "table"}
- Dashboard dash-1 {:name "rand-name2"
- :description "rand-name2"
- :creator_id (mt/user->id :crowberto)}
- Dashboard dash-2 {:name "rand-name2"
- :description "rand-name2"
- :creator_id (mt/user->id :crowberto)}
- Dashboard dash-3 {:name "rand-name2"
- :description "rand-name2"
- :archived true
- :creator_id (mt/user->id :crowberto)}
- Table table-1 {:name "rand-name"}]
- (mt/with-test-user :crowberto
- (doseq [{:keys [topic event]} [{:topic :event/dashboard-read :event {:object-id (:id dash-1)}}
- {:topic :event/dashboard-read :event {:object-id (:id dash-2)}}
- {:topic :event/dashboard-read :event {:object-id (:id dash-3)}}
- {:topic :event/card-query :event {:card-id (:id card-1)}}
- {: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 non-archived dashboard."
- (is (= (assoc dash-2 :collection nil :view_count 0)
- (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"
- (is (nil? (mt/user-http-request :rasta :get 204
- "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 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)})
- (testing "If the user has no permissions for the dashboard, return a 204"
- (mt/with-non-admin-groups-no-root-collection-perms
- (is (nil? (mt/user-http-request :rasta :get 204
- "activity/most_recently_viewed_dashboard"))))))))
+ (mt/test-helpers-set-global-values!
+ (mt/with-temporary-setting-values [synchronous-batch-updates true]
+ (mt/with-temp [Card card-1 {:name "rand-name"
+ :creator_id (mt/user->id :crowberto)
+ :display "table"}
+ Dashboard dash-1 {:name "rand-name2"
+ :description "rand-name2"
+ :creator_id (mt/user->id :crowberto)}
+ Dashboard dash-2 {:name "rand-name2"
+ :description "rand-name2"
+ :creator_id (mt/user->id :crowberto)}
+ Dashboard dash-3 {:name "rand-name2"
+ :description "rand-name2"
+ :archived true
+ :creator_id (mt/user->id :crowberto)}
+ Table table-1 {:name "rand-name"}]
+ (mt/with-test-user :crowberto
+ (doseq [{:keys [topic event]} [{:topic :event/dashboard-read :event {:object-id (:id dash-1)}}
+ {:topic :event/dashboard-read :event {:object-id (:id dash-2)}}
+ {:topic :event/dashboard-read :event {:object-id (:id dash-3)}}
+ {:topic :event/card-query :event {:card-id (:id card-1)}}
+ {: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 non-archived dashboard."
+ (is (= (assoc dash-2 :collection nil :view_count 1)
+ (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"
+ (is (nil? (mt/user-http-request :rasta :get 204
+ "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)
+ (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)})
+ (testing "If the user has no permissions for the dashboard, return a 204"
+ (mt/with-non-admin-groups-no-root-collection-perms
+ (is (nil? (mt/user-http-request :rasta :get 204
+ "activity/most_recently_viewed_dashboard"))))))))))
-(deftest most-recently-viewed-dashboard-views-include-colllection-test
- (mt/with-temp [:model/Collection coll {:name "Analytics"}
- :model/Dashboard dash-1 {:collection_id (t2/select-one-pk :model/Collection :personal_owner_id (mt/user->id :crowberto))}
- :model/Dashboard dash-2 {:collection_id (:id coll)}]
- (mt/with-model-cleanup [:model/RecentViews]
- (mt/with-test-user :crowberto
- (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 0)
- (mt/user-http-request :crowberto :get 200
- "activity/most_recently_viewed_dashboard")))))
+(deftest most-recently-viewed-dashboard-views-include-collection-test
+ (mt/test-helpers-set-global-values!
+ (mt/with-temporary-setting-values [synchronous-batch-updates true]
+ (mt/with-temp [:model/Collection coll {:name "Analytics"}
+ :model/Dashboard dash-1 {:collection_id (t2/select-one-pk :model/Collection :personal_owner_id (mt/user->id :crowberto))}
+ :model/Dashboard dash-2 {:collection_id (:id coll)}]
+ (mt/with-model-cleanup [:model/RecentViews]
+ (mt/with-test-user :crowberto
+ (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)
+ (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 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)
+ (mt/user-http-request :crowberto :get 200
+ "activity/most_recently_viewed_dashboard"))))))))))
(deftest recent-views-test
(clear-recent-views-for-user :crowberto)
diff --git a/test/metabase/api/dashboard_test.clj b/test/metabase/api/dashboard_test.clj
index 978b9c066df..dc5e25a80de 100644
--- a/test/metabase/api/dashboard_test.clj
+++ b/test/metabase/api/dashboard_test.clj
@@ -745,74 +745,76 @@
(deftest update-dashboard-test
(testing "PUT /api/dashboard/:id"
- (t2.with-temp/with-temp [Dashboard {dashboard-id :id} {:name "Test Dashboard"}]
- (with-dashboards-in-writeable-collection [dashboard-id]
- (testing "GET before update"
- (is (= (merge dashboard-defaults {:name "Test Dashboard"
- :creator_id (mt/user->id :rasta)
- :collection false
- :collection_id true})
- (dashboard-response (t2/select-one Dashboard :id dashboard-id)))))
-
- (testing "PUT response"
- (let [put-response (mt/user-http-request :rasta :put 200 (str "dashboard/" dashboard-id)
- {:name "My Cool Dashboard"
- :description "Some awesome description"
- :cache_ttl 1234
- ;; these things should fail to update
- :creator_id (mt/user->id :trashbird)})
- get-response (mt/user-http-request :rasta :get 200 (format "dashboard/%d" dashboard-id))]
- (is (=? (merge dashboard-defaults {:name "My Cool Dashboard"
- :dashcards []
- :tabs []
- :description "Some awesome description"
- :creator_id (mt/user->id :rasta)
- :cache_ttl 1234
- :last-edit-info {:timestamp true :id true :first_name "Rasta"
- :last_name "Toucan" :email "rasta@metabase.com"}
- :collection true
- :collection_id true})
- (dashboard-response put-response)))
- (testing "A PUT should return the updated value so a follow-on GET is not needed (#34828)"
- (is (= (update put-response :last-edit-info dissoc :timestamp)
- (update get-response :last-edit-info dissoc :timestamp))))))
-
- (testing "GET after update"
- (is (= (merge dashboard-defaults {:name "My Cool Dashboard"
- :description "Some awesome description"
- :cache_ttl 1234
- :creator_id (mt/user->id :rasta)
- :collection false
- :collection_id true
- :view_count 0})
- (dashboard-response (t2/select-one Dashboard :id dashboard-id)))))
-
- (testing "No-op PUT: Do not return 500"
- (t2.with-temp/with-temp [Card {card-id :id} {}
- DashboardCard dashcard {:card_id card-id, :dashboard_id dashboard-id}]
- ;; so, you can't actually set `:cards` with THIS endpoint (you have to use PUT /api/dashboard/:id/cards)
- ;; but the e2e tests are trying to do it. With Toucan 1, it would silently do nothing and return truthy for
- ;; whatever reason (I'm guessing it was a bug?) if you did something like (update! Dashboard 1 {}). Toucan 2
- ;; returns falsey, since it doesn't do anything, which is what Toucan 1 SAID it was supposed to do.
- ;;
- ;; In the interest of un-busting the e2e tests let's just check to make sure the endpoint no-ops
- (is (=? {:id dashboard-id}
- (mt/user-http-request :rasta :put 200 (str "dashboard/" dashboard-id)
- {:cards [(select-keys dashcard [:id :card_id :row_col :size_x :size_y])]})))))))
- (testing "auto_apply_filters test"
- (doseq [enabled? [true false]]
- (t2.with-temp/with-temp [Dashboard {dashboard-id :id} {:name "Test Dashboard"
- :auto_apply_filters enabled?}]
- (testing "Can set it"
- (mt/user-http-request :rasta :put 200 (str "dashboard/" dashboard-id)
- {:auto_apply_filters (not enabled?)})
- (is (= (not enabled?)
- (t2/select-one-fn :auto_apply_filters Dashboard :id dashboard-id))))
- (testing "If not in put it is not changed"
- (mt/user-http-request :rasta :put 200 (str "dashboard/" dashboard-id)
- {:description "foo"})
- (is (= (not enabled?)
- (t2/select-one-fn :auto_apply_filters Dashboard :id dashboard-id)))))))))
+ (mt/test-helpers-set-global-values!
+ (mt/with-temporary-setting-values [synchronous-batch-updates true]
+ (t2.with-temp/with-temp [Dashboard {dashboard-id :id} {:name "Test Dashboard"}]
+ (with-dashboards-in-writeable-collection [dashboard-id]
+ (testing "GET before update"
+ (is (= (merge dashboard-defaults {:name "Test Dashboard"
+ :creator_id (mt/user->id :rasta)
+ :collection false
+ :collection_id true})
+ (dashboard-response (t2/select-one Dashboard :id dashboard-id)))))
+
+ (testing "PUT response"
+ (let [put-response (mt/user-http-request :rasta :put 200 (str "dashboard/" dashboard-id)
+ {:name "My Cool Dashboard"
+ :description "Some awesome description"
+ :cache_ttl 1234
+ ;; these things should fail to update
+ :creator_id (mt/user->id :trashbird)})
+ get-response (mt/user-http-request :rasta :get 200 (format "dashboard/%d" dashboard-id))]
+ (is (=? (merge dashboard-defaults {:name "My Cool Dashboard"
+ :dashcards []
+ :tabs []
+ :description "Some awesome description"
+ :creator_id (mt/user->id :rasta)
+ :cache_ttl 1234
+ :last-edit-info {:timestamp true :id true :first_name "Rasta"
+ :last_name "Toucan" :email "rasta@metabase.com"}
+ :collection true
+ :collection_id true})
+ (dashboard-response put-response)))
+ (testing "A PUT should return the updated value so a follow-on GET is not needed (#34828)"
+ (is (= (update put-response :last-edit-info dissoc :timestamp)
+ (update get-response :last-edit-info dissoc :timestamp))))))
+
+ (testing "GET after update"
+ (is (= (merge dashboard-defaults {:name "My Cool Dashboard"
+ :description "Some awesome description"
+ :cache_ttl 1234
+ :creator_id (mt/user->id :rasta)
+ :collection false
+ :collection_id true
+ :view_count 1})
+ (dashboard-response (t2/select-one Dashboard :id dashboard-id)))))
+
+ (testing "No-op PUT: Do not return 500"
+ (t2.with-temp/with-temp [Card {card-id :id} {}
+ DashboardCard dashcard {:card_id card-id, :dashboard_id dashboard-id}]
+ ;; so, you can't actually set `:cards` with THIS endpoint (you have to use PUT /api/dashboard/:id/cards)
+ ;; but the e2e tests are trying to do it. With Toucan 1, it would silently do nothing and return truthy for
+ ;; whatever reason (I'm guessing it was a bug?) if you did something like (update! Dashboard 1 {}). Toucan 2
+ ;; returns falsey, since it doesn't do anything, which is what Toucan 1 SAID it was supposed to do.
+ ;;
+ ;; In the interest of un-busting the e2e tests let's just check to make sure the endpoint no-ops
+ (is (=? {:id dashboard-id}
+ (mt/user-http-request :rasta :put 200 (str "dashboard/" dashboard-id)
+ {:cards [(select-keys dashcard [:id :card_id :row_col :size_x :size_y])]})))))))
+ (testing "auto_apply_filters test"
+ (doseq [enabled? [true false]]
+ (t2.with-temp/with-temp [Dashboard {dashboard-id :id} {:name "Test Dashboard"
+ :auto_apply_filters enabled?}]
+ (testing "Can set it"
+ (mt/user-http-request :rasta :put 200 (str "dashboard/" dashboard-id)
+ {:auto_apply_filters (not enabled?)})
+ (is (= (not enabled?)
+ (t2/select-one-fn :auto_apply_filters Dashboard :id dashboard-id))))
+ (testing "If not in put it is not changed"
+ (mt/user-http-request :rasta :put 200 (str "dashboard/" dashboard-id)
+ {:description "foo"})
+ (is (= (not enabled?)
+ (t2/select-one-fn :auto_apply_filters Dashboard :id dashboard-id)))))))))))
(deftest update-dashboard-guide-columns-test
diff --git a/test/metabase/events/view_log_test.clj b/test/metabase/events/view_log_test.clj
index 3819e48eae9..740785598f4 100644
--- a/test/metabase/events/view_log_test.clj
+++ b/test/metabase/events/view_log_test.clj
@@ -6,6 +6,7 @@
[metabase.api.embed-test :as embed-test]
[metabase.api.public-test :as public-test]
[metabase.events :as events]
+ [metabase.events.view-log :as events.view-log]
[metabase.http-client :as client]
[metabase.models.data-permissions :as data-perms]
[metabase.models.permissions-group :as perms-group]
@@ -13,6 +14,8 @@
[metabase.util :as u]
[toucan2.core :as t2]))
+(set! *warn-on-reflection* true)
+
(defn latest-view
"Returns the most recent view for a given user and model ID"
[user-id model-id]
@@ -100,52 +103,74 @@
(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)}]
- (testing "Card read events are recorded by a card's view_count"
- (is (= 0 (:view_count card))
- "view_count should be 0 before the event is published")
- (events/publish-event! :event/card-read {:object-id (:id card) :user-id (u/the-id user) :context :question})
- (is (= 1 (t2/select-one-fn :view_count :model/Card (:id card))))
- (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)
-#_
+ (mt/test-helpers-set-global-values!
+ (mt/with-temporary-setting-values [synchronous-batch-updates true]
+ (mt/with-temp [:model/User user {}
+ :model/Card card {:creator_id (u/id user)}]
+ (testing "Card read events are recorded by a card's view_count"
+ (is (= 0 (:view_count card))
+ "view_count should be 0 before the event is published")
+ (events/publish-event! :event/card-read {:object-id (:id card) :user-id (u/the-id user) :context :question})
+ (is (= 1 (t2/select-one-fn :view_count :model/Card (:id card))))
+ (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)))))))))
+
(deftest dashboard-read-view-count-test
- (mt/with-temp [:model/User user {}
- :model/Dashboard dashboard {:creator_id (u/id user)}
- :model/Card card {:name "Dashboard Test Card"}
- :model/DashboardCard _dashcard {:dashboard_id (u/id dashboard) :card_id (u/id card)}]
- (let [dashboard (t2/hydrate dashboard [:dashcards :card])]
- (testing "Dashboard read events are recorded by a dashboard's view_count"
- (is (= 0 (:view_count dashboard) (:view_count card))
- "view_count should be 0 before the event is published")
- (events/publish-event! :event/dashboard-read {:object-id (:id dashboard) :user-id (u/the-id user)})
- (is (= 1 (t2/select-one-fn :view_count :model/Dashboard (:id dashboard))))
- (is (= 0 (t2/select-one-fn :view_count :model/Card (:id card)))
- "view_count for cards on the dashboard should not be incremented")
- (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)
-#_
+ (mt/test-helpers-set-global-values!
+ (mt/with-temporary-setting-values [synchronous-batch-updates true]
+ (mt/with-temp [:model/User user {}
+ :model/Dashboard dashboard {:creator_id (u/id user)}
+ :model/Card card {:name "Dashboard Test Card"}
+ :model/DashboardCard _dashcard {:dashboard_id (u/id dashboard) :card_id (u/id card)}]
+ (let [dashboard (t2/hydrate dashboard [:dashcards :card])]
+ (testing "Dashboard read events are recorded by a dashboard's view_count"
+ (is (= 0 (:view_count dashboard) (:view_count card))
+ "view_count should be 0 before the event is published")
+ (events/publish-event! :event/dashboard-read {:object-id (:id dashboard) :user-id (u/the-id user)})
+ (is (= 1 (t2/select-one-fn :view_count :model/Dashboard (:id dashboard))))
+ (is (= 0 (t2/select-one-fn :view_count :model/Card (:id card)))
+ "view_count for cards on the dashboard should not be incremented")
+ (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))))))))))
+
(deftest table-read-view-count-test
- (mt/with-temp [:model/User user {}
- :model/Table table {}]
- (testing "Card read events are recorded by a card's view_count"
- (is (= 0 (:view_count table))
- "view_count should be 0 before the event is published")
- (events/publish-event! :event/table-read {:object table :user-id (u/the-id user)})
- (is (= 1 (t2/select-one-fn :view_count :model/Table (:id table)))
- "view_count should be incremented")
- (events/publish-event! :event/table-read {:object table :user-id (u/the-id user)})
- (is (= 2 (t2/select-one-fn :view_count :model/Table (:id table)))
- "view_count should be incremented"))))
+ (mt/test-helpers-set-global-values!
+ (mt/with-temporary-setting-values [synchronous-batch-updates true]
+ (mt/with-temp [:model/User user {}
+ :model/Table table {}]
+ (testing "Card read events are recorded by a card's view_count"
+ (is (= 0 (:view_count table))
+ "view_count should be 0 before the event is published")
+ (events/publish-event! :event/table-read {:object table :user-id (u/the-id user)})
+ (is (= 1 (t2/select-one-fn :view_count :model/Table (:id table)))
+ "view_count should be incremented")
+ (events/publish-event! :event/table-read {:object table :user-id (u/the-id user)})
+ (is (= 2 (t2/select-one-fn :view_count :model/Table (:id table)))
+ "view_count should be incremented"))))))
+(deftest increment-view-counts!*-test
+ (mt/with-temp [:model/Card {card-1-id :id} {}
+ :model/Card {card-2-id :id} {:view_count 2}
+ :model/Table {table-id :id} {}]
+ (t2/with-call-count [call-count]
+ (testing "increment-view-counts!* update the view_count correctly"
+ (#'events.view-log/increment-view-counts!* [;; table-id : 1 views, card-id-1: 2 views, card-id 2: 2 views
+ {:model :model/Table :id table-id}
+ {:model :model/Card :id card-1-id}
+ {:model :model/Card :id card-1-id}
+ {:model :model/Card :id card-2-id}
+ {:model :model/Card :id card-2-id}])
+ (is (= 2 ;; one for update card, one for table
+ (call-count))
+ "and groups db calls by frequency")
+ (is (= 1 (t2/select-one-fn :view_count :model/Table table-id))
+ "view_count for table-id should be 1")
+ (is (= 2 (t2/select-one-fn :view_count :model/Card card-1-id))
+ "view_count for card-1 should be 2")
+ (is (= 4 ;; 2 old + 2 new
+ (t2/select-one-fn :view_count :model/Card card-2-id))
+ "view_count for card-2 should be 2")))))
;;; ---------------------------------------- API tests begin -----------------------------------------
diff --git a/test/metabase/query_processor/middleware/process_userland_query_test.clj b/test/metabase/query_processor/middleware/process_userland_query_test.clj
index be214857575..bc3cbe1c0d8 100644
--- a/test/metabase/query_processor/middleware/process_userland_query_test.clj
+++ b/test/metabase/query_processor/middleware/process_userland_query_test.clj
@@ -179,26 +179,28 @@
#_
(deftest save-field-usage-test
(testing "execute an userland query will capture field usages"
- (mt/with-model-cleanup [:model/FieldUsage]
- (mt/with-temp [:model/Field {field-id :id} {:table_id (mt/id :products)
- :name "very_interesting_field"
- :base_type :type/Integer}
- :model/Card card {:dataset_query (mt/mbql-query products
- {:filter [:> [:field field-id nil] 1]})}]
- (binding [qp.util/*execute-async?* false
- qp.pipeline/*execute* (fn [_driver _query respond]
- (respond {} []))]
- (mt/user-http-request :crowberto :post 202 (format "/card/%d/query" (:id card)))
- (is (=? [{:filter_op :>
- :breakout_temporal_unit nil
- :breakout_binning_strategy nil
- :breakout_binning_bin_width nil
- :breakout_binning_num_bins nil
- :used_in :filter
- :aggregation_function nil
- :field_id field-id
- :query_execution_id (mt/malli=? pos-int?)}]
- (t2/select :model/FieldUsage :field_id field-id))))))))
+ (mt/test-helpers-set-global-values!
+ (mt/with-model-cleanup [:model/FieldUsage]
+ (mt/with-temporary-setting-values [synchronous-batch-updates true]
+ (mt/with-temp [:model/Field {field-id :id} {:table_id (mt/id :products)
+ :name "very_interesting_field"
+ :base_type :type/Integer}
+ :model/Card card {:dataset_query (mt/mbql-query products
+ {:filter [:> [:field field-id nil] 1]})}]
+ (binding [qp.util/*execute-async?* false
+ qp.pipeline/*execute* (fn [_driver _query respond]
+ (respond {} []))]
+ (mt/user-http-request :crowberto :post 202 (format "/card/%d/query" (:id card)))
+ (is (=? [{:filter_op :>
+ :breakout_temporal_unit nil
+ :breakout_binning_strategy nil
+ :breakout_binning_bin_width nil
+ :breakout_binning_num_bins nil
+ :used_in :filter
+ :aggregation_function nil
+ :field_id field-id
+ :query_execution_id (mt/malli=? pos-int?)}]
+ (t2/select :model/FieldUsage :field_id field-id))))))))))
(deftest query-result-should-not-contains-preprocessed-query-test
(let [query (mt/mbql-query venues {:limit 1})]
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 d79ed0f1719..696021fcd67 100644
--- a/test/metabase/query_processor/middleware/update_used_cards_test.clj
+++ b/test/metabase/query_processor/middleware/update_used_cards_test.clj
@@ -1,74 +1,88 @@
(ns metabase.query-processor.middleware.update-used-cards-test
(:require
[clojure.test :refer :all]
+ [java-time.api :as t]
[metabase.dashboard-subscription-test :as dashboard-subscription-test]
[metabase.pulse :as pulse]
[metabase.pulse-test :as pulse-test]
[metabase.query-processor :as qp]
+ [metabase.query-processor.middleware.update-used-cards :as qp.update-used-cards]
[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
+(defmacro with-used-cards-setup!
[& body]
- `(binding [qp.pipeline/*execute* (fn [_driver# _query# respond#] (respond# {} []))
- qp.util/*execute-async?* false]
- ~@body))
+ `(mt/test-helpers-set-global-values!
+ (binding [qp.pipeline/*execute* (fn [_driver# _query# respond#] (respond# {} []))
+ 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
+(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)))))
+ (let [original-last-used-at (card-last-used-at card-id)]
+ (mt/with-temporary-setting-values [synchronous-batch-updates true]
+ (thunk))
+ (testing "last_used_at should be updated after executing the query"
+ (is (not= original-last-used-at (card-last-used-at card-id))))))
-(deftest ^:parallel nested-cards-test
- (with-used-cards-setup
+(deftest nested-cards-test
+ (with-used-cards-setup!
(mt/with-temp [:model/Card {card-id :id} {:dataset_query (mt/mbql-query venues)}]
- (do-test card-id #(qp/process-query (mt/mbql-query nil {:source-table (format "card__%d" card-id)}))))))
+ (do-test! card-id #(qp/process-query (mt/mbql-query nil {:source-table (format "card__%d" card-id)}))))))
-(deftest ^:parallel joined-card-test
- (with-used-cards-setup
+(deftest joined-card-test
+ (with-used-cards-setup!
(mt/with-temp [:model/Card {card-id :id} {:dataset_query (mt/mbql-query products)}]
- (do-test card-id #(qp/process-query (mt/mbql-query orders {:joins [{:fields "all",
- :source-table (format "card__%d" card-id)
- :condition [:= $orders.product_id &product.products.id]
- :alias "product"}]}))))))
+ (do-test! card-id #(qp/process-query (mt/mbql-query orders {:joins [{:fields "all",
+ :source-table (format "card__%d" card-id)
+ :condition [:= $orders.product_id &product.products.id]
+ :alias "product"}]}))))))
-(deftest ^:parallel card-reference-in-native-query-test
- (with-used-cards-setup
+(deftest card-reference-in-native-query-test
+ (with-used-cards-setup!
(mt/with-temp [:model/Card {card-id :id} {:dataset_query (mt/mbql-query venues)}]
- (do-test card-id #(qp/process-query (mt/native-query {:query "SELECT * FROM {{#card}}"
- :template-tags {"#card" {:card-id card-id
- :display-name "card"
- :id "card"
- :name "card"
- :type "card"}}}))))))
+ (do-test! card-id #(qp/process-query (mt/native-query {:query "SELECT * FROM {{#card}}"
+ :template-tags {"#card" {:card-id card-id
+ :display-name "card"
+ :id "card"
+ :name "card"
+ :type "card"}}}))))))
-(deftest ^:parallel alert-test
- (with-used-cards-setup
+(deftest alert-test
+ (with-used-cards-setup!
(mt/with-temp [:model/Card {card-id :id} {:dataset_query (mt/mbql-query venues)}]
(pulse-test/with-pulse-for-card [pulse {:card card-id}]
- (do-test card-id #(pulse/send-pulse! pulse))))))
+ (do-test! card-id #(pulse/send-pulse! pulse))))))
-(deftest ^:parallel dashboard-subscription-test
- (with-used-cards-setup
+(deftest dashboard-subscription-test
+ (with-used-cards-setup!
(mt/with-temp [:model/Dashboard dash {}
:model/Card {card-id :id} {:dataset_query (mt/mbql-query venues)}]
(dashboard-subscription-test/with-dashboard-sub-for-card [pulse {:card card-id
:dashboard dash}]
- (do-test card-id #(pulse/send-pulse! pulse))))))
+ (do-test! card-id #(pulse/send-pulse! pulse))))))
+
+(deftest update-used-card-timestamp-test
+ (let [now (t/offset-date-time)
+ one-hour-ago (t/minus now (t/hours 1))
+ two-hours-ago (t/minus now (t/hours 2))]
+ (testing "update with multiple cards of the same ids will set timestamp to the latest"
+ (mt/with-temp
+ [:model/Card {card-id-1 :id} {:last_used_at two-hours-ago}]
+ (#'qp.update-used-cards/update-used-cards!* [{:id card-id-1 :timestamp two-hours-ago}
+ {:id card-id-1 :timestamp one-hour-ago}])
+ (is (= one-hour-ago (t2/select-one-fn :last_used_at :model/Card card-id-1)))))
+
+ (testing "if the existing last_used_at is greater than the updating values, do not override it"
+ (mt/with-temp
+ [:model/Card {card-id-2 :id} {:last_used_at now}]
+ (#'qp.update-used-cards/update-used-cards!* [{:id card-id-2 :timestamp one-hour-ago}])
+ (is (= now (t2/select-one-fn :last_used_at :model/Card card-id-2)))))))
diff --git a/test/metabase/test/util/async.clj b/test/metabase/test/util/async.clj
index 6c3d9a8d3a3..9fa75330304 100644
--- a/test/metabase/test/util/async.clj
+++ b/test/metabase/test/util/async.clj
@@ -1,7 +1,9 @@
-(ns metabase.test.util.async
+(ns ^{:added "0.50.0"} metabase.test.util.async
(:require
[clojure.core.async :as a]))
+(set! *warn-on-reflection* true)
+
(defmacro with-open-channels
"Like [[with-open]], but closes core.async channels at the conclusion of `body`."
[[binding chan & more] & body]
diff --git a/test/metabase/util/grouper_test.clj b/test/metabase/util/grouper_test.clj
new file mode 100644
index 00000000000..de5c875abdb
--- /dev/null
+++ b/test/metabase/util/grouper_test.clj
@@ -0,0 +1,19 @@
+(ns metabase.util.grouper-test
+ (:require
+ [clojure.test :refer :all]
+ [metabase.test :as mt]
+ [metabase.util :as u]
+ [metabase.util.grouper :as grouper]))
+
+(deftest synchronous-batch-updates-test
+ (testing "with grouper disabled, the submitted item should be processed immediately"
+ (mt/with-temporary-setting-values [synchronous-batch-updates true]
+ (let [processed? (atom nil)
+ g (grouper/start!
+ (fn [items]
+ (reset! processed? items))
+ :capacity 5
+ :interval (* 10 1000))]
+ (u/with-timeout 1000
+ (grouper/submit! g 1))
+ (is (= [1] @processed?))))))
--
GitLab