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

[SecuritySolution] Replace risk score over time with Lens Embeddable #149035

Merged
merged 16 commits into from
Jan 24, 2023

Conversation

angorayc
Copy link
Contributor

@angorayc angorayc commented Jan 17, 2023

Summary

Implements: #149015

Please Enable feature flags. Please add this to kibana.dev.yml xpack.securitySolution.enableExperimental: ['chartEmbeddablesEnabled']


Replace risk score over time with Lens Embeddable

Before:
Screenshot 2023-01-17 at 10 28 23

After:
Screenshot 2023-01-24 at 15 03 40

Alerts By severity on host / network / user details should apply global filters

Screenshot 2023-01-19 at 11 06 21

Styling for donuts on Entity Analytics dashboard

(Moving the legend to left side of the chart so its actions button wouldn't overlap with chart action)
Screenshot 2023-01-19 at 11 08 47

Preview:
Host risk score over time
User risk score over time
https://p.elstc.co/paste/2MIN+pHd#TETZwPh15r64HQ2z0Cn26Z321XCxe+2DqliqF5-CHmr
Designers' review:
#149123

Known issue:
#149513

Checklist

Delete any items that are not applicable to this PR.

@angorayc angorayc added release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting Security Solution Threat Hunting Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Explore v8.7.0 backport:skip This commit does not require backporting labels Jan 17, 2023
@angorayc angorayc changed the title replace risk score over time with lens [SecuritySolution] Replace risk score over time with lens Jan 17, 2023
@angorayc angorayc changed the title [SecuritySolution] Replace risk score over time with lens [SecuritySolution] Replace risk score over time with Lens Embeddable Jan 17, 2023
@angorayc angorayc marked this pull request as ready for review January 23, 2023 09:22
@angorayc angorayc requested review from a team as code owners January 23, 2023 09:22
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

Copy link
Member

@machadoum machadoum left a comment

Choose a reason for hiding this comment

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

Hey Angela! Thank you for updating the risk score over time chart.

I noticed that there are some donut chart code changes in the branch. Was it intentional?

Comment on lines +166 to +169
domain={{
min: 0,
max: 100,
}}
Copy link
Member

Choose a reason for hiding this comment

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

@angorayc I made it look more similar to the previous design by setting max-min to 0-100. And removing the horizontal lines.
Screenshot 2023-01-23 at 16 20 32
Is it possible to apply the same changes by configuration?

I also noticed that you are using the 'average' function for grouping and I believe that a 'maximum' function would fit better in this use case.

Copy link
Contributor Author

@angorayc angorayc Jan 24, 2023

Choose a reason for hiding this comment

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

Thanks for the tips! I've applied the domain settings for Y axis, and uses maximum function for rendering the value.
Screenshot 2023-01-24 at 15 51 36

@@ -71,6 +72,12 @@ export const DonutTextWrapper = styled(EuiFlexGroup)<
max-width: 77px;
position: absolute;
z-index: 1;

&.risk-score {
Copy link
Member

Choose a reason for hiding this comment

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

This CSS selector doesn't seem very maintainable. We shouldn't make references to specific pages or components from inside a generic donut chart. I would suggest to have a particular property for this behaviour, like emptyDataTopPosition, or reuse the class you receive as props in the selector:

&.${donutTextWrapperClassName} {
   ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I have updated the css rules here, so we can just pass a custom class and the styles to the component.

@angorayc
Copy link
Contributor Author

Hey Angela! Thank you for updating the risk score over time chart.

I noticed that there are some donut chart code changes in the branch. Was it intentional?

Yes, it implements Styling for donuts on Entity Analytics dashboard from the summary section of this PR.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 3530 3531 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 12.8MB 12.8MB +3.8KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Member

@machadoum machadoum left a comment

Choose a reason for hiding this comment

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

It looks great. Thank you!

@angorayc angorayc merged commit 7c4789d into elastic:main Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Explore Team:Threat Hunting Security Solution Threat Hunting Team v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants