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

fix: 🐛 correctly create error on no_matching_indices #61257

Merged
merged 6 commits into from
Apr 7, 2020
Merged

fix: 🐛 correctly create error on no_matching_indices #61257

merged 6 commits into from
Apr 7, 2020

Conversation

streamich
Copy link
Contributor

@streamich streamich commented Mar 25, 2020 •
edited
Loading

Closes #56189

  • After some refactoring it seems a regression was introduced and no_matching_indices was not used in the New Platform. This PR makes no_matching_indices used again to create errors of IndexPatternMissingIndices class. It changes the behaviour to display warning like it used to before:

image

@streamich streamich requested a review from a team as a code owner March 25, 2020 13:10
@streamich streamich added bug Fixes for quality problems that affect the customer experience Feature:Data Views Data Views code and UI - index patterns before 8.0 review Team:AppArch v7.7.0 v8.0.0 labels Mar 25, 2020
@elasticmachine
Copy link
Contributor

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

@streamich streamich added the release_note:skip Skip the PR/issue when compiling release notes label Mar 25, 2020
@streamich
Copy link
Contributor Author

retest

@streamich
Copy link
Contributor Author

@elasticmachine merge upstream

@streamich
Copy link
Contributor Author

@elasticmachine merge upstream

@neptunian
Copy link
Contributor

neptunian commented Mar 31, 2020 •
edited
Loading

Thanks. This messaging is an improvement, but I am thinking it might make sense to remove this error altogether. It's not uncommon after the Ingest Manager is released that this scenario occurs. When the user installs a package we create some index patterns by default that the user might not actually be using but go look at. We also install dashboards and if they click on one where no data yet exists, it looks like this:

Screen Shot 2020-03-31 at 9 06 52 AM

It seems like the messaging "No data to display.." on the dashboards is enough. For the index patterns page in the Management app, they can't make an index pattern through the UI so if they create one manually where no data exists, it may have been intentional (like Ingest Management) and no error is necessary.

Thoughts?

@streamich
Copy link
Contributor Author

I can remove this "No matching indices found..." warning. Will ping people in Slack to comment on this.

@ppisljar
Copy link
Member

i think we might not want to just swallow the errors, as else user will not know why the dashboard is empty. what would be nice however (not in this pr) would be to update our notification service so it would group errors together (no need to see no index pattern 10 times)

@alexh97
Copy link
Contributor

alexh97 commented Mar 31, 2020

I agree with @ppisljar. We shouldn't swallow the errors. Not every usecase will have index patterns that don't match simply because the index hasn't been created yet - in some cases it will be an actual configuration problem and we'd want to point it out. Grouping the errors seems like the right direction to me.

@ruflin
Copy link
Member

ruflin commented Mar 31, 2020

Could we make it a config option on the index pattern if it should show errors if no index exists or not so we can adjust it to the different use cases?

@alexh97 Could you share the use case where this error is useful?

@neptunian neptunian mentioned this pull request Apr 2, 2020
Closed
lukeelmers
lukeelmers approved these changes Apr 6, 2020
View reviewed changes
Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Code changes LGTM; tested Chrome macOS & verified the error is displaying as described.

@streamich
Copy link
Contributor Author

@elasticmachine merge upstream

@streamich streamich mentioned this pull request Apr 6, 2020
Closed
2 tasks
@streamich
Copy link
Contributor Author

streamich commented Apr 6, 2020 •
edited
Loading

@ppisljar @alexh97 @ruflin I'll merge this once CI is green as this PR fixes a regression.

For further discussion and improving these warning messages I've created an issue here—please continue discussion there.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@streamich streamich merged commit afb8d8d into elastic:master Apr 7, 2020
@streamich streamich deleted the better-error-message branch April 7, 2020 10:19
This was referenced Apr 7, 2020
Merged
Merged
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 7, 2020
@gmmorris
Merge branch 'master' into alerting/tls-disable
a328e16 
* master: (36 commits)
  [data.search.aggs] Remove service getters from agg types (elastic#61628)
  fixing APM internationalization (elastic#62757)
  fix: 🐛 correctly create error on no_matching_indices (elastic#61257)
  [Lens] Remove all legacy imports (elastic#62596)
  Add label for ace editor (elastic#62588)
  [ML] Show better file structure finder explanations (elastic#62316)
  Fix old pathes in eslintrc (elastic#62580)
  [Uptime] Improve Telemetry test (elastic#62428)
  [SIEM] Adds sort rules Cypress test (elastic#62700)
  [Uptime]Abstracted 'access:uptime-read' tag into a wrapper for… (elastic#62576)
  fixing bug (elastic#62577)
  [Maps] Allow updating requestType for ESGeoGridSource (elastic#62365)
  [Maps] do not show circle border when symbol size is zero (elastic#62644)
  [Maps] Always show current zoom level (elastic#62684)
  bc5 siem rules merge (elastic#62679)
  Revert "[Monitoring] Cluster state watch to Kibana alerting (elastic#61685)"
  Fix visual tests (elastic#62660)
  [Telemetry] update crypto packages (elastic#62469)
  [DOCS] Removed references to left (elastic#60807)
  [Maps] Move layers to np maps (elastic#61877)
  ...
streamich added a commit that referenced this pull request Apr 7, 2020
@streamich @elasticmachine
fix: 🐛 correctly create error on no_matching_indices (#61257) (#62755)
992e4a8 
* fix: 🐛 correctly create error on no_matching_indices

* feat: 🎸 improve error type checking

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
streamich added a commit that referenced this pull request Apr 8, 2020
@streamich @elasticmachine
fix: 🐛 correctly create error on no_matching_indices (#61257) (#62756)
14b41cf 
* fix: 🐛 correctly create error on no_matching_indices

* feat: 🎸 improve error type checking

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@neptunian neptunian mentioned this pull request May 5, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukeelmers lukeelmers lukeelmers approved these changes

Assignees
No one assigned
Labels
bug Fixes for quality problems that affect the customer experience Feature:Data Views Data Views code and UI - index patterns before 8.0 release_note:skip Skip the PR/issue when compiling release notes review v7.7.0 v7.8.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[EPM] Error trying to view index pattern when no matching indices exist
8 participants
@streamich @elasticmachine @neptunian @ppisljar @alexh97 @ruflin @kibanamachine @lukeelmers