Don't kill an entire dashboard because of one bad request #11337

stacey-gammon · 2017-04-19T20:05:29Z

Summary: Previously if a visualization caused a request error to be thrown, the entire dashboard would fail to load. We changed that so now the rest of the visualizations will continue to load successfully, helping you narrow down which visualizations the errors are coming from.

Note: I will clean the code up if this way forward is approved (and add a hefty amount of comments) but waiting for feedback before spending the time.

It filters out failing requests, and considered them "aborted". I briefly investigated marking them as failures, which might work too, but it doesn't seem to make a difference.

The logic is extremely convoluted and I'm not confident this doesn't raise issues in other situations. I started working on some clean up but it went farther and farther and decided to extricate myself. I could continue back down that path, but it's time consuming.

So @spalger I'd like to get your opinion on this and what you think the best way forward is. It looks like our courier was never meant to handle partial failures. Making this more explicit would require some refactoring and I'm not sure the time investment is worth it.

But this implementation does solve the issue, and dashboards will now show all visualizations except the failing ones which are blank. They should probably show the error inside the visualization, but because it's a top level notify error, this isn't clear.

Before:

After:

The fix, while a bit hacky, could prove really helpful in debugging user issues because it helps a user narrow down the faulty visualizations. The field error is pretty common and takes some time to figure out which field is killing the visualization which is killing the entire dashboard.

stacey-gammon · 2017-04-24T18:07:24Z

I've seen this flaky failure before:

INFO - GAbrnsK - meta_data_create_index_service - [.kibana] creating index, cause [api], templates [], shards [1]/[1], mappings [server, config]
  log   [15:49:56.457] [info][status][plugin:[email protected]] Status changed from yellow to green - Kibana index ready
  log   [15:49:56.457] [info][status][ui settings] Status changed from yellow to green - Ready
Warning: Error: remote failed to start within 2 minutes
    at /var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/test/functional/services/remote/leadfoot_command.js:13:13
    at undefined.next (native)
    at step (/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/test/functional/services/remote/leadfoot_command.js:5:1)
    at /var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/test/functional/services/remote/leadfoot_command.js:5:1� Use --force to continue.

Aborted due to warnings.
runbld>>> <<<<<<<<<<<< SCRIPT EXECUTION END <<<<<<<<<<<<
runbld>>> DURATION: 696655ms

jenkins, test this

stacey-gammon · 2017-05-01T10:32:21Z

Tests are now passing. When you get a chance, lmk what you think about this fix, @spalger.

spalger · 2017-05-02T03:04:10Z

src/ui/public/courier/fetch/call_client.js

