[SIEM] Replace Eui chart with elastic charts for siem kpis #36660

angorayc · 2019-05-19T20:19:01Z

Summary

This PR is to isolate charts from stat items component and replace Eui-chart with elastic-charts for siem kpis. (https://github.com/elastic/ingest-dev/issues/445)

Currently with Elastic Charts:

With Win8 IE11

Previously with EUI chart:

Known issue:

Unable to customise the ticks atm but will be able to configure the number of tick we are going to show.
The value in tooltip is not formatted and no abbreviation for y axis of barcharts as currently they share the same formatter, but will be able to specify separate formatter for both.

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-05-19T20:57:43Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-20T16:02:20Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-21T14:50:35Z

Pinging @elastic/secops

elasticmachine · 2019-05-22T06:06:56Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

angorayc · 2019-05-22T16:36:07Z

x-pack/plugins/siem/public/components/charts/areachart.test.tsx

-    it('should render a customized y-asix', () => {
-      expect(wrapper.find('EuiYAxis')).toHaveLength(1);
+    it('should render Chart', () => {
+      expect(wrapper.find('Chart')).toHaveLength(1);


Removing further testing for charts' detail here because

It is render by canvas so not able to be tested from outside of the library. - Make the chart testable from outside elastic-charts#215

Most of the cases have been tested inside elastic-charts.

angorayc · 2019-05-22T16:39:58Z

x-pack/plugins/siem/public/components/charts/common.tsx

+  LINE = 'line',
+}
+
+export const getSeriesStyle = (


reference: Customise series color in elastic-charts

angorayc · 2019-05-22T16:44:46Z

x-pack/plugins/siem/public/components/charts/common.tsx

+  return value.toLocaleString && value.toLocaleString();
+};
+
+export enum SeriesType {


Align with the seriesType mentioned in elastic-charts overview

elasticmachine · 2019-05-22T17:51:34Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

angorayc · 2019-05-22T18:26:40Z

reset please

angorayc · 2019-05-22T18:27:33Z

retest this

angorayc · 2019-05-22T18:38:42Z

retest this

elasticmachine · 2019-05-22T20:08:36Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-22T21:07:17Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

angorayc · 2019-05-23T05:07:52Z

reset this

angorayc · 2019-05-23T12:34:32Z

reset this

elasticmachine · 2019-05-23T15:04:31Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-23T16:03:42Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

angorayc · 2019-05-23T17:19:51Z

Temporary close this as a problem was found on IE crashes the app.

angorayc · 2019-05-24T06:55:57Z

Reopening this PR as the problem has been fixed in @elastic-charts 4.2.6:
#36753

elasticmachine · 2019-05-24T08:21:53Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-24T18:20:37Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

FrankHassanabad · 2019-05-24T23:16:18Z

package.json

@@ -434,4 +434,4 @@
    "node": "10.15.2",
    "yarn": "^1.10.1"
  }
-}
+}


new line here. That's weird your IDE didn't auto-add it.

Somehow it allowed a new line for all the files but this one (It was not only not adding it, but removing it if I added) lol. Thanks for notice, just upgrade my VSCode and added a rule for it :D

FrankHassanabad · 2019-05-24T23:33:49Z

x-pack/plugins/siem/public/components/charts/areachart.tsx

+      )}
+    </AutoSizer>
+  )
+);


Really clean and great code! For these pure functions I would recommend changing them to use React.Memo like so:

export const AreaChart = React.memo<{ areaChart: AreaChartData[] | null | undefined }>( ({ areaChart }) => ( <AutoSizer detectAnyWindowResize={false} content> {({ measureRef, content: { height, width } }) => ( <WrappedByAutoSizer innerRef={measureRef}> <AreaChartWithCustomPrompt data={areaChart} height={height} width={width} /> </WrappedByAutoSizer> )} </AutoSizer> ) );

For new code moving forward I think we can remove our usage of recompose and just utilize React when we write new code which is pretty awesome.

Sure thing, thanks! I am happy to move away from pure component. I'll have another PR for enhancing kpi network, and do the cleanup for it as well.

