Skip to content
Snippets Groups Projects
Commit 12246da1 authored by Cam Saül's avatar Cam Saül
Browse files

Include query_average_duration with Dashboard cards instead of requiring second API call

parent df15b266
No related branches found
No related tags found
No related merge requests found
Showing with 200 additions and 46 deletions
......@@ -18,6 +18,8 @@ import cx from "classnames";
import _ from "underscore";
import { getIn } from "icepick";
const DATASET_USUALLY_FAST_THRESHOLD = 15 * 1000;
const HEADER_ICON_SIZE = 16;
const HEADER_ACTION_STYLE = {
......@@ -28,7 +30,7 @@ export default class DashCard extends Component {
static propTypes = {
dashcard: PropTypes.object.isRequired,
dashcardData: PropTypes.object.isRequired,
cardDurations: PropTypes.object.isRequired,
slowCards: PropTypes.object.isRequired,
parameterValues: PropTypes.object.isRequired,
markNewCardSeen: PropTypes.func.isRequired,
fetchCardData: PropTypes.func.isRequired,
......@@ -50,7 +52,7 @@ export default class DashCard extends Component {
}
render() {
const { dashcard, dashcardData, cardDurations, parameterValues, isEditing, isEditingParameter, onAddSeries, onRemove, linkToCard } = this.props;
const { dashcard, dashcardData, slowCards, parameterValues, isEditing, isEditingParameter, onAddSeries, onRemove, linkToCard } = this.props;
const mainCard = {
...dashcard.card,
......@@ -61,13 +63,14 @@ export default class DashCard extends Component {
.map(card => ({
...getIn(dashcardData, [dashcard.id, card.id]),
card: card,
duration: cardDurations[card.id]
isSlow: slowCards[card.id],
isUsuallyFast: card.query_average_duration < DATASET_USUALLY_FAST_THRESHOLD
}));
const loading = !(series.length > 0 && _.every(series, (s) => s.data));
const expectedDuration = Math.max(...series.map((s) => s.duration ? s.duration.average : 0));
const usuallyFast = _.every(series, (s) => s.duration && s.duration.average < s.duration.fast_threshold);
const isSlow = loading && _.some(series, (s) => s.duration) && (usuallyFast ? "usually-fast" : "usually-slow");
const expectedDuration = Math.max(...series.map((s) => s.card.query_average_duration || 0));
const usuallyFast = _.every(series, (s) => s.isUsuallyFast);
const isSlow = loading && _.some(series, (s) => s.isSlow) && (usuallyFast ? "usually-fast" : "usually-slow");
const parameterMap = dashcard && dashcard.parameter_mappings && dashcard.parameter_mappings
.reduce((map, mapping) => ({...map, [mapping.parameter_id]: mapping}), {});
......
......@@ -16,7 +16,7 @@ const DEFAULT_PROPS = {
dashcardData: {
1: { cols: [], rows: [] }
},
cardDurations: {},
slowCards: {},
parameterValues: {},
markNewCardSeen: () => {},
fetchCardData: () => {}
......@@ -36,10 +36,7 @@ describe("DashCard", () => {
expect(dashCard.find(".Card--slow")).toHaveLength(0);
});
it("should render slow card with Card--slow className", () => {
const props = assocIn(DEFAULT_PROPS, ["cardDurations", 1], {
average: 1,
fast_threshold: 1
});
const props = assocIn(DEFAULT_PROPS, ["slowCards", 1], true);
const dashCard = render(<DashCard {...props} />);
expect(dashCard.find(".Card--recent")).toHaveLength(0);
expect(dashCard.find(".Card--unmapped")).toHaveLength(0);
......
......@@ -186,7 +186,7 @@ export default class DashboardGrid extends Component {
dashcard={dc}
dashcardData={this.props.dashcardData}
parameterValues={this.props.parameterValues}
cardDurations={this.props.cardDurations}
slowCards={this.props.slowCards}
fetchCardData={this.props.fetchCardData}
markNewCardSeen={this.props.markNewCardSeen}
isEditing={this.props.isEditing}
......
......@@ -10,7 +10,7 @@ import Dashboard from "../components/Dashboard.jsx";
import { fetchDatabaseMetadata } from "metabase/redux/metadata";
import { setErrorPage } from "metabase/redux/app";
import { getIsEditing, getIsEditingParameter, getIsDirty, getDashboardComplete, getCardList, getRevisions, getCardData, getCardDurations, getDatabases, getEditingParameter, getParameters, getParameterValues } from "../selectors";
import { getIsEditing, getIsEditingParameter, getIsDirty, getDashboardComplete, getCardList, getRevisions, getCardData, getSlowCards, getDatabases, getEditingParameter, getParameters, getParameterValues } from "../selectors";
import { getUserIsAdmin } from "metabase/selectors/user";
import * as dashboardActions from "../dashboard";
......@@ -28,7 +28,7 @@ const mapStateToProps = (state, props) => {
cards: getCardList(state, props),
revisions: getRevisions(state, props),
dashcardData: getCardData(state, props),
cardDurations: getCardDurations(state, props),
slowCards: getSlowCards(state, props),
databases: getDatabases(state, props),
editingParameter: getEditingParameter(state, props),
parameters: getParameters(state, props),
......
......@@ -22,12 +22,11 @@ import { getPositionForNewDashCard } from "metabase/lib/dashboard_grid";
import { addParamValues, fetchDatabaseMetadata } from "metabase/redux/metadata";
import { DashboardApi, MetabaseApi, CardApi, RevisionApi, PublicApi, EmbedApi } from "metabase/services";
import { DashboardApi, CardApi, RevisionApi, PublicApi, EmbedApi } from "metabase/services";
import { getDashboard, getDashboardComplete } from "./selectors";
const DATASET_SLOW_TIMEOUT = 15 * 1000;
const DATASET_USUALLY_FAST_THRESHOLD = 15 * 1000;
const DATASET_SLOW_TIMEOUT = 15 * 1000;
// normalizr schemas
const dashcard = new schema.Entity('dashcard');
......@@ -60,7 +59,7 @@ export const SAVE_DASHCARD = "metabase/dashboard/SAVE_DASHCARD";
export const FETCH_DASHBOARD_CARD_DATA = "metabase/dashboard/FETCH_DASHBOARD_CARD_DATA";
export const FETCH_CARD_DATA = "metabase/dashboard/FETCH_CARD_DATA";
export const FETCH_CARD_DURATION = "metabase/dashboard/FETCH_CARD_DURATION";
export const MARK_CARD_AS_SLOW = "metabase/dashboard/MARK_CARD_AS_SLOW";
export const CLEAR_CARD_DATA = "metabase/dashboard/CLEAR_CARD_DATA";
export const FETCH_REVISIONS = "metabase/dashboard/FETCH_REVISIONS";
......@@ -281,10 +280,10 @@ export const fetchCardData = createThunkAction(FETCH_CARD_DATA, function(card, d
let result = null;
// start a timer that will fetch the expected card duration if the query takes too long
// start a timer that will show the expected card duration if the query takes too long
let slowCardTimer = setTimeout(() => {
if (result === null) {
dispatch(fetchCardDuration(card, datasetQuery));
dispatch(markCardAsSlow(card, datasetQuery));
}
}, DATASET_SLOW_TIMEOUT);
......@@ -316,18 +315,11 @@ export const fetchCardData = createThunkAction(FETCH_CARD_DATA, function(card, d
};
});
export const fetchCardDuration = createThunkAction(FETCH_CARD_DURATION, function(card, datasetQuery) {
return async function(dispatch, getState) {
let result = await MetabaseApi.dataset_duration(datasetQuery);
return {
id: card.id,
result: {
fast_threshold: DATASET_USUALLY_FAST_THRESHOLD,
...result
}
};
};
});
export const markCardAsSlow = createAction(MARK_CARD_AS_SLOW, (card) => ({
id: card.id,
result: true
}));
export const fetchDashboard = createThunkAction(FETCH_DASHBOARD, function(dashId, queryParams, enableDefaultParameters = true) {
let result;
......@@ -611,8 +603,8 @@ const dashcardData = handleActions({
}
}, {});
const cardDurations = handleActions({
[FETCH_CARD_DURATION]: { next: (state, { payload: { id, result }}) => ({ ...state, [id]: result }) }
const slowCards = handleActions({
[MARK_CARD_AS_SLOW]: { next: (state, { payload: { id, result }}) => ({ ...state, [id]: result }) }
}, {});
const parameterValues = handleActions({
......@@ -632,6 +624,6 @@ export default combineReducers({
editingParameterId,
revisions,
dashcardData,
cardDurations,
slowCards,
parameterValues
});
......@@ -34,7 +34,7 @@ export const getCards = state => state.dashboard.cards;
export const getDashboards = state => state.dashboard.dashboards;
export const getDashcards = state => state.dashboard.dashcards;
export const getCardData = state => state.dashboard.dashcardData;
export const getCardDurations = state => state.dashboard.cardDurations;
export const getSlowCards = state => state.dashboard.slowCards;
export const getCardIdList = state => state.dashboard.cardList;
export const getRevisions = state => state.dashboard.revisions;
export const getParameterValues = state => state.dashboard.parameterValues;
......
......@@ -15,7 +15,7 @@ import EmbedFrame from "../components/EmbedFrame";
import { fetchDatabaseMetadata } from "metabase/redux/metadata";
import { setErrorPage } from "metabase/redux/app";
import { getDashboardComplete, getCardData, getCardDurations, getParameters, getParameterValues } from "metabase/dashboard/selectors";
import { getDashboardComplete, getCardData, getSlowCards, getParameters, getParameterValues } from "metabase/dashboard/selectors";
import * as dashboardActions from "metabase/dashboard/dashboard";
......@@ -29,7 +29,7 @@ const mapStateToProps = (state, props) => {
dashboardId: props.params.dashboardId || props.params.uuid || props.params.token,
dashboard: getDashboardComplete(state, props),
dashcardData: getCardData(state, props),
cardDurations: getCardDurations(state, props),
slowCards: getSlowCards(state, props),
parameters: getParameters(state, props),
parameterValues: getParameterValues(state, props)
}
......
......@@ -5,14 +5,18 @@
[metabase
[events :as events]
[util :as u]]
[metabase.api.common :as api]
[metabase.api
[common :as api]
[dataset :as dataset]]
[metabase.models
[card :refer [Card]]
[dashboard :as dashboard :refer [Dashboard]]
[dashboard-card :refer [create-dashboard-card! DashboardCard delete-dashboard-card! update-dashboard-card!]]
[dashboard-favorite :refer [DashboardFavorite]]
[interface :as mi]
[query :as query :refer [Query]]
[revision :as revision]]
[metabase.query-processor.util :as qp-util]
[metabase.util.schema :as su]
[schema.core :as s]
[toucan
......@@ -59,6 +63,9 @@
parameters [su/Map]}
(dashboard/create-dashboard! dashboard api/*current-user-id*))
;;; ------------------------------------------------------------ Hiding Unreadable Cards ------------------------------------------------------------
(defn- hide-unreadable-card
"If CARD is unreadable, replace it with an object containing only its `:id`."
[card]
......@@ -69,10 +76,87 @@
(defn- hide-unreadable-cards
"Replace the `:card` and `:series` entries from dashcards that they user isn't allowed to read with empty objects."
[dashboard]
(update dashboard :ordered_cards (partial mapv (fn [dashcard]
(-> dashcard
(update :card hide-unreadable-card)
(update :series (partial mapv hide-unreadable-card)))))))
(update dashboard :ordered_cards (fn [dashcards]
(vec (for [dashcard dashcards]
(-> dashcard
(update :card hide-unreadable-card)
(update :series (partial mapv hide-unreadable-card))))))))
;;; ------------------------------------------------------------ Query Average Duration Info ------------------------------------------------------------
;; Adding the average execution time to all of the Cards in a Dashboard efficiently is somewhat involved. There are a few things that make this tricky:
;;
;; 1. Queries are usually executed with `:constraints` that different from how they're actually definied, but not always. This means we should look
;; up hashes for both the query as-is and for the query with `default-query-constraints` and use whichever one we find
;; 2. The structure of DashCards themselves is complicated. It has a top-level `:card` property and (optionally) a sequence of additional Cards under `:series`
;; 3. Query hashes are byte arrays, and two idential byte arrays aren't equal to each other in Java; thus they don't work as one would expect when being used as map keys
;;
;; Here's an overview of the approach used to efficiently add the info:
;;
;; 1. Build a sequence of query hashes (both as-is and with default constraints) for every card and series in the dashboard cards
;; 2. Fetch all matching entires from Query in the DB and build a map of hash (converted to a Clojure vector) -> average execution time
;; 3. Iterate back over each card and look for matching entries in the `hash-vec->avg-time` for either the normal hash or the hash with default constraints,
;; and add the result as `:average_execution_time`
(defn- card->query-hashes
"Return a tuple of possible hashes that would be associated with executions of CARD.
The first is the hash of the query dictionary as-is; the second is one with the `default-query-constraints`,
which is how it will most likely be run."
[{:keys [dataset_query]}]
(u/ignore-exceptions
[(qp-util/query-hash dataset_query)
(qp-util/query-hash (assoc dataset_query :constraints dataset/default-query-constraints))]))
(defn- dashcard->query-hashes
"Return a sequence of all the query hashes for this DASHCARD, including the top-level Card and any Series."
[{:keys [card series]}]
(reduce concat
(card->query-hashes card)
(for [card series]
(card->query-hashes card))))
(defn- dashcards->query-hashes
"Return a sequence of all the query hashes used in a DASHCARDS."
[dashcards]
(apply concat (for [dashcard dashcards]
(dashcard->query-hashes dashcard))))
(defn- hashes->hash-vec->avg-time
"Given some query HASHES, return a map of hashes (as normal Clojure vectors) to the average query durations.
(The hashes are represented as normal Clojure vectors because identical byte arrays aren't considered equal to one another, and thus do not
work as one would expect when used as map keys.)"
[hashes]
(when (seq hashes)
(into {} (for [[k v] (db/select-field->field :query_hash :average_execution_time Query :query_hash [:in hashes])]
{(vec k) v}))))
(defn- add-query-average-duration-to-card
"Add `:query_average_duration` info to a CARD (i.e., the `:card` property of a DashCard or an entry in its `:series` array)."
[card hash-vec->avg-time]
(assoc card :query_average_duration (some (fn [query-hash]
(hash-vec->avg-time (vec query-hash)))
(card->query-hashes card))))
(defn- add-query-average-duration-to-dashcards
"Add `:query_average_duration` to the top-level Card and any Series in a sequence of DASHCARDS."
([dashcards]
(add-query-average-duration-to-dashcards dashcards (hashes->hash-vec->avg-time (dashcards->query-hashes dashcards))))
([dashcards hash-vec->avg-time]
(for [dashcard dashcards]
(-> dashcard
(update :card add-query-average-duration-to-card hash-vec->avg-time)
(update :series (fn [series]
(for [card series]
(add-query-average-duration-to-card card hash-vec->avg-time))))))))
(defn add-query-average-durations
"Add a `average_execution_time` field to each card (and series) belonging to DASHBOARD."
[dashboard]
(update dashboard :ordered_cards add-query-average-duration-to-dashcards))
;;; ------------------------------------------------------------------------------------------------------------------------------------------------------
(api/defendpoint GET "/:id"
"Get `Dashboard` with ID."
......@@ -81,7 +165,8 @@
(hydrate :creator [:ordered_cards [:card :creator] :series])
api/read-check
api/check-not-archived
hide-unreadable-cards)
hide-unreadable-cards
add-query-average-durations)
(events/publish-event! :dashboard-read (assoc <> :actor_id api/*current-user-id*))))
......
......@@ -34,6 +34,7 @@
(let [query (assoc body :constraints default-query-constraints)]
(qp/dataset-query query {:executed-by api/*current-user-id*, :context :ad-hoc})))
;; TODO - this is no longer used. Should we remove it?
(api/defendpoint POST "/duration"
"Get historical query execution duration."
[:as {{:keys [database], :as query} :body}]
......
......@@ -8,7 +8,8 @@
[metabase.api
[card :as card-api]
[common :as api]
[dataset :as dataset-api]]
[dataset :as dataset-api]
[dashboard :as dashboard-api]]
[metabase.models
[card :refer [Card]]
[dashboard :refer [Dashboard]]
......@@ -176,6 +177,7 @@
(-> (api/check-404 (apply db/select-one [Dashboard :name :description :id :parameters], :archived false, conditions))
(hydrate [:ordered_cards :card :series])
add-field-values-for-parameters
dashboard-api/add-query-average-durations
(update :ordered_cards (fn [dashcards]
(for [dashcard dashcards]
(-> (select-keys dashcard [:id :card :card_id :dashboard_id :series :col :row :sizeX :sizeY :parameter_mappings :visualization_settings])
......
......@@ -132,7 +132,8 @@
:display "table"
:query_type nil
:dataset_query {}
:visualization_settings {}})
:visualization_settings {}
:query_average_duration nil})
:series []}]})
;; fetch a dashboard WITH a dashboard card on it
(tt/with-temp* [Dashboard [{dashboard-id :id} {:name "Test Dashboard"}]
......@@ -547,3 +548,63 @@
(tt/with-temp Dashboard [dashboard {:enable_embedding true}]
(for [dash ((user->client :crowberto) :get 200 "dashboard/embeddable")]
(m/map-vals boolean (select-keys dash [:name :id]))))))
;;; ------------------------------------------------------------ Tests for including query average duration info ------------------------------------------------------------
(tu/resolve-private-vars metabase.api.dashboard
dashcard->query-hashes
dashcards->query-hashes
add-query-average-duration-to-dashcards)
(expect
[[-109 -42 53 92 -31 19 -111 13 -11 -111 127 -110 -12 53 -42 -3 -58 -61 60 97 123 -65 -117 -110 -27 -2 -99 102 -59 -29 49 27]
[43 -96 52 23 -69 81 -59 15 -74 -59 -83 -9 -110 40 1 -64 -117 -44 -67 79 -123 -9 -107 20 113 -59 -93 25 60 124 -110 -30]]
(tu/vectorize-byte-arrays
(dashcard->query-hashes {:card {:dataset_query {:database 1}}})))
(expect
[[89 -75 -86 117 -35 -13 -69 -36 -17 84 37 86 -121 -59 -3 1 37 -117 -86 -42 -127 -42 -74 101 83 72 10 44 75 -126 43 66]
[55 56 16 11 -121 -29 71 -99 -89 -92 41 25 87 -78 34 100 54 -3 53 -9 38 41 -75 -121 63 -119 43 23 57 11 63 32]
[-90 55 65 61 72 22 -99 -75 111 49 -3 21 -80 68 -14 120 30 -84 -103 16 -68 73 -121 -93 -55 54 72 84 -8 118 -101 114]
[116 69 -44 77 100 8 -40 -67 25 -4 27 -21 111 98 -45 85 83 -27 -39 8 63 -25 -88 74 32 -10 -2 35 102 -72 -104 111]
[-84 -2 87 22 -4 105 68 48 -113 93 -29 52 3 102 123 -70 -123 36 31 76 -16 87 70 116 -93 109 -88 108 125 -36 -43 73]
[90 127 103 -71 -76 -36 41 -107 -7 -13 -83 -87 28 86 -94 110 74 -86 110 -54 -128 124 102 -73 -127 88 77 -36 62 5 -84 -100]]
(tu/vectorize-byte-arrays
(dashcard->query-hashes {:card {:dataset_query {:database 2}}
:series [{:dataset_query {:database 3}}
{:dataset_query {:database 4}}]})))
(expect
[[-109 -42 53 92 -31 19 -111 13 -11 -111 127 -110 -12 53 -42 -3 -58 -61 60 97 123 -65 -117 -110 -27 -2 -99 102 -59 -29 49 27]
[43 -96 52 23 -69 81 -59 15 -74 -59 -83 -9 -110 40 1 -64 -117 -44 -67 79 -123 -9 -107 20 113 -59 -93 25 60 124 -110 -30]
[89 -75 -86 117 -35 -13 -69 -36 -17 84 37 86 -121 -59 -3 1 37 -117 -86 -42 -127 -42 -74 101 83 72 10 44 75 -126 43 66]
[55 56 16 11 -121 -29 71 -99 -89 -92 41 25 87 -78 34 100 54 -3 53 -9 38 41 -75 -121 63 -119 43 23 57 11 63 32]
[-90 55 65 61 72 22 -99 -75 111 49 -3 21 -80 68 -14 120 30 -84 -103 16 -68 73 -121 -93 -55 54 72 84 -8 118 -101 114]
[116 69 -44 77 100 8 -40 -67 25 -4 27 -21 111 98 -45 85 83 -27 -39 8 63 -25 -88 74 32 -10 -2 35 102 -72 -104 111]
[-84 -2 87 22 -4 105 68 48 -113 93 -29 52 3 102 123 -70 -123 36 31 76 -16 87 70 116 -93 109 -88 108 125 -36 -43 73]
[90 127 103 -71 -76 -36 41 -107 -7 -13 -83 -87 28 86 -94 110 74 -86 110 -54 -128 124 102 -73 -127 88 77 -36 62 5 -84 -100]]
(tu/vectorize-byte-arrays (dashcards->query-hashes [{:card {:dataset_query {:database 1}}}
{:card {:dataset_query {:database 2}}
:series [{:dataset_query {:database 3}}
{:dataset_query {:database 4}}]}])))
(expect
[{:card {:dataset_query {:database 1}, :query_average_duration 111}
:series []}
{:card {:dataset_query {:database 2}, :query_average_duration 333}
:series [{:dataset_query {:database 3}, :query_average_duration 555}
{:dataset_query {:database 4}, :query_average_duration 777}]}]
(add-query-average-duration-to-dashcards
[{:card {:dataset_query {:database 1}}}
{:card {:dataset_query {:database 2}}
:series [{:dataset_query {:database 3}}
{:dataset_query {:database 4}}]}]
{[-109 -42 53 92 -31 19 -111 13 -11 -111 127 -110 -12 53 -42 -3 -58 -61 60 97 123 -65 -117 -110 -27 -2 -99 102 -59 -29 49 27] 111
[43 -96 52 23 -69 81 -59 15 -74 -59 -83 -9 -110 40 1 -64 -117 -44 -67 79 -123 -9 -107 20 113 -59 -93 25 60 124 -110 -30] 222
[89 -75 -86 117 -35 -13 -69 -36 -17 84 37 86 -121 -59 -3 1 37 -117 -86 -42 -127 -42 -74 101 83 72 10 44 75 -126 43 66] 333
[55 56 16 11 -121 -29 71 -99 -89 -92 41 25 87 -78 34 100 54 -3 53 -9 38 41 -75 -121 63 -119 43 23 57 11 63 32] 444
[-90 55 65 61 72 22 -99 -75 111 49 -3 21 -80 68 -14 120 30 -84 -103 16 -68 73 -121 -93 -55 54 72 84 -8 118 -101 114] 555
[116 69 -44 77 100 8 -40 -67 25 -4 27 -21 111 98 -45 85 83 -27 -39 8 63 -25 -88 74 32 -10 -2 35 102 -72 -104 111] 666
[-84 -2 87 22 -4 105 68 48 -113 93 -29 52 3 102 123 -70 -123 36 31 76 -16 87 70 116 -93 109 -88 108 125 -36 -43 73] 777
[90 127 103 -71 -76 -36 41 -107 -7 -13 -83 -87 28 86 -94 110 74 -86 110 -54 -128 124 102 -73 -127 88 77 -36 62 5 -84 -100] 888}))
......@@ -299,3 +299,16 @@
{:style/indent 0}
[& body]
`(do-with-log-messages (fn [] ~@body)))
(defn vectorize-byte-arrays
"Walk form X and convert any byte arrays in the results to standard Clojure vectors.
This is useful when writing tests that return byte arrays (such as things that work with query hashes),
since identical arrays are not considered equal."
{:style/indent 0}
[x]
(walk/postwalk (fn [form]
(if (instance? (Class/forName "[B") form)
(vec form)
form))
x))
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