[ML] Fix appState/globalState #52987

walterra · 2019-12-13T12:55:36Z

Summary

Fixes #52986.
Fixes #54317.

Replaces appState/globalState with a custom hook useUrlState().

Some notes for reviewers:

Migrates some more components and utils to TypeScript.
Removed our own injectObservablesAsProps() utility, replaced with a pattern based on useObservable() hook.
Removed jobSelectService$, events are no longer passed around using it, instead useUrlState() replaces it to subscribe and update these settings.
skipRefresh in the job selector is no longer necessary because updates are no longer triggered by both date picker and job selector.
useJobSelection() replaces some utilities from explorer and relies no longer on internal mutable state.
loading the data for the Anomaly Explorer is now wrapped in a switchMap() to avoid triggering multiple updates.
The diff for the Explorer (explorer.js) component is quite large because it's no longer necessary to wrap it in injectObservablesAsProps and injectI18n, but the interals didn't change that much.
All appState related actions are gone from EXPLORER_ACTION, appStateReducer is no longer necessary. Instead, explorerAppState$ now transforms the explorer's internal state into the format we store in the URL. setAppState from useUrlState() is then used to update the URL. The custom hook useSelectedCells() is now used to manage cell selection and store in it the URL (instead of appStateReducer).
Anomaly Explorer no longer needs different behavior for initialization and switching jobs.
The <TimeSeriesExplorer /> component no longer relies on the custom refresh() method, instead React's componentDidUpdate() lifecycle method is used to react to updating props. Its internal methods have been revamped to trigger less setStates() and rerenders.
Reduces passing around the whole timefilter, instead just passes down bounds as props.
The controls for showCharts/severity/interval no longer rely on custom state management using observables, instead they make use of useUrlState() and provide custom hooks to access/set their setting via a custom hook.

Todo

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
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

elasticmachine · 2019-12-13T12:55:38Z

Pinging @elastic/ml-ui (:ml)

peteharverson

Was there also some legacy code around the severity and interval controls that needed to be removed once this work was done?

peteharverson · 2019-12-17T15:10:13Z

If I select a cell in the (view by) swimlane, and then refresh the browser, the time filter setting resets to Last 15 minutes and the view is empty:

peteharverson · 2019-12-17T15:12:18Z

Viewed farequote, returned to the Jobs List, then clicked on the Anomaly Explorer link for a different job, the Anomaly Explorer loads to show the data for the previous job:

peteharverson · 2019-12-17T15:14:28Z

If I switch jobs in the Single Metric Viewer to one with a different time range and entities, the previous settings get retained:

kibanamachine · 2019-12-27T17:07:26Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 796a06250a3bc5b42ddd3f9a8016eaaafcc94af6

History

💔 Build #16082 failed a70d4c95e42499a9db607b664ff4c4f8049ab043
💔 Build #16057 failed 8b94b47b9f6260432552d5f8770d027bb4689b97
💔 Build #15472 failed c4875a37d8a5a24d375a448b3a2471076e36588f
💔 Build #15373 failed f7ad1c7bf77fec71c4662387dfe462849f386972

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

kibanamachine · 2019-12-27T20:07:47Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: f179414a7b687fe6728271ed9a83b7ed91b600e8

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

kibanamachine · 2019-12-30T13:15:40Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 3965e0681aa8ef631879b2dfab0c61bb9450a9ea

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

walterra · 2020-01-02T09:06:44Z

@peteharverson I pushed some updates:

With further updates and better use of useObservable I was able to remove a lot of legacy code and some of our own utilities. The code for severity and interval controls no longer has custom services.
If I select a cell in the (view by) swimlane, and then refresh the browser, the time filter setting should no longer reset to last 15 minutes and the view should be populated.
Moving back between jobs list and anomaly explorer should load the correct data.

I'm still working on fixing Single Metric Viewer but Anomaly Explorer should be ready for another go at testing.

…alid job.

…xplorer.

jgowdyelastic · 2020-01-10T16:55:17Z

I've found an issue when switching jobs with multiple detectors in the single metric viewer.
Job A - 2 detectors
Job B - 1 detector

