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.

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

[SIEM] Add Authentications histogram #48260

Conversation

patrykkopycinski
Copy link
Contributor

@patrykkopycinski patrykkopycinski commented Oct 15, 2019

Summary

https://github.com/elastic/siem-team/issues/467
Add authentications histogram to hosts / host details page (#42461)
The histogram shows
Will add a dropdown so users can switch between total authentications count, top users/sources/destinations.
mockup: https://www.figma.com/file/YvAwDnS860sgOUTdLemnuF/Histograms?node-id=14%3A3468

image

image

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

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
Contributor

Pinging @elastic/siem (Team:SIEM)

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

import { i18n } from '@kbn/i18n';

export const AUTHENTICATIONS_COUNT = i18n.translate(
'xpack.siem.authenticationsOverTime.authenticationCount',
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be xpack.siem.authenticationsOverTime.authenticationCountTitle.
There's a specific naming rules we can follow here:
https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/GUIDELINE.md#message-types

@angorayc
Copy link
Contributor

Nicely done, thank you! Played around with it and found the legend shows incorrectly when only once group of data is given. This might be an existing problem, but given that this case seems to be very common for auth histogram, I would prefer to fix it in this PR. Besides that all looks good!
authHistoogram

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
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.

Summary some feedback from @markov00 for the problem we have:
That happens because when using splitSeries the chart determine the number of series in the data and use the value from the splitted groups, in your case the value of your g as the name of the series. If only one series exist on the chart, then we use the name or the id of the series.

We can work around this via

  1. Add the missing data: { x: 1571290200000, y: null, g: 'authentication_success' } or
  2. What we are doing in this PR or
  3. Create every group a series (Like what we are doing for barcharts in KPIs)

Since they've got a ticket elastic/elastic-charts#245,
I think we can comment this ticket in the code and have a proper fix for it later

@patrykkopycinski
Copy link
Contributor Author

Thank you @angorayc and @markov00 for help 💪
I've chosen option #2

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@angorayc
Copy link
Contributor

Awesome, LGTM, thank you so much!

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@angorayc
Copy link
Contributor

Hey Patryk,
Thanks for updating the color scheme as well!
Since that we'll have similar request for other histograms, could we apply the similar approach as what we've got for kpi hosts and host details, so we can reuse the logic generically?

@angorayc
Copy link
Contributor

I’m happy to have a temporary solution here in this PR, and submit another one for a generic solution for it.

…entications-histogram

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

💚 Build Succeeded

@patrykkopycinski patrykkopycinski merged commit 93269e5 into elastic:master Oct 28, 2019
@patrykkopycinski patrykkopycinski deleted the feat/add-authentications-histogram branch October 28, 2019 16:43
patrykkopycinski added a commit to patrykkopycinski/kibana that referenced this pull request Oct 28, 2019
patrykkopycinski added a commit to patrykkopycinski/kibana that referenced this pull request Oct 28, 2019
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.

3 participants