Skip to content

Commit

Permalink
[ML] Single metric viewer: Fix race condition when changing jobs. (el…
Browse files Browse the repository at this point in the history 
…astic#45420)

Fixes a race condition where changing jobs in Single Metric Viewer could result in the focus chart being empty.
Applying the job change would cause two events to be triggered which would cause the Single Metric Viewer to refresh twice: The updated time range would be triggered via timefilter, the updated jobs via the globalState listener.
This PR solves the problem by setting a global flag skipRefresh which allows us to skip updating single metric viewer and only update again once both the new time range and jobs are set. The PR also adds some checks to avoid unnecessary updates in d3 code triggered by React render calls.
  • Loading branch information
@walterra
walterra committed Sep 13, 2019
1 parent cca4d85 commit 97ecf9c
Show file tree
Hide file tree
Showing 4 changed files with 115 additions and 32 deletions.
22 changes: 21 additions & 1 deletion x-pack/legacy/plugins/ml/public/components/job_selector/job_select_service_utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,12 +101,32 @@ function loadJobIdsFromGlobalState(globalState) { // jobIds, groups
return { jobIds, selectedGroups: groups };
}

export function setGlobalState(globalState, { selectedIds, selectedGroups }) {
// TODO:
// Merge `setGlobalStateSkipRefresh()` and `setGlobalState()` into
// a single function similar to how we do `appStateHandler()`.
// When changing jobs in job selector it would trigger multiple events
// which in return would be consumed by Single Metric Viewer and could cause
// race conditions when updating the whole page. Because we don't control
// the internals of the involved timefilter event triggering, we use
// a global `skipRefresh` to control when Single Metric Viewer should
// skip updates triggered by timefilter.
export function setGlobalStateSkipRefresh(globalState, skipRefresh) {
globalState.fetch();
if (globalState.ml === undefined) {
globalState.ml = {};
}
globalState.ml.skipRefresh = skipRefresh;
globalState.save();
}

export function setGlobalState(globalState, { selectedIds, selectedGroups, skipRefresh }) {
globalState.fetch();
if (globalState.ml === undefined) {
globalState.ml = {};
}
globalState.ml.jobIds = selectedIds;
globalState.ml.groups = selectedGroups || [];
globalState.ml.skipRefresh = !!skipRefresh;
globalState.save();
}

Expand Down
44 changes: 37 additions & 7 deletions x-pack/legacy/plugins/ml/public/components/job_selector/job_selector.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/



import { isEqual } from 'lodash';
import React, { useState, useEffect, useRef, useCallback } from 'react';
import { PropTypes } from 'prop-types';
import moment from 'moment';
Expand All @@ -15,7 +14,7 @@ import { JobSelectorTable } from './job_selector_table/';
import { IdBadges } from './id_badges';
import { NewSelectionIdBadges } from './new_selection_id_badges';
import { timefilter } from 'ui/timefilter';
import { getGroupsFromJobs, normalizeTimes, setGlobalState } from './job_select_service_utils';
import { getGroupsFromJobs, normalizeTimes, setGlobalState, setGlobalStateSkipRefresh } from './job_select_service_utils';
import { toastNotifications } from 'ui/notify';
import {
EuiButton,
Expand Down Expand Up @@ -139,12 +138,21 @@ export function JobSelector({
handleResize();
}, [handleResize, jobs]);

function closeFlyout() {
// On opening and closing the flyout, optionally update a global `skipRefresh` flag.
// This allows us to circumvent race conditions which could happen by triggering both
// timefilter and job selector related events in Single Metric Viewer.
function closeFlyout(setSkipRefresh = true) {
setIsFlyoutVisible(false);
if (setSkipRefresh) {
setGlobalStateSkipRefresh(globalState, false);
}
}

function showFlyout() {
function showFlyout(setSkipRefresh = true) {
setIsFlyoutVisible(true);
if (setSkipRefresh) {
setGlobalStateSkipRefresh(globalState, true);
}
}

function handleJobSelectionClick() {
Expand Down Expand Up @@ -173,7 +181,6 @@ export function JobSelector({
}

function applySelection() {
closeFlyout();
// allNewSelection will be a list of all job ids (including those from groups) selected from the table
const allNewSelection = [];
const groupSelection = [];
Expand All @@ -191,10 +198,33 @@ export function JobSelector({
// create a Set to remove duplicate values
const allNewSelectionUnique = Array.from(new Set(allNewSelection));

const isPrevousSelection = isEqual(
{ selectedJobIds, selectedGroups },
{ selectedJobIds: allNewSelectionUnique, selectedGroups: groupSelection }
);

setSelectedIds(newSelection);
setNewSelection([]);

// If the job selection is unchanged, then we close the modal and
// disable skipping the timefilter listener flag in globalState.
// If the job selection changed, this will not
// update skipRefresh yet to avoid firing multiple events via
// applyTimeRangeFromSelection() and setGlobalState().
closeFlyout(isPrevousSelection);

// If the job selection changed, then when
// calling `applyTimeRangeFromSelection()` here
// Single Metric Viewer will skip an update
// triggered by timefilter to avoid a race
// condition caused by the job update listener
// that's also going to be triggered.
applyTimeRangeFromSelection(allNewSelectionUnique);
setGlobalState(globalState, { selectedIds: allNewSelectionUnique, selectedGroups: groupSelection });

// Set `skipRefresh` again to `false` here so after
// both the time range and jobs have been updated
// Single Metric Viewer should again update itself.
setGlobalState(globalState, { selectedIds: allNewSelectionUnique, selectedGroups: groupSelection, skipRefresh: false });
}

function applyTimeRangeFromSelection(selection) {
Expand Down
39 changes: 28 additions & 11 deletions ...gacy/plugins/ml/public/timeseriesexplorer/components/timeseries_chart/timeseries_chart.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ const TimeseriesChartIntl = injectI18n(class TimeseriesChart extends React.Compo
focusAnnotationData: PropTypes.array,
focusChartData: PropTypes.array,
focusForecastData: PropTypes.array,
loading: PropTypes.bool.isRequired,
skipRefresh: PropTypes.bool.isRequired,
modelPlotEnabled: PropTypes.bool.isRequired,
renderFocusChartOnly: PropTypes.bool.isRequired,
selectedJob: PropTypes.object,
Expand All @@ -113,7 +113,9 @@ const TimeseriesChartIntl = injectI18n(class TimeseriesChart extends React.Compo
swimlaneData: PropTypes.array,
timefilter: PropTypes.object.isRequired,
zoomFrom: PropTypes.object,
zoomTo: PropTypes.object
zoomTo: PropTypes.object,
zoomFromFocusLoaded: PropTypes.object,
zoomToFocusLoaded: PropTypes.object
};

rowMouseenterSubscriber = null;
Expand Down Expand Up @@ -203,7 +205,7 @@ const TimeseriesChartIntl = injectI18n(class TimeseriesChart extends React.Compo
}

componentDidUpdate() {
if (this.props.loading) {
if (this.props.skipRefresh) {
return;
}

Expand Down Expand Up @@ -383,12 +385,17 @@ const TimeseriesChartIntl = injectI18n(class TimeseriesChart extends React.Compo

if ((focusLoadFrom !== contextXMin) || (focusLoadTo !== contextXMax)) {
this.setContextBrushExtent(new Date(focusLoadFrom), new Date(focusLoadTo), true);
const newSelectedBounds = { min: moment(new Date(focusLoadFrom)), max: moment(focusLoadFrom) };
this.selectedBounds = newSelectedBounds;
} else {
// Don't set the brush if the selection is the full context chart domain.
this.setBrushVisibility(false);
const selectedBounds = this.contextXScale.domain();
this.selectedBounds = { min: moment(new Date(selectedBounds[0])), max: moment(selectedBounds[1]) };
contextChartSelected({ from: selectedBounds[0], to: selectedBounds[1] });
const contextXScaleDomain = this.contextXScale.domain();
const newSelectedBounds = { min: moment(new Date(contextXScaleDomain[0])), max: moment(contextXScaleDomain[1]) };
if (!_.isEqual(newSelectedBounds, this.selectedBounds)) {
this.selectedBounds = newSelectedBounds;
contextChartSelected({ from: contextXScaleDomain[0], to: contextXScaleDomain[1] });
}
}
}

Expand Down Expand Up @@ -512,7 +519,9 @@ const TimeseriesChartIntl = injectI18n(class TimeseriesChart extends React.Compo
showAnnotations,
showForecast,
showModelBounds,
intl
intl,
zoomFromFocusLoaded,
zoomToFocusLoaded,
} = this.props;

if (focusChartData === undefined) {
Expand Down Expand Up @@ -543,10 +552,11 @@ const TimeseriesChartIntl = injectI18n(class TimeseriesChart extends React.Compo
// Elasticsearch aggregation returns points at start of bucket,
// so set the x-axis min to the start of the first aggregation interval,
// and the x-axis max to the end of the last aggregation interval.
const bounds = this.selectedBounds;
if (typeof bounds === 'undefined') {
if (zoomFromFocusLoaded === undefined || zoomToFocusLoaded === undefined) {
return;
}
const bounds = { min: moment(zoomFromFocusLoaded.getTime()), max: moment(zoomToFocusLoaded.getTime()) };

const aggMs = focusAggregationInterval.asMilliseconds();
const earliest = moment(Math.floor((bounds.min.valueOf()) / aggMs) * aggMs);
const latest = moment(Math.ceil((bounds.max.valueOf()) / aggMs) * aggMs);
Expand Down Expand Up @@ -1046,7 +1056,9 @@ const TimeseriesChartIntl = injectI18n(class TimeseriesChart extends React.Compo
rightHandle.attr('x', contextXScale(brushExtent[1]) + 0);

topBorder.attr('x', contextXScale(brushExtent[0]) + 1);
topBorder.attr('width', contextXScale(brushExtent[1]) - contextXScale(brushExtent[0]) - 2);
// Use Math.max(0, ...) to make sure we don't end up
// with a negative width which would cause an SVG error.
topBorder.attr('width', Math.max(0, contextXScale(brushExtent[1]) - contextXScale(brushExtent[0]) - 2));
}

this.setBrushVisibility(show);
Expand All @@ -1061,8 +1073,11 @@ const TimeseriesChartIntl = injectI18n(class TimeseriesChart extends React.Compo

const that = this;
function brushed() {
if (that.props.skipRefresh) {
return;
}

const isEmpty = brush.empty();
showBrush(!isEmpty);

const selectedBounds = isEmpty ? contextXScale.domain() : brush.extent();
const selectionMin = selectedBounds[0].getTime();
Expand All @@ -1077,6 +1092,8 @@ const TimeseriesChartIntl = injectI18n(class TimeseriesChart extends React.Compo
return;
}

showBrush(!isEmpty);

// Set the color of the swimlane cells according to whether they are inside the selection.
contextGroup.selectAll('.swimlane-cell')
.style('fill', (d) => {
Expand Down
42 changes: 29 additions & 13 deletions x-pack/legacy/plugins/ml/public/timeseriesexplorer/timeseriesexplorer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,8 @@ function getTimeseriesexplorerDefaultState() {
tableData: undefined,
zoomFrom: undefined,
zoomTo: undefined,
zoomFromFocusLoaded: undefined,
zoomToFocusLoaded: undefined,

// Toggles display of model bounds in the focus chart
showModelBounds: true,
Expand Down Expand Up @@ -238,8 +240,8 @@ export class TimeSeriesExplorer extends React.Component {
focusChartData,
jobs,
selectedJob,
zoomFrom,
zoomTo,
zoomFromFocusLoaded,
zoomToFocusLoaded,
} = this.state;


Expand All @@ -265,10 +267,15 @@ export class TimeSeriesExplorer extends React.Component {
appStateHandler(APP_STATE_ACTION.UNSET_ZOOM);
}

this.setState({
zoomFrom: selection.from,
zoomTo: selection.to,
});

if (
(this.contextChartSelectedInitCallDone === false && focusChartData === undefined) ||
(zoomFrom.getTime() !== selection.from.getTime()) ||
(zoomTo.getTime() !== selection.to.getTime())
(zoomFromFocusLoaded.getTime() !== selection.from.getTime()) ||
(zoomToFocusLoaded.getTime() !== selection.to.getTime())
) {
this.contextChartSelectedInitCallDone = true;

Expand Down Expand Up @@ -297,6 +304,8 @@ export class TimeSeriesExplorer extends React.Component {
this.setState({
loading: true,
fullRefresh: false,
zoomFrom: selection.from,
zoomTo: selection.to,
});

getFocusData(
Expand All @@ -316,16 +325,13 @@ export class TimeSeriesExplorer extends React.Component {
...refreshFocusData,
loading: false,
showModelBoundsCheckbox: (modelPlotEnabled === true) && (refreshFocusData.focusChartData.length > 0),
zoomFromFocusLoaded: selection.from,
zoomToFocusLoaded: selection.to,
});
});

// Load the data for the anomalies table.
this.loadAnomaliesTableData(searchBounds.min.valueOf(), searchBounds.max.valueOf());

this.setState({
zoomFrom: selection.from,
zoomTo: selection.to,
});
}
}

Expand Down Expand Up @@ -479,7 +485,13 @@ export class TimeSeriesExplorer extends React.Component {
}

refresh = (fullRefresh = true) => {
if (this.state.loading && fullRefresh === false) {
// Skip the refresh if:
// a) The global state's `skipRefresh` was set to true by the job selector to avoid race conditions
// when loading the Single Metric Viewer after a job/group and time range update.
// b) A 'soft' refresh without a full page reload is already happening.
if (get(this.props.globalState, 'ml.skipRefresh') || (
this.state.loading && fullRefresh === false
)) {
return;
}

Expand Down Expand Up @@ -821,7 +833,7 @@ export class TimeSeriesExplorer extends React.Component {
const jobs = createTimeSeriesJobData(mlJobService.jobs);

this.contextChartSelectedInitCallDone = false;
this.setState({ showForecastCheckbox: false });
this.setState({ fullRefresh: false, loading: true, showForecastCheckbox: false });

const timeSeriesJobIds = jobs.map(j => j.id);

Expand Down Expand Up @@ -907,7 +919,7 @@ export class TimeSeriesExplorer extends React.Component {
timefilter.enableTimeRangeSelector();
timefilter.enableAutoRefreshSelector();

this.subscriptions.add(timefilter.getTimeUpdate$().subscribe(this.refresh));
this.subscriptions.add(timefilter.getTimeUpdate$().subscribe(() => this.refresh(false)));

// Required to redraw the time series chart when the container is resized.
this.resizeChecker = new ResizeChecker(this.resizeRef.current);
Expand Down Expand Up @@ -961,6 +973,8 @@ export class TimeSeriesExplorer extends React.Component {
tableData,
zoomFrom,
zoomTo,
zoomFromFocusLoaded,
zoomToFocusLoaded,
} = this.state;

const chartProps = {
Expand All @@ -974,10 +988,12 @@ export class TimeSeriesExplorer extends React.Component {
focusChartData,
focusForecastData,
focusAggregationInterval,
loading,
skipRefresh: loading || !!get(this.props.globalState, 'ml.skipRefresh'),
svgWidth,
zoomFrom,
zoomTo,
zoomFromFocusLoaded,
zoomToFocusLoaded,
autoZoomDuration,
};

Expand Down

0 comments on commit 97ecf9c

Please sign in to comment.