Skip to content
Snippets Groups Projects
Unverified Commit 143b9fa3 authored by Robert Roland's avatar Robert Roland Committed by GitHub
Browse files

Fixing Google Analytics failures on new instances (#12595)

This stops a failure during fingerprinting of a table from causing the
entire sync to fail.

Fingerprinting is now skipped for Google Analytics datasources, as they
do not support this (all queries against GA require a `metrics`
parameter)

Add an assert to ensure that `:metrics` is present in a GA query. The
Google library will fail with a NPE when it's not.

Resolves #12411

[ci googleanalytics]
parent 3e304881
No related branches found
No related tags found
No related merge requests found
......@@ -114,6 +114,8 @@
(json/parse-string query keyword)
query)
client (client/database->client database)]
(assert (not (str/blank? (:metrics query)))
":metrics is required in a Google Analytics query")
;; `end-date` is inclusive!!!
(u/prog1 (.get (.ga (.data client))
(:ids query)
......
......@@ -32,7 +32,9 @@
:fingerprint_version i/latest-fingerprint-version
:last_analyzed nil))
(defn- empty-stats-map [fields-count]
(defn empty-stats-map
"The default stats before any fingerprints happen"
[fields-count]
{:no-data-fingerprints 0
:failed-fingerprints 0
:updated-fingerprints 0
......@@ -159,7 +161,12 @@
"Generate and save fingerprints for all the Fields in TABLE that have not been previously analyzed."
[table :- i/TableInstance]
(if-let [fields (fields-to-fingerprint table)]
(fingerprint-table! table fields)
(let [stats (sync-util/with-error-handling
(format "Error fingerprinting %s" (sync-util/name-for-logging table))
(fingerprint-table! table fields))]
(if (instance? Exception stats)
(empty-stats-map 0)
stats))
(empty-stats-map 0)))
(s/defn fingerprint-fields-for-db!
......@@ -167,11 +174,14 @@
[database :- i/DatabaseInstance
tables :- [i/TableInstance]
log-progress-fn]
;; TODO: Maybe the driver should have a function to tell you if it supports fingerprinting?
(qp.store/with-store
;; store is bound so DB timezone can be used in date coercion logic
(qp.store/store-database! database)
(apply merge-with + (for [table tables
:let [result (fingerprint-fields! table)]]
:let [result (if (= :googleanalytics (:engine database))
(empty-stats-map 0)
(fingerprint-fields! table))]]
(do
(log-progress-fn "fingerprint-fields" table)
result)))))
(ns metabase.sync.analyze.fingerprint-test
"Basic tests to make sure the fingerprint generatation code is doing something that makes sense."
(:require [expectations :refer :all]
(:require [clojure.test :refer :all]
[expectations :refer :all]
[metabase
[db :as mdb]
[util :as u]]
......@@ -224,3 +225,17 @@
fingerprinters/fingerprinter (constantly (fingerprinters/constant-fingerprinter {:experimental {:fake-fingerprint? true}}))]
[(#'fingerprint/fingerprint-table! (Table (data/id :venues)) [field])
(into {} (db/select-one [Field :fingerprint :fingerprint_version :last_analyzed] :id (u/get-id field)))])))
(deftest test-fingerprint-failure
(testing "if fingerprinting fails, the exception should not propagate"
(with-redefs [fingerprint/fingerprint-table! (fn [_ _] (throw (Exception. "expected")))]
(is (= (fingerprint/empty-stats-map 0)
(fingerprint/fingerprint-fields! (Table (data/id :venues))))))))
(deftest test-fingerprint-skipped-for-ga
(testing "Google Analytics doesn't support fingerprinting fields"
(let [fake-db (-> (data/db)
(assoc :engine :googleanalytics))]
(with-redefs [fingerprint/fingerprint-table! (fn [_] (throw (Exception. "this should not be called!")))]
(is (= (fingerprint/empty-stats-map 0)
(fingerprint/fingerprint-fields-for-db! fake-db [(Table (data/id :venues))] (fn [_ _]))))))))
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment