[ML] Single Metric Viewer: Fix zoom on forecast selection. #55685

walterra · 2020-01-23T11:56:45Z

Summary

Part of #52986.

Fixes adjusting the focus chart zoom range when a forecast gets selected. The code is cleaned up so appStateHandler to set the zoom range in the url is only called in one place within contextChartSelected().

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 · 2020-01-23T11:56:48Z

Pinging @elastic/ml-ui (:ml)

peteharverson

Tested and LGTM

kibanamachine · 2020-01-23T13:17:53Z

💚 Build Succeeded

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

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

pheyos

Tested locally and it worked fine 🎉
LGTM

darnautov · 2020-01-23T15:14:54Z

x-pack/legacy/plugins/ml/public/application/routing/routes/timeseriesexplorer.tsx

@@ -213,6 +218,11 @@ export const TimeSeriesExplorerUrlStateManager: FC<TimeSeriesExplorerUrlStateMan

  const boundsMinMs = bounds?.min?.valueOf();
  const boundsMaxMs = bounds?.max?.valueOf();
+
+  const [selectedForecastIdProp, setSelectedForecastIdProp] = useState<string | undefined>(
+    appState?.mlTimeSeriesExplorer?.forecastId


question: why the init value is not selectedForecastId?

In this case I wanted to make sure it gets the initial value from the URL/appState. selectedForecastId could be undefined if isJobChange is true.

darnautov · 2020-01-23T15:29:44Z

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

+  /**
+   * Returns field names that don't have a selection yet.
+   */
+  getFieldNamesWithEmptyValues = () => {


no need in an arrow function, could be a method

The whole file has a mix of styles, I'll keep it like that since the "surrounding" functions have a similar style :)

darnautov · 2020-01-23T15:29:55Z

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

+  /**
+   * Checks if all entity control dropdowns have a selection.
+   */
+  arePartitioningFieldsProvided = () => {


no need in an arrow function, could be a method

The whole file has a mix of styles, I'll keep it like that since the "surrounding" functions have a similar style :)

darnautov · 2020-01-23T15:35:01Z

x-pack/legacy/plugins/ml/public/application/routing/routes/timeseriesexplorer.tsx

@@ -221,6 +231,9 @@ export const TimeSeriesExplorerUrlStateManager: FC<TimeSeriesExplorerUrlStateMan
      selectedJob !== undefined &&
      selectedForecastId !== undefined
    ) {
+      if (selectedForecastIdProp !== selectedForecastId) {
+        setSelectedForecastIdProp(undefined);


this state update will cause the re-render, then you call getForecastDateRange with selectedForecastId and update the state again. Is it necessary?
And why don't you update the appState instead?

It's a bit fiddly but it's the way I was able to fix it without a major rewrite of the inner workings of timeseriesexplorer.js :). The combination of selectedForecastId (source of truth in the URL) and selectedForecastIdProp allows me track if a forecast update was completed. Otherwise multiple independent useEffects would just trigger a bunch of rerenders and we cannot tell within timeseriesexplorer.js if we got everything we need for a forecast update.

The main pain point is that the inner charts still trigger a programmatic zoom update and with the current code there's no way to tell if that was triggered by a user or by another update. Fixing this would have required a much larger refactor.

darnautov · 2020-01-23T15:36:28Z

x-pack/legacy/plugins/ml/public/application/routing/routes/timeseriesexplorer.tsx

@@ -282,6 +282,11 @@ export const TimeSeriesExplorerUrlStateManager: FC<TimeSeriesExplorerUrlStateMan
    return null;
  }

+  const zoomProp: AppStateZoom | undefined =
+    typeof selectedForecastId === 'string' && selectedForecastIdProp === undefined


does zoom prop always depend on the forecast id?

This is related to the comment above. The combination of selectedForecastId and selectedForecastIdProp allows us to decide wheter to pass on zoomProp as undefined. If it's undefined, the timeseriesexplorer.js will recalculate the zoom which in turn moves it to the desired location to span half the forecast and have the existing section.

darnautov

LGTM

…5685) Fixes adjusting the focus chart zoom range when a forecast gets selected. The code is cleaned up so appStateHandler to set the zoom range in the url is only called in one place within contextChartSelected().

…55722) Fixes adjusting the focus chart zoom range when a forecast gets selected. The code is cleaned up so appStateHandler to set the zoom range in the url is only called in one place within contextChartSelected().

…55721) Fixes adjusting the focus chart zoom range when a forecast gets selected. The code is cleaned up so appStateHandler to set the zoom range in the url is only called in one place within contextChartSelected().

walterra added 2 commits January 23, 2020 12:29

[ML] Single Metric Viewer: Fix forecast zoom update.

d8f38d5

[ML] Fix restore from URL.

487235b

walterra added bug Fixes for quality problems that affect the customer experience regression :ml Feature:Anomaly Detection ML anomaly detection v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v7.6.0 labels Jan 23, 2020

walterra requested review from darnautov, alvarezmelissa87, peteharverson and jgowdyelastic January 23, 2020 11:56

walterra requested a review from a team as a code owner January 23, 2020 11:56

walterra self-assigned this Jan 23, 2020

[ML] AppStateZoom interface.

5fa079e

walterra mentioned this pull request Jan 23, 2020

[ML] Fix appState/globalState #52986

Closed

21 tasks

peteharverson approved these changes Jan 23, 2020

View reviewed changes

pheyos approved these changes Jan 23, 2020

View reviewed changes

darnautov reviewed Jan 23, 2020

View reviewed changes

darnautov approved these changes Jan 23, 2020

View reviewed changes

walterra merged commit a4cf4f4 into elastic:master Jan 23, 2020

walterra deleted the ml-fix-forecast-zoom branch January 23, 2020 16:16

walterra mentioned this pull request Jan 23, 2020

[7.x] [ML] Single Metric Viewer: Fix zoom on forecast selection. (#55685) #55721

Merged

walterra mentioned this pull request Jan 23, 2020

[7.6] [ML] Single Metric Viewer: Fix zoom on forecast selection. (#55685) #55722

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Single Metric Viewer: Fix zoom on forecast selection. #55685

[ML] Single Metric Viewer: Fix zoom on forecast selection. #55685

walterra commented Jan 23, 2020 •

edited by peteharverson

Loading

elasticmachine commented Jan 23, 2020

peteharverson left a comment

kibanamachine commented Jan 23, 2020

pheyos left a comment

darnautov Jan 23, 2020

walterra Jan 23, 2020

darnautov Jan 23, 2020 •

edited

Loading

walterra Jan 23, 2020

darnautov Jan 23, 2020 •

edited

Loading

walterra Jan 23, 2020

darnautov Jan 23, 2020

walterra Jan 23, 2020

darnautov Jan 23, 2020

walterra Jan 23, 2020

darnautov left a comment

[ML] Single Metric Viewer: Fix zoom on forecast selection. #55685

[ML] Single Metric Viewer: Fix zoom on forecast selection. #55685

Conversation

walterra commented Jan 23, 2020 • edited by peteharverson Loading

Summary

Checklist

For maintainers

elasticmachine commented Jan 23, 2020

peteharverson left a comment

Choose a reason for hiding this comment

kibanamachine commented Jan 23, 2020

💚 Build Succeeded

pheyos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darnautov Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darnautov Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darnautov left a comment

Choose a reason for hiding this comment

walterra commented Jan 23, 2020 •

edited by peteharverson

Loading

darnautov Jan 23, 2020 •

edited

Loading

darnautov Jan 23, 2020 •

edited

Loading