Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Kpi host #35065

Merged
merged 27 commits into from
May 9, 2019
Merged

Kpi host #35065

merged 27 commits into from
May 9, 2019

Conversation

angorayc
Copy link
Contributor

@angorayc angorayc commented Apr 15, 2019 •
edited
Loading

This PR is to add kpi matrix on hosts page: elastic/ingest-dev#352 and apply the latest design.

Questions (Please scroll down to find the detail):

  1. How to do unit test for those components wrapped by AutoSizer.
  2. Not sure what's the best way to apply color scheme.
  3. The response in schema unit test looks strange.
  4. Please double check my queries, they are formed by following these reference:

Host kpi loading
Screenshot 2019-05-05 at 15 21 53

Host kpi with data
Screenshot 2019-05-06 at 23 28 54
Screenshot 2019-05-06 at 23 29 20

Host kpi without data
Screenshot 2019-05-06 at 23 37 28

Screenshot 2019-05-06 at 23 25 45

This PR updates a shared component stat_item, and therefore kpis on network page are updated as well:
Screenshot 2019-05-05 at 17 18 25
Screenshot 2019-05-05 at 17 18 22

Dark Mode
Screenshot 2019-05-05 at 17 49 56

Known issue:
Charts in EUI is going to be deprecated in June.

Todo:

@angorayc angorayc mentioned this pull request Apr 15, 2019
Closed
10 tasks
@elasticmachine

This comment has been minimized.

Sign in to view
@tsg tsg added the Team:SIEM label Apr 15, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/secops

@angorayc
Copy link
Contributor Author

Park here and waiting for further discussion about what KPIs we want to show here. https://docs.google.com/document/d/1LwkKY9kv45fEvrlvm72m4Z8km6fRblQ8yWe_4rRBPuo/edit

Also waiting for a fix from EUI for spinner
elastic/eui#1845

@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
@elasticmachine

This comment has been minimized.

