Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf(dashboard): make loading datasets non-blocking #15699

Merged
merged 4 commits into from
Jul 15, 2021
Merged

perf(dashboard): make loading datasets non-blocking #15699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
92fda2d
perf(dashboard): make loading datasets non-blocking
ktmud Jul 15, 2021
ddee3e9
Remove console.log
ktmud Jul 15, 2021
e3ccef1
Fix tests
ktmud Jul 15, 2021
f6a090b
Fix chart re-renders
ktmud Jul 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion superset-frontend/src/chart/Chart.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ const propTypes = {
annotationData: PropTypes.object,
actions: PropTypes.object,
chartId: PropTypes.number.isRequired,
datasource: PropTypes.object.isRequired,
datasource: PropTypes.object,
// current chart is included by dashboard
dashboardId: PropTypes.number,
// original selected values for FilterBox viz
Expand Down
5 changes: 3 additions & 2 deletions superset-frontend/src/chart/ChartRenderer.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ const propTypes = {
annotationData: PropTypes.object,
actions: PropTypes.object,
chartId: PropTypes.number.isRequired,
datasource: PropTypes.object.isRequired,
datasource: PropTypes.object,
initialValues: PropTypes.object,
formData: PropTypes.object.isRequired,
height: PropTypes.number,
Expand Down Expand Up @@ -93,6 +93,7 @@ class ChartRenderer extends React.Component {
nextProps.queriesResponse !== this.props.queriesResponse;
return (
this.hasQueryResponseChange ||
nextProps.datasource !== this.props.datasource ||
nextProps.annotationData !== this.props.annotationData ||
nextProps.ownState !== this.props.ownState ||
nextProps.filterState !== this.props.filterState ||
Expand Down Expand Up @@ -223,7 +224,7 @@ class ChartRenderer extends React.Component {
width={width}
height={height}
annotationData={annotationData}
datasource={datasource}
datasource={datasource || {}}
initialValues={initialValues}
formData={formData}
ownState={ownState}
Expand Down
7 changes: 4 additions & 3 deletions superset-frontend/src/common/hooks/apiResources/dashboards.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@
* under the License.
*/

import Dashboard from 'src/types/Dashboard';
import { Dashboard, Datasource } from 'src/dashboard/types';
import { Chart } from 'src/types/Chart';
import { useApiV1Resource, useTransformedResource } from './apiResources';

export const useDashboard = (idOrSlug: string | number) =>
Expand All @@ -33,10 +34,10 @@ export const useDashboard = (idOrSlug: string | number) =>

// gets the chart definitions for a dashboard
export const useDashboardCharts = (idOrSlug: string | number) =>
useApiV1Resource(`/api/v1/dashboard/${idOrSlug}/charts`);
useApiV1Resource<Chart[]>(`/api/v1/dashboard/${idOrSlug}/charts`);

// gets the datasets for a dashboard
// important: this endpoint only returns the fields in the dataset
// that are necessary for rendering the given dashboard
export const useDashboardDatasets = (idOrSlug: string | number) =>
useApiV1Resource(`/api/v1/dashboard/${idOrSlug}/datasets`);
useApiV1Resource<Datasource[]>(`/api/v1/dashboard/${idOrSlug}/datasets`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: perhaps we should rename the Datasource type to Dataset? Or at least alias it for now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are many places where the Datasource/Dataset type is defined---including two places in superset-ui. I think we need a separate PR to consolidate those. Although they do have small differences depending on the context.

50 changes: 31 additions & 19 deletions ...tend/src/dashboard/actions/datasources.js → ...tend/src/dashboard/actions/datasources.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,26 +16,44 @@
* specific language governing permissions and limitations
* under the License.
*/
import { Dispatch } from 'redux';
import { SupersetClient } from '@superset-ui/core';
import { getClientErrorObject } from '../../utils/getClientErrorObject';
import { Datasource, RootState } from 'src/dashboard/types';

export const SET_DATASOURCE = 'SET_DATASOURCE';
export function setDatasource(datasource, key) {
return { type: SET_DATASOURCE, datasource, key };
// update datasources index for Dashboard
export enum DatasourcesAction {
SET_DATASOURCES = 'SET_DATASOURCES',
SET_DATASOURCE = 'SET_DATASOURCE',
}

export const FETCH_DATASOURCE_STARTED = 'FETCH_DATASOURCE_STARTED';
export function fetchDatasourceStarted(key) {
return { type: FETCH_DATASOURCE_STARTED, key };
export type DatasourcesActionPayload =
| {
type: DatasourcesAction.SET_DATASOURCES;
datasources: Datasource[] | null;
}
| {
type: DatasourcesAction.SET_DATASOURCE;
key: Datasource['uid'];
datasource: Datasource;
};

export function setDatasources(datasources: Datasource[] | null) {
return {
type: DatasourcesAction.SET_DATASOURCES,
datasources,
};
}

export const FETCH_DATASOURCE_FAILED = 'FETCH_DATASOURCE_FAILED';
export function fetchDatasourceFailed(error, key) {
return { type: FETCH_DATASOURCE_FAILED, error, key };
Copy link
Member Author

@ktmud ktmud Jul 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleaned up FETCH_DATASOURCE_STARTED and FETCH_DATASOURCE_FAILED because there are no corresponding reducers for them.

export function setDatasource(datasource: Datasource, key: string) {
return {
type: DatasourcesAction.SET_DATASOURCE,
key,
datasource,
};
}

export function fetchDatasourceMetadata(key) {
return (dispatch, getState) => {
export function fetchDatasourceMetadata(key: string) {
return (dispatch: Dispatch, getState: () => RootState) => {
const { datasources } = getState();
const datasource = datasources[key];

Expand All @@ -45,12 +63,6 @@ export function fetchDatasourceMetadata(key) {

return SupersetClient.get({
endpoint: `/superset/fetch_datasource_metadata?datasourceKey=${key}`,
})
.then(({ json }) => dispatch(setDatasource(json, key)))
.catch(response =>
getClientErrorObject(response).then(({ error }) =>
dispatch(fetchDatasourceFailed(error, key)),
),
);
}).then(({ json }) => dispatch(setDatasource(json as Datasource, key)));
};
}
5 changes: 2 additions & 3 deletions superset-frontend/src/dashboard/actions/hydrate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/
/* eslint-disable camelcase */
import { isString, keyBy } from 'lodash';
import { isString } from 'lodash';
import {
Behavior,
CategoricalColorNamespace,
Expand Down Expand Up @@ -60,7 +60,7 @@ import extractUrlParams from '../util/extractUrlParams';

export const HYDRATE_DASHBOARD = 'HYDRATE_DASHBOARD';

export const hydrateDashboard = (dashboardData, chartData, datasourcesData) => (
export const hydrateDashboard = (dashboardData, chartData) => (
dispatch,
getState,
) => {
Expand Down Expand Up @@ -329,7 +329,6 @@ export const hydrateDashboard = (dashboardData, chartData, datasourcesData) => (
return dispatch({
type: HYDRATE_DASHBOARD,
data: {
datasources: keyBy(datasourcesData, 'uid'),
sliceEntities: { ...initSliceEntities, slices, isLoading: false },
charts: chartQueries,
// read-only data
Expand Down
5 changes: 3 additions & 2 deletions superset-frontend/src/dashboard/components/gridComponents/Chart.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ const propTypes = {
// from redux
chart: chartPropShape.isRequired,
formData: PropTypes.object.isRequired,
datasource: PropTypes.object.isRequired,
datasource: PropTypes.object,
slice: slicePropShape.isRequired,
sliceName: PropTypes.string.isRequired,
timeout: PropTypes.number.isRequired,
Expand Down Expand Up @@ -125,7 +125,8 @@ export default class Chart extends React.Component {
if (
nextState.width !== this.state.width ||
nextState.height !== this.state.height ||
nextState.descriptionHeight !== this.state.descriptionHeight
nextState.descriptionHeight !== this.state.descriptionHeight ||
nextProps.datasource !== this.props.datasource
) {
return true;
}
Expand Down
17 changes: 10 additions & 7 deletions superset-frontend/src/dashboard/containers/Dashboard.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,21 @@
*/
import { bindActionCreators, Dispatch } from 'redux';
import { connect } from 'react-redux';
import Dashboard from '../components/Dashboard';
import { RootState } from 'src/dashboard/types';
import Dashboard from 'src/dashboard/components/Dashboard';
import {
addSliceToDashboard,
removeSliceFromDashboard,
} from '../actions/dashboardState';
import { triggerQuery } from '../../chart/chartAction';
import { logEvent } from '../../logger/actions';
import { getActiveFilters } from '../util/activeDashboardFilters';
} from 'src/dashboard/actions/dashboardState';
import { setDatasources } from 'src/dashboard/actions/datasources';

import { triggerQuery } from 'src/chart/chartAction';
import { logEvent } from 'src/logger/actions';
import { getActiveFilters } from 'src/dashboard/util/activeDashboardFilters';
import {
getAllActiveFilters,
getRelevantDataMask,
} from '../util/activeAllDashboardFilters';
import { RootState } from '../types';
} from 'src/dashboard/util/activeAllDashboardFilters';

function mapStateToProps(state: RootState) {
const {
Expand Down Expand Up @@ -80,6 +82,7 @@ function mapDispatchToProps(dispatch: Dispatch) {
return {
actions: bindActionCreators(
{
setDatasources,
addSliceToDashboard,
removeSliceFromDashboard,
triggerQuery,
Expand Down
77 changes: 44 additions & 33 deletions superset-frontend/src/dashboard/containers/DashboardPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,19 @@
* specific language governing permissions and limitations
* under the License.
*/
import React, { useEffect, useState, FC } from 'react';
import React, { useEffect, FC } from 'react';
import { t } from '@superset-ui/core';
import { useDispatch } from 'react-redux';
import { useParams } from 'react-router-dom';
import { useToasts } from 'src/messageToasts/enhancers/withToasts';
import Loading from 'src/components/Loading';
import {
useDashboard,
useDashboardCharts,
useDashboardDatasets,
} from 'src/common/hooks/apiResources';
import { ResourceStatus } from 'src/common/hooks/apiResources/apiResources';
import { hydrateDashboard } from 'src/dashboard/actions/hydrate';
import { setDatasources } from 'src/dashboard/actions/datasources';
import injectCustomCss from 'src/dashboard/util/injectCustomCss';

const DashboardContainer = React.lazy(
Expand All @@ -40,48 +42,57 @@ const DashboardContainer = React.lazy(

const DashboardPage: FC = () => {
const dispatch = useDispatch();
const { addDangerToast } = useToasts();
const { idOrSlug } = useParams<{ idOrSlug: string }>();
const [isHydrated, setHydrated] = useState(false);
const dashboardResource = useDashboard(idOrSlug);
const chartsResource = useDashboardCharts(idOrSlug);
const datasetsResource = useDashboardDatasets(idOrSlug);
const error = [dashboardResource, chartsResource, datasetsResource].find(
resource => resource.status === ResourceStatus.ERROR,
)?.error;
const { result: dashboard, error: dashboardApiError } = useDashboard(
idOrSlug,
);
const { result: charts, error: chartsApiError } = useDashboardCharts(
idOrSlug,
);
const { result: datasets, error: datasetsApiError } = useDashboardDatasets(
idOrSlug,
);

const error = dashboardApiError || chartsApiError;
const readyToRender = Boolean(dashboard && charts);
const { dashboard_title, css } = dashboard || {};

useEffect(() => {
if (dashboardResource.result) {
document.title = dashboardResource.result.dashboard_title;
if (dashboardResource.result.css) {
// returning will clean up custom css
// when dashboard unmounts or changes
return injectCustomCss(dashboardResource.result.css);
}
if (readyToRender) {
dispatch(hydrateDashboard(dashboard, charts));
}
return () => {};
}, [dashboardResource.result]);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [readyToRender]);

const shouldBeHydrated =
dashboardResource.status === ResourceStatus.COMPLETE &&
chartsResource.status === ResourceStatus.COMPLETE &&
datasetsResource.status === ResourceStatus.COMPLETE;
useEffect(() => {
if (dashboard_title) {
document.title = dashboard_title;
}
}, [dashboard_title]);

useEffect(() => {
if (shouldBeHydrated) {
dispatch(
hydrateDashboard(
dashboardResource.result,
chartsResource.result,
datasetsResource.result,
),
if (css) {
// returning will clean up custom css
// when dashboard unmounts or changes
return injectCustomCss(css);
}
return () => {};
}, [css]);

useEffect(() => {
if (datasetsApiError) {
addDangerToast(
t('Error loading chart datasources. Filters may not work correctly.'),
);
setHydrated(true);
} else {
dispatch(setDatasources(datasets));
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [shouldBeHydrated]);
}, [addDangerToast, datasets, datasetsApiError, dispatch]);

if (error) throw error; // caught in error boundary
if (!isHydrated) return <Loading />;
if (!readyToRender) return <Loading />;

return <DashboardContainer />;
};

Expand Down
46 changes: 22 additions & 24 deletions ...end/src/dashboard/reducers/datasources.js → ...end/src/dashboard/reducers/datasources.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,30 +16,28 @@
* specific language governing permissions and limitations
* under the License.
*/
import { SET_DATASOURCE } from '../actions/datasources';
import { HYDRATE_DASHBOARD } from '../actions/hydrate';
import { keyBy } from 'lodash';
import { DatasourcesState } from 'src/dashboard/types';
import {
DatasourcesActionPayload,
DatasourcesAction,
} from '../actions/datasources';

export default function datasourceReducer(datasources = {}, action) {
const actionHandlers = {
[HYDRATE_DASHBOARD]() {
return action.data.datasources;
},
[SET_DATASOURCE]() {
return action.datasource;
},
};

if (action.type in actionHandlers) {
if (action.key) {
return {
...datasources,
[action.key]: actionHandlers[action.type](
datasources[action.key],
action,
),
};
}
return actionHandlers[action.type]();
export default function datasourcesReducer(
datasources: DatasourcesState | undefined,
action: DatasourcesActionPayload,
) {
if (action.type === DatasourcesAction.SET_DATASOURCES) {
return {
...datasources,
...keyBy(action.datasources, 'uid'),
};
}
if (action.type === DatasourcesAction.SET_DATASOURCE) {
return {
...datasources,
[action.key]: action.datasource,
};
}
return datasources;
return datasources || {};
}
Loading