Harden insights (#12289)

src/metabase/sync/analyze/fingerprint/fingerprinters.clj

@@ -108,7 +108,8 @@
        (reduced result#)
        result#)))
 
-(defn- with-error-handling
+(defn with-error-handling
+  "Wrap `rf` in an error-catching transducer."
   [rf msg]
   (fn
     ([] (with-reduced-error msg (rf)))

src/metabase/sync/analyze/fingerprint/insights.clj

@@ -8,7 +8,10 @@
             [metabase.mbql.util :as mbql.u]
             [metabase.models.field :as field]
             [metabase.sync.analyze.fingerprint.fingerprinters :as f]
-            [metabase.util.date-2 :as u.date]
+            [metabase.sync.util :as sync-util]
+            [metabase.util
+             [date-2 :as u.date]
+             [i18n :refer [trs]]]
             [redux.core :as redux])
   (:import [java.time Instant LocalDate LocalDateTime LocalTime OffsetDateTime OffsetTime ZonedDateTime]))
 
@@ -178,8 +181,6 @@
       Instant        (t/to-millis-from-epoch t)
       OffsetDateTime (t/to-millis-from-epoch t)
       ZonedDateTime  (t/to-millis-from-epoch t)
-      ;; TODO - really not convinced this behavior makes sense. Not sure what `xfn` below is actually supposed to be
-      ;; doing? This roughly matches the old behavior when we were using `java.util.Date`.
       LocalDate      (->millis-from-epoch (t/offset-date-time t (t/local-time 0) (t/zone-offset 0)))
       LocalDateTime  (->millis-from-epoch (t/offset-date-time t (t/zone-offset 0)))
       LocalTime      (->millis-from-epoch (t/offset-date-time (t/local-date "1970-01-01") t (t/zone-offset 0)))
@@ -195,31 +196,34 @@
                             f/->temporal
                             ->millis-from-epoch
                             ms->day)]
-    (apply redux/juxt
-           (for [number-col numbers]
-             (redux/post-complete
-              (let [y-position (:position number-col)
-                    yfn        #(nth % y-position)]
-                (redux/juxt ((map yfn) (last-n 2))
-                            ((map xfn) (last-n 2))
-                            (stats/simple-linear-regression xfn yfn)
-                            (best-fit xfn yfn)))
-              (fn [[[y-previous y-current] [x-previous x-current] [offset slope] best-fit]]
-                (let [unit         (if (or (nil? (:unit datetime))
-                                           (->> datetime :unit mbql.u/normalize-token (= :default)))
-                                     (infer-unit x-previous x-current)
-                                     (:unit datetime))
-                      show-change? (valid-period? x-previous x-current unit)]
-                  {:last-value     y-current
-                   :previous-value (when show-change?
-                                     y-previous)
-                   :last-change    (when show-change?
-                                     (change y-current y-previous))
-                   :slope          slope
-                   :offset         offset
-                   :best-fit       best-fit
-                   :col            (:name number-col)
-                   :unit           unit})))))))
+    (f/with-error-handling
+      (apply redux/juxt
+             (for [number-col numbers]
+               (redux/post-complete
+                (let [y-position (:position number-col)
+                      yfn        #(nth % y-position)]
+                  (redux/juxt ((map yfn) (last-n 2))
+                              ((map xfn) (last-n 2))
+                              (stats/simple-linear-regression xfn yfn)
+                              (best-fit xfn yfn)))
+                (fn [[[y-previous y-current] [x-previous x-current] [offset slope] best-fit]]
+                  (let [unit         (if (or (nil? (:unit datetime))
+                                             (->> datetime :unit mbql.u/normalize-token (= :default)))
+                                       (infer-unit x-previous x-current)
+                                       (:unit datetime))
+                        show-change? (valid-period? x-previous x-current unit)]
+                    {:last-value     y-current
+                     :previous-value (when show-change?
+                                       y-previous)
+                     :last-change    (when show-change?
+                                       (change y-current y-previous))
+                     :slope          slope
+                     :offset         offset
+                     :best-fit       best-fit
+                     :col            (:name number-col)
+                     :unit           unit})))))
+      (trs "Error generating timeseries insight keyed by: {0}"
+           (sync-util/name-for-logging (field/map->FieldInstance datetime))))))
 
 (defn insights
   "Based on the shape of returned data construct a transducer to statistically analyize data."

src/metabase/sync/analyze/query_results.clj

@@ -90,4 +90,5 @@
                            (assoc metadata :fingerprint fingerprint)))
                        fingerprints
                        cols)
-        :insights insights}))))
+        :insights (when-not (instance? Throwable insights)
+                    insights)}))))

test/metabase/sync/analyze/query_results_test.clj

@@ -9,10 +9,15 @@
             [metabase.mbql.schema :as mbql.s]
             [metabase.models.card :refer [Card]]
             [metabase.query-processor.test-util :as qp.test-util]
-            [metabase.sync.analyze.fingerprint.fingerprinters :as fprint]
+            [metabase.sync.analyze.fingerprint
+             [fingerprinters :as fprint]
+             [insights :as insights]]
             [metabase.sync.analyze.query-results :as qr]
-            [metabase.test.mock.util :as mock.u]
-            [metabase.test.util :as tu]))
+            [metabase.test
+             [data :as data]
+             [sync :as sync-test]
+             [util :as tu]]
+            [metabase.test.mock.util :as mock.u]))
 
 (defn- column->name-keyword [field-or-column-metadata]
   (-> field-or-column-metadata
@@ -99,3 +104,18 @@
     (mt/with-temp Card [card {:dataset_query {:database (mt/id), :type :native, :native {:query "select longitude from venues"}}}]
       (is (= (select-keys mock.u/venue-fingerprints [:longitude])
              (name->fingerprints (query->result-metadata (query-for-card card))))))))
+
+(defn- timeseries-dataset
+  []
+  (->> {:aggregation [[:count]]
+        :breakout    [[:datetime-field [:field-id (data/id :checkins :date)] :month]]}
+       (mt/run-mbql-query checkins)
+       :data))
+
+(deftest error-resilience-test
+  (testing "Data should come back even if there is an error during fingerprinting"
+    (is (= 36 (with-redefs [fprint/earliest sync-test/crash-fn]
+                (-> (timeseries-dataset) :rows count)))))
+  (testing "Data should come back even if there is an error when calculating insights"
+    (is (= 36 (with-redefs [insights/change sync-test/crash-fn]
+                (-> (timeseries-dataset) :rows count))))))