-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Security Solution] [RAC] Add alerts count table to Security Solutions alerts page #106358
[Security Solution] [RAC] Add alerts count table to Security Solutions alerts page #106358
Conversation
5b21bc7
to
511be2b
Compare
...gins/security_solution/common/detection_engine/schemas/request/query_signals_index_schema.ts
Show resolved
Hide resolved
...on/public/detections/components/alerts_kpis/alerts_histogram_panel/alerts_histogram.test.tsx
Show resolved
Hide resolved
.../security_solution/public/detections/components/alerts_kpis/alerts_histogram_panel/index.tsx
Show resolved
Hide resolved
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/security-threat-hunting (Team:Threat Hunting) |
...curity_solution/public/detections/components/alerts_kpis/alerts_count_panel/alerts_count.tsx
Outdated
Show resolved
Hide resolved
...curity_solution/public/detections/components/alerts_kpis/alerts_count_panel/alerts_count.tsx
Outdated
Show resolved
Hide resolved
cfe3fc1
to
ff91116
Compare
Running into some issues getting this PR going, so apologies if these are obvious. Will there be 10,000 results in the 'Count' table and it would scroll? Or is that just the query and we only show top 5 results? Do we need to say 'Trend' or 'Count'. I would think the histogram might be clear that it's alert count breakdown over time. Whereas we may need a title for the table (again, suggesting Top 5 {Rules, Hosts...} or something there). Ideally we would also be able to combine the dropdowns into a single that changes both. I know I'm reiterating some conversations here, but just getting them down for posterity. And I think it's likely worthwhile to bring this back into design for the 2nd iteration of this. So I'm mainly trying to figure out a more simple first solution to this section. cc/ @paulewing |
@elasticmachine merge upstream |
93bbd91
to
3affd8d
Compare
Yes, we are displaying a max of 10,000 items and allowing the user to scroll. However, it might be harmful performance-wise to show 10.000 items. We are investigating if we can easily hide rows that are not visible. @paulewing Would it be possible to set a lower limit like "top 5" or "top 100" rows? |
3affd8d
to
ef20841
Compare
@dhurley14 I implemented the feature using |
x-pack/plugins/security_solution/public/detections/components/alerts_kpis/common/hooks.ts
Outdated
Show resolved
Hide resolved
value: AlertsStackByField; | ||
} | ||
|
||
export type AlertsStackByField = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe in a follow-up PR can you add a comment saying why these fields specifically? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked myself the same question. "Why these fields?".
The only answer I got is that these values are hardcoded since the first implementation. The original PR that introduced this feature lists them #53742.
Maybe someone else could help us here. 😵💫
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally and works great. Switching between the field types was smooth, 👍🏾
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
x-pack/plugins/security_solution/public/detections/pages/detection_engine/detection_engine.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one nit as far as using numbers and not Eui variables.
One change I'd like to propose is to use popovers instead of the form dropdowns. My reason for this is primarily to reduce the amount of exposed dropdowns and treat 'configuration' options the same across the page. These are based off new mockups, so I apologize for the late updates.
And with the popover selector:
The button label that triggers the popover would then be updated with the selected field value.
I do not have the search option shown for the popover, but I think if the field options list grows any further, we may want to consider that.
If you feel this is better suited for another PR, I'm fine with that and can approve this (with the small nit) and repost in a new PR.
.../security_solution/public/detections/components/alerts_kpis/alerts_histogram_panel/index.tsx
Outdated
Show resolved
Hide resolved
e965f85
to
51db368
Compare
Hi @mdefazio, thanks for taking a look at this PR. It would be better to implement the popover in another PR. So, we can unblock this one. Btw, I replaced the pixel size padding for an EuiVariable. Soon, I will create another PR and add you as a reviewer. It would be helpful if you could send me the link to the Figma mockup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Popover styling edits can be done in a separate PR. Thanks!
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / Firefox UI Functional Tests.test/functional/apps/discover/_field_data·ts.discover app discover tab field data the search term should be highlighted in the field dataStandard Out
Stack Trace
Metrics [docs]Module Count
Async chunks
Unknown metric groupsReferences to deprecated APIs
History
To update your PR or re-run it, just comment with: cc @machadoum |
…s alerts page (elastic#106358) * Add alerts count table to Security Solutions alerts page * Refactor alerts_histogram_panel to reuse logic on alerts_count * Improve AlertsCount mobile layout # Conflicts: # x-pack/plugins/translations/translations/zh-CN.json
…s alerts page (elastic#106358) * Add alerts count table to Security Solutions alerts page * Refactor alerts_histogram_panel to reuse logic on alerts_count * Improve AlertsCount mobile layout
Summary
issue: https://github.com/elastic/security-team/issues/1273
Add a table that shows the count of alerts for each stacked by field. The table is hidden under the "Count" Tab.
Changes:
AlertHistogram
to allowAlertsCount
to reuse its code.AlertsCount
panel.AlertsCount
that fetches 10.000 items.AlertsCount
panel to alerts page.Checklist
Delete any items that are not applicable to this PR.