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

[SIEM] Adds actions and selection to Signals Table #53101

Merged
merged 13 commits into from
Dec 19, 2019
Merged

[SIEM] Adds actions and selection to Signals Table #53101

merged 13 commits into from
Dec 19, 2019

Conversation

spong
Copy link
Member

@spong spong commented Dec 16, 2019

Summary

This is Part II of II for adding the Signals Table to the main Detection Engine landing page (meta issue).

Part II includes:

  • Adding selection, selectAll & selectAllGlobal (i.e. query select) functionality to the EventsViewer
  • Includes ability to specify a fieldset when storing selection state so it can be used by custom actions
  • Introduces following new Timeline state:
    • deletedEventIds: string[]
    • loadingEventIds: string[]
    • selectedEventIds: Record<string, TimelineNonEcsData[]>
    • showCheckboxes: boolean
    • showRowRenderers: boolean
  • Adds Send to Timeline overflow/batch action (detailed here)
  • Adds Update Signal Status overflow/batch action

Resolves #51785

Selection / Update Signal Status

update_signal_state

Send Signal to Timeline Action

Checklist

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

For maintainers

@spong spong added Team:SIEM v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.6.0 labels Dec 16, 2019
@spong spong self-assigned this Dec 16, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

@angorayc angorayc mentioned this pull request Dec 16, 2019
Merged
3 tasks
XavierM
XavierM reviewed Dec 17, 2019
View reviewed changes
x-pack/legacy/plugins/siem/public/components/events_viewer/events_viewer.tsx
Comment on lines +149 to +152
const totalCountMinusDeleted =
totalCount > 0 ? totalCount - deletedEventIds.length : 0;

// TODO: Reset eventDeletedIds/eventLoadingIds on refresh/loadmore (getUpdatedAt)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe TimelineQuery can take care of that if you passed the parameter and default it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Perfect! I was hesitant to move that logic into TimelineQuery and wanted to chat with you first -- this will work out well.

XavierM
XavierM reviewed Dec 17, 2019
View reviewed changes
x-pack/legacy/plugins/siem/public/components/events_viewer/events_viewer.tsx
/>
<StatefulBody
browserFields={browserFields}
data={events.filter(e => !deletedEventIds.includes(e._id))}
Copy link
Contributor

Choose a reason for hiding this comment

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

I really do think that TimelineQuery need to take care of that because that will cause re-rendering

spong added 2 commits December 17, 2019 20:16
@spong
Fixes from comments, hoists defaultFilters and to/from from SignalsTa…
88765e4 
…ble, fixes selection issues, support for updating status of all signals, and send multi-signal to timeline support
@spong
Merge branch 'master' of github.com:elastic/kibana into de-signals-ac…
c587651 
…tions
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

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

XavierM
XavierM reviewed Dec 19, 2019
View reviewed changes
x-pack/legacy/plugins/siem/public/components/timeline/body/stateful_body.tsx
@@ -150,6 +203,13 @@ const StatefulBodyComponent = React.memo<StatefulBodyComponentProps>(
[id]
);

