[ML] Fixes Anomaly Explorer Swimlane race condition, adds tests. by walterra · Pull Request #22814 · elastic/kibana

walterra · 2018-09-07T14:13:37Z

This PR addresses parts of #22642.

It gets rid of the use of var that = this;.
dragSelect's action strings are moved to a constants file.
Adds jest tests for the ExplorerSwimlane component.

Note for reviewers: The diff for explorer_swimlane.js is rather big - it's because I moved at lot of methods which were defined in componentDidMount() like this.fnName = ... to be methods on the class itself like fnName() { ... }, the methods themselves didn't change though.

This also fixes the following bugs:

The way we subscribe listeners to the events of the dragSelect library could result in the same event being triggered multiple times. This in turn could cause race conditions when on each event new data gets fetched but in between angular's scope gets updated and could end up in a non-intended way. The result of this were view-by swimlanes not updating correctly or anomaly charts showing non-related charts. This PR fixes it by filtering out consecutive swimlane click events.
When the angular based chart container wrapper directive gets destroyed/re-esetup when using the job pick, it missed unmounting the react component, it didn't trigger componentWillUnmount()and didn't unsubscribe from dragSelectListener.

elasticmachine · 2018-09-07T14:13:38Z

Pinging @elastic/ml-ui

jgowdyelastic

LGTM

jgowdyelastic · 2018-09-07T14:18:22Z

x-pack/plugins/ml/public/explorer/explorer_controller.js

nit, keys shouldn't be quoted

jgowdyelastic · 2018-09-07T14:19:45Z

x-pack/plugins/ml/public/explorer/explorer_controller.js

nit, this could be broken over a few lines to make it more readable

peteharverson

LGTM

elasticmachine · 2018-09-07T15:46:44Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

walterra · 2018-09-07T15:48:10Z

retest

elasticmachine · 2018-09-07T16:38:45Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-09-07T17:19:37Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-09-08T10:54:48Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…es while they are loading.

elasticmachine · 2018-09-09T10:15:40Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

walterra · 2018-09-10T06:12:15Z

retest

elasticmachine · 2018-09-10T07:46:39Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-09-10T11:54:50Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

jgowdyelastic · 2018-09-10T16:12:12Z

x-pack/plugins/ml/public/explorer/explorer_controller.js

+  // and avoid race conditions ending up with the wrong charts.
+  let lastRequestTime = null;
  function loadDataForCharts(jobIds, influencers, earliestMs, latestMs) {
+    const requestTime = new Date().getTime();


i think lastRequestTime could just be a counter that is incremented on each call

peteharverson

Tested your latest changes and all selection events in the swimlane are behaving correctly for me (tested on Chrome and Edge).

elasticmachine · 2018-09-10T17:17:25Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…ues.

elasticmachine · 2018-09-10T19:34:29Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

jgowdyelastic · 2018-09-11T05:02:17Z

x-pack/plugins/ml/public/explorer/explorer_controller.js

+    }
+
+    const newRequestCount = requestCount + 1;
+    requestCount = newRequestCount;


nit, these two lines could be
const newRequestCount = ++requestCount;

…ame.

elasticmachine · 2018-09-11T10:19:22Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…stic#22814) This PR addresses parts of elastic#22642: - It gets rid of the use of var that = this;. - dragSelect's action strings are moved to a constants file. - Adds jest tests for the ExplorerSwimlane component. This also fixes the following bugs: - The way we subscribe listeners to the events of the dragSelect library could result in the same event being triggered multiple times. This in turn could cause race conditions when on each event new data gets fetched but in between angular's scope gets updated and could end up in a non-intended way. The result of this were view-by swimlanes not updating correctly or anomaly charts showing non-related charts. This PR fixes it by filtering out consecutive swimlane click events. - When the angular based chart container wrapper directive gets destroyed/re-esetup when using the job pick, it missed unmounting the react component, it didn't trigger componentWillUnmount()and didn't unsubscribe from dragSelectListener.

) (#22923) This PR addresses parts of #22642: - It gets rid of the use of var that = this;. - dragSelect's action strings are moved to a constants file. - Adds jest tests for the ExplorerSwimlane component. This also fixes the following bugs: - The way we subscribe listeners to the events of the dragSelect library could result in the same event being triggered multiple times. This in turn could cause race conditions when on each event new data gets fetched but in between angular's scope gets updated and could end up in a non-intended way. The result of this were view-by swimlanes not updating correctly or anomaly charts showing non-related charts. This PR fixes it by filtering out consecutive swimlane click events. - When the angular based chart container wrapper directive gets destroyed/re-esetup when using the job pick, it missed unmounting the react component, it didn't trigger componentWillUnmount()and didn't unsubscribe from dragSelectListener.

- This fixes a regression introduced in #22814. The influencer list wouldn't update if no cell in the swimlanes was selected. - Renames getTopInfluencers to loadTopInfluencers to be in line with the other functions loadDataForCharts and loadAnomaliesTableData - Changes the order of arguments for loadDataForCharts so they are the same like in loadTopInfluencers.

- This fixes a regression introduced in elastic#22814. The influencer list wouldn't update if no cell in the swimlanes was selected. - Renames getTopInfluencers to loadTopInfluencers to be in line with the other functions loadDataForCharts and loadAnomaliesTableData - Changes the order of arguments for loadDataForCharts so they are the same like in loadTopInfluencers.

…2989) - This fixes a regression introduced in #22814. The influencer list wouldn't update if no cell in the swimlanes was selected. - Renames getTopInfluencers to loadTopInfluencers to be in line with the other functions loadDataForCharts and loadAnomaliesTableData - Changes the order of arguments for loadDataForCharts so they are the same like in loadTopInfluencers.

