[Discover] Experimental usage of ES fields API #75407
Closed
kertal wants to merge 13 commits intoelastic:masterfrom
Closed
[Discover] Experimental usage of ES fields API #75407kertal wants to merge 13 commits intoelastic:masterfrom
kertal wants to merge 13 commits intoelastic:masterfrom
Conversation
kertal
commented
Aug 19, 2020
|
|
||
| // detect changes that should trigger fetching of new data | ||
| const changes = ['interval', 'sort'].filter( | ||
| const changes = ['interval', 'sort', 'columns'].filter( |
Member
Author
There was a problem hiding this comment.
need to resubmit the query to display subfields like name.keyword
Contributor
There was a problem hiding this comment.
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
Member
Author
There was a problem hiding this comment.
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?
Contributor
There was a problem hiding this comment.
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.
…-19-use-es-fields-api
…-19-use-es-fields-api
…-19-use-es-fields-api
kertal
commented
Sep 9, 2020
Comment on lines
467
to
469
| const remainingFields = difference(fields, keys(body.script_fields)); | ||
| body.stored_fields = remainingFields; | ||
| body.fields = remainingFields; | ||
| setWith(body, '_source.includes', remainingFields, (nsValue) => |
Member
Author
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
@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.
Member
Author
There was a problem hiding this comment.
@jtibshirani thx, i've solved it differently now!
Contributor
💔 Build Failed
Failed CI StepsTest FailuresChrome X-Pack UI Functional Tests.x-pack/test/functional/apps/ml/data_visualizer/file_data_visualizer·ts.machine learning data visualizer file based with an artificial server log displays and imports a fileStandard OutStack TraceChrome X-Pack UI Functional Tests.x-pack/test/functional/apps/ml/data_visualizer/file_data_visualizer·ts.machine learning data visualizer file based with an artificial server log displays and imports a fileStandard OutStack TraceBuild metricsasync chunks size
page load bundle size
History
To update your PR or re-run it, just comment with: |
timroes
reviewed
Sep 10, 2020
| case 'fields': | ||
| const fields = uniq((data[key] || []).concat(val)); | ||
| return addToRoot(key, fields); | ||
| case 'fieldsApi': |
Contributor
There was a problem hiding this comment.
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.
Member
Author
There was a problem hiding this comment.
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/
There was a problem hiding this comment.
++ 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.
Contributor
There was a problem hiding this comment.
thanks @kertal, we'll discuss this and get back to you. is this a blocker for discover at the moment ?
Contributor
There was a problem hiding this comment.
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.
Member
Author
There was a problem hiding this comment.
#77107 is the Discover specific issue that this PR attempts to fix.
7 tasks
Member
Author
|
Closing this since it's a POC, and there will be no iteration |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR experiments with the usage of the fields API, as a side effect it enables displaying alias fields in Discover in a better way (there were no values displayed in the table).
Info & PR:
https://www.elastic.co/guide/en/elasticsearch/reference/master/search-fields.html. And here's the ES meta issue elastic/elasticsearch#55363
Testdata
Checklist
Delete any items that are not applicable to this PR.
For maintainers