FrankHassanabad · 2019-05-24T23:37:19Z

x-pack/plugins/siem/public/components/charts/common.tsx

+  <FlexGroup justifyContent="center" alignItems="center">
+    <EuiFlexItem grow={false}>
+      <EuiText size="s" textAlign="center" color="subdued">
+        Chart Data Not Available


You will need a i18n key here for Chart Data Not Available. It doesn't have to be a new separate file if you don't want it to be. Some people prefer to use i18n inline within their files and I think that's fine.

FrankHassanabad · 2019-05-24T23:48:18Z

x-pack/plugins/siem/public/components/stat_items/index.tsx

@@ -49,28 +36,10 @@ export interface StatItem {
  description?: string;
  value: number | undefined | null;
  color?: string;
-  icon?: 'storage' | 'cross' | 'check' | 'visMapCoordinate';
+  icon?: 'storage' | 'cross' | 'check' | 'visMapCoordinate' | 'globe';


Would this just be better as the exported Icon Type from EUI?:

import { IconType } from '@elastic/eui';

icon?: IconType

Then you don't have to retype it for each new icon you want to add here.

Cool, that makes the code much easier to maintain! Thanks!

elasticmachine · 2019-05-25T10:04:02Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-28T15:25:17Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-05-28T16:29:52Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

angorayc · 2019-05-28T17:47:36Z

package.json

@@ -349,7 +349,7 @@
    "dedent": "^0.7.0",
    "delete-empty": "^2.0.0",
    "enzyme": "^3.7.0",
-    "enzyme-adapter-react-16": "^1.9.0",
+    "enzyme-adapter-react-16": "^1.10.0",


Upgrade enzyme-adapter-react-16 to support React.memo
enzymejs/enzyme#1914

elasticmachine · 2019-05-28T19:35:32Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

FrankHassanabad

Checked it out, played with it, and really appreciate the upgrade for enzyme for everyone so we can use Rect.memo.

Looks great to me!

…6660) * move charts to separate components * replace areachart * apply custom styles * customize barchart color * customize color for areachart * move reusable functions into common * exchange x & y value in barchart dataset * replace pure component with react memo and upgrade enzyme adapter

…37317) * move charts to separate components * replace areachart * apply custom styles * customize barchart color * customize color for areachart * move reusable functions into common * exchange x & y value in barchart dataset * replace pure component with react memo and upgrade enzyme adapter

…6660) * move charts to separate components * replace areachart * apply custom styles * customize barchart color * customize color for areachart * move reusable functions into common * exchange x & y value in barchart dataset * replace pure component with react memo and upgrade enzyme adapter

angorayc self-assigned this May 21, 2019

angorayc added the Team:SIEM label May 21, 2019

angorayc force-pushed the el-chart branch from fd99bab to ddb36e4 Compare May 22, 2019 16:16

angorayc commented May 22, 2019

View reviewed changes

angorayc marked this pull request as ready for review May 22, 2019 17:12

angorayc changed the title ~~replace Eui chart with elastic charts for siem kpis~~ Replace Eui chart with elastic charts for siem kpis May 22, 2019

angorayc force-pushed the el-chart branch from cb215f8 to e992d3f Compare May 22, 2019 17:32

spong added the release_note:skip Skip the PR/issue when compiling release notes label May 23, 2019

angorayc force-pushed the el-chart branch from ca85058 to eb2f03e Compare May 23, 2019 14:41

angorayc closed this May 23, 2019

angorayc reopened this May 24, 2019

angorayc force-pushed the el-chart branch from eb2f03e to 92484db Compare May 24, 2019 16:59

FrankHassanabad reviewed May 24, 2019

View reviewed changes

angorayc force-pushed the el-chart branch from 92484db to c4334dc Compare May 25, 2019 09:07

angorayc commented May 28, 2019

View reviewed changes

angorayc added 8 commits May 29, 2019 02:34

move charts to separate components

db673d4

replace areachart

