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.

Sign up for GitHub

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

Jump to bottom

[Search] Error Alignment 2 #80965

Merged
merged 10 commits into from
Oct 26, 2020
Merged

[Search] Error Alignment 2 #80965

merged 10 commits into from
Oct 26, 2020

Conversation

lizozom
Copy link
Contributor

@lizozom lizozom commented Oct 19, 2020 •
edited
Loading

Summary

Improve the display of ES errors and HTTP errors

Elasticsearch errors

This PR adds explicit handling for all internal Elasticsearch errors, including the one mentioned on this issue. The new error message exposes the reason returned, rather than showing the same stack trace every time.

image

HTTP errors

Network or response parsing errors result in a very non descriptive errors, making SDH resolution more difficult.

This PR adds a more descriptive error for cases where core.http.fetch fails for various reasons.

image
image

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@lizozom lizozom added Feature:Search Querying infrastructure in Kibana v8.0.0 Team:AppArch release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Oct 19, 2020
@lizozom lizozom requested a review from a team as a code owner October 19, 2020 12:04
@lizozom lizozom self-assigned this Oct 19, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@lizozom lizozom requested review from kertal and gchaps October 19, 2020 12:06
@gchaps
Copy link
Contributor

gchaps commented Oct 19, 2020

It's better to use a title that describes the error.

Elasticsearch errors

Title: Cannot retrieve search results

Description: A description is not needed as the title gives the reason

Http errors

Title: Cannot retrieve your data

Description: Check your network and proxy configuration. If the problem persists, contact your network administrator.

I'm assuming that the text in the blue box comes directly from ES or the HTTP response and can't be edited. Let me know otherwise.

@lizozom
Copy link
Contributor Author

lizozom commented Oct 21, 2020

@elasticmachine merge upstream

@spalger spalger mentioned this pull request Oct 21, 2020
Closed
kertal
kertal approved these changes Oct 22, 2020
View reviewed changes
Copy link
Member

@kertal kertal left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tested and LGTM, those error messages are a big improvement, and will help users (and also devs) to find the cause of the issue more quickly!

@lizozom lizozom requested a review from a team as a code owner October 22, 2020 13:04
gchaps
gchaps approved these changes Oct 22, 2020
View reviewed changes
Copy link
Contributor

@gchaps gchaps left a comment

Choose a reason for hiding this comment

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

LGTM

lukasolson
lukasolson approved these changes Oct 22, 2020
View reviewed changes
Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

LGTM, added a couple of very minor things below.

src/plugins/data/public/search/errors/painless_error.tsx
if (!isEsError(err)) return false;

const rootCause = getRootCause(err as EsError);
const rootCause = getRootCause(err as IEsError);
Copy link
Member

Choose a reason for hiding this comment

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

Is the cast here necessary (since you've checked isEsError above, which is implemented as a type check)?

src/plugins/data/public/search/errors/utils.ts
export function getFailedShards(err: IEsError) {
const failedShards =
err.body?.attributes?.error?.failed_shards ||
err.body?.attributes?.error?.caused_by?.failed_shards;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Prefer ?? to ||

@lizozom
Copy link
Contributor Author

lizozom commented Oct 26, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
data 575 578 +3

page load bundle size

id before after diff
data 1.1MB 1.1MB +4.5KB

History

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

@lizozom lizozom merged commit 046c840 into elastic:master Oct 26, 2020
lizozom added a commit to lizozom/kibana that referenced this pull request Oct 26, 2020
@lizozom
[Search] Error Alignment 2 (elastic#80965)
c962cfe 
* Improve the display of ES errors and Http errors

* fixes

* Improve text

* fix ts

* update limit

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	packages/kbn-optimizer/limits.yml
@lizozom lizozom mentioned this pull request Oct 26, 2020
Merged
jloleysens added a commit to jloleysens/kibana that referenced this pull request Oct 26, 2020
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into ilm/migrate-w…
7b2f0ba 
…arm-phase-to-formlib

* 'master' of github.com:elastic/kibana:
  [Trigger Actions UI] Properly unmount app (elastic#81436)
  skip flaky suite (elastic#81576)
  skip flaky suite (elastic#78373)
  [Security Solution] Fix styling of EditDataProvider content (elastic#81456)
  [Search] Error Alignment 2 (elastic#80965)
  [APM] Unskip test (elastic#81574)
  [ML] Fix partition value selection on the Single Metric Viewer (elastic#81585)
  cleaning up expression service types (elastic#80643)
  Fix suggestions dropdown for query input (elastic#80990)
  [Usage collection] Make `schema` mandatory (elastic#79999)
  [ILM] Update show/hide data tier logic on cloud (elastic#81455)
  added brace import to advanced settings (elastic#81458)
  chore(NA): add spec-to-console and plugin-helpers as devOnly dependencies
lizozom added a commit that referenced this pull request Oct 26, 2020
@lizozom
[Search] Error Alignment 2 (#80965) (#81614)
e32e3aa 
* Improve the display of ES errors and Http errors

* fixes

* Improve text

* fix ts

* update limit

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
# Conflicts:
#	packages/kbn-optimizer/limits.yml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gchaps gchaps gchaps approved these changes

@mistic mistic mistic approved these changes

@lukasolson lukasolson lukasolson approved these changes

@kertal kertal kertal approved these changes

Assignees

@lizozom lizozom

Labels
Feature:Search Querying infrastructure in Kibana release_note:skip Skip the PR/issue when compiling release notes v7.11.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@lizozom @elasticmachine @gchaps @kibanamachine @kertal @lukasolson @mistic