Skip to content

DM-8214 - handling server errors in charts #244

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

Merged
merged 1 commit into from
Nov 29, 2016
Merged

DM-8214 - handling server errors in charts #244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 src/firefly/js/charts/ChartDataType.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {logError} from '../util/WebUtil.js';
* @prop {string} id - unique chart data type id
* @prop {Function} fetchData - function to load chart data element data: fetchData(dispatch, chartId, chartDataElementId)
* @prop {Function} fetchParamsChanged - function to determine if fetch is necessary: fetchParamsChanged(oldOptions, newOptions)
* @prop {Function} getUpdatedOptions - function to resolve the options, which depend on table or chart data getUpdatedOptions(options, tblId, data, meta)
* @prop {Function} [getUpdatedOptions=undefined] - function to resolve the options, which depend on table or chart data getUpdatedOptions(options, tblId, data, meta)
* @prop {boolean} [fetchOnTblSort=true] - if false, don't re-fetch data on tbl sort
*/

Expand Down
22 changes: 22 additions & 0 deletions src/firefly/js/charts/ChartsCntlr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,9 @@ function doChartDataFetch(dispatch, payload, getChartDataType) {
* @param payload.chartId
* @param payload.chartDataElementId
* @param payload.isDataReady
* @param payload.error
* @param payload.error.message
* @param payload.error.reason
* @param payload.data
* @param [payload.options]
* @param [payload.meta]
Expand Down Expand Up @@ -522,6 +525,25 @@ export function getChartDataElement(chartId, chartDataElementId=FIRST_CDEL_ID) {
return get(flux.getState(), [CHART_SPACE_PATH, 'data', chartId, 'chartDataElements', chartDataElementId]);
}

/**
* Get error object associated with the given chart data element
* @param chartId
* @returns {Array<{message:string, reason:object}>} an array of error objects
*/
export function getErrors(chartId) {
const chartDataElements = get(flux.getState(), [CHART_SPACE_PATH, 'data', chartId, 'chartDataElements']);
const errors = [];
if (chartDataElements) {
Object.keys(chartDataElements).forEach((id) => {
const error = chartDataElements[id].error;
if (error) {
errors.push(error);
}
});
}
return errors;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

-just for your reference, using 'reduce' is nicer than 'forEach' in terms of functional nature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the note. Indeed, looks like in my code I use forEach a lot, even where simple for loop would be more performant. (See https://jsperf.com/array-reduce-vs-foreach/9) I just find forEach more readable, especially when the values need to be accumulated in an array.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI - we should not worry about performance for stuff like this unless it is clear there is a need. Most of our objects and arrays are so small the speed difference is negligible anyway. Those comparisons will do the same thing like 10,000 times and then compare.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. The only place where it matters is when processing arrays of hundred thousands elements.

Copy link
Contributor

@loitly loitly Nov 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Object.entries() is a good alternative.

const errors = Object.entries(chartDataElements)
                                    .filter( ([k,v]) -> v.error)
                                    .map( ([k,v]) -> v.error);


export function getExpandedChartProps() {
const chartId = get(flux.getState(), [CHART_SPACE_PATH, 'ui', 'expanded']);
return {chartId};
Expand Down
12 changes: 11 additions & 1 deletion src/firefly/js/charts/dataTypes/HistogramCDT.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,14 +161,24 @@ function fetchColData(dispatch, chartId, chartDataElementId) {
chartId,
chartDataElementId,
isDataReady: true,
error: undefined,
options : histogramParams,
data: histogramData,
meta: {tblSource}
}));
}
).catch(
(reason) => {
console.error(`Failed to fetch histogram data: ${reason}`);
const message = 'Failed to fetch histogram data';
logError(`${message}: ${reason}`);
dispatch(chartDataUpdate(
{
chartId,
chartDataElementId,
isDataReady: true,
error: {message, reason},
data: undefined
}));
}
);
}
17 changes: 14 additions & 3 deletions src/firefly/js/charts/dataTypes/XYColsCDT.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ import {updateSet, logError} from '../../util/WebUtil.js';
import {get, omitBy, isEmpty, isString, isUndefined} from 'lodash';