Open job A, select the second detector so the URL contains detectorIndex:1
Switch to job B using the Edit job selection flyout.

It appears it attempts to load the detector at index 1 in job B which doesn't exist. This causes the page to crash.

…s returned.

…b is selected.

walterra · 2020-01-13T08:12:06Z

@jgowdyelastic thx for spotting the issue with detectorIndex, I pushed a fix in ff12232.

walterra · 2020-01-13T08:13:25Z

@alvarezmelissa87 I pushed another fix for restoring the correct view-by field when multiple jobs are selected in 8edb24d.

…jobs are present.

peteharverson · 2020-01-13T15:28:39Z

If I view a job in the Anomaly Explorer which is not supported in the Single Metric Viewer (such as a categorization job without model plot enabled), and then switch to the Single Metric Viewer directly by the nav menu, the content area is blank, with errors in the console:

alvarezmelissa87

Tested and latest edits LGTM ⚡️

peteharverson · 2020-01-13T16:11:00Z

Probably one for a follow-up, but for a job with multiple influencer fields, if I'm viewing by one of the influencers, but then filter by one of the other influencers, the combination of the view by dropdown value and info message is confusing:

Ideally the dropdown value above should stay as 'instance'. or it should be changed to by 'region'.

walterra · 2020-01-13T16:13:21Z

@peteharverson thanks for identifying those, suggest to do those in a follow up.

jgowdyelastic

LGTM. Great job!

peteharverson

Overall LGTM! Only found a couple of minor edge cases, which can be addressed in follow-up fixes.

Replaces appState/globalState with a custom hook useUrlState().

Replaces appState/globalState with a custom hook useUrlState(). Co-authored-by: Elastic Machine <[email protected]>

kibanamachine · 2020-01-16T10:45:10Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 5cba065

History

💚 Build #19742 succeeded 5cba065
💔 Build #19543 failed 374b99d
💔 Build #19539 failed 743f963
💔 Build #19530 failed f01a4b5
💔 Build #19276 failed 83d2f51

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Replaces appState/globalState with a custom hook useUrlState().

walterra added bug Fixes for quality problems that affect the customer experience regression Feature:New Platform :ml Feature:Anomaly Detection ML anomaly detection refactoring v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.6.0 labels Dec 13, 2019

walterra self-assigned this Dec 13, 2019

walterra force-pushed the ml-fix-react-router-appstate branch from c4875a3 to 2354a4c Compare December 17, 2019 12:15

walterra marked this pull request as ready for review December 17, 2019 12:40

walterra requested a review from a team as a code owner December 17, 2019 12:40

walterra requested review from jgowdyelastic, peteharverson, darnautov and alvarezmelissa87 December 17, 2019 12:40

peteharverson reviewed Dec 17, 2019

View reviewed changes

walterra force-pushed the ml-fix-react-router-appstate branch from a70d4c9 to 796a062 Compare December 27, 2019 15:48

elastic deleted a comment from elasticmachine Dec 27, 2019

walterra force-pushed the ml-fix-react-router-appstate branch from df9770d to 8a58e37 Compare January 2, 2020 09:35

[ML] Introduces useUrlState.

17d532d

walterra added 2 commits January 10, 2020 13:57

[ML] Fix restoring invalid job id from appState and switch to first v…

8df71ad

…alid job.

[ML] Fix restoring view-by fieldname and filter from URL in Anomaly E…

83d2f51

…xplorer.

walterra added 7 commits January 11, 2020 13:39

[ML] Fix setting view-by swimlane to not loading if the same data get…

7eef24a

…s returned.

[ML] Fix jest tests.

fceccfa

Merge branch 'master' into ml-fix-react-router-appstate

f01a4b5

[ML] Fix restoring custom view-by selection when one more than one jo…

8edb24d

…b is selected.

[ML] Fix clearing appState in single metric viewer when changing jobs.

ff12232

[ML] Fix SelectSeverity tests.

743f963

[ML] Fix SelectInterval tests.

374b99d

