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

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

Merged

Conversation

Projects
None yet
4 participants
@stacey-gammon
Copy link
Contributor

commented Apr 19, 2017

Fixes #9747

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:
screen shot 2017-04-19 at 4 01 52 pm

After:
screen shot 2017-04-19 at 4 02 18 pm

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 stacey-gammon requested a review from spalger Apr 19, 2017

@stacey-gammon stacey-gammon force-pushed the stacey-gammon:continue-on-request-failure branch from b10ec0b to 336bf73 Apr 24, 2017

@stacey-gammon

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2017

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:elasticsearch@6.0.0-alpha1] 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 stacey-gammon force-pushed the stacey-gammon:continue-on-request-failure branch from 336bf73 to 5e10b8b Apr 25, 2017

@tbragin tbragin added the :Sharing label Apr 26, 2017

@stacey-gammon stacey-gammon force-pushed the stacey-gammon:continue-on-request-failure branch 2 times, most recently from e942bde to 3105fbe Apr 28, 2017

@stacey-gammon

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2017

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

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

This comment has been minimized.

Copy link
@spalger

spalger May 2, 2017

Member

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 })
})

This comment has been minimized.

Copy link
@stacey-gammon

stacey-gammon May 2, 2017

Author Contributor

👏 Much simpler, thanks!

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

This comment has been minimized.

Copy link
@spalger

spalger May 2, 2017

Member

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 😎

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

This comment has been minimized.

Copy link
@spalger

spalger May 2, 2017

Member

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

This comment has been minimized.

Copy link
@spalger

spalger May 2, 2017

Member

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

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

This comment has been minimized.

Copy link
@spalger

spalger May 2, 2017

Member

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.

@@ -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 comment has been minimized.

Copy link
@spalger

spalger May 2, 2017

Member

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.

stacey-gammon added some commits Apr 19, 2017

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.
Remove promiseMapSettled
Use a much simpler implementation

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

@stacey-gammon

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2017

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

Jenkins, test this

@stacey-gammon

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2017

All changes should be addressed @spalger.

@stacey-gammon stacey-gammon requested a review from jbudz May 5, 2017

@spalger

spalger approved these changes May 5, 2017

@spalger

This comment has been minimized.

Copy link
Member

commented May 5, 2017

@stacey-gammon LGTM, thanks!

@jbudz

jbudz approved these changes May 8, 2017

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

2 checks passed

CLA Commit author has signed the CLA
Details
kibana-ci Build finished.
Details

stacey-gammon added a commit to stacey-gammon/kibana that referenced this pull request May 8, 2017

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

* 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 added a commit to stacey-gammon/kibana that referenced this pull request May 8, 2017

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

* 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 added a commit that referenced this pull request May 8, 2017

Don't kill an entire dashboard because of one bad request (#11337) (#…
…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

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

snide added a commit to snide/kibana that referenced this pull request May 30, 2017

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

* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.