-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[search source] ES Query rule loads fewer fields on query execution #183694
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
Changes from all commits
12a7764
2d76169
ccf3289
6252119
eb3b687
dd0fe49
b898c03
e278819
ea2da7f
085f283
12726e2
3b44f04
33de2e8
31ab1c9
7c3bca9
bb876f9
376da3b
fc90fff
a74727a
0ff11ab
ebaaed8
e1db2a8
b638477
48e3563
478b45e
e7d97cf
e4797cc
43865f2
12e8b33
d7b36eb
5d3ab0d
0a87f30
69826e5
e3d2d67
c0a9614
56b36ae
33e7723
8096fb1
e6f70cf
72d709c
f77b0f2
5499ae0
48f6695
885d3dc
e79c326
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 | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -77,13 +77,15 @@ import type * as estypes from '@elastic/elasticsearch/lib/api/typesWithBodyKey'; | |||||||||||||||||||||||
| import { | ||||||||||||||||||||||||
| buildEsQuery, | ||||||||||||||||||||||||
| Filter, | ||||||||||||||||||||||||
| fromKueryExpression, | ||||||||||||||||||||||||
| isOfQueryType, | ||||||||||||||||||||||||
| isPhraseFilter, | ||||||||||||||||||||||||
| isPhrasesFilter, | ||||||||||||||||||||||||
| getKqlFieldNames, | ||||||||||||||||||||||||
| } from '@kbn/es-query'; | ||||||||||||||||||||||||
| import { fieldWildcardFilter } from '@kbn/kibana-utils-plugin/common'; | ||||||||||||||||||||||||
| import { getHighlightRequest } from '@kbn/field-formats-plugin/common'; | ||||||||||||||||||||||||
| import type { DataView } from '@kbn/data-views-plugin/common'; | ||||||||||||||||||||||||
| import { DataView, DataViewLazy, DataViewsContract } from '@kbn/data-views-plugin/common'; | ||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||
| ExpressionAstExpression, | ||||||||||||||||||||||||
| buildExpression, | ||||||||||||||||||||||||
|
|
@@ -134,6 +136,7 @@ export const searchSourceRequiredUiSettings = [ | |||||||||||||||||||||||
| export interface SearchSourceDependencies extends FetchHandlers { | ||||||||||||||||||||||||
| aggs: AggsStart; | ||||||||||||||||||||||||
| search: ISearchGeneric; | ||||||||||||||||||||||||
| dataViews: DataViewsContract; | ||||||||||||||||||||||||
| scriptedFieldsEnabled: boolean; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
|
@@ -712,7 +715,7 @@ export class SearchSource { | |||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| private readonly getFieldName = (fld: SearchFieldValue): string => | ||||||||||||||||||||||||
| typeof fld === 'string' ? fld : (fld.field as string); | ||||||||||||||||||||||||
| typeof fld === 'string' ? fld : (fld?.field as string); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| private getFieldsWithoutSourceFilters( | ||||||||||||||||||||||||
| index: DataView | undefined, | ||||||||||||||||||||||||
|
|
@@ -778,6 +781,47 @@ export class SearchSource { | |||||||||||||||||||||||
| return field; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| public async loadDataViewFields(dataView: DataViewLazy) { | ||||||||||||||||||||||||
| const request = this.mergeProps(this, { body: {} }); | ||||||||||||||||||||||||
| let fields = dataView.timeFieldName ? [dataView.timeFieldName] : []; | ||||||||||||||||||||||||
| const sort = this.getField('sort'); | ||||||||||||||||||||||||
| if (sort) { | ||||||||||||||||||||||||
| const sortArr = Array.isArray(sort) ? sort : [sort]; | ||||||||||||||||||||||||
| for (const s of sortArr) { | ||||||||||||||||||||||||
| const keys = Object.keys(s); | ||||||||||||||||||||||||
| fields = fields.concat(keys); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| for (const query of request.query) { | ||||||||||||||||||||||||
| if (query.query) { | ||||||||||||||||||||||||
| const nodes = fromKueryExpression(query.query); | ||||||||||||||||||||||||
| const queryFields = getKqlFieldNames(nodes); | ||||||||||||||||||||||||
| fields = fields.concat(queryFields); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| const filters = request.filters; | ||||||||||||||||||||||||
| if (filters) { | ||||||||||||||||||||||||
| const filtersArr = Array.isArray(filters) ? filters : [filters]; | ||||||||||||||||||||||||
| for (const f of filtersArr) { | ||||||||||||||||||||||||
| fields = fields.concat(f.meta.key); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| fields = fields.filter((f) => Boolean(f)); | ||||||||||||||||||||||||
|
mattkime marked this conversation as resolved.
|
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if (dataView.getSourceFiltering() && dataView.getSourceFiltering().excludes.length) { | ||||||||||||||||||||||||
| // if source filtering is enabled, we need to fetch all the fields | ||||||||||||||||||||||||
| return (await dataView.getFields({ fieldName: ['*'] })).getFieldMapSorted(); | ||||||||||||||||||||||||
|
Comment on lines
+811
to
+813
Member
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 there's another case where we need to load all fields. like @lukasolson mentioned about his code that is used for translating KQL to ES DSL: kibana/packages/kbn-es-query/src/kuery/functions/is.ts Lines 109 to 112 in 46fbfd3
@lukasolson: I think this is essentially for if you do something like * or : So if we detect there's just a
Contributor
Author
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 not following but this clearly needs to be addressed before this PR is merged. @lukasolson can you explain?
Member
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. it seems my copy paste did swallow significant details, when users search
Contributor
Author
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. @kertal are you saying all fields need to loaded via field_caps? If so, how does the field_cap content inform the request?
Member
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. well if one of the
Contributor
Author
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. A field might be part of a request without first being loaded by field caps.
Member
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. how? in this part of the code, we collect all fields that we might need later on to build the ES query, which field might be missed?
Member
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. we talked in our 1:1 about this, the last missing fields should be all date fields, but these are just used for formatting. to trigger an alert this formatting is not necessary, but the question is, in an alert notification you can add the document triggering the alert, here it might be necessary to apply correct formatting. This is solveable sending 2 field caps requests. 1 for the mandatory fields of query, filter,timestamp ... 1 for all date fields for formatting, combining those into 1 field list on the data view. just 1 request would be better, however 2 requests for a smaller amount of fields should also be a big improvement. |
||||||||||||||||||||||||
| } else if (fields.length) { | ||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||
| await dataView.getFields({ | ||||||||||||||||||||||||
| fieldName: fields, | ||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||
| ).getFieldMapSorted(); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| // no fields needed to be loaded for query | ||||||||||||||||||||||||
| return {}; | ||||||||||||||||||||||||
|
Comment on lines
+815
to
+822
Member
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. The way I see it, the only thing for feature parity to build the same request like before is to get all time fields which are added to to ensure they have the same formatting: kibana/src/plugins/data/common/search/search_source/search_source.ts Lines 765 to 775 in 844f515
this code was introduced in #91478 to overcome elastic/elasticsearch#67063 We could keep this behavior by adding another request, just for So the question is should we add fetching of all date/date_nanos fields for this case? wdyt @elastic/kibana-data-discovery ?
Contributor
Author
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. If I understand correctly, this sounds like an optimization over the current code in this PR. I'd like to capture this idea in and issue along with any other compromises with this implementation. I currently have some optimism this can be resolved through ES support for field formatters by type.
Member
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. yes, this can definitely be resolved by ES, and it's good to list the compromises. In this case I'm not sure we need to resolve it, since it has no impact on the functionality of an alert, also in case it's configured, the values are returned correctly, it's just the formatting might differ .. if the alert is configured to embed documents in the notification
Contributor
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. Yeah, just played around with this a bit and I'm noticing that alert messages can actually change after this PR... So imagine I have a document with two fields, Prior to this PR it would actually show the value for each of those: After this PR it only shows the first three:
Contributor
Author
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. As best I understand at this point - we have the choice of either always fetching all date fields OR we need to risk breakage (however small) of
Member
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. @lukasolson thx for testing, the date_nanos case is interesting, also that I expected {{fields.date_nanos}} to have a value available, that's strange. If the like @mattkime mentions we could prevent this by fetching all date/data_nanos fields additionally to the fields we extracted by query + filter. And then we could revert this when elastic/elasticsearch#103705 is implemented, which allows just to assign one format for all date/date_nanos fields
Member
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. Update from my end: The way it was implemented before, date fields were just available because of the formatting in the request. No other fields were available, just |
||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| private flatten() { | ||||||||||||||||||||||||
| const { getConfig } = this.dependencies; | ||||||||||||||||||||||||
| const metaFields = getConfig(UI_SETTINGS.META_FIELDS) ?? []; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.