// Sync to timelineTypeContext.selectAll so parent components can select all events
useEffect(() => {
if (timelineTypeContext.selectAll) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For My Information, can use that later on also to unselect all if we are putting it back the value to false

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's on the provider of the TimelineTypeContext prop to StatefulEventViewer that's responsible for resetting it, so there's some flexibility. I've made a note to better document this in the TimelineTypeContext docs as this is a good one to know about. Thanks for mentioning! 🙂

XavierM
XavierM reviewed Dec 19, 2019
View reviewed changes
x-pack/legacy/plugins/siem/public/pages/detection_engine/detection_engine.tsx
@@ -65,8 +66,7 @@ export const DetectionEngineComponent = React.memo(() => {
</EuiPanel>

<EuiSpacer />

<SignalsTable />
<GlobalTime>{({ to, from }) => <SignalsTable from={from} to={to} />}</GlobalTime>
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I ended up yanking it out as the histogram needs it as well. Once we update our redux/hooks libs, I wonder if it'll be better/cleaner to just wire up the components individually? Will think about that one.

Copy link
Contributor

Choose a reason for hiding this comment

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

No please do not do it individually, or it will break everything

XavierM
XavierM reviewed Dec 19, 2019
View reviewed changes
x-pack/legacy/plugins/siem/public/pages/detection_engine/signals/actions.tsx
export const getFilterAndRuleBounds = (
data: TimelineNonEcsData[][]
): [string[], number, number] => {
const stringFilter = data?.[0].filter(d => d.field === 'signal.rule.filters')?.[0]?.value ?? [];
Copy link
Contributor

Choose a reason for hiding this comment

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

if a string why default to Array

Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if we should write a query to get the min and max of the time with an aggregation. It might be cleaner and safer. what do you think? no need to do it now, just wondering

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like it defaults to Array as a byproduct of createTimeline()'s filters arg preferring an [].

And I like that idea -- that also opens possibilities around the ability to send all signals for the current query (select all signals on all pages) as well, which we don't currently allow.

XavierM
XavierM reviewed Dec 19, 2019
View reviewed changes
.../siem/public/pages/detection_engine/signals/components/signals_utility_bar/batch_actions.tsx
? i18n.BATCH_ACTION_CLOSE_SELECTED
: i18n.BATCH_ACTION_OPEN_SELECTED;

return [
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use your nice technique that you did above with JSX element maybe ;)

XavierM
XavierM approved these changes Dec 19, 2019
View reviewed changes
Copy link
Contributor

@XavierM XavierM left a comment

Choose a reason for hiding this comment

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

LGTM, Enjoyable user experience here, no I am joking it is awesome!!!

When reviewing the code I think that there is a way to refresh the data when deleting a signal with re-fetch functionality.

@spong spong merged commit 26ddfb2 into elastic:master Dec 19, 2019
@spong spong deleted the de-signals-actions branch December 19, 2019 03:45
@spong spong mentioned this pull request Dec 19, 2019
Merged
spong added a commit that referenced this pull request Dec 19, 2019
@spong
[SIEM] Adds actions and selection to Signals Table (#53101) (#53551)
3b0e778 
## Summary

This is `Part II` of `II` for adding the `Signals Table` to the main Detection Engine landing page ([meta issue](#50405)).

`Part II` includes:
* Adding `selection`, `selectAll` & `selectAllGlobal` (i.e. query select) functionality to the EventsViewer
* Includes ability to specify a fieldset when storing selection state so it can be used by custom actions
 * Introduces following new Timeline state:
    * `deletedEventIds: string[]`
    * `loadingEventIds: string[]`
    *  `selectedEventIds: Record<string, TimelineNonEcsData[]>`
    * `showCheckboxes: boolean`
    * `showRowRenderers: boolean`
* Adds Send to Timeline overflow/batch action (detailed [here](#50405 (comment)))
* Adds Update Signal Status overflow/batch action

Resolves #51785

##### Selection / Update Signal Status
![update_signal_state](https://user-images.githubusercontent.com/2946766/70887496-61d59280-1f9b-11ea-8483-ab30e3936738.gif)

##### Send Signal to Timeline Action

### 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](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility)
- [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)
- [ ] [Documentation](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#writing-documentation) was added for features that require explanation or tutorials
- [ ] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios
- [ ] ~This was checked for [keyboard-only and screenreader accessibility](https://developer.mozilla.org/en-US/docs/Learn/Tools_and_testing/Cross_browser_testing/Accessibility#Accessibility_testing_checklist)~

### For maintainers

- [ ] ~This was checked for breaking API changes and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)~
- [ ] ~This includes a feature addition or change that requires a release note and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)~
@spong spong mentioned this pull request Dec 19, 2019
Closed
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XavierM XavierM XavierM approved these changes

Assignees

@spong spong

Labels
release_note:skip Skip the PR/issue when compiling release notes Team:SIEM v7.6.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[SIEM] [Detection Engine] Create Signals Table
3 participants
@spong @elasticmachine @XavierM