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

[SecuritySolution] Finishing touches on the alert prevalence #128295

Merged
merged 6 commits into from
Mar 24, 2022
Merged

[SecuritySolution] Finishing touches on the alert prevalence #128295

merged 6 commits into from
Mar 24, 2022

Conversation

janmonschke
Copy link
Contributor

Summary

After talking to product and design we came up with a list of smaller changes to touch up the alert prevalence work:

  • (a40ab7f) Use sentence case instead of double-upper case (is that the proper naming for that?)
  • (3c38cc4) When the prevalence request fails or is not available, we're now rendering the default empty value placeholder
  • (f4e5264) Adding the source event id to the highlighted fields.

Screenshot 2022-03-22 at 17 31 17

Checklist

Delete any items that are not applicable to this PR.

@janmonschke janmonschke added release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. auto-backport Deprecated - use backport:version if exact versions are needed Team:Threat Hunting:Investigations Security Solution Investigations Team v8.2.0 labels Mar 22, 2022
@janmonschke janmonschke requested a review from a team as a code owner March 22, 2022 17:17
janmonschke
janmonschke commented Mar 22, 2022
View reviewed changes
x-pack/plugins/security_solution/public/common/components/event_details/summary_view.tsx
@@ -47,7 +47,7 @@ const summaryColumns: Array<EuiBasicTableColumn<AlertSummaryRow>> = [
<EuiIconTip
type="iInCircle"
color="subdued"
title="Alert Prevalence"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michaelolo24 I assume this one was forgotten in the initial PR. Let me know if we should keep the English copy here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, thanks! I missed this translation.

@janmonschke
Copy link
Contributor Author

@monina-n @paulewing Notice that in the screenshot above it says Source event id in the highlighted fields instead of kibana.original_event.id. This is how it would look like without the custom label:
Screenshot 2022-03-22 at 14 51 24

Let me know if I should change it back to the field id ✌️

@elasticmachine
Copy link
Contributor

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

michaelolo24
michaelolo24 approved these changes Mar 23, 2022
View reviewed changes
Copy link
Contributor

@michaelolo24 michaelolo24 left a comment

Choose a reason for hiding this comment

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

Thanks for making these changes. LGTM 🚀 !

@michaelolo24 michaelolo24 added backport:skip This commit does not require backporting and removed auto-backport Deprecated - use backport:version if exact versions are needed labels Mar 23, 2022
@YulNaumenko YulNaumenko self-requested a review March 23, 2022 17:16
YulNaumenko
YulNaumenko reviewed Mar 23, 2022
View reviewed changes
...k/plugins/security_solution/public/common/components/event_details/table/prevalence_cell.tsx Outdated
@@ -23,7 +24,7 @@ const PrevalenceCell = React.memo<AlertSummaryRow['description']>(
if (loading) {
return <EuiLoadingSpinner />;
} else if (error) {
return null;
return <>{getEmptyValue()}</>;
Copy link
Contributor

@YulNaumenko YulNaumenko Mar 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

nit: Maybe better to use defaultToEmptyTag?
But in general it also uses a getEmptyTagValue and getEmptyStringTag which are really the same, that is more than weird how many duplications we have :)
x-pack/plugins/timelines/public/components/empty_value/index.tsx needs a clean up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, works either way I guess 👍 (e2f321f)

@janmonschke
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
securitySolution 4.8MB 4.8MB +151.0B

History

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

@janmonschke janmonschke merged commit 46f5c03 into elastic:main Mar 24, 2022
@janmonschke janmonschke deleted the security/alert-prevalence-refinement branch March 24, 2022 16:33
@nastasha-solomon nastasha-solomon mentioned this pull request Apr 5, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelolo24 michaelolo24 michaelolo24 approved these changes

@YulNaumenko YulNaumenko YulNaumenko approved these changes

Assignees
No one assigned
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:Investigations Security Solution Investigations Team v8.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@janmonschke @elasticmachine @kibana-ci @michaelolo24 @YulNaumenko @kibanamachine