- Fixes a regression introduced in #22814. Because of the stricter checking for scope/props updates, resizing the browser window would miss updating the Anomaly Explorer Charts widths. This fixes it by adding a check to trigger anomalyDataChange in redrawOnResize(). - Additionally, if only one chart is up for display, this update makes sure a single chart always spans across the full available width.

- Fixes a regression introduced in elastic#22814. Because of the stricter checking for scope/props updates, resizing the browser window would miss updating the Anomaly Explorer Charts widths. This fixes it by adding a check to trigger anomalyDataChange in redrawOnResize(). - Additionally, if only one chart is up for display, this update makes sure a single chart always spans across the full available width.

- Fixes a regression introduced in #22814. Because of the stricter checking for scope/props updates, resizing the browser window would miss updating the Anomaly Explorer Charts widths. This fixes it by adding a check to trigger anomalyDataChange in redrawOnResize(). - Additionally, if only one chart is up for display, this update makes sure a single chart always spans across the full available width.

walterra added v7.0.0 :ml Feature:ml-results legacy - do not use v6.5.0 labels Sep 7, 2018

walterra self-assigned this Sep 7, 2018

walterra requested review from jgowdyelastic and peteharverson September 7, 2018 14:13

jgowdyelastic approved these changes Sep 7, 2018

View reviewed changes

peteharverson approved these changes Sep 7, 2018

View reviewed changes

walterra force-pushed the ml-explorer-swimlane-react-2 branch from 1963c4e to 5d2addb Compare September 8, 2018 09:21

walterra added 9 commits September 9, 2018 10:40

[ML] First Anomaly Explorer Swimlane jest tests.

e1f3e7e

[ML] Reduced size of data mock file.

ce530a0

[ML] Refactor ExplorerSwimlane to avoid that=this.

04d1fae

[ML] Use constants for drag select actions.

3e4eb71

[ML] Added expects to check mocked functions calls.

b67bae8

[ML] Updated overall swimlane test.

9dd47c0

[ML] Fixes a possible race condition with swimlaneCellClickListener.

1226842

[ML] Code style tweaks.

4bc2cbe

[ML] Adds skipCellClicks to stop listening to mouse clicks on swimlan…

7ae6643

…es while they are loading.

walterra force-pushed the ml-explorer-swimlane-react-2 branch from 5d2addb to 7ae6643 Compare September 9, 2018 08:41

[ML] Remove console.log

d1bff92

[ML] Fixes race conditions.

b6559ce

jgowdyelastic reviewed Sep 10, 2018

View reviewed changes

peteharverson reviewed Sep 10, 2018

View reviewed changes

[ML] Fixes even more race conditions and proper state propagation iss…

eb45c3e

…ues.

jgowdyelastic approved these changes Sep 11, 2018

View reviewed changes

walterra added 3 commits September 11, 2018 09:53

[ML] Tweak counter code. Re-enable console.log.

a97b89a

[ML] Disabling test to inspect CI fails.

2cf68c8

[ML] Set a default timezone for tests so snapshot render always the s…

f5256d3

…ame.

walterra merged commit 7de9f69 into elastic:master Sep 11, 2018

walterra deleted the ml-explorer-swimlane-react-2 branch September 11, 2018 10:39

walterra mentioned this pull request Sep 11, 2018

[6.x] [ML] Fixes Anomaly Explorer Swimlane race condition, adds tests. (#22814) #22923

Merged

This was referenced Sep 12, 2018

[ML] Fixes loading the influencers for Anomaly Explorer. #22963

Merged

[ML] Fix reloading anomaly charts on resize. #22967

Merged

lcawl added the bug Fixes for quality problems that affect the customer experience label Oct 29, 2018

sophiec20 added the Feature:Anomaly Detection ML anomaly detection label Jun 19, 2019

Comments

Conversation

walterra commented Sep 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Sep 7, 2018

Uh oh!

jgowdyelastic left a comment

Choose a reason for hiding this comment

Uh oh!

jgowdyelastic Sep 7, 2018

Choose a reason for hiding this comment

Uh oh!

jgowdyelastic Sep 7, 2018

Choose a reason for hiding this comment

Uh oh!

peteharverson left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 7, 2018

💔 Build Failed

Uh oh!

walterra commented Sep 7, 2018

Uh oh!

elasticmachine commented Sep 7, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Sep 7, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Sep 8, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Sep 9, 2018

💔 Build Failed

Uh oh!

walterra commented Sep 10, 2018

Uh oh!

elasticmachine commented Sep 10, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Sep 10, 2018

💔 Build Failed

Uh oh!

jgowdyelastic Sep 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peteharverson left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 10, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Sep 10, 2018

💔 Build Failed

Uh oh!

jgowdyelastic Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 11, 2018

💚 Build Succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

walterra commented Sep 7, 2018 •

edited

Loading

jgowdyelastic Sep 10, 2018 •

edited

Loading

jgowdyelastic Sep 11, 2018 •

edited

Loading