a9c2bd5

apply custom styles

b281a9b

customize barchart color

68660fa

customize color for areachart

d3055f8

move reusable functions into common

5a426db

exchange x & y value in barchart dataset

0cf7877

replace pure component with react memo and upgrade enzyme adapter

6e17bf6

angorayc force-pushed the el-chart branch from 01eab23 to 6e17bf6 Compare May 28, 2019 18:35

angorayc changed the title ~~Replace Eui chart with elastic charts for siem kpis~~ [SIEM] Replace Eui chart with elastic charts for siem kpis May 28, 2019

angorayc added v7.3.0 v8.0.0 labels May 28, 2019

FrankHassanabad approved these changes May 28, 2019

View reviewed changes

angorayc merged commit ac3eb85 into elastic:master May 29, 2019

angorayc mentioned this pull request May 29, 2019

[7.x] [SIEM] Replace Eui chart with elastic charts for siem kpis (#36660) #37317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SIEM] Replace Eui chart with elastic charts for siem kpis #36660

[SIEM] Replace Eui chart with elastic charts for siem kpis #36660

angorayc commented May 19, 2019 •

edited

Loading

elasticmachine commented May 19, 2019

elasticmachine commented May 20, 2019

elasticmachine commented May 21, 2019

elasticmachine commented May 22, 2019

angorayc May 22, 2019

angorayc May 22, 2019

angorayc May 22, 2019

elasticmachine commented May 22, 2019

angorayc commented May 22, 2019

angorayc commented May 22, 2019

angorayc commented May 22, 2019

elasticmachine commented May 22, 2019

elasticmachine commented May 22, 2019

angorayc commented May 23, 2019

angorayc commented May 23, 2019

elasticmachine commented May 23, 2019

elasticmachine commented May 23, 2019

angorayc commented May 23, 2019 •

edited

Loading

angorayc commented May 24, 2019

elasticmachine commented May 24, 2019

elasticmachine commented May 24, 2019

FrankHassanabad May 24, 2019

angorayc May 28, 2019

FrankHassanabad May 24, 2019

angorayc May 28, 2019

FrankHassanabad May 24, 2019

FrankHassanabad May 24, 2019

angorayc May 28, 2019

elasticmachine commented May 25, 2019

elasticmachine commented May 28, 2019

elasticmachine commented May 28, 2019

angorayc May 28, 2019

elasticmachine commented May 28, 2019

FrankHassanabad left a comment

[SIEM] Replace Eui chart with elastic charts for siem kpis #36660

[SIEM] Replace Eui chart with elastic charts for siem kpis #36660

Conversation

angorayc commented May 19, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented May 19, 2019

💔 Build Failed

elasticmachine commented May 20, 2019

💔 Build Failed

elasticmachine commented May 21, 2019

elasticmachine commented May 22, 2019

💔 Build Failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented May 22, 2019

💔 Build Failed

angorayc commented May 22, 2019

angorayc commented May 22, 2019

angorayc commented May 22, 2019

elasticmachine commented May 22, 2019

💚 Build Succeeded

elasticmachine commented May 22, 2019

💔 Build Failed

angorayc commented May 23, 2019

angorayc commented May 23, 2019

elasticmachine commented May 23, 2019

💔 Build Failed

elasticmachine commented May 23, 2019

💚 Build Succeeded

angorayc commented May 23, 2019 • edited Loading

angorayc commented May 24, 2019

elasticmachine commented May 24, 2019

💚 Build Succeeded

elasticmachine commented May 24, 2019

💚 Build Succeeded

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

Choose a reason for hiding this comment

elasticmachine commented May 25, 2019

💔 Build Failed

elasticmachine commented May 28, 2019

💔 Build Failed

elasticmachine commented May 28, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

elasticmachine commented May 28, 2019

💚 Build Succeeded

FrankHassanabad left a comment

Choose a reason for hiding this comment

angorayc commented May 19, 2019 •

edited

Loading

angorayc commented May 23, 2019 •

edited

Loading