Sign in to view
FrankHassanabad
FrankHassanabad reviewed May 8, 2019
View reviewed changes
x-pack/plugins/siem/server/graphql/kpi_hosts/schema.test.ts Outdated
}
}
}
`,
Copy link
Contributor

@FrankHassanabad FrankHassanabad May 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'd pull the tick to the left at column 0

spong
spong reviewed May 8, 2019
View reviewed changes
x-pack/plugins/siem/public/components/stat_items/areachart.tsx Outdated
import React from 'react';
import { pure } from 'recompose';
import styled from 'styled-components';
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to remove this @ts-ignore and no errors were shown.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for giving it a try!

spong
spong reviewed May 8, 2019
View reviewed changes
x-pack/plugins/siem/public/components/stat_items/areachart.tsx Outdated
height: number | null | undefined;
}>(({ data, ...chartConfigs }) =>
chartConfigs.width && chartConfigs.height ? (
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to remove this @ts-ignore and no errors were shown.

spong
spong reviewed May 8, 2019
View reviewed changes
x-pack/plugins/siem/public/components/stat_items/areachart.tsx
/**
* Placing ts-ignore here for fillOpacity
* */
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to remove this @ts-ignore and no errors were shown.

spong
spong reviewed May 8, 2019
View reviewed changes
x-pack/plugins/siem/public/components/stat_items/barchart.tsx

import React from 'react';
import {
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't look like this @ts-ignore is required. I was able to remove it without error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mm, mine complains about no exported member EuiSeriesChartUtils after removing ts-ignore here, so keeping this here.

spong
spong reviewed May 8, 2019
View reviewed changes
x-pack/plugins/siem/public/components/stat_items/barchart.tsx Outdated
} from '@elastic/eui';
import { pure } from 'recompose';
import styled from 'styled-components';
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't look like this @ts-ignore is required. I was able to remove it without error.

spong
spong reviewed May 8, 2019
View reviewed changes
x-pack/plugins/siem/public/components/stat_items/barchart.tsx Outdated
>
{data.map(series => {
return (
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't look like this @ts-ignore is required. I was able to remove it without error.

spong
spong reviewed May 8, 2019
View reviewed changes
x-pack/plugins/siem/public/components/stat_items/barchart.tsx Outdated
);
});

export const BarChart = pure<{ barChart: BarChartData[] | [] | null | undefined }>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the [] type needed here?

Tracing the usage of BarChart to StatItemsComponent, it looks like the type of barChart will only ever be BarChartData[] | null | undefined:

export interface StatItemsProps extends StatItems {
  key: string;
  areaChart?: AreaChartData[];
  barChart?: BarChartData[];
}

spong
spong reviewed May 8, 2019
View reviewed changes
x-pack/plugins/siem/public/components/stat_items/index.test.tsx Outdated
import { AreaChart } from './areachart';
import { EuiHorizontalRule } from '@elastic/eui';
// @ts-ignore
import { EuiAreaSeries, EuiBarSeries } from '@elastic/eui/lib/experimental';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these unused imports necessary? I was able to remove them and these tests passed.

FrankHassanabad
FrankHassanabad approved these changes May 8, 2019
View reviewed changes
Copy link
Contributor

@FrankHassanabad FrankHassanabad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has taken so much feedback and looks so great. I left some optionals but with something of this size I am fine with a part 2 or touch ups or even just everything as is.

Thanks for our first charting and our first charting queries!

spong
spong reviewed May 8, 2019
View reviewed changes
x-pack/plugins/siem/public/containers/kpi_hosts/index.gql_query.ts
KpiHosts(timerange: $timerange, filterQuery: $filterQuery) {
hosts
hostsHistogram {
...ChartFields
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realize you could use the spread operator in gql templates -- nice!

spong
spong reviewed May 8, 2019
View reviewed changes
x-pack/plugins/siem/server/graphql/kpi_hosts/resolvers.test.ts Outdated

const context = { req };

describe('Test Source Resolvers', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you added API Integration tests (awesome!) these resolvers.test.ts tests are no longer necessary -- feel free to remove this entire file :)

spong
spong reviewed May 8, 2019
View reviewed changes
x-pack/plugins/siem/server/graphql/kpi_hosts/schema.test.ts Outdated
},
};

describe('SIEM Hosts GQL Schema', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you added API Integration tests these schema.test.ts tests are no longer necessary -- feel free to remove this entire file :)

spong
spong reviewed May 8, 2019
View reviewed changes
x-pack/test/api_integration/apis/siem/kpi_hosts.ts Outdated
.then(resp => {
const kpiHosts = resp.data.source.KpiHosts;
expect(kpiHosts!.hosts).to.equal(1);
expect(kpiHosts!.hostsHistogram).to.eql([
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be helpful for readability to pull out these expected results into their own mock objects above the expect's.

spong
spong approved these changes May 8, 2019
View reviewed changes
Copy link
Member

@spong spong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work here @angorayc!! Good to see all the collaboration and congrats on bringing in the first charts to the SIEM app! 🎉

I've left a few optionals/cleanup comments if you want to take care of them as part of this, but if not feel free to open an issue and address them in a follow-up. I also provided some answers to the questions you mentioned in the description, but let me know if you come across anything extra.

Anyway, LGTM! 📈 📊 👍

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@angorayc
Copy link
Contributor Author

angorayc commented May 9, 2019

rest this please

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@angorayc angorayc merged commit 6f0fc73 into elastic:master May 9, 2019
angorayc added a commit to angorayc/kibana that referenced this pull request May 9, 2019
@angorayc
Kpi host (elastic#35065)
d9ebc34 
* init graphql

* update KPIs

* add charts

* isolate charts

* fix app crashes on screen resizing

* gen types

* rename variables

* simplify flex groups, add icon var, rm loading

* add icon names, change flex grows

* rm overlay, enable axes, show first/last ticks

* unabbreviate destination

* update UI

* update histogram data

* customize chart axis

* update x-axis for area chart

* handle no data cases

* update unit test

* fix lint error

fix type error

fix unit itest

* rename i18n var

* fix response data type

* add unit test for areachart

* add unit test for barchart

* fix UI

* add more test case for barchart

* fix for code review

* gathering mock data in kpi_host integration test
@angorayc angorayc mentioned this pull request May 9, 2019
Merged
angorayc added a commit that referenced this pull request May 9, 2019
@angorayc
Kpi host (#35065) (#36350)
e395b57 
* init graphql

* update KPIs

* add charts

* isolate charts

* fix app crashes on screen resizing

* gen types

* rename variables

* simplify flex groups, add icon var, rm loading

* add icon names, change flex grows

* rm overlay, enable axes, show first/last ticks

* unabbreviate destination

* update UI

* update histogram data

* customize chart axis

* update x-axis for area chart

* handle no data cases

* update unit test

* fix lint error

fix type error

fix unit itest

* rename i18n var

* fix response data type

* add unit test for areachart

* add unit test for barchart

* fix UI

* add more test case for barchart

* fix for code review

* gathering mock data in kpi_host integration test
@angorayc angorayc mentioned this pull request May 10, 2019
Merged
1 task
@spong spong added the release_note:skip Skip the PR/issue when compiling release notes label May 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snide snide snide left review comments

@tsg tsg tsg left review comments

@spong spong spong approved these changes

@FrankHassanabad FrankHassanabad FrankHassanabad approved these changes

Assignees
No one assigned
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:SIEM v7.2.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@angorayc @elasticmachine @tsg @MichaelMarcialis @snide @FrankHassanabad @spong