From 94496eb579f860385e38847cfc6d169a4525c008 Mon Sep 17 00:00:00 2001
From: bryan <bryan.maass@gmail.com>
Date: Wed, 16 Oct 2024 12:00:20 -0600
Subject: [PATCH] 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 | 6 ++-
src/metabase/analytics/sdk.clj | 38 ++++++++++++---
src/metabase/events/schema.clj | 4 +-
src/metabase/models/view_log.clj | 4 +-
src/metabase/models/view_log_impl.clj | 5 ++
src/metabase/server/handler.clj | 2 +-
test/metabase/server/middleware/sdk_test.clj | 51 +++++++++++++++-----
7 files changed, 87 insertions(+), 23 deletions(-)
create mode 100644 src/metabase/models/view_log_impl.clj
diff --git a/src/metabase/analytics/prometheus.clj b/src/metabase/analytics/prometheus.clj
index 7ef9cf4fa32..7b449ad4120 100644
--- a/src/metabase/analytics/prometheus.clj
+++ b/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
diff --git a/src/metabase/analytics/sdk.clj b/src/metabase/analytics/sdk.clj
index df6109a8da3..208033559a2 100644
--- a/src/metabase/analytics/sdk.clj
+++ b/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)))))))
diff --git a/src/metabase/events/schema.clj b/src/metabase/events/schema.clj
index 28eaf3c787f..1a2f0c9a6d2 100644
--- a/src/metabase/events/schema.clj
+++ b/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}
diff --git a/src/metabase/models/view_log.clj b/src/metabase/models/view_log.clj
index 4302b1833ed..b00e23263ae 100644
--- a/src/metabase/models/view_log.clj
+++ b/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]
diff --git a/src/metabase/models/view_log_impl.clj b/src/metabase/models/view_log_impl.clj
new file mode 100644
index 00000000000..040bba22669
--- /dev/null
+++ b/src/metabase/models/view_log_impl.clj
@@ -0,0 +1,5 @@
+(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])
diff --git a/src/metabase/server/handler.clj b/src/metabase/server/handler.clj
index 6220fc31afe..4db434eb0f3 100644
--- a/src/metabase/server/handler.clj
+++ b/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
diff --git a/test/metabase/server/middleware/sdk_test.clj b/test/metabase/server/middleware/sdk_test.clj
index 572ed27d1e8..87f85236d48 100644
--- a/test/metabase/server/middleware/sdk_test.clj
+++ b/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"
--
GitLab