-      return Promise.try(req.getFetchParams, void 0, req)
-      .then(function (fetchParams) {
-        return (req.fetchParams = fetchParams);
+    promiseMapSettled(


I think that promiseMapSettled() would be more clearly implemented as two lines at the bottom of Promise.map()'s mapper:

Promise.map(arr, item => { return Promise.try(...) .then(value => ({ resolved: value }) .catch(err => ({ rejected: error }) })

👏 Much simpler, thanks!

spalger · 2017-05-02T04:33:45Z

src/ui/public/courier/fetch/call_client.js

+      },
+      Promise)
+    .then(function (results) {
+      const requestsWithFetchParams = results.map((result, index) => {


I'm not in love with this solution, but I've spent a couple hours trying to come up with a better one and none of my ideas will take less than a couple days... This works, and the scope of the change is impressively small, so I'm happy with it 😎

spalger · 2017-05-02T04:34:21Z

src/ui/public/courier/fetch/call_client.js

+        } else {
+          const request = executable[index];
+          request.handleFailure(result.rejected);
+          executable[index] = undefined;


I think that if we return undefined here it will be more clear why_.remove(requestsWithFetchParams, request => request === undefined); is necessary

Otherwise I think it might make more sense to just build up requestsWithFetchParams in a forEach loop rather than using .map() followed by deletes

spalger · 2017-05-02T04:36:16Z

src/ui/public/courier/fetch/call_client.js

-    })
-    .then(function (reqsFetchParams) {
-      return strategy.reqsFetchParamsToBody(reqsFetchParams);
+      _.remove(executable, request => request === undefined);


Can we use x = x.filter(...) so that both executable and requestsWithFetchParams have to be defined with let and signify that they are modified below.

spalger · 2017-05-02T04:39:20Z

src/ui/public/routes/__tests__/_work_queue.js

@@ -3,7 +3,7 @@ import expect from 'expect.js';
 import ngMock from 'ng_mock';
 import { WorkQueue } from 'ui/routes/work_queue';
 import sinon from 'auto-release-sinon';
-import 'ui/promises';
+import 'ui/promises/index';


This should not be necessary since: 9bdbc26

If for some reason it is please file an issue at https://github.com/elastic/webpack-directory-name-as-main, but if we remove promiseMapSettled() then this doesn't really mean much.

Some tests get rid of the angular promise library so it works better if it’s a separate function then on the actual angular Promise class.

Use a much simpler implementation

stacey-gammon · 2017-05-02T16:28:29Z

Aborted by unknown
Publish artifacts to S3 Bucket Skipping publishing on S3 because build aborted
Finished: ABORTED

Jenkins, test this

stacey-gammon · 2017-05-05T18:14:42Z

All changes should be addressed @spalger.

spalger · 2017-05-05T19:39:36Z

@stacey-gammon LGTM, thanks!

) * Don't kill an entire dashboard because of one bad request Some tests get rid of the angular promise library so it works better if it’s a separate function then on the actual angular Promise class. * Fix promise path references after creation of index file. * Remove index suffix from import paths * Remove promiseMapSettled Use a much simpler implementation * Clean up

…11652) * Don't kill an entire dashboard because of one bad request Some tests get rid of the angular promise library so it works better if it’s a separate function then on the actual angular Promise class. * Fix promise path references after creation of index file. * Remove index suffix from import paths * Remove promiseMapSettled Use a much simpler implementation * Clean up

) * Don't kill an entire dashboard because of one bad request Some tests get rid of the angular promise library so it works better if it’s a separate function then on the actual angular Promise class. * Fix promise path references after creation of index file. * Remove index suffix from import paths * Remove promiseMapSettled Use a much simpler implementation * Clean up

stacey-gammon requested a review from spalger April 19, 2017 20:05

stacey-gammon force-pushed the continue-on-request-failure branch from b10ec0b to 336bf73 Compare April 24, 2017 15:23

stacey-gammon force-pushed the continue-on-request-failure branch from 336bf73 to 5e10b8b Compare April 25, 2017 12:59

tbragin added the :Sharing label Apr 26, 2017

stacey-gammon force-pushed the continue-on-request-failure branch 2 times, most recently from e942bde to 3105fbe Compare April 29, 2017 10:40

stacey-gammon added review v5.5.0 labels May 1, 2017

spalger suggested changes May 2, 2017

View reviewed changes

stacey-gammon added 4 commits May 2, 2017 09:52

Don't kill an entire dashboard because of one bad request

5e71826

Some tests get rid of the angular promise library so it works better if it’s a separate function then on the actual angular Promise class.

Fix promise path references after creation of index file.

3556166

Remove index suffix from import paths

58d0e4a

Remove promiseMapSettled

19f2b8a

Use a much simpler implementation

stacey-gammon force-pushed the continue-on-request-failure branch from 3105fbe to 19f2b8a Compare May 2, 2017 14:48

Clean up

7d9bd2c

stacey-gammon requested a review from jbudz May 5, 2017 18:15

spalger approved these changes May 5, 2017

View reviewed changes

jbudz approved these changes May 8, 2017

View reviewed changes

stacey-gammon merged commit b5e6489 into elastic:master May 8, 2017

stacey-gammon mentioned this pull request May 8, 2017

[5.x] Don't kill an entire dashboard because of one bad request (#11337) #11652

Merged

stacey-gammon deleted the continue-on-request-failure branch May 8, 2017 16:15

stacey-gammon added the release_note:fix label Jun 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't kill an entire dashboard because of one bad request #11337

Don't kill an entire dashboard because of one bad request #11337

stacey-gammon commented Apr 19, 2017 •

edited

Loading

stacey-gammon commented Apr 24, 2017

stacey-gammon commented May 1, 2017

spalger May 2, 2017

stacey-gammon May 2, 2017

spalger May 2, 2017

spalger May 2, 2017

spalger May 2, 2017

spalger May 2, 2017

spalger May 2, 2017

stacey-gammon commented May 2, 2017

stacey-gammon commented May 5, 2017

spalger commented May 5, 2017

Don't kill an entire dashboard because of one bad request #11337

Don't kill an entire dashboard because of one bad request #11337

Conversation

stacey-gammon commented Apr 19, 2017 • edited Loading

stacey-gammon commented Apr 24, 2017

stacey-gammon commented May 1, 2017

spalger May 2, 2017

Choose a reason for hiding this comment

stacey-gammon May 2, 2017

Choose a reason for hiding this comment

spalger May 2, 2017

Choose a reason for hiding this comment

spalger May 2, 2017

Choose a reason for hiding this comment

spalger May 2, 2017

Choose a reason for hiding this comment

spalger May 2, 2017

Choose a reason for hiding this comment

spalger May 2, 2017

Choose a reason for hiding this comment

stacey-gammon commented May 2, 2017

stacey-gammon commented May 5, 2017

spalger commented May 5, 2017

stacey-gammon commented Apr 19, 2017 •

edited

Loading