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] Detections add alert & signal tab #55127

Merged
merged 10 commits into from
Jan 18, 2020

Conversation

XavierM
Copy link
Contributor

@XavierM XavierM commented Jan 16, 2020

Summary

@angorayc and I worked together to get the alert tab on detection engine,

image

@patrykkopycinski be aware of this PR for your breadcrumb fix on the detection engine.

Checklist

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

For maintainers

@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

@MichaelMarcialis

This comment has been minimized.

@XavierM

This comment has been minimized.

@MichaelMarcialis
Copy link
Contributor

@MichaelMarcialis I should do the same for rule details since we have tabs there too

@XavierM: Actually, that shouldn't be necessary on the rule details page, since the tabs are supposed to be below the definition, about and schedule panels. It looks as though the tabs on that page are in the wrong spot right now. I'll make sure to record that in my full detection engine design review.

Copy link
Contributor

@MichaelMarcialis MichaelMarcialis 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, @XavierM and @angorayc! It's looking great. Here's a few notes from my review.

  • With the addition of third-party alerts, the “Last signal” timestamp subtitle in the page header no longer makes sense. There are a few possibilities to fix this, but for the sake of simplicity, would it be possible to replace the "Last signal" subtitle with a new "Last detection" subtitle that uses either the most recent signal or alert relative timestamp (whichever is the most recent)?
    We agree to keep it the way it is

  • The alerts histogram header text appears to change in accordance with the selection the user has made in the stack selector. However, the signals histogram does not do this. For consistency’s sake, can we have the alerts histogram header not change at all, thus matching the signals histogram? I think simply having the text be a static “Alert detection frequency” will do (assuming this doesn’t have any ill effects elsewhere).

  • Currently, the alerts histogram legend is aligned on the right side of the panel, while the signals histogram's legend is bottom aligned. For consistency, can align the signals histogram legend to the right (and possibly all other histograms found in the hosts and network sections, if time permits)?

  • The signals table header text is currently "All signals". Can we change this to “Signals” to match with alerts table wording?

  • The current rules page header text is "Rules". Can we now change the rules page header text to be “Signal detection rules” to match the wording of our new button text?

@andrew-goldstein
Copy link
Contributor

  • The alerts table header text is currently "Alerts". Can we change this to “All alerts” to match with signals table wording?

@MichaelMarcialis technically, the All Signals table on the Detection engine page is always showing a subset of all the signals that have been created. This is true because the date picker (+ KQL) ensure only some of the signals that have been created (24 hours by default) are being shown.

Also per the screenshot below, it's only possible to view only Open signals or Closed signals, but never both of them at the same time:

All-signals

Thus ignoring the date picker, the table still is only showing a subset of signals (open or closed).

If you agree that the table is technically not showing all of the signals that have been created, or even all of the signals (simultaneously) for the selected date range, please consider recommending an alternative to All in All signals

@MichaelMarcialis
Copy link
Contributor

@MichaelMarcialis technically, the All Signals table on the Detection engine page is always showing a subset of all the signals that have been created. This is true because the date picker (+ KQL) ensure only some of the signals that have been created (24 hours by default) are being shown.

Good point. I'll update my comment to reflect this. Thanks!

@andrew-goldstein
Copy link
Contributor

