[ML] Fix Limit Dropdown, simplify state management of Anomaly Explorer. by walterra · Pull Request #23388 · elastic/kibana

walterra · 2018-09-21T07:17:12Z

This fixes the limit dropdown behavior. The fix for that is actually just the $scope.appState.fetch(); statements in explorer_controller.js, they avoid to run the information stored in appState across modules out of sync.

Additionally, the aim of this PR is to simplify the state management of Anomaly Explorer in the context of selecting cells in the swimlanes and updating the influencers list, charts and table accordingly.

Previously, esp. restoring a selection from URL/AppState involved a lot of complexity, something like:

Initial Load -> Load Overall Swimlane -> Load View By Swimlane -> Inside the swimlane code, check for a selection in AppState, if yes, use that data to trigger a swimlane selection click programmatically -> trigger the click listener again in Anomaly Explorer controller -> update $scope.cellData and trigger updating swimlanes/influencers/charts/table again

This has now been reduced to:

Check URL/AppState on initial load and restore $scope.cellData -> use updateExplorer() to update swimlanes/charts/influencers/table considering the selection

Here's some more detail about what's done in this PR:

The swimlanes state handling has been improved: click events no longer manipulate the DOM, only renderSwimlane() does that now. That means: click events only trigger listeners and send data (data out, no DOM manipulation); props include all the information necessary to render the swimlanes including selections (data in, update the DOM)
The swimlanes wrapper directives no longer require to pass through angularjs scope from the Anomaly Explorer controller. The props for the swimlanes are now prepared in the controller and handed on via the swimlaneDataChange listener.
Some issues with race conditions have been fixed involved with restoring a selection from URL/AppState, for example the Anomaly Explorer controller now waits for isChartsContainerInitialized to make sure the Charts Container directive is ready before passing on data to the charts. Previously child directives could miss updates if their link wasn't called yet which included setting up event listeners.
updateExplorer() was introduced to be able to trigger the same kind of update when we restore or do a selection on load or via click to avoid the event circling described in the beginning. That way the complexity/redundancy of swimlaneCellClickListener() could be reduced, it now just saves a new selection in appState and $scope.cellData, then triggers updateExporer().
~~The limit dropdown has been migrated to use React, this is more or less a copy of the interval dropdown.~~ Reverted that conversion - it was originally done while trying to fix the bug related to the dropdown but it turned out to be not related. With that conversion the dropdowns for "view by" and "limit" would end up having different appearance so that should be done in a separate PR.

Everything of the above is a preparation to move explorer_controller at one point out of the anguarljs universe and make it an independent service that just manages state, similar to how it was done for explorerChartsContainerService.

Part of #22642 and #18374.

elasticmachine · 2018-09-21T07:17:14Z

Pinging @elastic/ml-ui

elasticmachine · 2018-09-21T08:55:48Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…tener in explorer_controller.

…ent handling.

elasticmachine · 2018-09-22T11:21:29Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

walterra · 2018-09-24T07:39:29Z

retest

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

peteharverson · 2018-09-24T08:50:29Z

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

      const selectedJobIds = $scope.getSelectedJobIds();
      const limit = mlSelectLimitService.state.get('limit');
-      const swimlaneLimit = (limit === undefined) ? 10 : limit.val;
+      const swimlaneLimit = (limit === undefined) ? 10 : limit;


Can the default limit of 10 be moved to a constant at the top of the file?

peteharverson · 2018-09-24T08:50:37Z

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

    const selectedJobIds = $scope.getSelectedJobIds();
    const limit = mlSelectLimitService.state.get('limit');
-    const swimlaneLimit = (limit === undefined) ? 10 : limit.val;
+    const swimlaneLimit = (limit === undefined) ? 10 : limit;


Can the default limit of 10 be moved to a constant at the top of the file?

peteharverson · 2018-09-24T09:10:35Z

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

  };
 }

