[Enterprise Search] Update enterpriseSearchRequestHandler to manage range of errors + add handleAPIErrors helper #77258

cee-chen · 2020-09-11T01:29:03Z

Summary

@JasonStoltz @scottybollinger - this PR addresses the issues I ran into passing back error messages from our internal API endpoints. CC @byronhulcher, since I'm essentially porting your handleAPIError helper here :)

TL;DR of changes by commit:

e57009c - Update enterpriseSearchRequestHandler to manage range of errors
(I recommend filtering out whitespace changes in this diff if possible, I reorganized/refactored the tests a good amount)
This commit attempts to address the issues I mentioned in Slack today (blank bodies, potential HTML errors, etc.) while correctly passing back JSON errors to the front-end.
I ended up trying to clean up / refactor how we handled various error responses to make the main body of createRequest more legible - not sure if I accomplished that, let me know if you like it or don't like it.

Example screenshot of working 404 response with errors array (ignore the engines URL, I was just hackily using that endpoint to hit the 404ing curations endpoint):

0e1f2eb - this was a whoops on my part when writing our original http interceptor logic. If I don't reject the promise correctly, subsequent http catches don't work. My bad!
2b949c5 - Adds a handleAPIError helper
This modifies the ent-search helper for our Kibana codebase where 1. we're preferring try/catch and async/await over promise chains, and 2. FlashMessagesLogic maintains its own internal state. I'm hoping we find the end result cleaner and more declarative than before (no need for currying/a callback, the handler manages setFlashMessages for you!) - let me know your thoughts 😄

Example screenshot of flash message set via catch (e) { handleAPIError(e) }:

QA

This one's a little hard since I was kind of "speculatively developing" / didn't have a real view or use case to aggressively test against. You can check out my constance/test-api-errors branch and visit the App Search & Workplace Search homes to see some example flash messages however!

I imagine we'll continue to tweak both our request handler and the handleAPIErrors to fit more future use cases - this is essentially a very basic/bare-bones implementation / migration for now.

Checklist

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

…point error behavior + pull out / refactor all errors into their own methods instead of relying on a single 'error connecting' message

- New and improved from ent-search - this one automatically sets flash messages for you so you don't have to pass in a callback!

cee-chen · 2020-09-11T01:30:57Z

x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.ts

+  attributes: {
+    errors: string[];
+  };


FYI: The attributes key comes from Kibana's response factory and as far as I could tell was the only way to pass custom data/objs through the response body. I tried setting other keys like data or just errors but they didn't come through on the front end, so I'm pretty sure only attributes works.

cee-chen · 2020-09-11T01:34:07Z

x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.ts

+        // Some of our internal endpoints return either an `error` or `errors` key,
+        // which can both return either a string or array of strings ¯\_(ツ)_/¯
+        const errors = json.error || json.errors || [statusText];
+        body = {
+          message: errors.toString(),
+          attributes: { errors: Array.isArray(errors) ? errors : [errors] },
+        };


This is essentially me handling/copying over this part of our previous handleAPIError logic. It's sorta nice having this layer in-between Enterprise Search and the front-end, so that now in the front-end going forward we know for sure we can always expect an errors key that is an array.

cee-chen · 2020-09-11T01:38:23Z

x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.ts

