adding watch visualization #35404

bmcconaghy · 2019-04-22T15:06:48Z

This PR adds a visualization when the threshold watch conditions are valid.

A couple of things need to get figured out:

The size of the visualization is currently hardcoded, should probably be dynamic based on viewport of browser
The old implementation refreshed the visualization every 60 seconds. This seems like a bad idea to me, so I left that out. We might want to add a refresh visualization button so the user could do this manually if she wishes.

elasticmachine · 2019-04-22T15:24:28Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-22T16:18:32Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-04-22T17:06:59Z

Pinging @elastic/es-ui

alisonelizabeth

@bmcconaghy nice work!

I tested locally. Not sure if I hit all the possible scenarios, but here are a couple things I noticed:

Tooltip missing when you hover over data points.
What do you think about having a chart loading state? Without it, I noticed the EuiCallOut sometimes appears briefly when changing field values.
In the old UI, when you select Grouped over, it looks like the UI switches to a slider-type presentation and multiple charts are rendered. I don’t think there’s support for that yet here, so instead the EuiCallOut message renders.

Old UI:

x-pack/plugins/watcher/public/sections/watch_edit/components/watch_visualization.tsx

alisonelizabeth · 2019-04-22T19:31:59Z

x-pack/plugins/watcher/public/sections/watch_edit/components/watch_visualization.tsx

+    colorValues: [],
+    specId: getSpecId('threshold'),
+  };
+  thresholdCustomSeriesColors.set(thresholdDataSeriesColorValues, 'red');


nit: should we use the hex from the eui color pallete? it's #BD271E

https://elastic.github.io/eui/#/guidelines/colors

makes sense, done

alisonelizabeth · 2019-04-22T19:38:36Z

x-pack/plugins/watcher/public/sections/watch_edit/components/watch_visualization.tsx

+          />
+        </Chart>
+      ) : (
+        <EuiCallOut title="No data" color="warning">


i18n for title here?

good catch, added

cjcenizal

Looks great! Just had a couple suggestions and a question.

cjcenizal · 2019-04-22T18:46:28Z

x-pack/plugins/watcher/public/sections/watch_edit/components/watch_visualization.tsx

+    return detectedTimezone;
+  }
+
+  const tzOffset = moment().format('Z');


What does this do? Could we add a comment explaining the intention behind this?

added a comment

cjcenizal · 2019-04-22T19:05:24Z

x-pack/plugins/watcher/public/sections/watch_edit/components/watch_visualization.tsx

