[ML] explorer controller refactor #28750

walterra · 2019-01-15T12:20:53Z

Summary

Refactors the application logic of Anomaly Explorer to reduce relying on angularjs. The updated event handling looks like this:

explorer_controller.js has been reduced to a bare wrapper. All that's left is $scope that's still necessary for now (e.g. jobs because of the job selector) and related events.
explorer_utils.js is a collection of functions previously housed in inline in explorer_controller.js. The function have been refactored to be more pure so they don't rely on outer state so they work stand-alone. This new structure will allow us to write unit tests for these functions in a follow up.
explorer_react_wrapper_directive.js no longer needs to pass through that much $scope from explorer_controller.js, it's now really just a shallow wrapper that passed through some props without applying further transformation.
explorer.js replaced most of explorer.html in a previous PR. Now it also includes previous methods from explorer_controller.js which managed angularjs $scope, but these methods have been refactored to manage React state.

Further updates 22.01.2019:

Refactored explorer.js state handling to be much more cleaned up and uni-directional. Data fetching and setState() is no longer in all kinds of different places, instead depending on actions (e.g. restoring a selection on reload, switching jobs, clicking on a cell) a stateUpdate update is prepared and passed on to updateExplorer(). Depending on stateUpdate (e.g. active Selection or not) updateExplorer() fetches necessary data and triggers setState() where appropriate. This results in much lesser render() calls and solves another race condition.
The above results in less spread out state related method calls, e.g. loadOverallData() is now only used 1 instead of 3 times throughout the code base, loadViewBySwimlaneData() is used 2 times instead of 6 times.
Most methods like loadOverallData() or loadViewBySwimlaneData() now feature a simple caching/memoizing capability. This is useful for example when the view is just resized, then the existing data doesn't need to be fetched again from the server. Instead of more complex state handling which tries to find out if for example loadOverallData() would need to be called again, using memoization the method can be just called again and acts as a state getter if the related method arguments didn't change.
All of the above was done to fix various issues with UI elements and the Anomaly Explorer not properly updating because the previous code was still more fragile than expected.

Review Checklist

Fix issue with EuiSelect value ending up as null. f2e3edf
Fix updating swimlanes on resize. 566b64d
Fix restoring swimlane selection from AppState. 031e176
Fix swimlane cell widths. fed6b94
Fix view by swimlanes showing only one job if two selected. fed6b94
Fix empty swimlanes after switching jobs. 0449a86

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
~~Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~Documentation was added for features that require explanation or tutorials~~
~~ Unit or functional tests were updated or added to match the most common scenarios~~ (follow up)
~~This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
~~This includes a feature addition or change that requires a release note and was labeled appropriately~~

Part of #18553 and #18374.

elasticmachine · 2019-01-15T12:20:54Z

Pinging @elastic/ml-ui

elasticmachine · 2019-01-15T12:41:51Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-01-15T13:05:02Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…oller-refactor

jgowdyelastic

LGTM

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

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

elasticmachine · 2019-01-15T13:35:11Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

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

…er React component.

elasticmachine · 2019-01-16T09:32:21Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…oller-refactor

elasticmachine · 2019-01-22T16:25:03Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-01-22T20:49:27Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

peteharverson

Latest changes LGTM!

…howing an empty view.

elasticmachine · 2019-01-23T14:47:05Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

jgowdyelastic

LGTM, added a few nitpicks, feel free to ignore

jgowdyelastic · 2019-01-23T14:47:31Z

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

