Count and Categorize Public Views in query executions table (#51079) (#51125)

* We need to look at the context field to categorize public views

* pull poll-unil into util ns

* fix comment typo

* add test

* update docstring

* remove true? call, it will return `true` on pass

* improve poll-until

Co-authored-by: bryan <bryan.maass@gmail.com>

src/metabase/analytics/stats.clj

@@ -600,19 +600,19 @@
   (u/prog1 eid-translation/default-counter
     (setting/set-value-of-type! :json :entity-id-translation-counter <>)))
 
-(defn- categorize-query-execution [{client :embedding_client executor :executor_id}]
+(defn- categorize-query-execution [{:keys [context embedding_client executor_id]}]
   (cond
-    (= "embedding-sdk-react" client)                     "sdk_embed"
-    (and (= "embedding-iframe" client) (some? executor)) "interactive_embed"
-    (and (= "embedding-iframe" client) (nil? executor))  "static_embed"
-    (and (#{"" nil} client) (nil? executor))             "public_link"
-    :else                                                "internal"))
+    (= "embedding-sdk-react" embedding_client)                        "sdk_embed"
+    (and (= "embedding-iframe" embedding_client) (some? executor_id)) "interactive_embed"
+    (and (= "embedding-iframe" embedding_client) (nil? executor_id))  "static_embed"
+    (some-> context name (str/starts-with? "public-"))                "public_link"
+    :else                                                             "internal"))
 
 (defn- ->one-day-ago []
   (t/minus (t/offset-date-time) (t/days 1)))
 
 (defn- ->snowplow-grouped-metric-info []
-  (let [qe (t2/select [:model/QueryExecution :embedding_client :executor_id :started_at])
+  (let [qe (t2/select [:model/QueryExecution :embedding_client :context :executor_id :started_at])
         one-day-ago (->one-day-ago)
         ;; reuse the query data:
         qe-24h (filter (fn [{started-at :started_at}] (t/after? one-day-ago started-at)) qe)]

test/metabase/api/card_test.clj

@@ -42,6 +42,7 @@
    [metabase.task.sync-databases :as task.sync-databases]
    [metabase.test :as mt]
    [metabase.test.data.users :as test.users]
+   [metabase.test.util :as tu]
    [metabase.upload-test :as upload-test]
    [metabase.util :as u]
    [toucan2.core :as t2]
@@ -311,25 +312,6 @@
         (is (= {:embedding_client client-string, :embedding_version version-string}
                (t2/select-one [:model/ViewLog :embedding_client :embedding_version] :model "card" :model_id (u/the-id card-1))))))))
 
-(defn- do-poll-until [^Long timeout-ms thunk]
-  (let [result-prom (promise)
-        _timeouter (future (Thread/sleep timeout-ms) (deliver result-prom ::timeout))
-        _runner (future (loop []
-                          (if-let [thunk-return (try (thunk) (catch Exception e e))]
-                            (deliver result-prom thunk-return)
-                            (recur))))
-        result @result-prom]
-    (cond (= result ::timeout) (throw (ex-info (str "Timeout after " timeout-ms "ms")
-                                               {:timeout-ms timeout-ms}))
-          (instance? Throwable result) (throw result)
-          :else result)))
-
-(defmacro ^:private poll-until
-  "A macro that continues to call the given body until it returns a truthy value or the timeout is reached.
-  Returns the truthy body, or re-throws any exception raised in body. Hence, this cannot be used to return nil, false, or a Throwable."
-  [timeout-ms & body]
-  `(do-poll-until ~timeout-ms (fn [] ~@body)))
-
 (deftest embedding-sdk-info-saves-query-execution
   (testing "GET /api/card with embedding headers set"
     (mt/with-temp [:model/Card card-1 {:name "Card 1"
@@ -343,9 +325,9 @@
                                                          "x-metabase-client-version" "2"}}})
       (is (=? {:embedding_client "client-B", :embedding_version "2"}
               ;; The query metadata is handled asynchronously, so we need to poll until it's available:
-              (poll-until 100
-                          (t2/select-one [:model/QueryExecution :embedding_client :embedding_version]
-                                         :card_id (u/the-id card-1))))))))
+              (tu/poll-until 100
+                             (t2/select-one [:model/QueryExecution :embedding_client :embedding_version]
+                                            :card_id (u/the-id card-1))))))))
 
 (deftest filter-by-bookmarked-test
   (testing "Filter by `bookmarked`"

test/metabase/api/public_test.clj

@@ -8,6 +8,7 @@
    [clojure.test :refer :all]
    [dk.ative.docjure.spreadsheet :as spreadsheet]
    [metabase.analytics.snowplow-test :as snowplow-test]
+   [metabase.analytics.stats :as stats]
    [metabase.api.card-test :as api.card-test]
    [metabase.api.dashboard-test :as api.dashboard-test]
    [metabase.api.pivots :as api.pivots]
@@ -25,6 +26,7 @@
    [metabase.query-processor :as qp]
    [metabase.query-processor.middleware.process-userland-query-test :as process-userland-query-test]
    [metabase.test :as mt]
+   [metabase.test.util :as tu]
    [metabase.util :as u]
    [throttle.core :as throttle]
    [toucan2.core :as t2]
@@ -61,7 +63,7 @@
                                   :values_source_config {:values ["African" "American" "Asian"]}}]})
                  (shared-obj)
                  m)]
-    (t2.with-temp/with-temp [Card card m]
+    (t2.with-temp/with-temp [:model/Card card m]
       ;; add :public_uuid back in to the value that gets bound because it might not come back from post-select if
       ;; public sharing is disabled; but we still want to test it
       (f (assoc card :public_uuid (:public_uuid m))))))
@@ -144,6 +146,22 @@
             (is (= "Not found."
                    (client/client :get 404 (str "public/card/" uuid))))))))))
 
+(deftest public-queries-are-counted-test
+  (testing "GET /api/public/card/:uuid/query coutns as a public query"
+    (mt/with-temporary-setting-values [enable-public-sharing true]
+      (with-temp-public-card [{uuid :public_uuid}]
+        (testing "should increment the public link query count when fetching a public Card"
+          (let [get-qe-count (fn get-qe-count [] (get-in (#'stats/->snowplow-grouped-metric-info)
+                                                         [:query-executions "public_link"]))
+                qe-count-before (get-qe-count)]
+            (client/client :get 202 (str "public/card/" uuid "/query"))
+            ;; The qe-count gets incremented asynchronously, so we need to poll until it's updated.
+            ;; We poll for 300ms, which should be enough time for the count to be updated.
+            ;; Once the qe-count is updated the test will pass, and stop polling.
+            ;; If it's not updated within 300ms, the test will fail.
+            (testing "the count should be incremented within 300 ms:"
+              (is (tu/poll-until 300 (> (get-qe-count) qe-count-before))))))))))
+
 (deftest make-sure-param-values-get-returned-as-expected
   (let [category-name-id (mt/id :categories :name)]
     (t2.with-temp/with-temp [Card card {:dataset_query

test/metabase/http_client.clj

@@ -435,7 +435,7 @@
    *  `query-params`         Key-value pairs that will be encoded and added to the URL as query params
 
   Note: One benefit of [[client]] over [[real-client]] is the call site and API execution are on the same thread,
-  so it's possible to run a test inside a transaction and bindings will works."
+  so it's possible to run a test inside a transaction and bindings will work."
   {:arglists '([credentials? method expected-status-code? endpoint request-options? http-body-map? & {:as query-params}])}
   [& args]
   (:body (apply client-full-response args)))

test/metabase/test/util.clj

@@ -1516,3 +1516,28 @@
   [bindings & body]
   `(fn [{:keys ~(mapv (comp symbol name) bindings)}]
      ~@body))
+
+(defn do-poll-until [^Long timeout-ms thunk]
+  (let [result-prom (promise)
+        _timeouter (future (Thread/sleep timeout-ms) (deliver result-prom ::timeout))
+        _runner (future (loop []
+                          (if-let [thunk-return (try (thunk) (catch Exception e e))]
+                            (deliver result-prom thunk-return)
+                            (recur))))
+        result @result-prom]
+    (cond (= result ::timeout) (throw (ex-info (str "Timeout after " timeout-ms "ms")
+                                               {:timeout-ms timeout-ms}))
+          (instance? Throwable result) (throw result)
+          :else result)))
+
+(defmacro poll-until
+  "A macro that continues to call the given body until it returns a truthy value or the timeout is reached.
+  Returns the truthy body, or re-throws any exception raised in body.
+
+  Hence, this cannot return nil, false, or a Throwable. [[thunk]] can check for those instead.
+
+  Pro tip: wrap your body with `time` macro to get a feel for how many calls to [[poll-body]] are made."
+  [timeout-ms & body]
+  `(do-poll-until
+    ~timeout-ms
+    (fn ~'poll-body [] ~@body)))