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

[Discover] Experimental usage of ES fields API #75407

Closed
wants to merge 13 commits into from
Closed

[Discover] Experimental usage of ES fields API #75407

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
4662f35
Convert ES body of discover doc request to use fields API
kertal Aug 19, 2020
379fd8c
Adapt search_source.ts and discover to display alias fields
kertal Aug 19, 2020
008bdc1
Add source to field
kertal Aug 19, 2020
0c7bf3a
Submit request once columns changed
kertal Aug 19, 2020
4650db8
Request all fields in doc view
kertal Aug 19, 2020
2d4b73a
Merge remote-tracking branch 'upstream/master' into kertal-pr-2020-08…
kertal Aug 21, 2020
3d5e8b7
Force rerendering of table row
kertal Aug 21, 2020
d19e7ee
Merge remote-tracking branch 'upstream/master' into kertal-pr-2020-08…
kertal Aug 24, 2020
17d9bce
Merge remote-tracking branch 'upstream/master' into kertal-pr-2020-08…
kertal Sep 9, 2020
f84b36d
Undo use_es_doc_search.ts changes
kertal Sep 9, 2020
32c0184
Merge selected fields with doc_value fields
kertal Sep 9, 2020
9ed5a95
Fix scripted fields tests
kertal Sep 9, 2020
9c5160f
Extend SearchSource with fieldsApi handling
kertal Sep 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/plugins/data/public/search/search_source/search_source.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,7 +465,7 @@ export class SearchSource {

// request the remaining fields from both stored_fields and _source
const remainingFields = difference(fields, keys(body.script_fields));
body.stored_fields = remainingFields;
body.fields = remainingFields;
setWith(body, '_source.includes', remainingFields, (nsValue) =>
Copy link
Member Author

Choose a reason for hiding this comment

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

dear @jtibshirani , with this change Discover via the underlying search service would use the fields API, and would therefore support runtime fields. Question is: could there be fields available in stored_fields that aren't in the fields API? thx!

Copy link

Choose a reason for hiding this comment

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

@kertal stored_fields are not included in the 'fields' option. So yes, there could be fields available in stored_fields that do not come back in fields.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jtibshirani thx, i've solved it differently now!

isObject(nsValue) ? {} : nsValue
);
Expand Down
13 changes: 12 additions & 1 deletion src/plugins/discover/public/application/angular/discover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ function discoverController($element, $route, $scope, $timeout, $window, Promise
$scope.state = { ...newState };

// detect changes that should trigger fetching of new data
const changes = ['interval', 'sort'].filter(
const changes = ['interval', 'sort', 'columns'].filter(
Copy link
Member Author

Choose a reason for hiding this comment

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

need to resubmit the query to display subfields like name.keyword

Copy link
Contributor

Choose a reason for hiding this comment

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

I am a bit concerned around performance of that part. Right now we're not doing additional queries (to fetch data) if we modify the columns. Especially given that the Discover query is atm very unoptimized (e.g. contains the aggregation for the chart in each fetch for the data), and thus can potentially also be rather slow-ish, doing this now for every column add/remove/reorder feels like a significant performance regression (e.g. on our issue instance, which is not having a significant large dataset, every discover request takes ~2s).

Especially when a user starts building a discover view, she'll need to first add a couple of columns, which will then all trigger another request and we could slow down the creation time and UX significantly with this change. We could optimize this code to e.g. exclude column reorders, but I have the feeling we're still in a problematic situation afterwards. I currently don't see how we could check if a change to column would require us to refetch, since we're not knowing which of the column fields could be runtime fields and thus require a refetch here.

I am happy for any suggestions here. cc @stacey-gammon

Copy link
Member Author

Choose a reason for hiding this comment

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

I am a bit concerned around performance of that part. Right now we're not doing additional queries (to fetch data) if we modify the columns.

Yes, this is the disadvantage of this solution, but we since we currently don't know which fields are runtime, we would currently need to fetch all fields + _source as an alternative.

I currently don't see how we could check if a change to column would require us to refetch, since we're not knowing which of the column fields could be runtime fields and thus require a refetch here.

Wouldn't each new fetch abort the old one?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is the disadvantage of this solution, but we since we currently don't know which fields are runtime, we would currently need to fetch all fields + _source as an alternative.

I think we need to evaluate and try to quantify the fallout of this change a bit better. We should not make this kind of larger performance impacts in the most uses application in Kibana willy-nilly and hopef or the best.

Wouldn't each new fetch abort the old one?

That would miticate the problem a bit, but without the quantification I have the feeling we still run into problems, since if we assume a 2 second request, this could correlate very roughly to the interaction time the user needs to get to the next field they want to add, so they actually never run into the situation where the previous request would be cancelled, but into a situation where the UI every time they want to add the next column is frozen for some time, because we're parsing some large JSON response and redrawing the page.

(prop) => !_.isEqual(newStatePartial[prop], oldStatePartial[prop])
);

Expand Down Expand Up @@ -948,8 +948,19 @@ function discoverController($element, $route, $scope, $timeout, $window, Promise

$scope.updateDataSource = () => {
const { indexPattern, searchSource } = $scope;
const indexPatternFields = indexPattern.getComputedFields();
const columns = $scope.state.columns.filter((name) => {
return !indexPatternFields.docvalueFields.find((docVal) => docVal.field === name);
});
const fields = [
...columns,
...indexPatternFields.docvalueFields,
...Object.keys(indexPatternFields.scriptFields),
];
searchSource
.setField('index', $scope.indexPattern)
.setField('fields', fields)
.setField('source', true)
.setField('size', $scope.opts.sampleSize)
.setField(
'sort',
Expand Down
10 changes: 7 additions & 3 deletions src/plugins/discover/public/application/angular/doc_table/components/table_row.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,12 @@ export function createTableRowDirective($compile: ng.ICompileService, $httpParam
$compile($detailsTr)($detailsScope);
};

$scope.$watchMulti(['indexPattern.timeFieldName', 'row.highlight', '[]columns'], () => {
createSummaryRow($scope.row);
});
$scope.$watchMulti(
['indexPattern.timeFieldName', 'row.highlight', '[]columns', 'row'],
() => {
createSummaryRow($scope.row);
}
);

$scope.inlineFilter = function inlineFilter($event: any, type: string) {
const column = $($event.target).data().column;
Expand Down Expand Up @@ -174,6 +177,7 @@ export function createTableRowDirective($compile: ng.ICompileService, $httpParam
let $cells = $el.children();
newHtmls.forEach(function (html, i) {
const $cell = $cells.eq(i);

if ($cell.data('discover:html') === html) return;

const reuse = find($cells.slice(i + 1), function (cell: any) {
Expand Down