SDK embedding prometheus (#48578)

* add prometheus tracking for sdk-embedding

* indentation

* improve track-sdk-response api

* register :metabase-sdk/response-{ok,error} + test

* avoid cyclic dependency whe nusing prometheus

fixes:
[ /metabase/api/common ]
<- /metabase/models/setting
<- /metabase/analytics/settings
<- /metabase/analytics/prometheus
<- /metabase/analytics/sdk
<- /metabase/models/view_log
<- /metabase/events/schema
<- /metabase/events
<- [ /metabase/api/common ]
<- /metabase/public_settings/premium_features
<- /metabase/auth_provider
<- /metabase/driver
<- /dev/debug_qp
<- /dev

* nix dependency: events.schema --> models.view-log

- make them both read context from view-log-impl instead

* cljfmt

src/metabase/analytics/prometheus.clj

@@ -198,7 +198,11 @@
   [(prometheus/counter :metabase-email/messages
                        {:description (trs "Number of emails sent.")})
    (prometheus/counter :metabase-email/message-errors
-                       {:description (trs "Number of errors when sending emails.")})])
+                       {:description (trs "Number of errors when sending emails.")})
+   (prometheus/counter :metabase-sdk/response-ok
+                       {:description (trs "Number of successful SDK requests.")})
+   (prometheus/counter :metabase-sdk/response-error
+                       {:description (trs "Number of errors when responding to SDK requests.")})])
 
 (defn- setup-metrics!
   "Instrument the application. Conditionally done when some setting is set. If [[prometheus-server-port]] is not set it

src/metabase/analytics/sdk.clj

@@ -8,7 +8,8 @@
 
   then we can use the information on the tables to track information about the embedding client,
   and TODO: send it out in `summarize-execution`."
-  (:require [metabase.util.malli :as mu]))
+  (:require [metabase.analytics.prometheus :as prometheus]
+            [metabase.util.malli :as mu]))
 
 (def ^:dynamic *version* "Used to track information about the metabase embedding client version." nil)
 (def ^:dynamic *client* "Used to track information about the metabase embedding client." nil)
@@ -21,11 +22,36 @@
       (update :embedding_client (fn [client] (or *client* client)))
       (update :embedding_version (fn [version] (or *version* version)))))
 
-(defn bind-embedding-mw
+(mu/defn- categorize-request :- [:maybe [:enum :ok :error]]
+  [{:keys [status]}]
+  (when status
+    (cond
+      (<= 200 status 299) :ok
+      (<= 400 status 599) :error
+      ;; ignore other status codes
+      :else nil)))
+
+(defn- track-sdk-response
+  "Tabulates the number of ok and erroring requests made by clients of the SDK."
+  [category]
+  (case category
+    :ok (prometheus/inc! :metabase-sdk/response-ok)
+    :error (prometheus/inc! :metabase-sdk/response-error)
+    nil nil))
+
+(defn embedding-mw
   "Reads Metabase Client and Version headers and binds them to *metabase-client{-version}*."
   [handler]
-  (fn bound-embedding
+  (fn embedding-mw-fn
     [request respond raise]
-    (binding [*client* (get-in request [:headers "x-metabase-client"])
-              *version* (get-in request [:headers "x-metabase-client-version"])]
-      (handler request respond raise))))
+    (let [sdk-client (get-in request [:headers "x-metabase-client"])
+          version (get-in request [:headers "x-metabase-client-version"])]
+      (binding [*client* sdk-client
+                *version* version]
+        (handler request
+                 (fn responder [response]
+                   (when sdk-client (track-sdk-response (categorize-request response)))
+                   (respond response))
+                 (fn raiser [response]
+                   (when sdk-client (track-sdk-response :error))
+                   (raise response)))))))

src/metabase/events/schema.clj

@@ -2,7 +2,7 @@
   (:require
    [malli.core :as mc]
    [malli.util :as mut]
-   [metabase.models.view-log :as view-log]
+   [metabase.models.view-log-impl :as view-log-impl]
    [metabase.util.malli.schema :as ms]
    [toucan2.core :as t2]))
 
