[SIEM] Overview page feedback #56261
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrew-goldstein
added
Team:SIEM
v8.0.0
release_note:skip
|
Pinging @elastic/siem (Team:SIEM) |
spong
reviewed
Jan 29, 2020
spong
reviewed
Jan 29, 2020
x-pack/legacy/plugins/siem/public/pages/overview/signals_by_category/index.tsx
Outdated
Show resolved
Hide resolved
spong
reviewed
Jan 29, 2020
x-pack/legacy/plugins/siem/public/pages/overview/events_by_dataset/index.tsx
Outdated
Show resolved
Hide resolved
|
nit: If you have the necessary data available, may want to use the single line placeholder when querying for timelines to avoid content jumping when there are no favorites. Guess this is a super-nit as this should be a rather unlikely scenario 😅
|
spong
approved these changes
Jan 29, 2020
There was a problem hiding this comment.
Checked out/tested locally, and performed code review. Left a couple nits, but overall looks GREAT to me! Thanks for taking care of all these last-minute requested fixes @andrew-goldstein! 🙂
|
Thanks @MichaelMarcialis, @spong, @XavierM, @rylnd @MikePaquette and @dcode for your collaboration on the Overview! 🙏 |
andrew-goldstein
force-pushed
the
overview-feedback
branch
from
4115a87
to
3523954
Compare
|
desk-tested locally in:
I also desk tested locally with:
|
andrew-goldstein
force-pushed
the
overview-feedback
branch
2 times, most recently
from
1c99b92
to
c0de715
Compare
Implements feedback and fixes to the Overview page ### Overview (default theme)  ### Overview (dark theme)  ## Highlights * The new order of widgets is Signals, Alerts, Events, Host Events, Network events, per elastic/siem-team#494 * Changed the default `External alerts count` `Stack by` to `event.module` elastic/siem-team#491 * Added `event.module` to the `Events count` histogram elastic/siem-team#491 * Widget titles will no longer include the currently selected `Stack by option`. The widgets will use the same static title text that appears on the other pages (i.e.. `Signals count`, `External alerts count`, and `Events count`) elastic/siem-team#491 * The `Signals count` includes a `Stack by` that defaults to `signal.rule.threat.tatic.name` * Standardized on a 300px widget height for all histograms in the app (thanks @MichaelMarcialis for paring on this!) * The `Open as duplicate timeline` action is `Recent timelines` is now only shown when hovering over a recent timeline ## Loading States * The `Recent timelines` and `Security news` widgets now use the horizontal bar loading indicator * The `Host events` and `Network events` widgets now use the horizontal bar loading indicator * The `Host events` and `Network events` Showing _n_ events subtitles are now hidden on initial load * The counts in the `Host events` and `Network events` Showing _n_ events subtitles are now hidden on initial load * We no longer hide some histogram subtitles after initial load, to prevent shifting of content when a user makes a `Stack by` selection ## News Feed Error State  * Fixed an issue where the `Security news` header was hidden when an invalid URL is configured * Added a space between the word `via` and the `SIEM advanced settings` link * Removed the capital “N” from "News" in the error message ## Misc Visual Changes * Fixed text truncation of the `Severity` column in the `Detections` page's `Signals` table * Added the “showing” subtitle to the `Signals count` histogram on the Detections page * Increased the `Stack by` histogram selector and the `View signals | alerts | events' buttons from 8 to 24px * Tweaked the border rendering in the Overview `Host Events` and `Network events` widget headers * Added 8px of spacing between the Overview `Host Events` and `Network events` widget accordion headers and their contents * Fixed an issue where the `Host events` and `Networ events` widgets didn't render in ie11 elastic/siem-team#499 ## Non-Visual Fixes * Removed an incorrect usage of `usememo` * Removed the placeholder client-side username query from `x-pack/legacy/plugins/siem/public/components/recent_timelines/index.tsx` * Updated the query of the Overview `Host events` widget to filter by "host.name exists" * Updated the query of the Overview `Network events` widget to filter by "source.ip exists or destination.ip : exists"
andrew-goldstein
force-pushed
the
overview-feedback
branch
from
c0de715
to
0622762
Compare
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-xpack-agent / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/machine_learning/anomaly_detection/advanced_job·ts.machine learning anomaly detection advanced job with categorization detector and default datafeed settings job creation displays details for the created job in the job listStandard OutStack TraceHistory
To update your PR or re-run it, just comment with: |
|
This is the first time I've encountered a
|
This was referenced Jan 29, 2020
andrew-goldstein
added a commit
to andrew-goldstein/kibana
that referenced
this pull request
Jan 29, 2020
## [SIEM] Overview page feedback Implements feedback and fixes to the Overview page ### Overview (default theme)  ### Overview (dark theme)  ## Highlights * The new order of widgets is Signals, Alerts, Events, Host Events, Network events, per elastic/siem-team#494 * Changed the default `External alerts count` `Stack by` to `event.module` elastic/siem-team#491 * Added `event.module` to the `Events count` histogram elastic/siem-team#491 * Widget titles will no longer include the currently selected `Stack by option`. The widgets will use the same static title text that appears on the other pages (i.e.. `Signals count`, `External alerts count`, and `Events count`) elastic/siem-team#491 * The `Signals count` includes a `Stack by` that defaults to `signal.rule.threat.tatic.name` * Standardized on a 300px widget height for all histograms in the app (thanks @MichaelMarcialis for paring on this!) * The `Open as duplicate timeline` action is `Recent timelines` is now only shown when hovering over a recent timeline ## Loading States * The `Recent timelines` and `Security news` widgets now use the horizontal bar loading indicator * The `Host events` and `Network events` widgets now use the horizontal bar loading indicator * The `Host events` and `Network events` Showing _n_ events subtitles are now hidden on initial load * The counts in the `Host events` and `Network events` Showing _n_ events subtitles are now hidden on initial load * We no longer hide some histogram subtitles after initial load, to prevent shifting of content when a user makes a `Stack by` selection ## News Feed Error State  * Fixed an issue where the `Security news` header was hidden when an invalid URL is configured * Added a space between the word `via` and the `SIEM advanced settings` link * Removed the capital “N” from "News" in the error message ## Misc Visual Changes * Fixed text truncation of the `Severity` column in the `Detections` page's `Signals` table * Added the “showing” subtitle to the `Signals count` histogram on the Detections page * Increased the `Stack by` histogram selector and the `View signals | alerts | events' buttons from 8 to 24px * Tweaked the border rendering in the Overview `Host Events` and `Network events` widget headers * Added 8px of spacing between the Overview `Host Events` and `Network events` widget accordion headers and their contents * Fixed an issue where the `Host events` and `Networ events` widgets didn't render in ie11 elastic/siem-team#499 ## Non-Visual Fixes * Removed an incorrect usage of `usememo` * Removed the placeholder client-side username query from `x-pack/legacy/plugins/siem/public/components/recent_timelines/index.tsx` * Updated the query of the Overview `Host events` widget to filter by "host.name exists" * Updated the query of the Overview `Network events` widget to filter by "source.ip exists or destination.ip : exists"
gmmorris
added a commit
to gmmorris/kibana
that referenced
this pull request
Jan 29, 2020
* master: (31 commits) [SIEM] Overview page feedback (elastic#56261) refactor (elastic#56131) [NP Cleanup] Remove ui/public/inspector (elastic#55677) [SIEM] [TIMELINE] Only add endpoint logo when on event.module === endgame (elastic#56263) Basic Functionality Alert List (elastic#55800) [SIEM] Fix filters on Hosts and Network page (elastic#56234) [SIEM] Adds ability to infer the newsfeed.enabled setting (elastic#56236) [SIEM][Detection Engine] critical blocker for updated rules [SIEM][Detection Engine] critical blocker, fixes ordering issue that causes rules to not run the first time [SIEM] Add link to endpoint app through reference.url (elastic#56211) [Metrics UI] Fixing title truncation in Metrics Explorer (elastic#55917) [SIEM] Put the notice for rules in comment block (elastic#56123) [SIEM][Detection Engine] critical blocker with the UI crashing Consistent timeouts for the Space onPostAuth interceptor tests (elastic#56158) Skip tests that depend on other skipped test [SIEM] [Detection Engine] Timestamps for rules (elastic#56197) Sort server-side in SavedObject export (elastic#55128) [Reporting] Document the 8.0 breaking changes (elastic#56187) Revert "[Monitoring] Change all configs to `monitoring.*`" (elastic#56214) add owners for es_archiver (elastic#56184) ...
andrew-goldstein
added a commit
that referenced
this pull request
Jan 29, 2020
## [SIEM] Overview page feedback Implements feedback and fixes to the Overview page ### Overview (default theme)  ### Overview (dark theme)  ## Highlights * The new order of widgets is Signals, Alerts, Events, Host Events, Network events, per elastic/siem-team#494 * Changed the default `External alerts count` `Stack by` to `event.module` elastic/siem-team#491 * Added `event.module` to the `Events count` histogram elastic/siem-team#491 * Widget titles will no longer include the currently selected `Stack by option`. The widgets will use the same static title text that appears on the other pages (i.e.. `Signals count`, `External alerts count`, and `Events count`) elastic/siem-team#491 * The `Signals count` includes a `Stack by` that defaults to `signal.rule.threat.tatic.name` * Standardized on a 300px widget height for all histograms in the app (thanks @MichaelMarcialis for paring on this!) * The `Open as duplicate timeline` action is `Recent timelines` is now only shown when hovering over a recent timeline ## Loading States * The `Recent timelines` and `Security news` widgets now use the horizontal bar loading indicator * The `Host events` and `Network events` widgets now use the horizontal bar loading indicator * The `Host events` and `Network events` Showing _n_ events subtitles are now hidden on initial load * The counts in the `Host events` and `Network events` Showing _n_ events subtitles are now hidden on initial load * We no longer hide some histogram subtitles after initial load, to prevent shifting of content when a user makes a `Stack by` selection ## News Feed Error State  * Fixed an issue where the `Security news` header was hidden when an invalid URL is configured * Added a space between the word `via` and the `SIEM advanced settings` link * Removed the capital “N” from "News" in the error message ## Misc Visual Changes * Fixed text truncation of the `Severity` column in the `Detections` page's `Signals` table * Added the “showing” subtitle to the `Signals count` histogram on the Detections page * Increased the `Stack by` histogram selector and the `View signals | alerts | events' buttons from 8 to 24px * Tweaked the border rendering in the Overview `Host Events` and `Network events` widget headers * Added 8px of spacing between the Overview `Host Events` and `Network events` widget accordion headers and their contents * Fixed an issue where the `Host events` and `Networ events` widgets didn't render in ie11 elastic/siem-team#499 ## Non-Visual Fixes * Removed an incorrect usage of `usememo` * Removed the placeholder client-side username query from `x-pack/legacy/plugins/siem/public/components/recent_timelines/index.tsx` * Updated the query of the Overview `Host events` widget to filter by "host.name exists" * Updated the query of the Overview `Network events` widget to filter by "source.ip exists or destination.ip : exists"
andrew-goldstein
added a commit
that referenced
this pull request
Jan 29, 2020
* [SIEM] Overview page feedback (#56261) ## [SIEM] Overview page feedback Implements feedback and fixes to the Overview page ### Overview (default theme)  ### Overview (dark theme)  ## Highlights * The new order of widgets is Signals, Alerts, Events, Host Events, Network events, per elastic/siem-team#494 * Changed the default `External alerts count` `Stack by` to `event.module` elastic/siem-team#491 * Added `event.module` to the `Events count` histogram elastic/siem-team#491 * Widget titles will no longer include the currently selected `Stack by option`. The widgets will use the same static title text that appears on the other pages (i.e.. `Signals count`, `External alerts count`, and `Events count`) elastic/siem-team#491 * The `Signals count` includes a `Stack by` that defaults to `signal.rule.threat.tatic.name` * Standardized on a 300px widget height for all histograms in the app (thanks @MichaelMarcialis for paring on this!) * The `Open as duplicate timeline` action is `Recent timelines` is now only shown when hovering over a recent timeline ## Loading States * The `Recent timelines` and `Security news` widgets now use the horizontal bar loading indicator * The `Host events` and `Network events` widgets now use the horizontal bar loading indicator * The `Host events` and `Network events` Showing _n_ events subtitles are now hidden on initial load * The counts in the `Host events` and `Network events` Showing _n_ events subtitles are now hidden on initial load * We no longer hide some histogram subtitles after initial load, to prevent shifting of content when a user makes a `Stack by` selection ## News Feed Error State  * Fixed an issue where the `Security news` header was hidden when an invalid URL is configured * Added a space between the word `via` and the `SIEM advanced settings` link * Removed the capital “N” from "News" in the error message ## Misc Visual Changes * Fixed text truncation of the `Severity` column in the `Detections` page's `Signals` table * Added the “showing” subtitle to the `Signals count` histogram on the Detections page * Increased the `Stack by` histogram selector and the `View signals | alerts | events' buttons from 8 to 24px * Tweaked the border rendering in the Overview `Host Events` and `Network events` widget headers * Added 8px of spacing between the Overview `Host Events` and `Network events` widget accordion headers and their contents * Fixed an issue where the `Host events` and `Networ events` widgets didn't render in ie11 elastic/siem-team#499 ## Non-Visual Fixes * Removed an incorrect usage of `usememo` * Removed the placeholder client-side username query from `x-pack/legacy/plugins/siem/public/components/recent_timelines/index.tsx` * Updated the query of the Overview `Host events` widget to filter by "host.name exists" * Updated the query of the Overview `Network events` widget to filter by "source.ip exists or destination.ip : exists" * Removed the following unused translations that were failing the i18n Compatibility Checks: ``` xpack.siem.overview.alertsCountByTitle xpack.siem.overview.eventsCountByTitle xpack.siem.overview.signalsByCategoryTitle ``` The following files were updated: * `x-pack/plugins/translations/translations/zh-CN.json` * `x-pack/plugins/translations/translations/ja-JP.json`
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Jan 29, 2020
…55831 * '7.x' of github.com:elastic/kibana: (78 commits) Re-enable watcher FireFox functional test (elastic#56112) (elastic#56294) [Metrics UI] Fixing title truncation in Metrics Explorer (elastic#55917) (elastic#56248) [APM] x-axis labels on Error occurrences chart are incorrect based on Kibana timezone (elastic#55686) (elastic#56288) Migrate saved_object_save_as_checkbox directive to timelion (elastic#56114) (elastic#56286) [APM] Treat error.exception.stacktrace.line as optional (elastic#55733) (elastic#55840) Remove alerts and actions from feature catalogue (elastic#56140) (elastic#56208) Migrate UI capabilities to use new platform APIs (elastic#56070) (elastic#56207) [ML] Add functional tests for analytics UI: creation addition and regression/outlier results (elastic#56059) (elastic#56191) [SIEM] Overview page feedback (elastic#56261) (elastic#56276) [NP Cleanup] Remove ui/public/inspector (elastic#55677) (elastic#56271) [Index template] Fix editor should support mappings types (elastic#55804) fixes map index message (elastic#56104) (elastic#56194) [SIEM] [TIMELINE] Only add endpoint logo when on event.module === endgame (elastic#56263) (elastic#56269) [SIEM] Fix filters on Hosts and Network page (elastic#56234) (elastic#56267) [SIEM] Adds ability to infer the newsfeed.enabled setting (elastic#56236) (elastic#56265) [SIEM][Detection Engine] critical blocker for updated rules (elastic#56259) [SIEM] Put the notice for rules in comment block (elastic#56123) (elastic#56246) [SIEM][Detection Engine] critical blocker, fixes ordering issue that causes rules to not run the first time (elastic#56256) [Reporting/NP] Migration of Reporting Security dependency (elastic#56046) (elastic#56198) [SIEM] Add link to endpoint app through reference.url (elastic#56211) (elastic#56250) ... # Conflicts: # x-pack/plugins/watcher/public/plugin.ts
rylnd
added a commit
to rylnd/kibana
that referenced
this pull request
Mar 5, 2020
This was left over as a result of elastic#56261
rylnd
added a commit
that referenced
this pull request
Mar 10, 2020
…59438) * Convert our manual throwing of TypeError to a custom Error Throwing a TypeError meant that our manual errors were indistinguishable from, say, trying to invoke a method on undefined. This adds a custom error, BadRequestError, that disambiguates that situation. * Present API Error messages to the user With Core's new HTTP client, an unsuccessful API call will raise an error containing the body of the response it received. In the case of SIEM endpoints, this will include a useful error message with potentially more specificity than e.g. 'Internal Server Error'. This adds a type predicate to check for such errors, and adds a handling case in our errorToToaster handler. If the error does not contain our SIEM-specific message, it will fall through as normal and the general error.message will be displayed in the toaster. * Remove unnecessary use of throwIfNotOk in our client API calls The new HTTP client raises an error on a 4xx or 5xx response, so there should not be a case where throwIfNotOk is actually going to throw an error. The established pattern on the frontend is to catch errors at the call site and handle them appropriately, so I'm mainly just verifying that these are caught where they're used, now. * Move errorToToaster and ToasterError to general location These were living in ML since that's where they originated. However, we have need of it (and already use it) elsewhere. The basic pattern for error handling on the frontend is: 1) API call throws error 2) caller catches error and dispatches a toast throwIfNotOk is meant to convert the error into a useful message in 1). We currently use both errorToToaster and displayErrorToast to display that in a toaster in 2) Now that errorToToaster handles a few different types of errors, and throwIfNotOk is going to be bypassed due to the new client behavior of throwing on error, we're going to start consolidating on: 1) Api call throws error 2) caller catches error and passes it to errorToToaster * Refactor Rules API functions to not use throwIfNotOk * Ensures that all callers of these methods properly catch errors * Updates error toasterification to use errorToToaster * Simplifies tests now that we mainly just invoke the http client and return the result. throwIfNotOk is not being used in the majority of cases, as the client raises an error and bypasses that call. The few cases this might break are where we return a 200 but have errors within the response. Whether throwIfNotOk handled this or not, I'll need a simpler helper to accomplish the same behavior. * Define a type for our BulkRule responses These can be an array of errors OR rules; typing it as such forces downstream to deal with both. enableRules was being handled correctly with the bucketing helper, and TS has confirmed the rest are as well. This obviates the need to raise from our API calls, as bulk errors are recoverable and we want to both a) continue on with any successful rules and b) handle the errors as necessary. This is highly dependent on the caller and so we can't/shouldn't handle it here. * Address case where bulk rules errors were not handled I'm not sure that we're ever using this non-dispatch version, but it was throwing a type error. Will bring it up in review. * Remove more throwIfNotOk uses from API calls These are unneeded as an error response will already throw an error to be handled at the call site. * Display an error toaster on newsfeed fetch failure * Remove dead code This was left over as a result of #56261 * Remove throwIfNotOk from case API calls Again, not needed because the client already throws. * Update use_get_tags for NP * Gets rid of throwIfNotOK usage * uses core http fetch * Remove throwIfNotOk from signals API * Remove throwIfNotOk This served the same purpose as errorToToaster, but in a less robust way. All usages have been replaced, so now we say goodbye. * Remove custom errors in favor of KibanaApiError and isApiError type predicate There was no functional difference between these two code paths, and removing these custom errors allowed us to delete a bunch of associated code as well.. * Fix test failures These were mainly related to my swapping any remaining fetch calls with the core router as good kibana denizens should :salute: * Replace use of core mocks with our simpler local ones This is enough to get our tests to pass. We can't use the core mocks for now since there are circular dependencies there, which breaks our build. * add signal api unit tests * privilege unit test api * Add unit tests on the signals container * Refactor signals API tests to use core mocks * Simplifies our mocking verbosity by leveraging core mocks * Simplifies test setup by isolating a reference to our fetch mock * Abstracts response structure to pure helper functions The try/catch tests had some false positives in that nothing would be asserted if the code did not throw an error. These proved to be masking a gap in coverage for our get/create signal index requests, which do not leverage `throwIfNotOk` but instead rely on the fetch to throw an error; once that behavior is verified we can update those tests to have our fetchMock throw errors, and we should be all set. * Simplify signals API tests now that the subjects do less We no longer re-throw errors, or parse the response, we just return the result of the client call. Simple! * Simplify API functions to use implict returns When possible. Also adds missing error-throwing documentation where necessary. * Revert "Display an error toaster on newsfeed fetch failure" This reverts commit 6421322. * Error property is readonly * Pull uuid generation into default argument value * Fix type predicate isApiError Uses has to properly inspect our errorish object. Turns out we have a 'message' property, not an 'error' property. * Fix test setup following modification of type predicate We need a message (via new Error), a body.message, and a body.status_code to satisfy isApiError. Co-authored-by: Xavier Mouligneau <189600+XavierM@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
rylnd
added a commit
that referenced
this pull request
Mar 10, 2020
…59438) (#59757) * Convert our manual throwing of TypeError to a custom Error Throwing a TypeError meant that our manual errors were indistinguishable from, say, trying to invoke a method on undefined. This adds a custom error, BadRequestError, that disambiguates that situation. * Present API Error messages to the user With Core's new HTTP client, an unsuccessful API call will raise an error containing the body of the response it received. In the case of SIEM endpoints, this will include a useful error message with potentially more specificity than e.g. 'Internal Server Error'. This adds a type predicate to check for such errors, and adds a handling case in our errorToToaster handler. If the error does not contain our SIEM-specific message, it will fall through as normal and the general error.message will be displayed in the toaster. * Remove unnecessary use of throwIfNotOk in our client API calls The new HTTP client raises an error on a 4xx or 5xx response, so there should not be a case where throwIfNotOk is actually going to throw an error. The established pattern on the frontend is to catch errors at the call site and handle them appropriately, so I'm mainly just verifying that these are caught where they're used, now. * Move errorToToaster and ToasterError to general location These were living in ML since that's where they originated. However, we have need of it (and already use it) elsewhere. The basic pattern for error handling on the frontend is: 1) API call throws error 2) caller catches error and dispatches a toast throwIfNotOk is meant to convert the error into a useful message in 1). We currently use both errorToToaster and displayErrorToast to display that in a toaster in 2) Now that errorToToaster handles a few different types of errors, and throwIfNotOk is going to be bypassed due to the new client behavior of throwing on error, we're going to start consolidating on: 1) Api call throws error 2) caller catches error and passes it to errorToToaster * Refactor Rules API functions to not use throwIfNotOk * Ensures that all callers of these methods properly catch errors * Updates error toasterification to use errorToToaster * Simplifies tests now that we mainly just invoke the http client and return the result. throwIfNotOk is not being used in the majority of cases, as the client raises an error and bypasses that call. The few cases this might break are where we return a 200 but have errors within the response. Whether throwIfNotOk handled this or not, I'll need a simpler helper to accomplish the same behavior. * Define a type for our BulkRule responses These can be an array of errors OR rules; typing it as such forces downstream to deal with both. enableRules was being handled correctly with the bucketing helper, and TS has confirmed the rest are as well. This obviates the need to raise from our API calls, as bulk errors are recoverable and we want to both a) continue on with any successful rules and b) handle the errors as necessary. This is highly dependent on the caller and so we can't/shouldn't handle it here. * Address case where bulk rules errors were not handled I'm not sure that we're ever using this non-dispatch version, but it was throwing a type error. Will bring it up in review. * Remove more throwIfNotOk uses from API calls These are unneeded as an error response will already throw an error to be handled at the call site. * Display an error toaster on newsfeed fetch failure * Remove dead code This was left over as a result of #56261 * Remove throwIfNotOk from case API calls Again, not needed because the client already throws. * Update use_get_tags for NP * Gets rid of throwIfNotOK usage * uses core http fetch * Remove throwIfNotOk from signals API * Remove throwIfNotOk This served the same purpose as errorToToaster, but in a less robust way. All usages have been replaced, so now we say goodbye. * Remove custom errors in favor of KibanaApiError and isApiError type predicate There was no functional difference between these two code paths, and removing these custom errors allowed us to delete a bunch of associated code as well.. * Fix test failures These were mainly related to my swapping any remaining fetch calls with the core router as good kibana denizens should :salute: * Replace use of core mocks with our simpler local ones This is enough to get our tests to pass. We can't use the core mocks for now since there are circular dependencies there, which breaks our build. * add signal api unit tests * privilege unit test api * Add unit tests on the signals container * Refactor signals API tests to use core mocks * Simplifies our mocking verbosity by leveraging core mocks * Simplifies test setup by isolating a reference to our fetch mock * Abstracts response structure to pure helper functions The try/catch tests had some false positives in that nothing would be asserted if the code did not throw an error. These proved to be masking a gap in coverage for our get/create signal index requests, which do not leverage `throwIfNotOk` but instead rely on the fetch to throw an error; once that behavior is verified we can update those tests to have our fetchMock throw errors, and we should be all set. * Simplify signals API tests now that the subjects do less We no longer re-throw errors, or parse the response, we just return the result of the client call. Simple! * Simplify API functions to use implict returns When possible. Also adds missing error-throwing documentation where necessary. * Revert "Display an error toaster on newsfeed fetch failure" This reverts commit 6421322. * Error property is readonly * Pull uuid generation into default argument value * Fix type predicate isApiError Uses has to properly inspect our errorish object. Turns out we have a 'message' property, not an 'error' property. * Fix test setup following modification of type predicate We need a message (via new Error), a body.message, and a body.status_code to satisfy isApiError. Co-authored-by: Xavier Mouligneau <189600+XavierM@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Xavier Mouligneau <189600+XavierM@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
jkelastic
pushed a commit
to jkelastic/kibana
that referenced
this pull request
Mar 12, 2020
…lastic#59438) * Convert our manual throwing of TypeError to a custom Error Throwing a TypeError meant that our manual errors were indistinguishable from, say, trying to invoke a method on undefined. This adds a custom error, BadRequestError, that disambiguates that situation. * Present API Error messages to the user With Core's new HTTP client, an unsuccessful API call will raise an error containing the body of the response it received. In the case of SIEM endpoints, this will include a useful error message with potentially more specificity than e.g. 'Internal Server Error'. This adds a type predicate to check for such errors, and adds a handling case in our errorToToaster handler. If the error does not contain our SIEM-specific message, it will fall through as normal and the general error.message will be displayed in the toaster. * Remove unnecessary use of throwIfNotOk in our client API calls The new HTTP client raises an error on a 4xx or 5xx response, so there should not be a case where throwIfNotOk is actually going to throw an error. The established pattern on the frontend is to catch errors at the call site and handle them appropriately, so I'm mainly just verifying that these are caught where they're used, now. * Move errorToToaster and ToasterError to general location These were living in ML since that's where they originated. However, we have need of it (and already use it) elsewhere. The basic pattern for error handling on the frontend is: 1) API call throws error 2) caller catches error and dispatches a toast throwIfNotOk is meant to convert the error into a useful message in 1). We currently use both errorToToaster and displayErrorToast to display that in a toaster in 2) Now that errorToToaster handles a few different types of errors, and throwIfNotOk is going to be bypassed due to the new client behavior of throwing on error, we're going to start consolidating on: 1) Api call throws error 2) caller catches error and passes it to errorToToaster * Refactor Rules API functions to not use throwIfNotOk * Ensures that all callers of these methods properly catch errors * Updates error toasterification to use errorToToaster * Simplifies tests now that we mainly just invoke the http client and return the result. throwIfNotOk is not being used in the majority of cases, as the client raises an error and bypasses that call. The few cases this might break are where we return a 200 but have errors within the response. Whether throwIfNotOk handled this or not, I'll need a simpler helper to accomplish the same behavior. * Define a type for our BulkRule responses These can be an array of errors OR rules; typing it as such forces downstream to deal with both. enableRules was being handled correctly with the bucketing helper, and TS has confirmed the rest are as well. This obviates the need to raise from our API calls, as bulk errors are recoverable and we want to both a) continue on with any successful rules and b) handle the errors as necessary. This is highly dependent on the caller and so we can't/shouldn't handle it here. * Address case where bulk rules errors were not handled I'm not sure that we're ever using this non-dispatch version, but it was throwing a type error. Will bring it up in review. * Remove more throwIfNotOk uses from API calls These are unneeded as an error response will already throw an error to be handled at the call site. * Display an error toaster on newsfeed fetch failure * Remove dead code This was left over as a result of elastic#56261 * Remove throwIfNotOk from case API calls Again, not needed because the client already throws. * Update use_get_tags for NP * Gets rid of throwIfNotOK usage * uses core http fetch * Remove throwIfNotOk from signals API * Remove throwIfNotOk This served the same purpose as errorToToaster, but in a less robust way. All usages have been replaced, so now we say goodbye. * Remove custom errors in favor of KibanaApiError and isApiError type predicate There was no functional difference between these two code paths, and removing these custom errors allowed us to delete a bunch of associated code as well.. * Fix test failures These were mainly related to my swapping any remaining fetch calls with the core router as good kibana denizens should :salute: * Replace use of core mocks with our simpler local ones This is enough to get our tests to pass. We can't use the core mocks for now since there are circular dependencies there, which breaks our build. * add signal api unit tests * privilege unit test api * Add unit tests on the signals container * Refactor signals API tests to use core mocks * Simplifies our mocking verbosity by leveraging core mocks * Simplifies test setup by isolating a reference to our fetch mock * Abstracts response structure to pure helper functions The try/catch tests had some false positives in that nothing would be asserted if the code did not throw an error. These proved to be masking a gap in coverage for our get/create signal index requests, which do not leverage `throwIfNotOk` but instead rely on the fetch to throw an error; once that behavior is verified we can update those tests to have our fetchMock throw errors, and we should be all set. * Simplify signals API tests now that the subjects do less We no longer re-throw errors, or parse the response, we just return the result of the client call. Simple! * Simplify API functions to use implict returns When possible. Also adds missing error-throwing documentation where necessary. * Revert "Display an error toaster on newsfeed fetch failure" This reverts commit 6421322. * Error property is readonly * Pull uuid generation into default argument value * Fix type predicate isApiError Uses has to properly inspect our errorish object. Turns out we have a 'message' property, not an 'error' property. * Fix test setup following modification of type predicate We need a message (via new Error), a body.message, and a body.status_code to satisfy isApiError. Co-authored-by: Xavier Mouligneau <189600+XavierM@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[SIEM] Overview page feedback
Implements feedback and fixes to the Overview page
Overview (default theme)
Overview (dark theme)
Highlights
The new order of widgets is Signals, Alerts, Events, Host Events, Network events, per https://github.com/elastic/siem-team/issues/494
Changed the default
External alerts countStack bytoevent.modulehttps://github.com/elastic/siem-team/issues/491Added
event.moduleto theEvents counthistogram https://github.com/elastic/siem-team/issues/491Widget titles will no longer include the currently selected
Stack by option. The widgets will use the same static title text that appears on the other pages (i.e..Signals count,External alerts count, andEvents count) https://github.com/elastic/siem-team/issues/491The
Signals countincludes aStack bythat defaults tosignal.rule.threat.tatic.nameStandardized on a 300px widget height for all histograms in the app (thanks @MichaelMarcialis for paring on this!)
The
Open as duplicate timelineaction isRecent timelinesis now only shown when hovering over a recent timelineLoading States
The
Recent timelinesandSecurity newswidgets now use the horizontal bar loading indicatorThe
Host eventsandNetwork eventswidgets now use the horizontal bar loading indicatorThe
Host eventsandNetwork eventsShowing n events subtitles are now hidden on initial loadThe counts in the
Host eventsandNetwork eventsShowing n events subtitles are now hidden on initial loadWe no longer hide some histogram subtitles after initial load, to prevent shifting of content when a user makes a
Stack byselectionNews Feed Error State
Fixed an issue where the
Security newsheader was hidden when an invalid URL is configuredAdded a space between the word
viaand theSIEM advanced settingslinkRemoved the capital “N” from "News" in the error message
Misc Visual Changes
Fixed text truncation of the
Severitycolumn in theDetectionspage'sSignalstableAdded the “showing” subtitle to the
Signals counthistogram on the Detections pageIncreased the
Stack byhistogram selector and the `View signals | alerts | events' buttons from 8 to 24pxTweaked the border rendering in the Overview
Host EventsandNetwork eventswidget headersAdded 8px of spacing between the Overview
Host EventsandNetwork eventswidget accordion headers and their contentsFixed an issue where the
Host eventsandNetwor eventswidgets didn't render in ie11 https://github.com/elastic/siem-team/issues/499Non-Visual Fixes
Removed an incorrect usage of
usememoRemoved the placeholder client-side username query from
x-pack/legacy/plugins/siem/public/components/recent_timelines/index.tsxUpdated the query of the Overview
Host eventswidget to filter by "host.name exists"Updated the query of the Overview
Network eventswidget to filter by "source.ip exists or destination.ip : exists"