[Security Solutions] Critical bug to add network responses to error toasters #97945

FrankHassanabad · 2021-04-22T01:24:02Z

Summary

When we updated our codebase to use the newer bsearch/async search, we ended up using error messages which were not the same as we had before which would show the network errors in the full message when the "see full error button" was clicked.

This has been bad lately as users and quality assurance people have been posting screen shots that have very little information, blank error messages, and/or stack traces instead of network errors. This makes them think the code has issues when it is a configuration issue or a networking issue that is happening.

This PR does the following:

Changes all the bsearch queries to use the use useAppToasts
Modifies the useAppToasts to be able to transform bsearch with the kibana global notification
Cleans up the useAppToasts some
Deprecates the GlobalErrorToaster in favor of the useAppToasts
Fixes and adds a few i18n missing strings found
Removes most of the deprecated error dispatch toasters from detection_engine except for 1 place where it is not a hook.

Before screen shot of errors with no buttons and messages that were not pointing to network errors:

After screen shot where you have a button and that button will show you the network error:

You can manually test this easily by making non ECS indexes to cause errors and then add them as a kibana index and use them in the data sourcer.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

…n favor the use hook along with tests

elasticmachine · 2021-04-22T03:14:18Z

Pinging @elastic/security-detections-response (Team:Detections and Resp)

FrankHassanabad · 2021-04-22T03:22:15Z

x-pack/plugins/security_solution/public/common/hooks/use_app_toasts.ts

