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

[Discover] Suppress "Missing index" toasts #149625

Merged
merged 43 commits into from
Feb 14, 2023

Conversation

jughosta
Copy link
Contributor

@jughosta jughosta commented Jan 26, 2023

Closes #129020

Screenshot 2023-01-27 at 16 56 16

@jughosta jughosta added release_note:enhancement Feature:Discover Discover Application backport:skip This commit does not require backporting Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Data, DataViews) labels Jan 26, 2023
@jughosta jughosta self-assigned this Jan 26, 2023
@kertal
Copy link
Member

kertal commented Jan 27, 2023

Just tested in by deleting the e-commerce data while keeping the data view. Works as expected 🥳

@jughosta jughosta marked this pull request as ready for review January 30, 2023 13:00
@jughosta jughosta requested review from a team as code owners January 30, 2023 13:00
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@stratoula
Copy link
Contributor

cc @drewdaemon

Maybe we could also hide the toast here to be consistent with Discover. I also like the error that is being displayed in Discover. Wdyt?

image

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Unified search changes LGTM!

@drewdaemon
Copy link
Contributor

@stratoula totally.

@kertal
Copy link
Member

kertal commented Jan 30, 2023

@stratoula @drewdaemon @stratoula unified_error_message, btw @drewdaemon about the one comment in your RFC I've added, the error message @stratoula is referring to, might also go away when

Object.values(adHocDataViews || {}).map((spec) => dataViews.create(spec))

is changed to dataViews.create(spec, false, false) ... the 3rd param is setting displayError to false

@stratoula
Copy link
Contributor

Created an issue here and I will add this to our 8.8 backlog #149836

Copy link
Contributor

@mattkime mattkime left a comment

Choose a reason for hiding this comment

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

Overall the changes are very good - and very helpful! While you spotted some necessary changes for the data_views service, I have questions regarding a couple.

