Skip to content

[SIEM] Add hosts and network anomalies histogram#50295

Merged
patrykkopycinski merged 20 commits intoelastic:masterfrom
patrykkopycinski:feat/add-hosts-anomalies-histogram
Nov 25, 2019
Merged

[SIEM] Add hosts and network anomalies histogram#50295
patrykkopycinski merged 20 commits intoelastic:masterfrom
patrykkopycinski:feat/add-hosts-anomalies-histogram

Conversation

@patrykkopycinski
Copy link
Copy Markdown
Contributor

@patrykkopycinski patrykkopycinski commented Nov 12, 2019

Summary

image

Implements #42461

  • Anomalies histogram on Hosts page
  • Anomalies hisogram on Host details page
  • Anomalies histogram on Network page
  • Anomalies histogram on IP details page (is hidden when no data to display)

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

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
Copy link
Copy Markdown
Contributor

Pinging @elastic/siem (Team:SIEM)

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@patrykkopycinski patrykkopycinski changed the title [SIEM] Add hosts anomalies histogram [SIEM] Add hosts and network anomalies histogram Nov 12, 2019
@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

filterQuery?: string | ESTermQuery;
}

export type AnomaliesTableComponent = Omit<AnomaliesNetworkTableProps, 'type'> & {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The name of this type does actually sounds like a component here, and at the same time we do also use the same name as a props/component, therefore I'd recommend to rename this type.

@angorayc
Copy link
Copy Markdown
Contributor

Hey Patryk, I had a quick look over it, it's nice to see it's been added on both hosts and network page. Good stuff!! Thanks a lot. I'll keep reading and playing around with it.

Since I'm also working on similar stuff, here are some questions I have that I'd like to confirm with @cwurm, may be worth considering here as well:

  1. The display name of this chart
  2. The data for x-axis: timestamp or job name (is it necessary to take the table below into consideration, to make it feel like we are visualising the table? )
  3. Is the histogram display the same kinds of data on different pages?

@patrykkopycinski
Copy link
Copy Markdown
Contributor Author

I also would like to check with you if it does make sense to have a histogram on IP details page? @cwurm @tsg @stephmilovic

@tsg
Copy link
Copy Markdown
Contributor

tsg commented Nov 13, 2019

From the user perspective, based on the screenshot, this looks good to me. Timestamp on the x-axis and job name as legend seems like what we need.

I also would like to check with you if it does make sense to have a histogram on IP details page?

I'd say it logically makes sense. The only consideration is that we don't have tabs there and we have to be ready that most often this chart will be empty. I wonder if it would make sense to hide it when empty?

@patrykkopycinski
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

…s-anomalies-histogram

# Conflicts:
#	x-pack/legacy/plugins/siem/public/pages/network/ip_details/index.tsx
@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

Copy link
Copy Markdown
Contributor

@angorayc angorayc left a comment

Choose a reason for hiding this comment

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

Thank you Patryk, nice and clean code as always!! Please feel free to merge after resolving the conflict.

…s-anomalies-histogram

# Conflicts:
#	x-pack/legacy/plugins/siem/server/lib/compose/kibana.ts
@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@angorayc
Copy link
Copy Markdown
Contributor

Hey Patryk, sorry the type check failed, I've tried another way to fix it and passed the type check locally, please let me know what you think
angorayc@711f762

@angorayc
Copy link
Copy Markdown
Contributor

After playing around with it, I think I'm fine with the original solution as otherwise it take more actions to fix that and not sure if it worth the time.

@patrykkopycinski patrykkopycinski force-pushed the feat/add-hosts-anomalies-histogram branch from 66db385 to 752fd6f Compare November 22, 2019 12:41
@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

…s-anomalies-histogram

# Conflicts:
#	x-pack/legacy/plugins/siem/public/pages/network/navigation/dns_query_tab_body.tsx
#	x-pack/legacy/plugins/siem/public/pages/network/navigation/network_routes.tsx
@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@patrykkopycinski
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants