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

[Lens] Handle failing existence check #70718

Merged
merged 8 commits into from
Jul 16, 2020
Merged

[Lens] Handle failing existence check #70718

merged 8 commits into from
Jul 16, 2020

Conversation

flash1293
Copy link
Contributor

Part of #70520

existence_error_handling

Currently if the field existence check of the data panel fails, the loading spinner will be shown forever. This PR handles it gracefully:

  • List all fields from the index pattern saved object as available
  • Show a warning icon the availability request didn't work

To test:

  • Install two index patterns
  • Go to Lens
  • Switch between both to make sure the local index pattern cache is poplulated
  • Set network to offline in dev tools
  • Switch index pattern again

@flash1293 flash1293 marked this pull request as ready for review July 3, 2020 16:04
@flash1293 flash1293 requested a review from a team July 3, 2020 16:04
@flash1293 flash1293 added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Jul 3, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@mbondyra
Copy link
Contributor

mbondyra commented Jul 6, 2020

Tested the behaviour in two ways:

  1. Suggested by @flash1293
  2. The following way:
  • Install two index patterns
  • Go to Lens
  • Switch between both to make sure the local index pattern cache is poplulated
  • In other tab, remove one of dataset
  • Switch index pattern again

Works as expected and it's definitely an improvement. I am not sure though if copy is clear enough for our users ("field availability could not be loaded") - maybe we need some input from @cchaos or @KOTungseth?

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@flash1293
Copy link
Contributor Author

Agreed @mbondyra , happy to improve that

@KOTungseth
Copy link
Contributor

Works as expected and it's definitely an improvement. I am not sure though if copy is clear enough for our users ("field availability could not be loaded") - maybe we need some input from @cchaos or @KOTungseth?

How about:

The index does not include available fields

or

There are no fields available

@flash1293
Copy link
Contributor Author

flash1293 commented Jul 6, 2020 •
edited
Loading

@KOTungseth That doesn’t quite capture what’s happening - if this message is shown we don’t know whether fields are available or not because the request was not successful and the field information could not be loaded.

@wylieconlon
Copy link
Contributor

I think the warning triangle with tooltip is accurate, but maybe the thing that's confusing is that the category is called "Available fields". What if the label becomes "All fields" when there's an error?

@KOTungseth
Copy link
Contributor

I agree with @wylieconlon. The word available throws me off. Maybe a small improvement could be:

The fields are unable to load

Also, could we provide an "out" for the user? How do they get the field information to load?

@flash1293
Copy link
Contributor Author

flash1293 commented Jul 9, 2020 •
edited
Loading

@KOTungseth The fields are unable to load together with Wylies suggestion about calling it "All fields" is definitely better, but still a little confusing IMHO. The thing which can't be loaded is the information about fields (what values they hold and so on). Maybe Field information can't be loaded? A way out is hard at that point because this is a generic "last resort" behavior. We could show an error toast with an error message. Do you think that would be enough?

@flash1293
Copy link
Contributor Author

Screenshot 2020-07-13 at 12 41 11

Adjusted like described in the comment above.

@KOTungseth
Copy link
Contributor

@flash1293 LGTM!

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

wylieconlon
wylieconlon approved these changes Jul 15, 2020
View reviewed changes
Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

Tested and LGTM!

@wylieconlon
Copy link
Contributor

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

page load bundle size

id value diff baseline
lens 865.1KB +1.3KB 863.8KB

History

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

@flash1293 flash1293 merged commit 7e533f2 into elastic:master Jul 16, 2020
@flash1293 flash1293 deleted the lens/field-list-error-handling branch July 16, 2020 07:54
flash1293 added a commit to flash1293/kibana that referenced this pull request Jul 16, 2020
@flash1293
[Lens] Handle failing existence check (elastic#70718)
790504c
@flash1293 flash1293 mentioned this pull request Jul 16, 2020
Merged
flash1293 added a commit to flash1293/kibana that referenced this pull request Jul 16, 2020
@flash1293
[Lens] Handle failing existence check (elastic#70718)
38d51e0
This was referenced Jul 16, 2020
Merged
Closed
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 16, 2020
@gmmorris
Merge branch 'master' into alerting/consumer-based-rbac
a6989b5 
* master: (37 commits)
  [Lens] Handle failing existence check (elastic#70718)
  [Security Solution]Fix in-app links and popup window text (elastic#71403)
  [esArchiver] automatically retry if alias creation fails (elastic#71910)
  Move data stream index pattern creation test to xpack (elastic#71511)
  [Maps] Improve language for mvt card (elastic#71947)
  [Security][Detections] Unskip failing modal tests (elastic#71969)
  skip flaky suite (elastic#71987)
  skip flaky suite (elastic#71979)
  [Security Solution] [Detections] Revert "[Security Solution] [Detections] Fixes bug for determining when we hit max signals after filtering with lists (elastic#71768)" (elastic#71956)
  rename ilm policy to remove -default (elastic#71952)
  Adjust ordering of Management category apps to make Ingest Manager higher (elastic#71948)
  skip flaky suite (elastic#71971)
  skip flaky suite (elastic#71951)
  [kbn/optimizer] ignore compressed files when reporting stats (elastic#71940)
  skip flaky suite (elastic#71867)
  [ML] Fix new job with must_not saved search (elastic#71831)
  [Resolver] Fix bug where process detail panel doesn't show up (elastic#71754)
  Cleanup (elastic#71849)
  [Resolver] aria-level and aria-flowto support enhancements (elastic#71887)
  skip flaky suite (elastic#71304)
  ...
flash1293 added a commit that referenced this pull request Jul 16, 2020
@flash1293
[Lens] Handle failing existence check (#70718) (#72009)
907e43b
flash1293 added a commit that referenced this pull request Jul 16, 2020
@flash1293
[Lens] Handle failing existence check (#70718) (#72010)
fd95825
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 16, 2020
@gmmorris
Merge branch 'alerting/consumer-based-rbac' into alerting/management-…
47a4b93 
…feature-privileges

* alerting/consumer-based-rbac: (491 commits)
  [Lens] Handle failing existence check (elastic#70718)
  [Security Solution]Fix in-app links and popup window text (elastic#71403)
  [esArchiver] automatically retry if alias creation fails (elastic#71910)
  Move data stream index pattern creation test to xpack (elastic#71511)
  [Maps] Improve language for mvt card (elastic#71947)
  [Security][Detections] Unskip failing modal tests (elastic#71969)
  skip flaky suite (elastic#71987)
  skip flaky suite (elastic#71979)
  [Security Solution] [Detections] Revert "[Security Solution] [Detections] Fixes bug for determining when we hit max signals after filtering with lists (elastic#71768)" (elastic#71956)
  rename ilm policy to remove -default (elastic#71952)
  Adjust ordering of Management category apps to make Ingest Manager higher (elastic#71948)
  skip flaky suite (elastic#71971)
  skip flaky suite (elastic#71951)
  [kbn/optimizer] ignore compressed files when reporting stats (elastic#71940)
  skip flaky suite (elastic#71867)
  [ML] Fix new job with must_not saved search (elastic#71831)
  [Resolver] Fix bug where process detail panel doesn't show up (elastic#71754)
  Cleanup (elastic#71849)
  [Resolver] aria-level and aria-flowto support enhancements (elastic#71887)
  skip flaky suite (elastic#71304)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wylieconlon wylieconlon wylieconlon approved these changes

Assignees
No one assigned
Labels
Feature:Lens release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.9.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@flash1293 @elasticmachine @mbondyra @KOTungseth @wylieconlon @kibanamachine