@@ -261,7 +265,10 @@ export interface DataViewsServicePublicMethods {
* @params savedObject - Data view saved object
* @params displayErrors - If set false, API consumer is responsible for displaying and handling errors.
*/
savedObjectToSpec: (savedObject: SavedObject<DataViewAttributes>) => DataViewSpec;
savedObjectToSpec: (
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why this is needed as savedObjectToSpec doesn't display toasts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattkime Right, I cleaned that up.

this.onNotification(
{ title: err.message, color: 'danger', iconType: 'alert' },
`refreshFields:${dataView.getIndexPattern()}`
if (displayErrors) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused about the work this change is doing. As best I can tell it swallows errors since the error is caught but not re-thrown which isn't desired.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattkime The previous code was causing an unhandled promise error which was not allowing to switch from SQL to regular mode in UI (for a data view with missing index)

if (!displayErrors) {
  return this.refreshFieldsFn(dataView);
}

Screenshot 2023-01-31 at 10 15 43

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you handle the exception in the api consumer instead of changing the api?

Copy link
Contributor Author

@jughosta jughosta Feb 1, 2023

Choose a reason for hiding this comment

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

@mattkime Thanks for your suggestion! The thing is that the consumer is the data views service itself

await this.refreshFields(dataView, displayErrors);

Would you suggest to add try-catch there depending on displayErrors?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, at least in theory the error will propagate up to the api consumer's call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, at least in theory the error will propagate up to the api consumer's call.

@mattkime dataView.get() could still return a data view object for its consumer if we suppress exceptions for refreshField. If we change as you suggest then the consumer code would not be able to proceed which conflicts with the purpose of this PR.

We can't both prevent unhandled errors AND pass errors to API consumers.

Agree with that. I'm not happy with the PR changes either. I can close it if that's our conclusion.

Copy link
Member

Choose a reason for hiding this comment

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

Agree, there should be one way to handle it: to suppress the errors or to pass errors to the API consumers. This should also be clearly communicated to consumers, currently it seems it's a mixed handling of those cases?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we change as you suggest then the consumer code would not be able to proceed

@jughosta Perhaps we should set up some time to discuss this. There's something I'm not understanding here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kertal

Agree, there should be one way to handle it: to suppress the errors or to pass errors to the API consumers. This should also be clearly communicated to consumers, currently it seems it's a mixed handling of those cases?

I think our language might be conflating things. Currently, the api either displays errors or throws errors - and thrown errors need to be caught by api consumer code. 'Suppressing errors' could ambiguously mean either of those.

First we need to remove the possibility of errors from our communication regarding errors to successfully deal with errors. 🥲

Copy link
Contributor

@mattkime mattkime Feb 3, 2023

Choose a reason for hiding this comment

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

I created https://github.com/mattkime/kibana/pull/new/129020-inline-missing-index-error-mk to illustrate what I see as the path forward, particularly https://github.com/elastic/kibana/pull/150246/files#diff-eeffc98614da75c1f8e6ce9b07802c11dce0ae41f80b9540784f0f44c2e86133R307 - Now I think I did this before understanding what @jughosta was saying.

@jughosta - It took me a second reading to understand what you were looking to do -

[@mattkime] dataView.get() could still return a data view object for its consumer if we suppress exceptions for refreshField. If we change as you suggest then the consumer code would not be able to proceed which conflicts with the purpose of this PR.

Basically you're saying you want to be able to get a data view even if you can't get the field list. This isn't something thats currently supported by the API. A data view without a field list is of very limited use in most circumstances. Its an interesting idea. Current API consumers always assume the full field list is there - they don't have a notion of a data view with an unloaded field list. This is something I'd like to see changed but it would be a significant effort on the part of API consumers. Its actually something requested by solutions which deal with very large numbers of fields - that way they can choose when the field list is loaded.

{dataViewPattern ? (
<FormattedMessage
id="discover.noResults.noDocumentsOrCheckIndicesAndPermissionsDescription"
defaultMessage="Make sure the indices matching {dataViewPattern} exist, that you have permission to view them and that they contain documents."
Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion - although it should probably get 👀 from @gchaps - Make sure data view {dataViewName} with index pattern {dataViewPattern} has matching indices and documents and that you have permission to view them.


Does this work properly for text based languages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattkime Thanks, updated!

Screenshot 2023-01-31 at 10 25 25

For SQL mode we show an error in this case rather than the message:
Screenshot 2023-01-31 at 10 15 15

Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion is to add the 2 words in bold:

Make sure that the data view {dataViewName} with index pattern {dataViewPattern} has matching indices and documents and that you have permission to view them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, can we update the error message:

Instead of

We encountered an error retrieving search results

how about

Unable to retrieve search results

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @gchaps !

@@ -218,7 +218,7 @@ export function useDiscoverState({
*/
const onChangeDataView = useCallback(
async (id: string) => {
const nextDataView = await dataViews.get(id);
const nextDataView = await dataViews.get(id, false);
Copy link
Contributor

@mattkime mattkime Feb 1, 2023

Choose a reason for hiding this comment

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

Anyplace where we're suppressing errors potentially needs a try/catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattkime Updated by wrapping with try/catch.

@kertal kertal requested a review from mattkime February 8, 2023 18:47
@mattkime mattkime requested a review from a team as a code owner February 9, 2023 05:13
@jughosta jughosta added the v8.8.0 label Feb 9, 2023
});
} catch (error) {
//
if (dataView?.isTimeBased() && dataView?.timeFieldName && query && mountedRef.current) {
Copy link
Contributor

Choose a reason for hiding this comment

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

dataView?.isTimeBased() includes checking dataView?.timeFieldName

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattkime Thanks!

Copy link
Contributor

@mattkime mattkime left a comment

Choose a reason for hiding this comment

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

One minor note, otherwise the code looks good and works well

@jughosta jughosta enabled auto-merge (squash) February 14, 2023 10:15
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
data 2578 2584 +6
dataViews 231 243 +12
total +18

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
discover 391.7KB 392.3KB +608.0B
unifiedSearch 264.7KB 264.7KB +42.0B
total +650.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
dataViews 44.0KB 44.0KB +83.0B
unifiedFieldList 38.9KB 39.0KB +109.0B
total +192.0B
Unknown metric groups

API count

id before after diff
data 3276 3280 +4
dataViews 1020 1028 +8
total +12

ESLint disabled line counts

id before after diff
unifiedFieldList 18 17 -1

References to deprecated APIs

id before after diff
discover 41 36 -5

Total ESLint disabled count

id before after diff
unifiedFieldList 18 17 -1

History

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

cc @jughosta

@jughosta jughosta merged commit 668fe89 into elastic:main Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Discover Discover Application release_note:enhancement Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Data, DataViews) v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Discover] Show "no matching indices found" error inside the callout
9 participants