+    specId: getSpecId('threshold'),
+  };
+  thresholdCustomSeriesColors.set(thresholdDataSeriesColorValues, 'red');
+  const theme = {


To make this work with dark theme, I think we should import DARK_THEME from the charts lib and extract this out into a helper function near the top of the file:

const getChartTheme = () => { const isDarkTheme = chrome.getUiSettingsClient().get('theme:darkMode'); const baseTheme = isDarkTheme ? DARK_THEME : LIGHT_THEME; return { ...baseTheme, lineSeriesStyle: { ...baseTheme.lineSeriesStyle, line: { ...baseTheme.lineSeriesStyle.line, strokeWidth: 3, }, point: { ...baseTheme.lineSeriesStyle.point, visible: false, }, }, }; };

good point, forgot about dark mode, done

cjcenizal · 2019-04-22T19:06:48Z

x-pack/plugins/watcher/public/sections/watch_edit/components/watch_visualization.tsx

+
+const WatchVisualizationUi = () => {
+  const { watch } = useContext(WatchContext);
+  const [watchVisualizationData, setWatchVisualizationData] = useState<number[][]>([]);


It doesn't have a functional impact, but in case you find it cleaner you could also extract this and the effect on line 95 into a custom hook:

const useWatchVisualizationData = watch => { const [watchVisualizationData, setWatchVisualizationData] = useState<number[][]>([]); useEffect( () => { loadWatchVisualizationData(watch, setWatchVisualizationData); }, [watch] ); return watchVisualizationData; }

Then consuming it would look like this:

const WatchVisualizationUi = () => { const { watch } = useContext(WatchContext); const watchVisualizationData = useWatchVisualizationData(watch); /* ... */ }

I think it prefer it in this way personally.

bmcconaghy · 2019-04-22T20:12:30Z

@alisonelizabeth the tooltip missing is a bug in EUI charts. If the data is continuous, they appear, if not, not. Chart loading state makes sense, but maybe that can be done in a separate PR. The paged charts thing just got implemented as a chart with multiple lines. This works pretty well for lower values of top N, but is obviously not great with higher values. Will probably need a revisit to sort out the proper UI here.

elasticmachine · 2019-04-22T20:42:41Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

alisonelizabeth · 2019-04-22T20:47:22Z

x-pack/plugins/watcher/public/sections/watch_edit/components/watch_visualization.tsx

+import { aggTypes } from '../../../models/watch/agg_types';
+const getChartTheme = () => {
+  const isDarkTheme = chrome.getUiSettingsClient().get('theme:darkMode');
+  const baseTheme = isDarkTheme ? DARK_THEME : LIGHT_THEME;


I think you need to import DARK_THEME from @elastic/charts

alisonelizabeth · 2019-04-22T20:49:13Z

@bmcconaghy thanks for making the changes!

I'm getting an error with the latest. Do you think you could take a look?

Version: 8.0.0
Build: 9007199254740991
Error: Uncaught TypeError: (0 , _charts.getSpecId) is not a function (http://localhost:5601/fca/bundles/kibana.bundle.js:130939)
    at push../src/legacy/ui/public/notify/notify.js.window.onerror (http://localhost:5601/fca/bundles/commons.bundle.js:135703:32)
    at Object.invokeGuardedCallbackDev (webpack://%5Bname%5D/./node_modules/react-dom/cjs/react-dom.development.js?:199:16)
    at invokeGuardedCallback (webpack://%5Bname%5D/./node_modules/react-dom/cjs/react-dom.development.js?:256:31)
    at replayUnitOfWork (webpack://%5Bname%5D/./node_modules/react-dom/cjs/react-dom.development.js?:18372:5)
    at renderRoot (webpack://%5Bname%5D/./node_modules/react-dom/cjs/react-dom.development.js?:19262:13)
    at performWorkOnRoot (webpack://%5Bname%5D/./node_modules/react-dom/cjs/react-dom.development.js?:20136:7)
    at performWork (webpack://%5Bname%5D/./node_modules/react-dom/cjs/react-dom.development.js?:20048:7)
    at performSyncWork (webpack://%5Bname%5D/./node_modules/react-dom/cjs/react-dom.development.js?:20022:3)
    at batchedUpdates$1 (webpack://%5Bname%5D/./node_modules/react-dom/cjs/react-dom.development.js?:20237:7)
    at batchedUpdates (webpack://%5Bname%5D/./node_modules/react-dom/cjs/react-dom.development.js?:2151:12)

bmcconaghy · 2019-04-22T22:18:01Z

@alisonelizabeth not able to reproduce that error, maybe we can pair and you can show it to me? I did find a React warning (missing key on array) that I pushed a fix for.

elasticmachine · 2019-04-22T22:35:04Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

alisonelizabeth · 2019-04-23T12:45:24Z

@bmcconaghy thanks for looking into it. i believe the error is something with my local dev env. latest changes LGTM.

elasticmachine · 2019-04-23T13:09:26Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

adding watch visualization

507227e

bmcconaghy requested review from cjcenizal, markov00 and alisonelizabeth April 22, 2019 15:06

fixing tsc errors

f77d843

cjcenizal added Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more Feature:Watcher non-issue Indicates to automation that a pull request should not appear in the release notes labels Apr 22, 2019

alisonelizabeth reviewed Apr 22, 2019

View reviewed changes

cjcenizal approved these changes Apr 22, 2019

View reviewed changes

rendering multiple lines when a terms agg is used

630b40b

responding to PR feedback

7c986c2

alisonelizabeth reviewed Apr 22, 2019

View reviewed changes

fixing React warning

ca894b1

adding missing var

838159b

alisonelizabeth approved these changes Apr 23, 2019

View reviewed changes

bmcconaghy merged commit 5faec7b into elastic:watcher-port Apr 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding watch visualization #35404

adding watch visualization #35404

bmcconaghy commented Apr 22, 2019

elasticmachine commented Apr 22, 2019

elasticmachine commented Apr 22, 2019

elasticmachine commented Apr 22, 2019

alisonelizabeth left a comment

alisonelizabeth Apr 22, 2019

bmcconaghy Apr 22, 2019

alisonelizabeth Apr 22, 2019

bmcconaghy Apr 22, 2019

cjcenizal left a comment

cjcenizal Apr 22, 2019

bmcconaghy Apr 22, 2019

cjcenizal Apr 22, 2019

bmcconaghy Apr 22, 2019

cjcenizal Apr 22, 2019 •

edited

Loading

bmcconaghy Apr 22, 2019

bmcconaghy commented Apr 22, 2019

elasticmachine commented Apr 22, 2019

alisonelizabeth Apr 22, 2019

alisonelizabeth commented Apr 22, 2019 •

edited

Loading

bmcconaghy commented Apr 22, 2019

elasticmachine commented Apr 22, 2019

alisonelizabeth commented Apr 23, 2019

elasticmachine commented Apr 23, 2019

adding watch visualization #35404

adding watch visualization #35404

Conversation

bmcconaghy commented Apr 22, 2019

elasticmachine commented Apr 22, 2019

💔 Build Failed

elasticmachine commented Apr 22, 2019

💚 Build Succeeded

elasticmachine commented Apr 22, 2019

alisonelizabeth left a 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

cjcenizal left a 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

cjcenizal Apr 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmcconaghy commented Apr 22, 2019

elasticmachine commented Apr 22, 2019

💔 Build Failed

Choose a reason for hiding this comment

alisonelizabeth commented Apr 22, 2019 • edited Loading

bmcconaghy commented Apr 22, 2019

elasticmachine commented Apr 22, 2019

💔 Build Failed

alisonelizabeth commented Apr 23, 2019

elasticmachine commented Apr 23, 2019

💚 Build Succeeded

cjcenizal Apr 22, 2019 •

edited

Loading

alisonelizabeth commented Apr 22, 2019 •

edited

Loading