+        // which can both return either a string or array of strings ¯\_(ツ)_/¯
+        const errors = json.error || json.errors || [statusText];
+        body = {
+          message: errors.toString(),


This is kind of a YOLO, if we actually get an array of errors from an endpoint (although I'm not sure which ones actually send back more than 1 error? does anyone know?), it'll output ['one', 'two', 'three'] => message: 'one,two,three' which is a bit smushed together.

We're not really using the message key though, so maybe it's not a big deal 🤷 It's there b/c it's required by Kibana's response factory, but we can definitely make it a bit fancier if we need to in the future!

cee-chen · 2020-09-11T01:41:25Z

x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.ts

+      'Enterprise Search encountered an internal server error. Please contact your system administrator if the problem persists.';
+
+    this.log.error(`Enterprise Search Server Error ${status} at <${url}>: ${message}`);
+    return response.customError({ statusCode: 502, body: errorMessage });


I'm a bit torn on making this a 502 error. Doing so means any 5xx state returned by an endpoint will result in the Error Connecting UI/state replacing whatever previous UI was on the page.

I'm not sure if that's too jarring compared to keeping the same page but showing an 'Server Error' flash message. On the other hand, the Error Connecting UI does have some troubleshooting steps & a guide that could help folks solve intermittent 500 issues 🤷

Any strong feelings one way or another from y'all?

What if we did the flash message with a link to the error connecting page?

I'd have to either have to change the 502 code (probably to just a generic 500?) or change our 502 http interceptor itself since that logic looks for all 502 statuses and sets the top-level app state to error connecting.

I don't feel super strongly one way or another honestly - happy to put this to a vote. If no one else feels super strongly, we could also leave it as-is and change it later if we encounter a bunch of 500s and we find this UX inferior the road? 🤷

cee-chen · 2020-09-11T01:45:39Z

x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.ts

+    ? error.body!.attributes.errors.map((message) => ({ type: 'error', message }))
+    : [{ type: 'error', message: defaultErrorMessage }];
+
+  FlashMessagesLogic.actions.setFlashMessages(errorFlashMessages);


I was actually pretty blown away that this Just Worked(™️) within both a Logic file:

https://github.com/elastic/kibana/blob/063882344769eb8e606e6b5d2d1e30e5b3264255/x-pack/plugins/enterprise_search/public/applications/workplace_search/views/overview/overview_logic.ts#L131-L138

and a React component:

https://github.com/elastic/kibana/blob/063882344769eb8e606e6b5d2d1e30e5b3264255/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/engine_overview.tsx#L52-L61

Kea is amazing! 💞

cee-chen · 2020-09-11T01:50:54Z

x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.ts

+/**
+ * Converts API/HTTP errors into user-facing Flash Messages
+ */
+export const handleAPIError = (error: HttpResponse<IErrorResponse>) => {


Also wanted to raise that I removed the onErrorCallback arg because I thought it was cleaner to have the helper be in charge of setting FlashMessages vs. passing in setFlashMessages each time.

It's possible we'll want/need to pass a custom callback in the future, but I can't think of a really useful case for a passed callback currently (esp with try/catch syntax) - can anyone else?

Another thought: since I'm making this handler specific to flash messages (I placed it in shared/flash_messages because I thought it made a lot of sense in there), should we consider renaming this to something like flashAPIErrors instead?

I think this approach is fine since its coupled to the messages.

Rad! I went ahead renamed handleAPIError to flashAPIErrors here: ad4f8fd

CC @byronhulcher as a heads up!

- Should be hopefully useful for calls that need to queue an error and then redirect to another page

x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.ts

JasonStoltz · 2020-09-11T15:51:54Z

So this essentially normalizes all Enterprise Search API responses to something easily consumable by our clients code ... that is awesome 👏

scottybollinger

This is amazing work Constance! Thanks for taking the lead on this.

scottybollinger · 2020-09-14T15:39:29Z

x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.ts

+      'Enterprise Search encountered an internal server error. Please contact your system administrator if the problem persists.';
+
+    this.log.error(`Enterprise Search Server Error ${status} at <${url}>: ${message}`);
+    return response.customError({ statusCode: 502, body: errorMessage });


What if we did the flash message with a link to the error connecting page?

scottybollinger · 2020-09-14T15:41:16Z

x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.ts

+/**
+ * Converts API/HTTP errors into user-facing Flash Messages
+ */
+export const handleAPIError = (error: HttpResponse<IErrorResponse>) => {


I think this approach is fine since its coupled to the messages.

scottybollinger

🎉

kibanamachine · 2020-09-14T17:34:59Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: ad4f8fd

Build metrics

async chunks size

id	value	diff	baseline
enterpriseSearch	430.0KB	+32.0B	430.0KB

History

💚 Build #73908 succeeded dcdd872
💔 Build #73903 failed 2b949c5

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

…ange of errors + add handleAPIErrors helper (#77258) (#77427) * Update/refactor EnterpriseSearchRequestHandler to manage internal endpoint error behavior + pull out / refactor all errors into their own methods instead of relying on a single 'error connecting' message * Fix http interceptor bug preventing 4xx/5xx responses from being caught * Add handleAPIError helper - New and improved from ent-search - this one automatically sets flash messages for you so you don't have to pass in a callback! * Fix type check * [Feedback] Add option to queue flash messages to handleAPIError - Should be hopefully useful for calls that need to queue an error and then redirect to another page * PR feedback: Add test case for bodies flagged as JSON which are not * Rename handleAPIError to flashAPIErrors

* master: (25 commits) [Security Solution] Add unit tests for Network search strategy (elastic#77416) [Alerting] Improves performance of the authorization filter in AlertsClient.find by skipping KQL parsing (elastic#77040) [Ingest Manager] Add route for package installation by upload (elastic#77044) [APM-UI][E2E] filter PRs from the uptime GH team (elastic#77359) [APM] Remove useLocation and some minor route improvements (elastic#76343) [Enterprise Search] Update enterpriseSearchRequestHandler to manage range of errors + add handleAPIErrors helper (elastic#77258) [SECURITY_SOLUTION] Task/hostname policy response ux updates (elastic#76444) Move remaining uses of serviceName away from urlParams (elastic#77248) [Lens] Move configuration popover to flyout (elastic#76046) [Ingest Manager] Manually build Fleet kuery with Node arguments (elastic#76589) skip flaky suite (elastic#59975) Neutral-naming in reporting plugin (elastic#77371) [Enterprise Search] Add UserIcon styles (elastic#77385) [RUM Dashboard] Added loading state to visitor breakdown pie charts (elastic#77201) [Ingest Manager] Fix polling for new agent action (elastic#77339) Remote cluster - Functional UI test to change the superuser to a test_user with limited role (elastic#77212) Stacked headers and navigational search (elastic#72331) [ML] DF Analytics creation wizard: Fixing field loading race condition (elastic#77326) [Monitoring] Handle no mappings found for sort and collapse fields (elastic#77099) Add Lens to Recently Accessed (elastic#77249) ...

* master: (293 commits) Fix tsvb filter ration for table (elastic#77272) [Security Solution] Add unit tests for Network search strategy (elastic#77416) [Alerting] Improves performance of the authorization filter in AlertsClient.find by skipping KQL parsing (elastic#77040) [Ingest Manager] Add route for package installation by upload (elastic#77044) [APM-UI][E2E] filter PRs from the uptime GH team (elastic#77359) [APM] Remove useLocation and some minor route improvements (elastic#76343) [Enterprise Search] Update enterpriseSearchRequestHandler to manage range of errors + add handleAPIErrors helper (elastic#77258) [SECURITY_SOLUTION] Task/hostname policy response ux updates (elastic#76444) Move remaining uses of serviceName away from urlParams (elastic#77248) [Lens] Move configuration popover to flyout (elastic#76046) [Ingest Manager] Manually build Fleet kuery with Node arguments (elastic#76589) skip flaky suite (elastic#59975) Neutral-naming in reporting plugin (elastic#77371) [Enterprise Search] Add UserIcon styles (elastic#77385) [RUM Dashboard] Added loading state to visitor breakdown pie charts (elastic#77201) [Ingest Manager] Fix polling for new agent action (elastic#77339) Remote cluster - Functional UI test to change the superuser to a test_user with limited role (elastic#77212) Stacked headers and navigational search (elastic#72331) [ML] DF Analytics creation wizard: Fixing field loading race condition (elastic#77326) [Monitoring] Handle no mappings found for sort and collapse fields (elastic#77099) ...

cee-chen added 3 commits September 10, 2020 17:02

Update/refactor EnterpriseSearchRequestHandler to manage internal end…

e57009c

…point error behavior + pull out / refactor all errors into their own methods instead of relying on a single 'error connecting' message

Fix http interceptor bug preventing 4xx/5xx responses from being caught

0e1f2eb

Add handleAPIError helper

2b949c5

- New and improved from ent-search - this one automatically sets flash messages for you so you don't have to pass in a callback!

cee-chen added Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Sep 11, 2020

cee-chen requested review from JasonStoltz, scottybollinger, byronhulcher and a team September 11, 2020 01:29

cee-chen commented Sep 11, 2020

View reviewed changes

cee-chen added 2 commits September 10, 2020 19:03

Fix type check

ab889ee

[Feedback] Add option to queue flash messages to handleAPIError

dcdd872

- Should be hopefully useful for calls that need to queue an error and then redirect to another page

JasonStoltz reviewed Sep 11, 2020

View reviewed changes

x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.ts Show resolved Hide resolved

cee-chen added 2 commits September 14, 2020 08:12

PR feedback: Add test case for bodies flagged as JSON which are not

9bf8069

Merge branch 'master' into handle-api-errors

5287c75

scottybollinger approved these changes Sep 14, 2020

View reviewed changes

Rename handleAPIError to flashAPIErrors

ad4f8fd

scottybollinger approved these changes Sep 14, 2020

View reviewed changes

cee-chen merged commit 98e6475 into elastic:master Sep 15, 2020

cee-chen deleted the handle-api-errors branch September 15, 2020 01:59

cee-chen mentioned this pull request Sep 15, 2020

[7.x] [Enterprise Search] Update enterpriseSearchRequestHandler to manage range of errors + add handleAPIErrors helper (#77258) #77427

Merged

JasonStoltz mentioned this pull request Sep 15, 2020

[Enterprise Search] Added Credentials Page #77239

Closed

17 tasks

cee-chen mentioned this pull request Sep 16, 2020

[Enterprise Search] Minor flashAPIMessages update #77570

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Enterprise Search] Update enterpriseSearchRequestHandler to manage range of errors + add handleAPIErrors helper #77258

[Enterprise Search] Update enterpriseSearchRequestHandler to manage range of errors + add handleAPIErrors helper #77258

cee-chen commented Sep 11, 2020

cee-chen Sep 11, 2020

cee-chen Sep 11, 2020

cee-chen Sep 11, 2020

cee-chen Sep 11, 2020

scottybollinger Sep 14, 2020

cee-chen Sep 14, 2020

cee-chen Sep 11, 2020

cee-chen Sep 11, 2020

scottybollinger Sep 14, 2020

cee-chen Sep 14, 2020

JasonStoltz commented Sep 11, 2020

scottybollinger left a comment

scottybollinger Sep 14, 2020

scottybollinger Sep 14, 2020

scottybollinger left a comment

kibanamachine commented Sep 14, 2020

[Enterprise Search] Update enterpriseSearchRequestHandler to manage range of errors + add handleAPIErrors helper #77258

[Enterprise Search] Update enterpriseSearchRequestHandler to manage range of errors + add handleAPIErrors helper #77258

Conversation

cee-chen commented Sep 11, 2020

Summary

QA

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonStoltz commented Sep 11, 2020

scottybollinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottybollinger left a comment

Choose a reason for hiding this comment

kibanamachine commented Sep 14, 2020

💚 Build Succeeded

Build metrics

async chunks size

History