+          stateUpdate.swimlaneViewByFieldName = VIEW_BY_JOB_LABEL;
+          // enforce a state update for swimlaneViewByFieldName
+          this.setState({ swimlaneViewByFieldName: VIEW_BY_JOB_LABEL }, () => {
+            this.updateExplorer(stateUpdate, true);


inconsistency nitpick - you're using stateUpdate which has been updated with Object.assign here, but a few lines down are creating the object on the fly using the spread operator.
Looks to me like either could be used, it would be good to be consistent. I'm pretty sure kibana's style guide used to have a rule about not using Object.assign but i can't find it anymore.

jgowdyelastic · 2019-01-23T14:50:14Z

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

+      const { dateFormatTz } = this.props;
+      const { selectedCells, swimlaneViewByFieldName, selectedJobs } = this.state;
+      this.setState({
+        tableData: await loadAnomaliesTableData(


nit, IMO this would read better if you created a tableData variable just above the setState call.
Personal preferences and all that, so ignore if you disagree

jgowdyelastic · 2019-01-23T14:52:25Z

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

+    state = getExplorerDefaultState();
+
+    // helper to avoid calling `setState()` in the listener for chart updates.
+    _isMounted = false;


the only variable to start with an underscore, is this to designate its insignificance?

jgowdyelastic · 2019-01-23T14:55:00Z

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

+      this.dragSelect.setSelectables(document.getElementsByClassName('sl-cell'));
+    };
+
+    componentDidMount() {


nit, i would normally expect to see the componentDidMount function towards the top of the class

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

jgowdyelastic · 2019-01-23T15:15:58Z

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

+
+        const currentSwimlaneType = _.get(this.state, 'selectedCells.type');
+        const currentShowTopFieldValues = _.get(this.state, 'selectedCells.showTopFieldValues', false);
+        const newSwimlaneType = _.get(swimlaneSelectedCells, 'type');


nit, this could just be swimlaneSelectedCells.type

elasticmachine · 2019-01-23T15:28:18Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

alvarezmelissa87

LGTM ⚡️

Refactores the application logic of Anomaly Explorer to reduce relying on angularjs.

elasticmachine · 2019-01-24T10:53:46Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

This was missed during the recent migrations to React of Anomaly Explorer in elastic#28750. It removes the now unnecessary `$scope.indexPatterns`.

This was missed during the recent migrations to React of Anomaly Explorer in #28750. It removes the now unnecessary `$scope.indexPatterns`.

This was missed during the recent migrations to React of Anomaly Explorer in elastic#28750. It removes the now unnecessary `$scope.indexPatterns`.

This was missed during the recent migrations to React of Anomaly Explorer in #28750. It removes the now unnecessary `$scope.indexPatterns`.

walterra added 3 commits January 14, 2019 09:14

[ML] Moving some scope related methods to explorer_utils.js

73c7781

[ML] Refactored explorer_controller to reduce use of scope.

597318a

[ML] Move outer explorer_controller.js props to explorer.js state.

25f5d6e

walterra added non-issue Indicates to automation that a pull request should not appear in the release notes v7.0.0 :ml refactoring Feature:ml-results legacy - do not use v6.7.0 labels Jan 15, 2019

walterra self-assigned this Jan 15, 2019

walterra requested review from alvarezmelissa87, peteharverson and jgowdyelastic January 15, 2019 12:20

[ML] Fixes explorer controller mocha test.

db91b13

Merge remote-tracking branch 'upstream/master' into ml-explorer-contr…

58d69b9

…oller-refactor

jgowdyelastic approved these changes Jan 15, 2019

View reviewed changes

peteharverson reviewed Jan 15, 2019

View reviewed changes

alvarezmelissa87 reviewed Jan 15, 2019

View reviewed changes

x-pack/plugins/ml/public/explorer/explorer.js Outdated Show resolved Hide resolved

walterra changed the title ~~Ml explorer controller refactor~~ [ML] explorer controller refactor Jan 15, 2019

walterra added 3 commits January 15, 2019 18:09

[ML] Fixes passing on selectedJobs.

65b1835

[ML] State handling optimized. AppState no longer necessary in Explor…

51fe618

…er React component.

[ML] Fix issue with EuiSelect value ending up as null.

f2e3edf

walterra added 3 commits January 16, 2019 11:15

Merge remote-tracking branch 'upstream/master' into ml-explorer-contr…

45f3a70

…oller-refactor

[ML] Fix updating swimlanes on resize.

566b64d

[ML] Updates and fixes ExplorerSwimlane props and tests.

3afd8d8

[ML] Move loading indicator check in a better place.

7a735a5

walterra added 2 commits January 22, 2019 20:55

[ML] Fixes swimlane selection issues.

0449a86

[ML] Moved pure functions to explorer_utils.

49b2fbd

peteharverson approved these changes Jan 23, 2019

View reviewed changes

[ML] Fixes a race condition where the Anomaly Explorer could end up s…

7db3323

…howing an empty view.

[ML] Fixes explorer_controller test to reflect code changes.

94cd009

jgowdyelastic approved these changes Jan 23, 2019

View reviewed changes

alvarezmelissa87 approved these changes Jan 23, 2019

View reviewed changes

walterra mentioned this pull request Jan 24, 2019

[ML] Anomaly Explorer Enhancements #18553

Open

42 tasks

walterra merged commit 735cc82 into elastic:master Jan 24, 2019

walterra deleted the ml-explorer-controller-refactor branch January 24, 2019 08:08

walterra mentioned this pull request Jan 24, 2019

[6.x] [ML] explorer controller refactor (#28750) #29244

Merged

walterra added a commit to walterra/kibana that referenced this pull request Jan 24, 2019

[ML] explorer controller refactor (elastic#28750)

0bcba53

Refactores the application logic of Anomaly Explorer to reduce relying on angularjs.

walterra added a commit that referenced this pull request Jan 24, 2019

[ML] explorer controller refactor (#28750) (#29244)

41f5fbe

Refactores the application logic of Anomaly Explorer to reduce relying on angularjs.

walterra mentioned this pull request Jan 30, 2019

[ML] Fixes caching/memoizing Anomaly Explorer data calls #29579

Merged

1 task

walterra mentioned this pull request Feb 8, 2019

[ML] Remove unneccessary indexPatterns call causing Anomaly Explorer to fail #30494

Merged

walterra added a commit that referenced this pull request Feb 8, 2019

[ML] Removes deprecated $scope.indexPatterns. (#30494)

3e64808

This was missed during the recent migrations to React of Anomaly Explorer in #28750. It removes the now unnecessary `$scope.indexPatterns`.

peteharverson pushed a commit that referenced this pull request Feb 12, 2019

[ML] Removes deprecated $scope.indexPatterns. (#30494) (#30518)

266a0c7

This was missed during the recent migrations to React of Anomaly Explorer in #28750. It removes the now unnecessary `$scope.indexPatterns`.

peteharverson pushed a commit that referenced this pull request Feb 13, 2019

[ML] Removes deprecated $scope.indexPatterns. (#30494) (#30519)

f98ba0d

This was missed during the recent migrations to React of Anomaly Explorer in #28750. It removes the now unnecessary `$scope.indexPatterns`.

alvarezmelissa87 mentioned this pull request Apr 17, 2019

[ML] Anomaly Explorer - ensure viewByFieldName is retained on explorer initialize #35260

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] explorer controller refactor #28750

[ML] explorer controller refactor #28750

walterra commented Jan 15, 2019 •

edited

Loading

elasticmachine commented Jan 15, 2019

elasticmachine commented Jan 15, 2019

elasticmachine commented Jan 15, 2019

jgowdyelastic left a comment

elasticmachine commented Jan 15, 2019

elasticmachine commented Jan 16, 2019

elasticmachine commented Jan 22, 2019

elasticmachine commented Jan 22, 2019

peteharverson left a comment

elasticmachine commented Jan 23, 2019

jgowdyelastic left a comment

jgowdyelastic Jan 23, 2019

jgowdyelastic Jan 23, 2019

jgowdyelastic Jan 23, 2019

jgowdyelastic Jan 23, 2019

jgowdyelastic Jan 23, 2019

elasticmachine commented Jan 23, 2019

alvarezmelissa87 left a comment

elasticmachine commented Jan 24, 2019

[ML] explorer controller refactor #28750

[ML] explorer controller refactor #28750

Conversation

walterra commented Jan 15, 2019 • edited Loading

Summary

Review Checklist

Checklist

For maintainers

elasticmachine commented Jan 15, 2019

elasticmachine commented Jan 15, 2019

💔 Build Failed

elasticmachine commented Jan 15, 2019

💔 Build Failed

jgowdyelastic left a comment

Choose a reason for hiding this comment

elasticmachine commented Jan 15, 2019

💔 Build Failed

elasticmachine commented Jan 16, 2019

💔 Build Failed

elasticmachine commented Jan 22, 2019

💚 Build Succeeded

elasticmachine commented Jan 22, 2019

💚 Build Succeeded

peteharverson left a comment

Choose a reason for hiding this comment

elasticmachine commented Jan 23, 2019

💔 Build Failed

jgowdyelastic left a comment

Choose a reason for hiding this comment

jgowdyelastic Jan 23, 2019

Choose a reason for hiding this comment

jgowdyelastic Jan 23, 2019

Choose a reason for hiding this comment

jgowdyelastic Jan 23, 2019

Choose a reason for hiding this comment

jgowdyelastic Jan 23, 2019

Choose a reason for hiding this comment

jgowdyelastic Jan 23, 2019

Choose a reason for hiding this comment

elasticmachine commented Jan 23, 2019

💚 Build Succeeded

alvarezmelissa87 left a comment

Choose a reason for hiding this comment

elasticmachine commented Jan 24, 2019

💚 Build Succeeded

walterra commented Jan 15, 2019 •

edited

Loading