diff --git a/enterprise/backend/src/metabase_enterprise/audit_app/pages/common/cards.clj b/enterprise/backend/src/metabase_enterprise/audit_app/pages/common/cards.clj index 3bc0921876094babd48bd0b610125a455dab6ffe..4c17ec0a052ae6d5a31454ae6172e50fe8af5954 100644 --- a/enterprise/backend/src/metabase_enterprise/audit_app/pages/common/cards.clj +++ b/enterprise/backend/src/metabase_enterprise/audit_app/pages/common/cards.clj @@ -1,5 +1,6 @@ (ns metabase-enterprise.audit-app.pages.common.cards (:require [metabase-enterprise.audit-app.pages.common :as common] + [metabase.db.connection :as mdb.connection] [metabase.util.honeysql-extensions :as hx])) (def avg-exec-time @@ -55,6 +56,15 @@ :from [:report_dashboardcard] :group-by [:card_id]}]) +(def dashboards-ids + "HoneySQL for a CTE to enumerate the dashboards for a Card. We get the actual ID's" + [:dash_card {:select [:card_id [(common/group-concat (hx/cast + (if (= (mdb.connection/db-type) :mysql) :char :text) + :report_dashboard.name) "|") :name_str]] + :from [:report_dashboardcard] + :join [:report_dashboard [:= :report_dashboardcard.dashboard_id :report_dashboard.id]] + :group-by [:card_id]}]) + (def views "HoneySQL for a CTE to include the total view count for each Card." [:card_views {:select [[:model_id :card_id] diff --git a/enterprise/backend/src/metabase_enterprise/audit_app/pages/queries.clj b/enterprise/backend/src/metabase_enterprise/audit_app/pages/queries.clj index 4bbe29faec4fef6408d74ea1cf0271b72782bdec..ce6fb8db865dc937f2457bf2f194034f3afac5cf 100644 --- a/enterprise/backend/src/metabase_enterprise/audit_app/pages/queries.clj +++ b/enterprise/backend/src/metabase_enterprise/audit_app/pages/queries.clj @@ -3,6 +3,7 @@ [metabase-enterprise.audit-app.interface :as audit.i] [metabase-enterprise.audit-app.pages.common :as common] [metabase-enterprise.audit-app.pages.common.cards :as cards] + [metabase.db.connection :as mdb.connection] [metabase.util.honeysql-extensions :as hx])) ;; DEPRECATED Query that returns data for a two-series timeseries chart with number of queries ran and average query @@ -70,6 +71,7 @@ [:collection_name {:display_name "Collection", :base_type :type/Text :remapped_from :collection_id}] [:database_id {:display_name "Database ID", :base_type :type/Integer :remapped_to :database_name}] [:database_name {:display_name "Database", :base_type :type/Text :remapped_from :database_id}] + [:schema_name {:display_name "Schema", :base_type :type/Text}] [:table_id {:display_name "Table ID", :base_type :type/Integer :remapped_to :table_name}] [:table_name {:display_name "Table", :base_type :type/Text :remapped_from :table_id}] [:last_run_at {:display_name "Last run at", :base_type :type/DateTime}] @@ -86,11 +88,19 @@ cards/dashboards-count] :select [[:card.id :card_id] [:card.name :card_name] - [(hsql/call :concat (hsql/call :substring :latest_qe.error 0 60) "...") :error_substr] + [(hsql/call :concat + (hsql/call + :substring + :latest_qe.error + (if (= (mdb.connection/db-type) :mysql) 1 0) + 60) + "...") + :error_substr] :collection_id [:coll.name :collection_name] :card.database_id [:db.name :database_name] + [:t.schema :schema_name] :card.table_id [:t.name :table_name] [:latest_qe.started_at :last_run_at] diff --git a/enterprise/backend/src/metabase_enterprise/audit_app/pages/query_detail.clj b/enterprise/backend/src/metabase_enterprise/audit_app/pages/query_detail.clj index 00903f7dd89c37bf49260602e05d7e301f2f527e..d3fe02573bb8fe5bb238543a45a41055fb7784ac 100644 --- a/enterprise/backend/src/metabase_enterprise/audit_app/pages/query_detail.clj +++ b/enterprise/backend/src/metabase_enterprise/audit_app/pages/query_detail.clj @@ -3,19 +3,69 @@ (:require [cheshire.core :as json] [metabase-enterprise.audit-app.interface :as audit.i] [metabase-enterprise.audit-app.pages.common :as common] + [metabase-enterprise.audit-app.pages.common.cards :as cards] [metabase.util.schema :as su] [ring.util.codec :as codec] [schema.core :as s])) +(defmethod audit.i/internal-query ::bad-card + [_ card-id] + {:metadata [[:card_id {:display_name "Question ID", :base_type :type/Integer :remapped_from :card_name}] + [:card_name {:display_name "Question", :base_type :type/Text :remapped_from :card_id}] + [:error_str {:display_name "Error", :base_type :type/Text :code true}] + [:collection_id {:display_name "Collection ID", :base_type :type/Integer :remapped_to :collection_name}] + [:collection_name {:display_name "Collection", :base_type :type/Text :remapped_from :collection_id}] + [:database_id {:display_name "Database ID", :base_type :type/Integer :remapped_to :database_name}] + [:database_name {:display_name "Database", :base_type :type/Text :remapped_from :database_id}] + [:schema_name {:display_name "Schema", :base_type :type/Text}] + [:table_id {:display_name "Table ID", :base_type :type/Integer :remapped_to :table_name}] + [:table_name {:display_name "Table", :base_type :type/Text :remapped_from :table_id}] + [:last_run_at {:display_name "Last run at", :base_type :type/DateTime}] + [:total_runs {:display_name "Total runs", :base_type :type/Integer}] + ;; Denormalize by string_agg in order to avoid having to deal with complicated left join + [:dash_name_str {:display_name "Dashboards it's in", :base_type :type/Text}] + [:user_id {:display_name "Created By ID", :base_type :type/Integer :remapped_to :user_name}] + [:user_name {:display_name "Created By", :base_type :type/Text :remapped_from :user_id}] + [:updated_at {:display_name "Updated At", :base_type :type/DateTime}]] + :results (common/reducible-query + {:with [cards/query-runs + cards/latest-qe + cards/dashboards-ids] + :select [[:card.id :card_id] + [:card.name :card_name] + [:latest_qe.error :error_str] + :collection_id + [:coll.name :collection_name] + :card.database_id + [:db.name :database_name] + [:t.schema :schema_name] + :card.table_id + [:t.name :table_name] + [:latest_qe.started_at :last_run_at] + [:query_runs.count :total_runs] + [:dash_card.name_str :dash_name_str] + [:card.creator_id :user_id] + [(common/user-full-name :u) :user_name] + [:card.updated_at :updated_at]] + :from [[:report_card :card]] + :left-join [[:collection :coll] [:= :card.collection_id :coll.id] + [:metabase_database :db] [:= :card.database_id :db.id] + [:metabase_table :t] [:= :card.table_id :t.id] + [:core_user :u] [:= :card.creator_id :u.id] + :latest_qe [:= :card.id :latest_qe.card_id] + :query_runs [:= :card.id :query_runs.card_id] + :dash_card [:= :card.id :dash_card.card_id]] + :where [:= :card.id card-id] })}) + ;; Details about a specific query (currently just average execution time). (s/defmethod audit.i/internal-query ::details [_ query-hash :- su/NonBlankString] {:metadata [[:query {:display_name "Query", :base_type :type/Dictionary}] [:average_execution_time {:display_name "Avg. Exec. Time (ms)", :base_type :type/Number}]] :results (common/reducible-query - {:select [:query - :average_execution_time] - :from [:query] - :where [:= :query_hash (codec/base64-decode query-hash)] - :limit 1}) + {:select [:query + :average_execution_time] + :from [:query] + :where [:= :query_hash (codec/base64-decode query-hash)] + :limit 1}) :xform (map #(update (vec %) 0 json/parse-string))}) diff --git a/enterprise/frontend/src/metabase-enterprise/audit_app/components/AuditParameters.jsx b/enterprise/frontend/src/metabase-enterprise/audit_app/components/AuditParameters.jsx index 2e4f39d4dec5cbbecbb240185e4207a7361dfd0d..69be736775344be23886c8e0276e062ca065e18d 100644 --- a/enterprise/frontend/src/metabase-enterprise/audit_app/components/AuditParameters.jsx +++ b/enterprise/frontend/src/metabase-enterprise/audit_app/components/AuditParameters.jsx @@ -23,6 +23,7 @@ const propTypes = { }), ), children: PropTypes.func, + hasResults: PropTypes.bool, }; export default class AuditParameters extends React.Component { @@ -48,8 +49,14 @@ export default class AuditParameters extends React.Component { }, DEBOUNCE_PERIOD); render() { - const { parameters, children, buttons } = this.props; + const { parameters, children, buttons, hasResults } = this.props; const { inputValues, committedValues } = this.state; + + const disabled = + hasResults === false && + inputValues && + Object.values(inputValues).every(v => v === ""); + return ( <div> <div className="pt4"> @@ -59,6 +66,7 @@ export default class AuditParameters extends React.Component { type="text" value={inputValues[key] || ""} placeholder={placeholder} + disabled={disabled} onChange={value => { this.changeValue(key, value); }} @@ -66,7 +74,13 @@ export default class AuditParameters extends React.Component { /> ))} {buttons?.map(({ key, onClick, label }) => ( - <Button primary key={key} onClick={onClick} className="ml2"> + <Button + primary + key={key} + onClick={onClick} + disabled={disabled} + className="ml2" + > {label} </Button> ))} diff --git a/enterprise/frontend/src/metabase-enterprise/audit_app/containers/AuditTable.jsx b/enterprise/frontend/src/metabase-enterprise/audit_app/containers/AuditTable.jsx index c3b608f81e391c921eaca3bf2d675c2199e555ef..4312d39cc120b4bc51c0d801adc03cee14dd0881 100644 --- a/enterprise/frontend/src/metabase-enterprise/audit_app/containers/AuditTable.jsx +++ b/enterprise/frontend/src/metabase-enterprise/audit_app/containers/AuditTable.jsx @@ -30,19 +30,31 @@ AuditTable.propTypes = { reload: PropTypes.bool, children: PropTypes.node, dispatch: PropTypes.func.isRequired, + onLoad: PropTypes.func, + mode: PropTypes.shape({ + name: PropTypes.string.isRequired, + drills: PropTypes.func.isRequired, + }), }; function AuditTable({ metadata, table, pageSize = DEFAULT_PAGE_SIZE, + mode = AuditMode, children, dispatch, + onLoad, ...rest }) { const [loadedCount, setLoadedCount] = useState(0); const { handleNextPage, handlePreviousPage, page } = usePagination(); + const handleOnLoad = results => { + setLoadedCount(results[0].row_count); + onLoad(results); + }; + const card = chain(table.card) .assoc("display", "audit-table") .assocIn(["dataset_query", "limit"], pageSize) @@ -60,10 +72,10 @@ function AuditTable({ className="mt3" question={question} metadata={metadata} - mode={AuditMode} + mode={mode} onChangeLocation={handleChangeLocation} onChangeCardAndRun={() => {}} - onLoad={results => setLoadedCount(results[0].row_count)} + onLoad={handleOnLoad} dispatch={dispatch} {...rest} /> diff --git a/enterprise/frontend/src/metabase-enterprise/audit_app/containers/QuestionLoadAndDisplay.jsx b/enterprise/frontend/src/metabase-enterprise/audit_app/containers/QuestionLoadAndDisplay.jsx index bad705f892bfb7587b828eca498c3eb382bdc26a..19b7b896e2d93295b0b9571e32212382082a7ea2 100644 --- a/enterprise/frontend/src/metabase-enterprise/audit_app/containers/QuestionLoadAndDisplay.jsx +++ b/enterprise/frontend/src/metabase-enterprise/audit_app/containers/QuestionLoadAndDisplay.jsx @@ -1,4 +1,4 @@ -import React, { useEffect, useRef } from "react"; +import React, { useEffect, useRef, useImperativeHandle } from "react"; import PropTypes from "prop-types"; import QuestionResultLoader from "metabase/containers/QuestionResultLoader"; import LoadingAndErrorWrapper from "metabase/components/LoadingAndErrorWrapper"; @@ -9,6 +9,7 @@ const propTypes = { keepPreviousWhileLoading: PropTypes.bool, reload: PropTypes.bool, onLoad: PropTypes.func, + reloadRef: PropTypes.shape({ current: PropTypes.func }), }; const QuestionLoadAndDisplay = ({ @@ -16,12 +17,15 @@ const QuestionLoadAndDisplay = ({ keepPreviousWhileLoading, reload, onLoad, + reloadRef, ...props }) => { - const reloadRef = useRef(); + const reloadFnRef = useRef(); + + useImperativeHandle(reloadRef, () => () => reloadFnRef.current?.()); useEffect(() => { - reload && reloadRef.current(); + reload && reloadFnRef.current(); }, [reload]); return ( @@ -32,7 +36,7 @@ const QuestionLoadAndDisplay = ({ > {({ loading, error, reload, ...resultProps }) => { const shouldShowLoader = loading && resultProps.results == null; - reloadRef.current = reload; + reloadFnRef.current = reload; return ( <LoadingAndErrorWrapper diff --git a/enterprise/frontend/src/metabase-enterprise/audit_app/lib/cards/queries.js b/enterprise/frontend/src/metabase-enterprise/audit_app/lib/cards/queries.js index c44bcc01a3105c0b18a9aae37af37908f33ff673..672428eeb2c1cef7c888a2049977775abafa3fcd 100644 --- a/enterprise/frontend/src/metabase-enterprise/audit_app/lib/cards/queries.js +++ b/enterprise/frontend/src/metabase-enterprise/audit_app/lib/cards/queries.js @@ -78,7 +78,7 @@ export const bad_table = ( { name: "error_substr", enabled: true }, { name: "collection_id", enabled: true }, { name: "database_id", enabled: true }, - { name: "schema_id", enabled: true }, + { name: "schema", enabled: true }, { name: "table_id", enabled: true }, { name: "last_run_at", enabled: true }, { name: "total_runs", enabled: true }, diff --git a/enterprise/frontend/src/metabase-enterprise/audit_app/lib/mode.js b/enterprise/frontend/src/metabase-enterprise/audit_app/lib/mode.js index b133f3dde8af300b5b1ef91b5c1e71dae1ffd483..42513af797962a538c687a35892bb289f7aefe80 100644 --- a/enterprise/frontend/src/metabase-enterprise/audit_app/lib/mode.js +++ b/enterprise/frontend/src/metabase-enterprise/audit_app/lib/mode.js @@ -12,7 +12,9 @@ export const getRowValuesByColumns = (row, cols) => }; }, {}); -const columnNameToUrl = { +export const columnNameToUrl = { + // No admin page for collections but still want to link to it + collection_id: value => `/collection/${value}`, user_id: value => `/admin/audit/member/${value}`, creator_id: value => `/admin/audit/member/${value}`, viewed_by_id: value => `/admin/audit/member/${value}`, diff --git a/enterprise/frontend/src/metabase-enterprise/tools/containers/ErrorDetail.jsx b/enterprise/frontend/src/metabase-enterprise/tools/containers/ErrorDetail.jsx new file mode 100644 index 0000000000000000000000000000000000000000..2e3972dec1be3e4aeac7055cbc623992689b65ce --- /dev/null +++ b/enterprise/frontend/src/metabase-enterprise/tools/containers/ErrorDetail.jsx @@ -0,0 +1,177 @@ +import React from "react"; + +import { connect } from "react-redux"; +import { push } from "react-router-redux"; +import { getMetadata } from "metabase/selectors/metadata"; + +import { t } from "ttag"; +import PropTypes from "prop-types"; +import { getIn } from "icepick"; + +import { formatColumn, formatValue } from "metabase/lib/formatting"; +import { CardApi } from "metabase/services"; +import Button from "metabase/components/Button"; +import Link from "metabase/components/Link"; +import Question from "metabase-lib/lib/Question"; +import { QuestionResultLoader } from "metabase/containers/QuestionResultLoader"; +import { columnNameToUrl } from "../../audit_app/lib/mode"; + +function idxToUrl(resRow, resCols, nameToResCol, colName) { + const idVal = resRow[nameToResCol[colName]]; + const urlVal = colName && idVal ? columnNameToUrl[colName](idVal) : ""; + const linkClass = urlVal === "" ? "" : "text-brand"; + return [urlVal, linkClass]; +} + +function ErrorDetailDisplay(props) { + const { result } = props; + const resRow = getIn(result, ["data", "rows", 0]); + const resCols = getIn(result, ["data", "cols"]); + if (resRow && resCols) { + const nameToResCol = resCols.reduce( + (obj, x, idx) => Object.assign(obj, { [x.name]: idx }), + {}, + ); + + const linkColumns = [ + null, + "collection_id", + "database_id", + null, + "table_id", + null, + "user_id", + null, + ]; + + const ordinaryRows = [ + "last_run_at", + "collection_name", + "database_name", + "schema_name", + "table_name", + "total_runs", + "user_name", + "updated_at", + ].map((x, idx) => { + const [urlVal, linkClass] = idxToUrl( + resRow, + resCols, + nameToResCol, + linkColumns[idx], + ); + const formattedVal = formatValue(resRow[nameToResCol[x]], { + column: resCols[nameToResCol[x]], + jsx: true, + rich: true, + type: "cell", + local: true, + }); + return ( + <tr key={x}> + <td align="right" className="m0 mt1 text-medium"> + {formatColumn(resCols[nameToResCol[x]])} + </td> + <td> + { + <Link to={urlVal} className={linkClass}> + {formattedVal} + </Link> + } + </td> + </tr> + ); + }); + + const dashIdRows = resRow[nameToResCol.dash_name_str] + ?.split("|") + ?.map((x, idx) => ( + <tr key={x}> + <td align="right" className="m0 mt1 text-medium"> + {idx === 0 && formatColumn(resCols[nameToResCol.dash_name_str])} + </td> + <td className="text-bold"> + {formatValue(x, { column: resCols[nameToResCol.dash_name_str] })} + </td> + </tr> + )); + + const [cardUrlVal, cardLinkClass] = idxToUrl( + resRow, + resCols, + nameToResCol, + "card_id", + ); + + return [ + <h2 className="PageTitle p1" key="card_name"> + { + <Link to={cardUrlVal} className={cardLinkClass}> + {resRow[nameToResCol.card_name]} + </Link> + } + </h2>, + <div key="error_str" className="p1 text-code"> + {resRow[nameToResCol.error_str]} + </div>, + <table key="table" className="ContentTable"> + <tbody>{[ordinaryRows, dashIdRows]}</tbody> + </table>, + ]; + } else { + return null; + } +} + +function ErrorDetail(props) { + const { params, errorRetry } = props; + const cardId = parseInt(params.cardId); + + // below card is not the card in question, but + // the card we're creating to query for the error details + const card = { + name: "Card Errors", + dataset_query: { + type: "internal", + fn: "metabase-enterprise.audit-app.pages.query-detail/bad-card", + args: [cardId], + }, + }; + const question = new Question(card, null); + + return ( + <div> + <QuestionResultLoader question={question}> + {({ rawSeries, result }) => <ErrorDetailDisplay result={result} />} + </QuestionResultLoader> + <Button className="float-right" onClick={() => errorRetry(cardId)}> + {t`Rerun Question`} + </Button> + </div> + ); +} + +const mapStateToProps = (state, props) => ({ + metadata: getMetadata(state), +}); + +const mapDispatchToProps = { + errorRetry: async cardId => { + await CardApi.query({ cardId: cardId }); + // we're imagining that we successfully reran, in which case we want to go back to overall table + return push("/admin/tools/errors/"); + }, +}; + +export default connect( + mapStateToProps, + mapDispatchToProps, +)(ErrorDetail); + +ErrorDetail.propTypes = { + params: PropTypes.object, + errorRetry: PropTypes.func, +}; +ErrorDetailDisplay.propTypes = { + result: PropTypes.object, +}; diff --git a/enterprise/frontend/src/metabase-enterprise/tools/containers/ErrorOverview.jsx b/enterprise/frontend/src/metabase-enterprise/tools/containers/ErrorOverview.jsx index e020b746885f8bf63aa2372efa6633e9218b55f6..0742f183ece7d89d043cad9c24777fbec1d53362 100644 --- a/enterprise/frontend/src/metabase-enterprise/tools/containers/ErrorOverview.jsx +++ b/enterprise/frontend/src/metabase-enterprise/tools/containers/ErrorOverview.jsx @@ -1,4 +1,4 @@ -import React, { useState } from "react"; +import React, { useState, useRef } from "react"; import { t } from "ttag"; import _ from "underscore"; @@ -8,15 +8,21 @@ import { CardApi } from "metabase/services"; import * as Queries from "../../audit_app/lib/cards/queries"; import AuditTable from "../../audit_app/containers/AuditTable"; import AuditParameters from "../../audit_app/components/AuditParameters"; +import { ErrorMode } from "../mode"; const getSortOrder = isAscending => (isAscending ? "asc" : "desc"); const CARD_ID_COL = 0; export default function ErrorOverview(props) { + const reloadRef = useRef(null); + // TODO: use isReloading to display a loading overlay + // eslint-disable-next-line no-unused-vars + const [isReloading, setIsReloading] = useState(false); + const [hasResults, setHasResults] = useState(false); const [sorting, setSorting] = useState({ - column: "card_name", - isAscending: true, + column: "last_run_at", + isAscending: false, }); const [rowChecked, setRowChecked] = useState({}); @@ -29,7 +35,9 @@ export default function ErrorOverview(props) { setRowChecked(newRowChecked); setRowToCardId(newRowToCardId); }; + const handleReloadSelected = async () => { + setIsReloading(true); const checkedCardIds = Object.values( _.pick(rowToCardId, (member, key) => rowChecked[key]), ); @@ -38,10 +46,16 @@ export default function ErrorOverview(props) { async member => await CardApi.query({ cardId: member }), ), ); - location.reload(); + reloadRef.current?.reload(); }; const handleSortingChange = sorting => setSorting(sorting); + + const handleLoad = result => { + setHasResults(result[0].row_count !== 0); + setIsReloading(false); + }; + return ( <AuditParameters parameters={[ @@ -56,10 +70,12 @@ export default function ErrorOverview(props) { onClick: handleReloadSelected, }, ]} + hasResults={hasResults} > {({ errorFilter, dbFilter, collectionFilter }) => ( <AuditTable {...props} + reloadRef={reloadRef} pageSize={50} isSortable isSelectable @@ -68,6 +84,8 @@ export default function ErrorOverview(props) { sorting={sorting} onSortingChange={handleSortingChange} onRowSelectClick={handleRowSelectClick} + onLoad={handleLoad} + mode={ErrorMode} table={Queries.bad_table( errorFilter, dbFilter, diff --git a/enterprise/frontend/src/metabase-enterprise/tools/mode.js b/enterprise/frontend/src/metabase-enterprise/tools/mode.js new file mode 100644 index 0000000000000000000000000000000000000000..f59162335c32dccf3902bbfdca8c02818213fb03 --- /dev/null +++ b/enterprise/frontend/src/metabase-enterprise/tools/mode.js @@ -0,0 +1,28 @@ +import { t } from "ttag"; +import { push } from "react-router-redux"; + +const CARD_ID_ROW_IDX = 0; + +const ErrorDrill = ({ clicked }) => { + if (!clicked) { + return []; + } + + const cardId = clicked.origin.row[CARD_ID_ROW_IDX]; + + return [ + { + name: "detail", + title: t`View this`, + default: true, + action() { + return push(`/admin/tools/errors/${cardId}`); + }, + }, + ]; +}; + +export const ErrorMode = { + name: "error", + drills: () => [ErrorDrill], +}; diff --git a/enterprise/frontend/src/metabase-enterprise/tools/routes.jsx b/enterprise/frontend/src/metabase-enterprise/tools/routes.jsx index 4c6bab69a63a74ccad81f00fe213f270ce9138ee..18533bb63bd6af05b1c1efdec202b1e32d8896f8 100644 --- a/enterprise/frontend/src/metabase-enterprise/tools/routes.jsx +++ b/enterprise/frontend/src/metabase-enterprise/tools/routes.jsx @@ -5,6 +5,7 @@ import { IndexRedirect } from "react-router"; import { t } from "ttag"; import ToolsApp from "./containers/ToolsApp"; import ErrorOverview from "./containers/ErrorOverview"; +import ErrorDetail from "./containers/ErrorDetail"; const getRoutes = (store: any) => ( <Route path="tools" title={t`Tools`} component={ToolsApp}> @@ -14,6 +15,7 @@ const getRoutes = (store: any) => ( title={t`Erroring Questions`} component={ErrorOverview} /> + <Route path="errors/:cardId" component={ErrorDetail} /> </Route> );