From e1e7218b9574abb75059932d5ffe243fee9f69a7 Mon Sep 17 00:00:00 2001 From: Luke Elmers Date: Thu, 21 Feb 2019 18:43:15 -0700 Subject: [PATCH] Surface vis loader errors in the UI. (#30594) --- .../__tests__/elasticsearch_error.js | 69 ------------ .../__tests__/is_term_size_zero_error.js | 55 ---------- .../elasticsearch_error.js | 62 ----------- .../ui/public/elasticsearch_errors/index.js | 21 ---- .../is_term_size_zero_error.js | 24 ---- .../loader/embedded_visualize_handler.ts | 48 +++++++- .../visualize/loader/pipeline_data_loader.ts | 2 - .../visualize/loader/visualize_data_loader.ts | 103 ++++++------------ 8 files changed, 77 insertions(+), 307 deletions(-) delete mode 100644 src/legacy/ui/public/elasticsearch_errors/__tests__/elasticsearch_error.js delete mode 100644 src/legacy/ui/public/elasticsearch_errors/__tests__/is_term_size_zero_error.js delete mode 100644 src/legacy/ui/public/elasticsearch_errors/elasticsearch_error.js delete mode 100644 src/legacy/ui/public/elasticsearch_errors/index.js delete mode 100644 src/legacy/ui/public/elasticsearch_errors/is_term_size_zero_error.js diff --git a/src/legacy/ui/public/elasticsearch_errors/__tests__/elasticsearch_error.js b/src/legacy/ui/public/elasticsearch_errors/__tests__/elasticsearch_error.js deleted file mode 100644 index bd30781a7d77b..0000000000000 --- a/src/legacy/ui/public/elasticsearch_errors/__tests__/elasticsearch_error.js +++ /dev/null @@ -1,69 +0,0 @@ -/* - * Licensed to Elasticsearch B.V. under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch B.V. licenses this file to you under - * the Apache License, Version 2.0 (the "License"); you may - * not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -import expect from 'expect.js'; -import { ElasticsearchError } from '../elasticsearch_error'; - -describe('ElasticsearchError', () => { - function createError(rootCauses = []) { - // Elasticsearch errors are characterized by the resp.error.root_cause array. - return { - resp: { - error: { - root_cause: rootCauses.map(rootCause => ({ - reason: rootCause, - })), - } - } - }; - } - - describe('interface', () => { - describe('constructor', () => { - it('throws an error if instantiated with a non-elasticsearch error', () => { - expect(() => new ElasticsearchError({})).to.throwError(); - }); - }); - - describe('getRootCauses', () => { - it(`returns the root_cause array's reason values`, () => { - const rootCauses = ['a', 'b']; - const error = createError(rootCauses); - const esError = new ElasticsearchError(error); - expect(esError.getRootCauses()).to.eql(rootCauses); - }); - }); - - describe('hasRootCause', () => { - it(`returns true if the cause occurs in the root_cause array's reasons, insensitive to case`, () => { - const rootCauses = ['a very detailed error', 'a slightly more detailed error']; - const error = createError(rootCauses); - const esError = new ElasticsearchError(error); - expect(esError.hasRootCause('slightly MORE')).to.be(true); - }); - - it(`returns false if the cause doesn't occur in the root_cause array's reasons`, () => { - const rootCauses = ['a very detailed error', 'a slightly more detailed error']; - const error = createError(rootCauses); - const esError = new ElasticsearchError(error); - expect(esError.hasRootCause('nonexistent error')).to.be(false); - }); - }); - }); -}); diff --git a/src/legacy/ui/public/elasticsearch_errors/__tests__/is_term_size_zero_error.js b/src/legacy/ui/public/elasticsearch_errors/__tests__/is_term_size_zero_error.js deleted file mode 100644 index 6820276ee7df5..0000000000000 --- a/src/legacy/ui/public/elasticsearch_errors/__tests__/is_term_size_zero_error.js +++ /dev/null @@ -1,55 +0,0 @@ -/* - * Licensed to Elasticsearch B.V. under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch B.V. licenses this file to you under - * the Apache License, Version 2.0 (the "License"); you may - * not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -import expect from 'expect.js'; -import { isTermSizeZeroError } from '../is_term_size_zero_error'; - -describe('isTermSizeZeroError', () => { - const identifyingString = 'size must be positive, got 0'; - - it('returns true if it contains the identifying string', () => { - const error = { - resp: { - error: { - root_cause: [{ - reason: `Some crazy Java exception: ${identifyingString}`, - }], - } - } - }; - expect(isTermSizeZeroError(error)).to.be(true); - }); - - it(`returns false if it doesn't contain the identifying string`, () => { - const error = { - resp: { - error: { - root_cause: [{ - reason: `Some crazy Java exception`, - }], - } - } - }; - expect(isTermSizeZeroError(error)).to.be(false); - }); - - it ('returns false for non-elasticsearch error input', () => { - expect(isTermSizeZeroError({ foo: 'bar' })).to.be(false); - }); -}); diff --git a/src/legacy/ui/public/elasticsearch_errors/elasticsearch_error.js b/src/legacy/ui/public/elasticsearch_errors/elasticsearch_error.js deleted file mode 100644 index 2275b328751a8..0000000000000 --- a/src/legacy/ui/public/elasticsearch_errors/elasticsearch_error.js +++ /dev/null @@ -1,62 +0,0 @@ -/* - * Licensed to Elasticsearch B.V. under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch B.V. licenses this file to you under - * the Apache License, Version 2.0 (the "License"); you may - * not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -import _ from 'lodash'; - -export class ElasticsearchError { - constructor(error) { - this.error = error; - - this.getRootCauses = this.getRootCauses.bind(this); - this.hasRootCause = this.hasRootCause.bind(this); - - if (!this.getRootCauses().length) { - throw new Error( - 'ElasticsearchError must be instantiated with an elasticsearch error, i.e. it must have' + - `a resp.error.root_cause property. Instead got ${JSON.stringify(error)}` - ); - } - } - - static hasRootCause(error, cause) { - try { - const esError = new ElasticsearchError(error); - return esError.hasRootCause(cause); - } catch (err) { - // we assume that any failure represents a validation error - // in the ElasticsearchError constructor - return false; - } - } - - getRootCauses() { - const rootCauses = _.get(this.error, 'resp.error.root_cause'); - return _.pluck(rootCauses, 'reason'); - } - - hasRootCause(cause) { - const normalizedCause = cause.toLowerCase(); - const rootCauses = this.getRootCauses(); - const matchingCauses = rootCauses.filter(rootCause => { - const normalizedRootCause = rootCause.toLowerCase(); - return normalizedRootCause.indexOf(normalizedCause) !== -1; - }); - return matchingCauses.length !== 0; - } -} diff --git a/src/legacy/ui/public/elasticsearch_errors/index.js b/src/legacy/ui/public/elasticsearch_errors/index.js deleted file mode 100644 index f2929b5b059ac..0000000000000 --- a/src/legacy/ui/public/elasticsearch_errors/index.js +++ /dev/null @@ -1,21 +0,0 @@ -/* - * Licensed to Elasticsearch B.V. under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch B.V. licenses this file to you under - * the Apache License, Version 2.0 (the "License"); you may - * not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -export { ElasticsearchError } from './elasticsearch_error'; -export { isTermSizeZeroError } from './is_term_size_zero_error'; diff --git a/src/legacy/ui/public/elasticsearch_errors/is_term_size_zero_error.js b/src/legacy/ui/public/elasticsearch_errors/is_term_size_zero_error.js deleted file mode 100644 index f83eca3d14cfe..0000000000000 --- a/src/legacy/ui/public/elasticsearch_errors/is_term_size_zero_error.js +++ /dev/null @@ -1,24 +0,0 @@ -/* - * Licensed to Elasticsearch B.V. under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch B.V. licenses this file to you under - * the Apache License, Version 2.0 (the "License"); you may - * not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -import { ElasticsearchError } from './elasticsearch_error'; - -export function isTermSizeZeroError(error) { - return ElasticsearchError.hasRootCause(error, 'size must be positive, got 0'); -} diff --git a/src/legacy/ui/public/visualize/loader/embedded_visualize_handler.ts b/src/legacy/ui/public/visualize/loader/embedded_visualize_handler.ts index 08c1b0f48f0df..51408f3c083dd 100644 --- a/src/legacy/ui/public/visualize/loader/embedded_visualize_handler.ts +++ b/src/legacy/ui/public/visualize/loader/embedded_visualize_handler.ts @@ -46,6 +46,9 @@ import { } from './types'; import { queryGeohashBounds } from './utils'; +import { i18n } from '@kbn/i18n'; +import { toastNotifications } from 'ui/notify'; + interface EmbeddedVisualizeHandlerParams extends VisualizeLoaderParams { Private: IPrivate; queryFilter: any; @@ -428,12 +431,47 @@ export class EmbeddedVisualizeHandler { this.dataLoaderParams.inspectorAdapters = this.inspectorAdapters; this.vis.filters = { timeRange: this.dataLoaderParams.timeRange }; + this.vis.requestError = undefined; + this.vis.showRequestError = false; + + return this.dataLoader + .fetch(this.dataLoaderParams) + .then(data => { + // Pipeline responses never throw errors, so we need to check for + // `type: 'error'`, and then throw so it can be caught below. + // TODO: We should revisit this after we have fully migrated + // to the new expression pipeline infrastructure. + if (data && data.type === 'error') { + throw data.error; + } - return this.dataLoader.fetch(this.dataLoaderParams).then(data => { - if (data && data.value) { - this.dataSubject.next(data.value); - } - return data; + if (data && data.value) { + this.dataSubject.next(data.value); + } + return data; + }) + .catch(this.handleDataLoaderError); + }; + + /** + * When dataLoader returns an error, we need to make sure it surfaces in the UI. + * + * TODO: Eventually we should add some custom error messages for issues that are + * frequently encountered by users. + */ + private handleDataLoaderError = (error: any): void => { + // TODO: come up with a general way to cancel execution of pipeline expressions. + this.dataLoaderParams.searchSource.cancelQueued(); + + this.vis.requestError = error; + this.vis.showRequestError = + error.type && ['NO_OP_SEARCH_STRATEGY', 'UNSUPPORTED_QUERY'].includes(error.type); + + toastNotifications.addDanger({ + title: i18n.translate('common.ui.visualize.dataLoaderError', { + defaultMessage: 'Error in visualization', + }), + text: error.message, }); }; diff --git a/src/legacy/ui/public/visualize/loader/pipeline_data_loader.ts b/src/legacy/ui/public/visualize/loader/pipeline_data_loader.ts index ac5c7c0a09bbb..a60e822fac5d9 100644 --- a/src/legacy/ui/public/visualize/loader/pipeline_data_loader.ts +++ b/src/legacy/ui/public/visualize/loader/pipeline_data_loader.ts @@ -24,8 +24,6 @@ export class PipelineDataLoader { constructor(private readonly vis: Vis) {} public async fetch(params: RequestHandlerParams): Promise { - this.vis.requestError = undefined; - this.vis.showRequestError = false; this.vis.pipelineExpression = buildPipeline(this.vis, params); return await runPipeline( diff --git a/src/legacy/ui/public/visualize/loader/visualize_data_loader.ts b/src/legacy/ui/public/visualize/loader/visualize_data_loader.ts index 3bddfcac88395..0342954a1268c 100644 --- a/src/legacy/ui/public/visualize/loader/visualize_data_loader.ts +++ b/src/legacy/ui/public/visualize/loader/visualize_data_loader.ts @@ -33,10 +33,6 @@ import { import { VisResponseData } from './types'; -// @ts-ignore No typing present -import { isTermSizeZeroError } from '../../elasticsearch_errors'; - -import { toastNotifications } from 'ui/notify'; import { decorateVisObject } from 'ui/visualize/loader/pipeline_helpers/build_pipeline'; function getHandler( @@ -71,74 +67,43 @@ export class VisualizeDataLoader { } public async fetch(params: RequestHandlerParams): Promise { - this.vis.filters = { timeRange: params.timeRange }; - this.vis.requestError = undefined; - this.vis.showRequestError = false; - // add necessary params to vis object (dimensions, bucket, metric, etc) decorateVisObject(this.vis, { timeRange: params.timeRange }); - try { - // searchSource is only there for courier request handler - const requestHandlerResponse = await this.requestHandler({ - partialRows: this.vis.params.partialRows || this.vis.type.requiresPartialRows, - isHierarchical: this.vis.isHierarchical(), - visParams: this.vis.params, - ...params, - filters: params.filters - ? params.filters.filter(filter => !filter.meta.disabled) - : undefined, - }); - - // No need to call the response handler when there have been no data nor has been there changes - // in the vis-state (response handler does not depend on uiStat - const canSkipResponseHandler = - this.previousRequestHandlerResponse && - this.previousRequestHandlerResponse === requestHandlerResponse && - this.previousVisState && - isEqual(this.previousVisState, this.vis.getState()); - - this.previousVisState = this.vis.getState(); - this.previousRequestHandlerResponse = requestHandlerResponse; - - if (!canSkipResponseHandler) { - this.visData = await Promise.resolve( - this.responseHandler(requestHandlerResponse, this.vis.params.dimensions) - ); - } - - return { - as: 'visualization', - value: { - visType: this.vis.type.name, - visData: this.visData, - visConfig: this.vis.params, - params: {}, - }, - }; - } catch (error) { - params.searchSource.cancelQueued(); - - this.vis.requestError = error; - this.vis.showRequestError = - error.type && ['NO_OP_SEARCH_STRATEGY', 'UNSUPPORTED_QUERY'].includes(error.type); - - // tslint:disable-next-line - console.error(error); - - if (isTermSizeZeroError(error)) { - toastNotifications.addDanger( - `Your visualization ('${this.vis.title}') has an error: it has a term ` + - `aggregation with a size of 0. Please set it to a number greater than 0 to resolve ` + - `the error.` - ); - return; - } - - toastNotifications.addDanger({ - title: 'Error in visualization', - text: error.message, - }); + // searchSource is only there for courier request handler + const requestHandlerResponse = await this.requestHandler({ + partialRows: this.vis.params.partialRows || this.vis.type.requiresPartialRows, + isHierarchical: this.vis.isHierarchical(), + visParams: this.vis.params, + ...params, + filters: params.filters ? params.filters.filter(filter => !filter.meta.disabled) : undefined, + }); + + // No need to call the response handler when there have been no data nor has been there changes + // in the vis-state (response handler does not depend on uiStat + const canSkipResponseHandler = + this.previousRequestHandlerResponse && + this.previousRequestHandlerResponse === requestHandlerResponse && + this.previousVisState && + isEqual(this.previousVisState, this.vis.getState()); + + this.previousVisState = this.vis.getState(); + this.previousRequestHandlerResponse = requestHandlerResponse; + + if (!canSkipResponseHandler) { + this.visData = await Promise.resolve( + this.responseHandler(requestHandlerResponse, this.vis.params.dimensions) + ); } + + return { + as: 'visualization', + value: { + visType: this.vis.type.name, + visData: this.visData, + visConfig: this.vis.params, + params: {}, + }, + }; } }