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

Share vis loader error handling. #30167

Closed
wants to merge 33 commits into from

Conversation

lukeelmers
Copy link
Member

@lukeelmers lukeelmers commented Feb 6, 2019

[Closed in favor of #30594]

Closes #29900

Fixes issue where errors were not caught and displayed in the UI when using the pipeline data loader.

I pulled the error handling logic out into a function that can be shared between VisualizeDataLoader and PipelineDataLoader.

Note for reviewers: It is recommended to review the diff with "Hide whitespace changes" selected.

// tslint:disable-next-line
console.error(error);

if (isTermSizeZeroError(error)) {
Copy link
Member Author

@lukeelmers lukeelmers Feb 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed isTermSizeZeroError and the corresponding elasticsearch_errors directory because it is no longer possible to set a term size: 0 in the UI. And if you add it elsewhere, e.g. URL params, you will get a different error before this line of code even executes.

(This is the only place isTermSizeZero error is used).

That said, I think it's worth considering if we can create some more helpful error messages like this in the future, at least for commonly encountered errors.

@lukeelmers lukeelmers added review Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v7.0.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure expression-issue labels Feb 6, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@elasticmachine
Copy link
Contributor

💔 Build Failed

@lukeelmers
Copy link
Member Author

^ appears to be the failing CCR test CJ mentioned earlier. I’ll rebase on latest master tomorrow.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code LGTM, tested it out on linux chrome and the error messages show up (with pipeline and without)

src/ui/public/visualize/loader/errors.ts Outdated Show resolved Hide resolved
src/ui/public/visualize/loader/errors.ts Outdated Show resolved Hide resolved
src/ui/public/visualize/loader/pipeline_data_loader.ts Outdated Show resolved Hide resolved
@@ -71,74 +67,43 @@ export class VisualizeDataLoader {
}

public async fetch(params: RequestHandlerParams): Promise<VisResponseData | void> {
this.vis.filters = { timeRange: params.timeRange };
this.vis.requestError = undefined;
this.vis.showRequestError = false;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up removing these lines since they could be added in embedded visualize handler, so that all the error-related stuff lives in one place.

if (!canSkipResponseHandler) {
this.visData = await Promise.resolve(
this.responseHandler(requestHandlerResponse, this.vis.params.dimensions)
);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since errors are now caught in embedded visualize handler, the try/catch was no longer necessary here.

@elasticmachine

This comment has been minimized.

@lukeelmers

This comment has been minimized.

@lukeelmers lukeelmers force-pushed the fix/query-error-hidden branch from bdc57c2 to 2d636ba Compare February 7, 2019 19:35
@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@lukeelmers

This comment has been minimized.

@elasticmachine

This comment has been minimized.

ogupte and others added 21 commits February 8, 2019 16:12
* [APM] closes elastic#29563 by rendering related errors link with error count in transaction flyout

* [APM] improved get_trace query by  narrowing indices and parallelizing queries, improved code org/readability

* [APM] code improvements, split get_trace queries into separate source files

* [APM] remove initial transaction details request in favor of looking up the
current transaction data within the trace (waterfall) data

* Add test for `getWaterfall`

* Revert change to `getWaterfallItems` test

* simplified aggregation, waterfall helpers code, and moved get_trace_errors_per_transaction.ts under 'errors'

* improved naming and readbility of waterfall properties

* removed unused routes and queries and fixed some invisible bugs in the waterfall helpers

* added trace.id in addition to the transaction.id filter in the kuery bar for related errors
Because the info tooltip has been migrated to React, the escaping necessary for the previous jQuery based tooltip resulting in a double escaping and special character visible in the tooltip. This fixes it by removing the now unnecessary escaping.
Modify the way migrations detect mapping changes. The previous approach simply
diffed the index mappings against the mappings defined in Kibana. The problem was
that sometimes the index mappings will *always* differ. For example, if an index template
is affecting the .kibana* indices. Or if Elasticsearch adds a new magical mapping that
appears in the index, even though not specified (this happened in the 7.0 release). So,
instead of diffing, we now store hashes of our mappings in _meta, and compare against
those.
This removes the custom build process for Canvas plugins. Canvas plugins are now regular
Kibana plugins. They can register server-side functions like so:

```js
server.plugins.interpreter.register({
  serverFunctions: [yourFunctionsHere],
});
```

And client-side functions need to be included in `uiExports.canvas`, then they need to
register themselves like so:

```js
// kbnInterpreter is a global, sadly, but it makes registration simple
kbnInterpreter.register({
  elements: [yourElementsHere],
  browserFunctions: [yourFnsHere],
});
```
* [ML] Removing old and broken start datafeed modal

* fixing typos

* removing need to load job from server

* small refactor

* adding comments

* removing unnecessary async/await
The way job audit messages were fetched didn't retrieve the expected results if there were deleted jobs with messages still present for these jobs.
This fix allows to specify a list of job IDs to filter the audit messages on. For the jobs list UI, the currently existing job IDs will be passed on to ignore messages from deleted jobs.
This was missed during the recent migrations to React of Anomaly Explorer in elastic#28750. It removes the now unnecessary `$scope.indexPatterns`.
… kibana.yml (elastic#30428)

* [Maps] do not display mapbox style inspector view unless turned on by kibana.yml

* use better name for config name
…c#30000)

* [Reporting/PDF/PNG] Hide the global header in browser capture

* fix a devastating mistake
* add state field for isPreserveLayoutSupported

* doc updates
**Removes APM reindexing**: Since indices created in 6.x will be required to
re-index in 7.x there is no longer a need to support migrating APM indices to
the ECS format since ECS is shipped starting in 7.0

Signed-off-by: Tyler Smalley <[email protected]>
…0459)

* [ftr/services/kbnServer] set uiSetting defaults more often

* [ftr/services/uiSettings] only init defaults when they're defined
…0171)

Updates discover, visualize, and dashboard to use the date picker added to the query bar in elastic#29130. There are still some areas where we use the top nav date picker, like Timelion and TSVB which don't have a query bar.
The test's setup routine loads a data directory into ES that is not
compatible with version 8.0.0, so ES fails to start and the test always
fails.

It is skipped for now to unblock CI, but this should be resolved for 8.0
@lukeelmers lukeelmers requested review from a team as code owners February 8, 2019 23:12
@lukeelmers
Copy link
Member Author

lukeelmers commented Feb 8, 2019

Gah, sorry everyone who just got a ping on this. Please ignore -- Botched merge 🙄

Closing in favor of #30594.

@lukeelmers lukeelmers closed this Feb 8, 2019
@lukeelmers lukeelmers deleted the fix/query-error-hidden branch February 8, 2019 23:19
@elasticmachine
Copy link
Contributor

💔 Build Failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.0.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.