import {MetaConst} from '../../data/MetaConst.js';
import {doFetchTable, getColumn, getTblById, isFullyLoaded, cloneRequest} from '../../tables/TableUtil.js';
import {fetchTable} from '../../rpc/SearchServicesJson.js';
import {getColumn, getTblById, isFullyLoaded, cloneRequest} from '../../tables/TableUtil.js';

import {getChartDataElement, dispatchChartAdd, dispatchChartOptionsUpdate, chartDataUpdate} from './../ChartsCntlr.js';
import {colWithName, getNumericCols, SCATTER} from './../ChartUtil.js';
Expand Down Expand Up @@ -325,7 +326,7 @@ function fetchPlotData(dispatch, chartId, chartDataElementId) {
});
req.tbl_id = `xy-${chartId}`;

doFetchTable(req).then(
fetchTable(req).then(
(tableModel) => {

// make sure we only save the data from the latest fetch
Expand Down Expand Up @@ -369,14 +370,24 @@ function fetchPlotData(dispatch, chartId, chartDataElementId) {
chartId,
chartDataElementId,
isDataReady: true,
error: undefined,
options: getUpdatedParams(xyPlotParams, tblId, xyPlotData),
data: xyPlotData,
meta: {tblSource, decimatedUnzoomed}
}));
}
).catch(
(reason) => {
logError(`Failed to fetch XY plot data: ${reason}`);
const message = 'Failed to fetch XY plot data';
logError(`${message}: ${reason}`);
dispatch(chartDataUpdate(
{
chartId,
chartDataElementId,
isDataReady: true,
error: {message, reason},
data: undefined
}));
}
);

Expand Down
37 changes: 34 additions & 3 deletions src/firefly/js/charts/ui/ChartPanel.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ class ChartPanelView extends Component {

var {widthPx, heightPx, componentKey, optionsShown} = this.state;
const knownSize = widthPx && heightPx;

const errors = ChartsCntlr.getErrors(chartId);

if (showChart) {
// chart with toolbar and options
Expand All @@ -113,7 +113,10 @@ class ChartPanelView extends Component {
<Resizable id='chart-resizer' onResize={this.onResize}
className='ChartPanel__chartresizer'>
<div style={{overflow:'auto',width:widthPx,height:heightPx}}>
{knownSize ? <Chart {...Object.assign({}, this.props, {widthPx, heightPx})}/> :
{knownSize ?
errors.length > 0 ?
<ErrorPanel errors={errors}/> :
<Chart {...Object.assign({}, this.props, {widthPx, heightPx})}/> :
<div/>}
</div>
</Resizable>
Expand All @@ -135,7 +138,11 @@ class ChartPanelView extends Component {
<Resizable id='chart-resizer' onResize={this.onResize}
className='ChartPanel__chartresizer'>
<div style={{overflow:'auto',width:widthPx,height:heightPx}}>
{knownSize ? <Chart {...Object.assign({}, this.props, {widthPx, heightPx})}/> :
{knownSize ?
errors.length > 0 ?
<ErrorPanel errors={errors}/> :
<Chart {...Object.assign({}, this.props, {widthPx, heightPx})}/> :

<div/>}
</div>
</Resizable>
Expand Down Expand Up @@ -191,6 +198,30 @@ ChartPanelView.defaultProps = {
showChart: true
};

function ErrorPanel({errors}) {
return (
<div style={{position: 'relative', width: '100%', height: '100%'}}>
{errors.map((error, i) => {
const {message='Error', reason=''} = error;
return (
<div key={i} style={{padding: 20, textAlign: 'center', overflowWrap: 'normal'}}>
<h3>{message}</h3>
{`${reason}`}
</div>
);
})}
</div>
);
}

ErrorPanel.propTypes = {
errors: PropTypes.arrayOf(
PropTypes.shape({
message: PropTypes.string,
reason: PropTypes.oneOfType([PropTypes.object,PropTypes.string]) // reason can be an Error object
}))
};

export class ChartPanel extends Component {

constructor(props) {
Expand Down