I wouldn't hold up this PR for it, but FYI i'm not seeing the Signals histogram refreshing when the date picker Update is clicked (on both the Detection engine and Overview page

Copy link
Contributor

@andrew-goldstein andrew-goldstein left a comment

Choose a reason for hiding this comment

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

Lot's of much needed fixes and great catches in this PR. Thanks @XavierM 🙏

I did some light desk testing and ran a subset of the cypress tests. I found one (unrelated to this PR) issue with refreshing the Signals histogram, but there's no reason to hold up this PR for it.

LGTM 🚀

@XavierM
Copy link
Contributor Author

XavierM commented Jan 17, 2020

I wouldn't hold up this PR for it, but FYI i'm not seeing the Signals histogram refreshing when the date picker Update is clicked (on both the Detection engine and Overview page

This is known issue, we will fix it in coming PR

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@XavierM XavierM merged commit 6760c33 into elastic:master Jan 18, 2020
XavierM added a commit to XavierM/kibana that referenced this pull request Jan 18, 2020
* add alert on detections

* review I + fix unit test

* review II

* review III

* review IV + bug fixes found during review

* review VI
XavierM added a commit to XavierM/kibana that referenced this pull request Jan 18, 2020
* add alert on detections

* review I + fix unit test

* review II

* review III

* review IV + bug fixes found during review

* review VI
XavierM added a commit that referenced this pull request Jan 18, 2020
* add alert on detections

* review I + fix unit test

* review II

* review III

* review IV + bug fixes found during review

* review VI
XavierM added a commit that referenced this pull request Jan 18, 2020
* add alert on detections

* review I + fix unit test

* review II

* review III

* review IV + bug fixes found during review

* review VI
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 20, 2020
* upstream/master: (24 commits)
  Show error page when accessing unavailable app (elastic#54656)
  [ML] Improving job wizards with datafeed aggregations (elastic#55180)
  remove flaly assetion. a license presence tested anyway (elastic#55289)
  fix commonly used ranges uptime (elastic#54930)
  [SIEM] Use proper icons on Detections view (elastic#55215)
  Fix: invalid translation referenced (elastic#54901)
  [State Management] Remove AppState from edit_index_pattern page (elastic#54104)
  Implements `getStartServices` on server-side (elastic#55156)
  Move vis_vega_type/data_model tests to jest (elastic#55186)
  [SIEM] [Detection Engine] Update status on rule details page (elastic#55201)
  Fix KQL value suggestions for nested fields (elastic#54820)
  Enforce camelCase format for a plugin id (elastic#53759)
  [SIEM] Detection engine cleanup for rule details/creation/edit page (elastic#55069)
  Remove nested root from index pattern (elastic#54978)
  [Reporting/Migration] ReportingSetup, LegacySetup (elastic#54198)
  [SIEM] [Detection Engine] Fixes duplicate rule action (elastic#55252)
  [SIEM] Detections add alert & signal tab (elastic#55127)
  Management API - redirect on disabled app path (elastic#55136)
  [SIEM][Detection Engine] Fixes critical regression on the backend with immutable and tags
  update local (elastic#55177)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 20, 2020
* master: (108 commits)
  [ML] Single Metric Viewer: Fix job check. (elastic#55191)
  Show error page when accessing unavailable app (elastic#54656)
  [ML] Improving job wizards with datafeed aggregations (elastic#55180)
  remove flaly assetion. a license presence tested anyway (elastic#55289)
  fix commonly used ranges uptime (elastic#54930)
  [SIEM] Use proper icons on Detections view (elastic#55215)
  Fix: invalid translation referenced (elastic#54901)
  [State Management] Remove AppState from edit_index_pattern page (elastic#54104)
  Implements `getStartServices` on server-side (elastic#55156)
  Move vis_vega_type/data_model tests to jest (elastic#55186)
  [SIEM] [Detection Engine] Update status on rule details page (elastic#55201)
  Fix KQL value suggestions for nested fields (elastic#54820)
  Enforce camelCase format for a plugin id (elastic#53759)
  [SIEM] Detection engine cleanup for rule details/creation/edit page (elastic#55069)
  Remove nested root from index pattern (elastic#54978)
  [Reporting/Migration] ReportingSetup, LegacySetup (elastic#54198)
  [SIEM] [Detection Engine] Fixes duplicate rule action (elastic#55252)
  [SIEM] Detections add alert & signal tab (elastic#55127)
  Management API - redirect on disabled app path (elastic#55136)
  [SIEM][Detection Engine] Fixes critical regression on the backend with immutable and tags
  ...
@XavierM XavierM deleted the detections-add-alerts branch June 4, 2020 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:SIEM v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants