[ML] Single metric viewer: Fix race condition when changing jobs. (#45420) (#45644)

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.

x-pack/legacy/plugins/ml/public/components/job_selector/job_select_service_utils.js

@@ -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();
 }
 

x-pack/legacy/plugins/ml/public/components/job_selector/job_selector.js

@@ -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';
@@ -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,
@@ -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() {
@@ -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 = [];
@@ -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) {

x-pack/legacy/plugins/ml/public/timeseriesexplorer/components/timeseries_chart/timeseries_chart.js

@@ -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,
@@ -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;
@@ -203,7 +205,7 @@ const TimeseriesChartIntl = injectI18n(class TimeseriesChart extends React.Compo
   }
 
   componentDidUpdate() {
-    if (this.props.loading) {
+    if (this.props.skipRefresh) {
       return;
     }
 
@@ -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] });
+      }
     }
   }
 
@@ -512,7 +519,9 @@ const TimeseriesChartIntl = injectI18n(class TimeseriesChart extends React.Compo
       showAnnotations,
       showForecast,
       showModelBounds,
-      intl
+      intl,
+      zoomFromFocusLoaded,
+      zoomToFocusLoaded,
     } = this.props;
 
     if (focusChartData === undefined) {
@@ -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);
@@ -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);
@@ -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();
@@ -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) => {

x-pack/legacy/plugins/ml/public/timeseriesexplorer/timeseriesexplorer.js

@@ -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,
@@ -238,8 +240,8 @@ export class TimeSeriesExplorer extends React.Component {
       focusChartData,
       jobs,
       selectedJob,
-      zoomFrom,
-      zoomTo,
+      zoomFromFocusLoaded,
+      zoomToFocusLoaded,
     } = this.state;
 
 
@@ -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;
 
@@ -297,6 +304,8 @@ export class TimeSeriesExplorer extends React.Component {
       this.setState({
         loading: true,
         fullRefresh: false,
+        zoomFrom: selection.from,
+        zoomTo: selection.to,
       });
 
       getFocusData(
@@ -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,
-      });
     }
   }
 
@@ -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;
     }
 
@@ -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);
 
@@ -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);
@@ -961,6 +973,8 @@ export class TimeSeriesExplorer extends React.Component {
       tableData,
       zoomFrom,
       zoomTo,
+      zoomFromFocusLoaded,
+      zoomToFocusLoaded,
     } = this.state;
 
     const chartProps = {
@@ -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,
     };