walterra added 2 commits January 13, 2020 11:29

Merge branch 'master' into ml-fix-react-router-appstate

6939ea8

[ML] Fix pages for anomaly explorer and single metric viewer when no …

5cba065

…jobs are present.

alvarezmelissa87 approved these changes Jan 13, 2020

View reviewed changes

jgowdyelastic approved these changes Jan 13, 2020

View reviewed changes

peteharverson approved these changes Jan 13, 2020

View reviewed changes

walterra merged commit 6826ece into elastic:master Jan 13, 2020

walterra mentioned this pull request Jan 13, 2020

[7.x] [ML] Fix appState/globalState (#52987) #54602

Merged

walterra added a commit to walterra/kibana that referenced this pull request Jan 13, 2020

[ML] Fix appState/globalState (elastic#52987)

409faeb

Replaces appState/globalState with a custom hook useUrlState().

walterra deleted the ml-fix-react-router-appstate branch January 13, 2020 17:18

jgowdyelastic pushed a commit that referenced this pull request Jan 14, 2020

[ML] Fix appState/globalState (#52987) (#54602)

428fba3

Replaces appState/globalState with a custom hook useUrlState(). Co-authored-by: Elastic Machine <[email protected]>

darnautov mentioned this pull request Jan 15, 2020

[ML] Index Data Visualizer url state error #54885

Closed

peteharverson mentioned this pull request Jan 15, 2020

[APM] ML integration: "View job" link is broken, doesn't show job data in single metric viewer in ML #54903

Closed

jkelastic pushed a commit to jkelastic/kibana that referenced this pull request Jan 17, 2020

[ML] Fix appState/globalState (elastic#52987)

b994184

Replaces appState/globalState with a custom hook useUrlState().

walterra mentioned this pull request Jan 17, 2020

[ML] Anomaly Explorer: Correctly pass on severity value to anomaly explorer charts. #55207

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Fix appState/globalState #52987

[ML] Fix appState/globalState #52987

walterra commented Dec 13, 2019 •

edited

Loading

elasticmachine commented Dec 13, 2019

peteharverson left a comment

peteharverson commented Dec 17, 2019

peteharverson commented Dec 17, 2019

peteharverson commented Dec 17, 2019

kibanamachine commented Dec 27, 2019

kibanamachine commented Dec 27, 2019

kibanamachine commented Dec 30, 2019

walterra commented Jan 2, 2020

jgowdyelastic commented Jan 10, 2020

walterra commented Jan 13, 2020

walterra commented Jan 13, 2020

peteharverson commented Jan 13, 2020 •

edited

Loading

alvarezmelissa87 left a comment

peteharverson commented Jan 13, 2020

walterra commented Jan 13, 2020

jgowdyelastic left a comment

peteharverson left a comment

kibanamachine commented Jan 16, 2020

[ML] Fix appState/globalState #52987

[ML] Fix appState/globalState #52987

Conversation

walterra commented Dec 13, 2019 • edited Loading

Summary

Todo

Checklist

For maintainers

elasticmachine commented Dec 13, 2019

peteharverson left a comment

Choose a reason for hiding this comment

peteharverson commented Dec 17, 2019

peteharverson commented Dec 17, 2019

peteharverson commented Dec 17, 2019

kibanamachine commented Dec 27, 2019

💔 Build Failed

History

kibanamachine commented Dec 27, 2019

💔 Build Failed

kibanamachine commented Dec 30, 2019

💔 Build Failed

walterra commented Jan 2, 2020

jgowdyelastic commented Jan 10, 2020

walterra commented Jan 13, 2020

walterra commented Jan 13, 2020

peteharverson commented Jan 13, 2020 • edited Loading

alvarezmelissa87 left a comment

Choose a reason for hiding this comment

peteharverson commented Jan 13, 2020

walterra commented Jan 13, 2020

jgowdyelastic left a comment

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

kibanamachine commented Jan 16, 2020

💚 Build Succeeded

History

walterra commented Dec 13, 2019 •

edited

Loading

peteharverson commented Jan 13, 2020 •

edited

Loading