@@ -68,7 +68,7 @@
      :event/card-read   (mc/schema
                          [:map {:closed true}
                           ;; context is deliberately coupled to view-log's context
-                          [:context [:and :some ::view-log/context]]
+                          [:context view-log-impl/context]
                           [:user-id [:maybe pos-int?]]
                           [:object-id [:maybe pos-int?]]])
      :event/card-query  [:map {:closed true}

src/metabase/models/view_log.clj

@@ -3,6 +3,7 @@
   (:require
    [metabase.analytics.sdk :as sdk]
    [metabase.models.interface :as mi]
+   [metabase.models.view-log-impl :as view-log-impl]
    [metabase.util.malli :as mu]
    [metabase.util.malli.fn :as mu.fn]
    [metabase.util.malli.registry :as mr]
@@ -21,8 +22,7 @@
   (derive ::mi/read-policy.always-allow)
   (derive ::mi/write-policy.always-allow))
 
-(mr/def ::context
-  [:maybe [:enum :dashboard :question :collection]])
+(mr/def ::context [:maybe view-log-impl/context])
 
 (t2/define-before-insert :model/ViewLog
   [log-entry]

src/metabase/models/view_log_impl.clj

+(ns metabase.models.view-log-impl)
+
+(def context
+  "The context of a view log entry. In other words, what sort of page generated this view?."
+  [:enum :dashboard :question :collection])

src/metabase/server/handler.clj

@@ -57,7 +57,7 @@
    #'mw.session/reset-session-timeout           ; Resets the timeout cookie for user activity to [[mw.session/session-timeout]]
    #'mw.session/bind-current-user               ; Binds *current-user* and *current-user-id* if :metabase-user-id is non-nil
    #'mw.session/wrap-current-user-info          ; looks for :metabase-session-id and sets :metabase-user-id and other info if Session ID is valid
-   #'sdk/bind-embedding-mw                      ; reads Metabase Client and Version headers and binds them to sdk/*client* and sdk/*version*
+   #'sdk/embedding-mw                           ; reads sdk client headers, binds them to *client* and *version*, and tracks sdk-response metrics
    #'mw.session/wrap-session-id                 ; looks for a Metabase Session ID and assoc as :metabase-session-id
    #'wrap-cookies                               ; Parses cookies in the request map and assocs as :cookies
    #'mw.misc/add-content-type                   ; Adds a Content-Type header for any response that doesn't already have one

test/metabase/server/middleware/sdk_test.clj

@@ -2,11 +2,12 @@
   (:require
    [clojure.string :as str]
    [clojure.test :refer [are deftest is]]
+   [metabase.analytics.prometheus :as prometheus]
    [metabase.analytics.sdk :as sdk]
    [metabase.util :as u]
    [ring.mock.request :as ring.mock]))
 
-(defn wonk-case [s]
+(defn- wonk-case [s]
   (str/join (for [char s]
               (let [f (if (rand-nth [true false]) u/upper-case-en u/lower-case-en)]
                 (f char)))))
@@ -20,25 +21,53 @@
 (deftest bind-client-test
   (are [client]
        (let [request (mock-request {:client client})
-             handler (sdk/bind-embedding-mw
-                      (fn [_request respond _raise] (respond client)))]
-         (handler request
-                  (fn [response] (= sdk/*client* response))
-                  (fn [e] (throw e))))
+             handler (sdk/embedding-mw
+                      (fn [_ respond _] (respond {:status 200 :body client})))
+             response (handler request identity identity)]
+         (is (= (:body response "no-body") client)))
     nil
     "embedding-iframe"))
 
 (deftest bind-client-version-test
   (are [version]
        (let [request (mock-request {:version version})
-             handler (sdk/bind-embedding-mw
-                      (fn [_request respond _raise] (respond version)))]
-         (handler request
-                  (fn [response] (=  sdk/*version* response))
-                  (fn [e] (throw e))))
+             handler (sdk/embedding-mw
+                      (fn [_ respond _] (respond {:status 200 :body version})))
+             response (handler request identity identity)]
+         (is (= (:body response "no-body") version)))
     nil
     "1.1.1"))
 
+(deftest embeding-mw-bumps-metrics-with-sdk-header
+  (let [prometheus-standin (atom {})]
+    (with-redefs [prometheus/inc! (fn [k] (swap! prometheus-standin update k (fnil inc 0)))]
+      (let [request (mock-request {:client "my-client"}) ;; <-- has X-Metabase-Client header => SDK context
+            good (sdk/embedding-mw (fn [_ respond _] (respond {:status 200})))
+            bad (sdk/embedding-mw (fn [_ respond _] (respond {:status 400})))
+            exception (sdk/embedding-mw (fn [_ _respond raise] (raise {})))]
+        (good request identity identity)
+        (is (= {:metabase-sdk/response-ok 1} @prometheus-standin))
+        (bad request identity identity)
+        (is (= {:metabase-sdk/response-ok 1
+                :metabase-sdk/response-error 1} @prometheus-standin))
+        (exception request identity identity)
+        (is (= {:metabase-sdk/response-ok 1
+                :metabase-sdk/response-error 2} @prometheus-standin))))))
+
+(deftest embeding-mw-does-not-bump-sdk-metrics-without-sdk-header
+  (let [prometheus-standin (atom {})]
+    (with-redefs [prometheus/inc! (fn [k] (swap! prometheus-standin update k (fnil inc 0)))]
+      (let [request (mock-request {}) ;; <= no X-Metabase-Client header => no SDK context
+            good (sdk/embedding-mw (fn [_ respond _] (respond {:status 200})))
+            bad (sdk/embedding-mw (fn [_ respond _] (respond {:status 400})))
+            exception (sdk/embedding-mw (fn [_ _respond raise] (raise {})))]
+        (good request identity identity)
+        (is (= {} @prometheus-standin))
+        (bad request identity identity)
+        (is (= {} @prometheus-standin))
+        (exception request identity identity)
+        (is (= {} @prometheus-standin))))))
+
 (deftest include-analytics-is-idempotent
   (let [m (atom {})]
     (binding [sdk/*client* "client-C"