+        const err = esErrorToRequestError(error);
+        return addError(err, options);
+      } else if (isAppError(error)) {
+        return addError(error, options);


One note @rylnd is that I chose to change this to use the default of error.message which is the toaster default to use if it is not given an explicit toastMessage.

I don't know if there was a reason to prefer using the error.body.message when it is an AppError for example that I'm not seeing or what the difference is?

@FrankHassanabad the old test seemed to indicate that body.message contained a more detailed message. Had you seen anything for/against that assertion?

e.g. the message field was just a restatement of the status code, while body.message contained an actual reason.

yctercero · 2021-04-22T03:48:02Z

x-pack/plugins/security_solution/public/common/containers/events/last_event_time/index.ts

@@ -96,15 +98,13 @@ export const useTimelineLastEventTime = ({
                }));
              } else if (isErrorResponse(response)) {
                setLoading(false);
-                // TODO: Make response error status clearer
-                notifications.toasts.addWarning(i18n.ERROR_LAST_EVENT_TIME);
+                addWarning(i18n.ERROR_LAST_EVENT_TIME);
              }
            },
            error: (msg) => {


We're still not able to type this are we?

If I understand what you're saying correct, we are not able to ingest a partial error/response at this moment which is correct.

addWarning(i18n.ERROR_LAST_EVENT_TIME);

That warning isn't going to be great. However, I also haven't seen that much if any at all and couldn't trip it during testing. I really don't know if/when it gets tripped to be totally honest. I think if we see that happening or we dig into it, we should make a follow up PR where we can add the response into the warning object and then push it onto the toaster if possible:

addWarning(response, i18n.ERROR_LAST_EVENT_TIME);

But won't be soon :-) (at least from myself)

yctercero

This is really huge. I checked out and tested on preview, hosts, and a few others. My only super duper minor comment would be maybe we could just add a use_app_toats.mock.ts as some itty bitty cleanup 😄 This is awesome, thanks Frank.

kibanamachine · 2021-04-22T05:16:24Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	2151	2153	+2

Async chunks

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

id	before	after	diff
`securitySolution`	6.9MB	6.9MB	+1.4KB

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

cc @FrankHassanabad

FrankHassanabad · 2021-04-22T14:52:08Z

@yara this is a mock:
x-pack/plugins/security_solution/public/common/hooks/use_app_toasts.mock.ts

I use it in a few areas and then in some areas, I use the existing ways of mocking things.

…oasters (elastic#97945) ## Summary When we updated our codebase to use the newer bsearch/async search, we ended up using error messages which were not the same as we had before which would show the network errors in the full message when the "see full error button" was clicked. This has been bad lately as users and quality assurance people have been posting screen shots that have very little information, blank error messages, and/or stack traces instead of network errors. This makes them think the code has issues when it is a configuration issue or a networking issue that is happening. This PR does the following: * Changes all the bsearch queries to use the use useAppToasts * Modifies the useAppToasts to be able to transform bsearch with the kibana global notification * Cleans up the useAppToasts some * Deprecates the GlobalErrorToaster in favor of the useAppToasts * Fixes and adds a few i18n missing strings found * Removes most of the deprecated error dispatch toasters from detection_engine except for 1 place where it is not a hook. Before screen shot of errors with no buttons and messages that were not pointing to network errors: <img width="422" alt="Screen Shot 2021-04-21 at 4 24 45 PM" src="https://user-images.githubusercontent.com/1151048/115642119-9a942300-a2d7-11eb-955a-b51097a8d3ec.png"> After screen shot where you have a button and that button will show you the network error: <img width="395" alt="Screen Shot 2021-04-21 at 3 26 12 PM" src="https://user-images.githubusercontent.com/1151048/115642216-c6170d80-a2d7-11eb-8c94-0a1c15186fab.png"> <img width="786" alt="Screen Shot 2021-04-21 at 3 26 21 PM" src="https://user-images.githubusercontent.com/1151048/115642222-ca432b00-a2d7-11eb-9ddd-4ed6c6270b94.png"> You can manually test this easily by making non ECS indexes to cause errors and then add them as a kibana index and use them in the data sourcer. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

kibanamachine · 2021-04-22T15:00:12Z

💚 Backport successful

Status	Branch	Result
✅	7.13

This backport PR will be merged automatically after passing CI.

rylnd · 2021-04-22T17:00:29Z

x-pack/plugins/security_solution/public/common/hooks/use_app_toasts.test.ts

@@ -37,31 +38,36 @@ describe('useDeleteList', () => {
    expect(addErrorMock).toHaveBeenCalledWith(error, { title: 'title' });
  });

-  it("uses a AppError's body.message as the toastMessage", async () => {
-    const kibanaApiError = {


This kibanaApiError was a representative error type that we intended to handle, so it's surprising to see support for it dropped here. Did we verify that this is no longer an error that we encounter/support?

…oasters (#97945) (#98029) ## Summary When we updated our codebase to use the newer bsearch/async search, we ended up using error messages which were not the same as we had before which would show the network errors in the full message when the "see full error button" was clicked. This has been bad lately as users and quality assurance people have been posting screen shots that have very little information, blank error messages, and/or stack traces instead of network errors. This makes them think the code has issues when it is a configuration issue or a networking issue that is happening. This PR does the following: * Changes all the bsearch queries to use the use useAppToasts * Modifies the useAppToasts to be able to transform bsearch with the kibana global notification * Cleans up the useAppToasts some * Deprecates the GlobalErrorToaster in favor of the useAppToasts * Fixes and adds a few i18n missing strings found * Removes most of the deprecated error dispatch toasters from detection_engine except for 1 place where it is not a hook. Before screen shot of errors with no buttons and messages that were not pointing to network errors: <img width="422" alt="Screen Shot 2021-04-21 at 4 24 45 PM" src="https://user-images.githubusercontent.com/1151048/115642119-9a942300-a2d7-11eb-955a-b51097a8d3ec.png"> After screen shot where you have a button and that button will show you the network error: <img width="395" alt="Screen Shot 2021-04-21 at 3 26 12 PM" src="https://user-images.githubusercontent.com/1151048/115642216-c6170d80-a2d7-11eb-8c94-0a1c15186fab.png"> <img width="786" alt="Screen Shot 2021-04-21 at 3 26 21 PM" src="https://user-images.githubusercontent.com/1151048/115642222-ca432b00-a2d7-11eb-9ddd-4ed6c6270b94.png"> You can manually test this easily by making non ECS indexes to cause errors and then add them as a kibana index and use them in the data sourcer. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios Co-authored-by: Frank Hassanabad <[email protected]>

…rs and error messages (#98220) ## Summary Follow up from feedback and testing of: #97945 This fixes a bug found with the older toaster notifications that can cause a screen stack trace: <img width="1520" alt="Screen Shot 2021-04-22 at 5 32 57 PM" src="https://user-images.githubusercontent.com/1151048/115929514-2d9e9b80-a445-11eb-8017-eb21d800c472.png"> Adds more information and robust error handling when the messages are Kibana/NodeJS error messages. Before: <img width="808" alt="Screen Shot 2021-04-22 at 5 37 37 PM" src="https://user-images.githubusercontent.com/1151048/115929643-5de63a00-a445-11eb-93bc-5a826f371ef3.png"> After: <img width="789" alt="Screen Shot 2021-04-22 at 6 23 52 PM" src="https://user-images.githubusercontent.com/1151048/115929651-62aaee00-a445-11eb-9c03-d56b488d238b.png"> ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

…rs and error messages (elastic#98220) ## Summary Follow up from feedback and testing of: elastic#97945 This fixes a bug found with the older toaster notifications that can cause a screen stack trace: <img width="1520" alt="Screen Shot 2021-04-22 at 5 32 57 PM" src="https://user-images.githubusercontent.com/1151048/115929514-2d9e9b80-a445-11eb-8017-eb21d800c472.png"> Adds more information and robust error handling when the messages are Kibana/NodeJS error messages. Before: <img width="808" alt="Screen Shot 2021-04-22 at 5 37 37 PM" src="https://user-images.githubusercontent.com/1151048/115929643-5de63a00-a445-11eb-93bc-5a826f371ef3.png"> After: <img width="789" alt="Screen Shot 2021-04-22 at 6 23 52 PM" src="https://user-images.githubusercontent.com/1151048/115929651-62aaee00-a445-11eb-9c03-d56b488d238b.png"> ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

…oasters (elastic#97945) ## Summary When we updated our codebase to use the newer bsearch/async search, we ended up using error messages which were not the same as we had before which would show the network errors in the full message when the "see full error button" was clicked. This has been bad lately as users and quality assurance people have been posting screen shots that have very little information, blank error messages, and/or stack traces instead of network errors. This makes them think the code has issues when it is a configuration issue or a networking issue that is happening. This PR does the following: * Changes all the bsearch queries to use the use useAppToasts * Modifies the useAppToasts to be able to transform bsearch with the kibana global notification * Cleans up the useAppToasts some * Deprecates the GlobalErrorToaster in favor of the useAppToasts * Fixes and adds a few i18n missing strings found * Removes most of the deprecated error dispatch toasters from detection_engine except for 1 place where it is not a hook. Before screen shot of errors with no buttons and messages that were not pointing to network errors: <img width="422" alt="Screen Shot 2021-04-21 at 4 24 45 PM" src="https://user-images.githubusercontent.com/1151048/115642119-9a942300-a2d7-11eb-955a-b51097a8d3ec.png"> After screen shot where you have a button and that button will show you the network error: <img width="395" alt="Screen Shot 2021-04-21 at 3 26 12 PM" src="https://user-images.githubusercontent.com/1151048/115642216-c6170d80-a2d7-11eb-8c94-0a1c15186fab.png"> <img width="786" alt="Screen Shot 2021-04-21 at 3 26 21 PM" src="https://user-images.githubusercontent.com/1151048/115642222-ca432b00-a2d7-11eb-9ddd-4ed6c6270b94.png"> You can manually test this easily by making non ECS indexes to cause errors and then add them as a kibana index and use them in the data sourcer. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

…rs and error messages (#98220) (#98429) ## Summary Follow up from feedback and testing of: #97945 This fixes a bug found with the older toaster notifications that can cause a screen stack trace: <img width="1520" alt="Screen Shot 2021-04-22 at 5 32 57 PM" src="https://user-images.githubusercontent.com/1151048/115929514-2d9e9b80-a445-11eb-8017-eb21d800c472.png"> Adds more information and robust error handling when the messages are Kibana/NodeJS error messages. Before: <img width="808" alt="Screen Shot 2021-04-22 at 5 37 37 PM" src="https://user-images.githubusercontent.com/1151048/115929643-5de63a00-a445-11eb-93bc-5a826f371ef3.png"> After: <img width="789" alt="Screen Shot 2021-04-22 at 6 23 52 PM" src="https://user-images.githubusercontent.com/1151048/115929651-62aaee00-a445-11eb-9c03-d56b488d238b.png"> ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios Co-authored-by: Frank Hassanabad <[email protected]>

…oasters (#97945) (#98430) ## Summary When we updated our codebase to use the newer bsearch/async search, we ended up using error messages which were not the same as we had before which would show the network errors in the full message when the "see full error button" was clicked. This has been bad lately as users and quality assurance people have been posting screen shots that have very little information, blank error messages, and/or stack traces instead of network errors. This makes them think the code has issues when it is a configuration issue or a networking issue that is happening. This PR does the following: * Changes all the bsearch queries to use the use useAppToasts * Modifies the useAppToasts to be able to transform bsearch with the kibana global notification * Cleans up the useAppToasts some * Deprecates the GlobalErrorToaster in favor of the useAppToasts * Fixes and adds a few i18n missing strings found * Removes most of the deprecated error dispatch toasters from detection_engine except for 1 place where it is not a hook. Before screen shot of errors with no buttons and messages that were not pointing to network errors: <img width="422" alt="Screen Shot 2021-04-21 at 4 24 45 PM" src="https://user-images.githubusercontent.com/1151048/115642119-9a942300-a2d7-11eb-955a-b51097a8d3ec.png"> After screen shot where you have a button and that button will show you the network error: <img width="395" alt="Screen Shot 2021-04-21 at 3 26 12 PM" src="https://user-images.githubusercontent.com/1151048/115642216-c6170d80-a2d7-11eb-8c94-0a1c15186fab.png"> <img width="786" alt="Screen Shot 2021-04-21 at 3 26 21 PM" src="https://user-images.githubusercontent.com/1151048/115642222-ca432b00-a2d7-11eb-9ddd-4ed6c6270b94.png"> You can manually test this easily by making non ECS indexes to cause errors and then add them as a kibana index and use them in the data sourcer. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

…rs and error messages (elastic#98220) ## Summary Follow up from feedback and testing of: elastic#97945 This fixes a bug found with the older toaster notifications that can cause a screen stack trace: <img width="1520" alt="Screen Shot 2021-04-22 at 5 32 57 PM" src="https://user-images.githubusercontent.com/1151048/115929514-2d9e9b80-a445-11eb-8017-eb21d800c472.png"> Adds more information and robust error handling when the messages are Kibana/NodeJS error messages. Before: <img width="808" alt="Screen Shot 2021-04-22 at 5 37 37 PM" src="https://user-images.githubusercontent.com/1151048/115929643-5de63a00-a445-11eb-93bc-5a826f371ef3.png"> After: <img width="789" alt="Screen Shot 2021-04-22 at 6 23 52 PM" src="https://user-images.githubusercontent.com/1151048/115929651-62aaee00-a445-11eb-9c03-d56b488d238b.png"> ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

…rs and error messages (#98220) (#98435) ## Summary Follow up from feedback and testing of: #97945 This fixes a bug found with the older toaster notifications that can cause a screen stack trace: <img width="1520" alt="Screen Shot 2021-04-22 at 5 32 57 PM" src="https://user-images.githubusercontent.com/1151048/115929514-2d9e9b80-a445-11eb-8017-eb21d800c472.png"> Adds more information and robust error handling when the messages are Kibana/NodeJS error messages. Before: <img width="808" alt="Screen Shot 2021-04-22 at 5 37 37 PM" src="https://user-images.githubusercontent.com/1151048/115929643-5de63a00-a445-11eb-93bc-5a826f371ef3.png"> After: <img width="789" alt="Screen Shot 2021-04-22 at 6 23 52 PM" src="https://user-images.githubusercontent.com/1151048/115929651-62aaee00-a445-11eb-9c03-d56b488d238b.png"> ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

FrankHassanabad added 3 commits April 21, 2021 17:38

Inital changes created

f9057ce

Merge branch 'master' into add-better-error-handling

6896820

Fixes the unit tests

e143df8

FrankHassanabad self-assigned this Apr 22, 2021

FrankHassanabad added the v8.0.0 label Apr 22, 2021

FrankHassanabad changed the title ~~[Security Solutions] Adds network responses to our error messaging~~ [Security Solutions] Critical bug where we do not have network responses for our error messaging Apr 22, 2021

FrankHassanabad added v7.13.0 Team:Detections and Resp Security Detection Response Team release_note:fix labels Apr 22, 2021

Fixes TODO's and adds test

2a2ecf3

FrankHassanabad changed the title ~~[Security Solutions] Critical bug where we do not have network responses for our error messaging~~ [Security Solutions] Critical bug to add network responses to error toasters Apr 22, 2021

Updates the detection engine to where most deprecations are removed i…

2b10a22

…n favor the use hook along with tests

FrankHassanabad marked this pull request as ready for review April 22, 2021 03:14

FrankHassanabad requested a review from a team as a code owner April 22, 2021 03:14

FrankHassanabad assigned rylnd, yctercero and spong and unassigned rylnd, spong and yctercero Apr 22, 2021

FrankHassanabad requested review from rylnd, spong and yctercero April 22, 2021 03:15

FrankHassanabad commented Apr 22, 2021

View reviewed changes

yctercero reviewed Apr 22, 2021

View reviewed changes

yctercero approved these changes Apr 22, 2021

View reviewed changes

FrankHassanabad added the auto-backport Deprecated - use backport:version if exact versions are needed label Apr 22, 2021

FrankHassanabad merged commit 2f679e6 into elastic:master Apr 22, 2021

FrankHassanabad deleted the add-better-error-handling branch April 22, 2021 14:57

kibanamachine mentioned this pull request Apr 22, 2021

[7.13] [Security Solutions] Critical bug to add network responses to error toasters (#97945) #98029

Merged

rylnd reviewed Apr 22, 2021

View reviewed changes

FrankHassanabad mentioned this pull request Apr 23, 2021

[Security Solutions] Follow up of critical bug fixes for error toasters and error messages #98220

Merged

1 task

FrankHassanabad mentioned this pull request Apr 26, 2021

[7.x] [Security Solutions] Critical bug to add network responses to error toasters (#97945) #98430

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solutions] Critical bug to add network responses to error toasters #97945

[Security Solutions] Critical bug to add network responses to error toasters #97945

FrankHassanabad commented Apr 22, 2021 •

edited

Loading

elasticmachine commented Apr 22, 2021

FrankHassanabad Apr 22, 2021 •

edited

Loading

rylnd Apr 22, 2021

rylnd Apr 22, 2021

yctercero Apr 22, 2021

FrankHassanabad Apr 22, 2021

yctercero left a comment

kibanamachine commented Apr 22, 2021

FrankHassanabad commented Apr 22, 2021

kibanamachine commented Apr 22, 2021

rylnd Apr 22, 2021

[Security Solutions] Critical bug to add network responses to error toasters #97945

[Security Solutions] Critical bug to add network responses to error toasters #97945

Conversation

FrankHassanabad commented Apr 22, 2021 • edited Loading

Summary

Checklist

elasticmachine commented Apr 22, 2021

FrankHassanabad Apr 22, 2021 • edited Loading

Choose a reason for hiding this comment

rylnd Apr 22, 2021

Choose a reason for hiding this comment

rylnd Apr 22, 2021

Choose a reason for hiding this comment

yctercero Apr 22, 2021

Choose a reason for hiding this comment

FrankHassanabad Apr 22, 2021

Choose a reason for hiding this comment

yctercero left a comment

Choose a reason for hiding this comment

kibanamachine commented Apr 22, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

FrankHassanabad commented Apr 22, 2021

kibanamachine commented Apr 22, 2021

💚 Backport successful

rylnd Apr 22, 2021

Choose a reason for hiding this comment

FrankHassanabad commented Apr 22, 2021 •

edited

Loading

FrankHassanabad Apr 22, 2021 •

edited

Loading