-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Changes from 12 commits
e4b39fe
5460f94
fa3e071
dd6ccc3
eafced6
4cc1910
6dc15f5
dd1a27c
0a98ad9
d92cce3
a1b4780
2b05940
8f1e30e
4c386d9
4902725
a82fcbe
07e3b6d
c176bbd
d7cb8dd
8a3ab85
55feb95
4ac71ab
bfa2052
f6c09e0
2ec2293
9b051f4
dc46db3
e317a65
eeff695
bce9870
10e3d8a
d58a97c
3b866c7
afb6834
1b05045
2a60fb9
5df9cf1
cf76c87
fa996e8
c472b93
c35a269
aee44c4
1aceefa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -215,8 +215,12 @@ export interface DataViewsServicePublicMethods { | |||
/** | ||||
* Get default data view, if it doesn't exist, choose and save new default data view and return it. | ||||
* @param refreshFields - refresh field list when true | ||||
* @param displayErrors - If set false, API consumer is responsible for displaying and handling errors. | ||||
*/ | ||||
getDefaultDataView: (refreshFields?: boolean) => Promise<DataView | null>; | ||||
getDefaultDataView: ( | ||||
refreshFields?: boolean, | ||||
displayErrors?: boolean | ||||
) => Promise<DataView | null>; | ||||
/** | ||||
* Get fields for data view | ||||
* @param dataView - Data view instance or spec | ||||
|
@@ -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: ( | ||||
savedObject: SavedObject<DataViewAttributes>, | ||||
displayErrors?: boolean | ||||
) => DataViewSpec; | ||||
/** | ||||
* Set default data view. | ||||
* @param id - Id of the data view to set as default. | ||||
|
@@ -570,30 +577,28 @@ export class DataViewsService { | |||
* @param displayErrors - If set false, API consumer is responsible for displaying and handling errors. | ||||
*/ | ||||
refreshFields = async (dataView: DataView, displayErrors: boolean = true) => { | ||||
if (!displayErrors) { | ||||
return this.refreshFieldsFn(dataView); | ||||
} | ||||
|
||||
try { | ||||
await this.refreshFieldsFn(dataView); | ||||
} catch (err) { | ||||
if (err instanceof DataViewMissingIndices) { | ||||
this.onNotification( | ||||
{ title: err.message, color: 'danger', iconType: 'alert' }, | ||||
`refreshFields:${dataView.getIndexPattern()}` | ||||
if (displayErrors) { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Would you suggest to add try-catch there depending on There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@mattkime
Agree with that. I'm not happy with the PR changes either. I can close it if that's our conclusion. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@jughosta Perhaps we should set up some time to discuss this. There's something I'm not understanding here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. 🥲 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 -
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. |
||||
if (err instanceof DataViewMissingIndices) { | ||||
this.onNotification( | ||||
{ title: err.message, color: 'danger', iconType: 'alert' }, | ||||
`refreshFields:${dataView.getIndexPattern()}` | ||||
); | ||||
} | ||||
|
||||
this.onError( | ||||
err, | ||||
{ | ||||
title: i18n.translate('dataViews.fetchFieldErrorTitle', { | ||||
defaultMessage: 'Error fetching fields for data view {title} (ID: {id})', | ||||
values: { id: dataView.id, title: dataView.getIndexPattern() }, | ||||
}), | ||||
}, | ||||
dataView.getIndexPattern() | ||||
); | ||||
} | ||||
|
||||
this.onError( | ||||
err, | ||||
{ | ||||
title: i18n.translate('dataViews.fetchFieldErrorTitle', { | ||||
defaultMessage: 'Error fetching fields for data view {title} (ID: {id})', | ||||
values: { id: dataView.id, title: dataView.getIndexPattern() }, | ||||
}), | ||||
}, | ||||
dataView.getIndexPattern() | ||||
); | ||||
} | ||||
}; | ||||
|
||||
|
@@ -610,7 +615,8 @@ export class DataViewsService { | |||
id: string, | ||||
title: string, | ||||
options: GetFieldsOptions, | ||||
fieldAttrs: FieldAttrs = {} | ||||
fieldAttrs: FieldAttrs = {}, | ||||
displayErrors: boolean = true | ||||
) => { | ||||
const fieldsAsArr = Object.values(fields); | ||||
const scriptedFields = fieldsAsArr.filter((field) => field.scripted); | ||||
|
@@ -630,10 +636,12 @@ export class DataViewsService { | |||
return { fields: this.fieldArrayToMap(updatedFieldList, fieldAttrs), indices }; | ||||
} catch (err) { | ||||
if (err instanceof DataViewMissingIndices) { | ||||
this.onNotification( | ||||
{ title: err.message, color: 'danger', iconType: 'alert' }, | ||||
`refreshFieldSpecMap:${title}` | ||||
); | ||||
if (displayErrors) { | ||||
this.onNotification( | ||||
{ title: err.message, color: 'danger', iconType: 'alert' }, | ||||
`refreshFieldSpecMap:${title}` | ||||
); | ||||
} | ||||
return {}; | ||||
} | ||||
|
||||
|
@@ -739,9 +747,11 @@ export class DataViewsService { | |||
private initFromSavedObjectLoadFields = async ({ | ||||
savedObjectId, | ||||
spec, | ||||
displayErrors = true, | ||||
}: { | ||||
savedObjectId: string; | ||||
spec: DataViewSpec; | ||||
displayErrors?: boolean; | ||||
}) => { | ||||
const { title, type, typeMeta, runtimeFieldMap } = spec; | ||||
const { fields, indices } = await this.refreshFieldSpecMap( | ||||
|
@@ -755,7 +765,8 @@ export class DataViewsService { | |||
rollupIndex: typeMeta?.params?.rollup_index, | ||||
allowNoIndex: spec.allowNoIndex, | ||||
}, | ||||
spec.fieldAttrs | ||||
spec.fieldAttrs, | ||||
displayErrors | ||||
); | ||||
|
||||
const runtimeFieldSpecs = this.getRuntimeFields(runtimeFieldMap, spec.fieldAttrs); | ||||
|
@@ -779,6 +790,7 @@ export class DataViewsService { | |||
const fieldsAndIndices = await this.initFromSavedObjectLoadFields({ | ||||
savedObjectId: savedObject.id, | ||||
spec, | ||||
displayErrors, | ||||
}); | ||||
fields = fieldsAndIndices.fields; | ||||
indices = fieldsAndIndices.indices; | ||||
|
@@ -883,7 +895,7 @@ export class DataViewsService { | |||
): Promise<DataView> => { | ||||
const dataViewFromCache = this.dataViewCache.get(id)?.then(async (dataView) => { | ||||
if (dataView && refreshFields) { | ||||
await this.refreshFields(dataView); | ||||
await this.refreshFields(dataView, displayErrors); | ||||
} | ||||
return dataView; | ||||
}); | ||||
|
@@ -1145,9 +1157,13 @@ export class DataViewsService { | |||
* If no possible data view found to become a default returns null. | ||||
* | ||||
* @param {boolean} refreshFields - if true, will refresh the fields of the default data view | ||||
* @param {boolean} displayErrors - If set false, API consumer is responsible for displaying and handling errors. | ||||
* @returns default data view | ||||
*/ | ||||
async getDefaultDataView(refreshFields?: boolean): Promise<DataView | null> { | ||||
async getDefaultDataView( | ||||
refreshFields?: boolean, | ||||
displayErrors: boolean = true | ||||
): Promise<DataView | null> { | ||||
const patterns = await this.getIdsWithTitle(); | ||||
let defaultId: string | undefined = await this.config.get('defaultIndex'); | ||||
const exists = defaultId ? patterns.some((pattern) => pattern.id === defaultId) : false; | ||||
|
@@ -1168,7 +1184,7 @@ export class DataViewsService { | |||
} | ||||
|
||||
if (defaultId) { | ||||
return this.get(defaultId, undefined, refreshFields); | ||||
return this.get(defaultId, displayErrors, refreshFields); | ||||
} else { | ||||
return null; | ||||
} | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,16 +8,29 @@ | |
|
||
import React from 'react'; | ||
import { FormattedMessage } from '@kbn/i18n-react'; | ||
import { EuiDescriptionList, EuiDescriptionListDescription } from '@elastic/eui'; | ||
import { EuiDescriptionList, EuiDescriptionListDescription, EuiCode } from '@elastic/eui'; | ||
import { useInternalStateSelector } from '../../../services/discover_internal_state_container'; | ||
|
||
export function NoResultsSuggestionDefault() { | ||
const dataViewPattern = useInternalStateSelector((state) => state.dataView?.getIndexPattern?.()); | ||
|
||
return ( | ||
<EuiDescriptionList compressed> | ||
<EuiDescriptionListDescription data-test-subj="discoverNoResultsCheckIndices"> | ||
<FormattedMessage | ||
id="discover.noResults.noDocumentsOrCheckPermissionsDescription" | ||
defaultMessage="Make sure you have permission to view the indices and that they contain documents." | ||
/> | ||
{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." | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My suggestion - although it should probably get 👀 from @gchaps - Does this work properly for text based languages? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My suggestion is to add the 2 words in bold:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, can we update the error message: Instead of
how about
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @gchaps ! |
||
values={{ | ||
dataViewPattern: <EuiCode>{dataViewPattern}</EuiCode>, | ||
}} | ||
/> | ||
) : ( | ||
<FormattedMessage | ||
id="discover.noResults.noDocumentsOrCheckPermissionsDescription" | ||
defaultMessage="Make sure you have permission to view the indices and that they contain documents." | ||
jughosta marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/> | ||
)} | ||
</EuiDescriptionListDescription> | ||
</EuiDescriptionList> | ||
); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Anyplace where we're suppressing errors potentially needs a try/catch There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mattkime Updated by wrapping with try/catch. |
||
if (nextDataView && dataView) { | ||
const nextAppState = getDataViewAppState( | ||
dataView, | ||
|
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.