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 all 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 ...pment/plugins/data/public/kibana-plugin-plugins-data-public.querystringinput.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@
<b>Signature:</b>

```typescript
QueryStringInput: React.FC<Pick<Props, "query" | "prepend" | "size" | "placeholder" | "onChange" | "onBlur" | "onSubmit" | "indexPatterns" | "dataTestSubj" | "screenTitle" | "disableAutoFocus" | "persistedLog" | "bubbleSubmitEvent" | "languageSwitcherPopoverAnchorPosition" | "onChangeQueryInputFocus">>
React.FC<Pick<Props, "query" | "prepend" | "size" | "className" | "placeholder" | "onChange" | "onBlur" | "onSubmit" | "indexPatterns" | "dataTestSubj" | "screenTitle" | "disableAutoFocus" | "persistedLog" | "bubbleSubmitEvent" | "languageSwitcherPopoverAnchorPosition" | "onChangeQueryInputFocus">>
```
1 change: 1 addition & 0 deletions ...ent/plugins/data/public/kibana-plugin-plugins-data-public.searchsourcefields.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export interface SearchSourceFields
| --- | --- | --- |
| [aggs](./kibana-plugin-plugins-data-public.searchsourcefields.aggs.md) | <code>any</code> | |
| [fields](./kibana-plugin-plugins-data-public.searchsourcefields.fields.md) | <code>NameList</code> | |
| [fieldsApi](./kibana-plugin-plugins-data-public.searchsourcefields.fieldsapi.md) | <code>NameList</code> | |
| [filter](./kibana-plugin-plugins-data-public.searchsourcefields.filter.md) | <code>Filter[] &#124; Filter &#124; (() =&gt; Filter[] &#124; Filter &#124; undefined)</code> | |
| [from](./kibana-plugin-plugins-data-public.searchsourcefields.from.md) | <code>number</code> | |
| [highlight](./kibana-plugin-plugins-data-public.searchsourcefields.highlight.md) | <code>any</code> | |
Expand Down
2 changes: 2 additions & 0 deletions src/plugins/data/public/public.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1771,6 +1771,8 @@ export interface SearchSourceFields {
// (undocumented)
fields?: NameList;
// (undocumented)
fieldsApi?: NameList;
// (undocumented)
filter?: Filter[] | Filter | (() => Filter[] | Filter | undefined);
// (undocumented)
from?: number;
Expand Down
2 changes: 2 additions & 0 deletions 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 @@ -381,6 +381,8 @@ export class SearchSource {
case 'fields':
const fields = uniq((data[key] || []).concat(val));
return addToRoot(key, fields);
case 'fieldsApi':
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to start this as a larger discussion with App Arch and Stacey (feel free to ping them here once you want to open this PR for review). I am a bit torn on already having a fields API in the search source and now add a fieldsApi parameter to it, that will set this to the fields part of the query. I think the following code as a consumer is just not really understandable:

searchSource
  .setField('fields', ['bar', 'baz'])
  .setField('fieldsApi', ['some_field']);

Since SearchSource is a rather present API, we're needing in a lot of places, I'd not want to dilute it for a work-around that we need in Discover.

The fields field in search source, already does multiple things, like stored_fields and docvalue_fields. I think we should rather make sure that key will also handle fields (in the ES query) correctly. Maybe we can even replace stored_fields and docvalue_fields in that API completely by using the fields API for them too.

tl;dr: I think we need to have some discussion with App Arch to find a proper longer term solution for this part, the other part looks as the initial workaround fine for me.

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 @elastic/kibana-app-arch & @stacey-gammon

so the requirements are, that SearchSource would would provide access to the search-fields API

https://www.elastic.co/guide/en/elasticsearch/reference/master/search-fields.html

Like Tim mentioned the fields field was already taken, modifying it's functionality broke several functional tests, so I've added a fieldsApi field, which would provide the functionality Discover would need. Maybe it would make sense to create a separate issue for this, unless everybody thinks the solution provided in this PR is fine. I also would prefer a single fields field in SearchSource, but the approach I've tried to implement here broke the following functional tests:
https://kibana-ci.elastic.co/job/elastic+kibana+pipeline-pull-request/73366/

Copy link
Contributor

Choose a reason for hiding this comment

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

++ to creating a separate issue to solve this. I think we should make sure the changes made to search source are developer friendly, documented, tested, etc, and not exposed as a hack to solve this particular problem in the short term.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's the issue: #77241

Copy link
Member

Choose a reason for hiding this comment

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

thanks @kertal, we'll discuss this and get back to you. is this a blocker for discover at the moment ?

Copy link
Member

Choose a reason for hiding this comment

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

hmm, that issue seems very general, should we open one for discover specifically ? what exactly is the issue for discover, the request searchsource sends out or the response provided ? searchsource will return full response so its up to you how you parse it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ppisljar I've created an issue #77300 but closed in favor of #77241. It's basically the question: how to extend SearchSource in a non-hacky way to have access to the fields API of search. Without the extension runtime fields don't work in Discover.

Copy link
Contributor

Choose a reason for hiding this comment

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

#77107 is the Discover specific issue that this PR attempts to fix.

return key && data[key] == null && addToBody('fields', val);
case 'index':
case 'type':
case 'highlightAll':
Expand Down
1 change: 1 addition & 0 deletions src/plugins/data/public/search/search_source/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ export interface SearchSourceFields {
source?: NameList;
version?: boolean;
fields?: NameList;
fieldsApi?: NameList;
index?: IndexPattern;
searchAfter?: EsQuerySearchAfter;
timeout?: string;
Expand Down
8 changes: 7 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,14 @@ function discoverController($element, $route, $scope, $timeout, $window, Promise

$scope.updateDataSource = () => {
const { indexPattern, searchSource } = $scope;
const { docvalueFields } = indexPattern.getComputedFields();
const columns = $scope.state.columns.filter(
(name) => !docvalueFields.find((docVal) => docVal.field === name)
);
searchSource
.setField('index', $scope.indexPattern)
.setField('fieldsApi', columns)
.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