+const SWIMLANE_TYPE = {


Would explorer_constants.js be a better place for these?

peteharverson · 2018-09-24T09:15:00Z

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

+  return limit;
+}
+
+class SelectLimit extends Component {


This needs a corresponding Jest test. Can the current Angular based tests inside __tests__/select_limit.js be removed if a Jest test is added?

elasticmachine · 2018-09-24T09:17:49Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

This reverts commit e00fe01.

jgowdyelastic

added a few small comments but otherwise LGTM

jgowdyelastic · 2018-09-24T09:57:23Z

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

+        ) {
+          let selectionExists = false;
+          $scope.cellData.lanes.forEach((lane) => {
+            if ($scope.viewBySwimlaneData.laneLabels.indexOf(lane) > -1) {


looks like this could be done with some to save those few extra loops :)

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

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

jgowdyelastic · 2018-09-24T10:20:38Z

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

+      // which is where d3 supplies a reference to the corresponding DOM element.
+      return function (lane) {
+        const bucketScore = getBucketScore(lane, time);
+        if (bucketScore === 0) { return; }


nit, rather than breaking out of this function early, you could invert the check:

if (bucketScore !== 0) { cellMouseover(this, lane, bucketScore, i, time); }

jgowdyelastic · 2018-09-24T10:25:36Z

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


    expect(wrapper.html()).toBe(
-      `<div class="ml-swimlanes"><div class="time-tick-labels"><svg height="25">` +
+      `<div class="ml-swimlanes"><div class="time-tick-labels"><svg width="800" height="25">` +


nit, chartWidth should be used for the width

jgowdyelastic · 2018-09-24T10:26:25Z

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

-      appState={mocks.appState}
-      lanes={[]}
-      mlExplorerDashboardService={mocks.mlExplorerDashboardService}
+      chartWidth={800}


nit, 800 should come from a constant variable

elasticmachine · 2018-09-24T11:11:39Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…the check.

…jest test.

jgowdyelastic · 2018-09-24T13:12:51Z

latest changes LGTM

peteharverson

Latest edits LGTM

elasticmachine · 2018-09-24T14:31:42Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…r. (elastic#23388) - This fixes the limit dropdown behavior. The fix for that is actually just the $scope.appState.fetch(); statements in explorer_controller.js, they avoid to run the information stored in appState across modules out of sync. - Additionally, the aim of this PR is to simplify the state management of Anomaly Explorer in the context of selecting cells in the swimlanes and updating the influencers list, charts and table accordingly.

…r. (#23388) (#23440) - This fixes the limit dropdown behavior. The fix for that is actually just the $scope.appState.fetch(); statements in explorer_controller.js, they avoid to run the information stored in appState across modules out of sync. - Additionally, the aim of this PR is to simplify the state management of Anomaly Explorer in the context of selecting cells in the swimlanes and updating the influencers list, charts and table accordingly.

alvarezmelissa87

I know I'm a bit late on this one but LGTM! 😸
Thanks for the great context in the description, as well!

elasticmachine · 2019-01-02T14:36:56Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

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

walterra self-assigned this Sep 21, 2018

walterra changed the title ~~[WIP] [ML] Simplify state management of Anomaly Explorer~~ [WIP] [ML] Fix Limit Dropdown, simplify state management of Anomaly Explorer. Sep 22, 2018

walterra added the WIP Work in progress label Sep 22, 2018

walterra added 16 commits September 22, 2018 10:56

[ML] Swimlanes don't rely on directive scope, gets now passed via lis…

ac284f0

…tener in explorer_controller.

[ML] Reduce swimlane state and event handling complexity.

51a9852

[ML] Trying to fix anomaly explorer issues.

9534c0a

[ML] Fixes show charts button.

d1e245f

[ML] Fixes view by jobs swimlane selection.

f17a764

[ML] Fixes restoring selection from URL/AppState.

0993274

[ML] Only trigger a swimlane update if the props are valid.

db4b60b

[ML] Migrates the limit dropdown to use React.

e00fe01

[ML] Fixes comment.

001acf9

[ML] Reuse updateExplorer() for the cell click listener.

27ece03

[ML] Revert if/else.

944285c

[ML] Simplifies the props required for the swimlanes.

69761a9

[ML] Don't pass bucket score around any longer.

96e3b65

[ML] Fix selection when view by swimlanes are showing jobs.

0a6b558

[ML] Remove unneccessary argument.

284705e

[ML] Make sure to fetch AppState before updating and saving it.

b7342f0

walterra force-pushed the ml-swimlane-fix-selection branch from 12efbb7 to b7342f0 Compare September 22, 2018 09:30

[ML] Updated PropTypes and jest tests to reflect new structure and ev…

c71fcfc

…ent handling.

walterra changed the title ~~[WIP] [ML] Fix Limit Dropdown, simplify state management of Anomaly Explorer.~~ [ML] Fix Limit Dropdown, simplify state management of Anomaly Explorer. Sep 22, 2018

walterra removed the WIP Work in progress label Sep 22, 2018

walterra requested review from alvarezmelissa87, jgowdyelastic and peteharverson September 24, 2018 07:38

peteharverson reviewed Sep 24, 2018

View reviewed changes

walterra added 2 commits September 24, 2018 11:24

Revert "[ML] Migrates the limit dropdown to use React."

193e710

This reverts commit e00fe01.

[ML] Use a constant for the limit's default value.

3e0b88a

walterra added the bug Fixes for quality problems that affect the customer experience label Sep 24, 2018

jgowdyelastic approved these changes Sep 24, 2018

View reviewed changes

walterra added 3 commits September 24, 2018 14:24

[ML] Use .some() instead of .forEach() to avoid unnecesary loops for …

5d883e3

…the check.

[ML] Move the chartWidth of 800 to constant 'mockChartWidth'.

dab438e

[ML] Review feedback: Changed bucketScore check and tweaked swimlane …

144b2f2

…jest test.

peteharverson approved these changes Sep 24, 2018

View reviewed changes

walterra merged commit c5d475f into elastic:master Sep 24, 2018

walterra mentioned this pull request Sep 24, 2018

[6.x] [ML] Fix Limit Dropdown, simplify state management of Anomaly Explorer. (#23388) #23440

Merged

alvarezmelissa87 reviewed Sep 26, 2018

View reviewed changes

alvarezmelissa87 mentioned this pull request Oct 2, 2018

[ML] Ensure charts loaded in Anomaly Explorer match swimlane selection #23690

Merged

walterra mentioned this pull request Jan 2, 2019

[ML] Make Severity threshold and Interval settings sticky #18538

Closed

walterra deleted the ml-swimlane-fix-selection branch January 2, 2019 14:35

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

Conversation

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

Uh oh!

Uh oh!

elasticmachine commented Sep 21, 2018

Uh oh!

elasticmachine commented Sep 21, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Sep 22, 2018

💔 Build Failed

Uh oh!

walterra commented Sep 24, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 24, 2018

💔 Build Failed

Uh oh!

jgowdyelastic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 24, 2018

💚 Build Succeeded

Uh oh!

jgowdyelastic commented Sep 24, 2018

Uh oh!

peteharverson left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 24, 2018

💚 Build Succeeded

Uh oh!

alvarezmelissa87 left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jan 2, 2019

💔 Build Failed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

walterra commented Sep 21, 2018 •

edited

Loading