-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Autocomplete] Support useTimeFilter option #81515
[Autocomplete] Support useTimeFilter option #81515
Conversation
…r-in-autocomplete
|
||
function resolver(title: string, field: IFieldType, query: string, boolFilter: any) { | ||
function resolver(title: string, field: IFieldType, query: string, filters: any[]) { |
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.
Can this be filters: Filter[]
in this whole file?
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.
These are not filters of type Filter
, but an array of whatever type BoolQuery.filter
is ( src/plugins/data/server/search/es_search/elasticsearch.ts
).
We could fix that, but I wouldn't want to do it in this PR.
I think it's reasonable for us to add an option for the time filter to be applied to the autocomplete suggestions. However, if we've been ignoring the time filter for autocomplete for the entirety of 7.x, or since its implementation mid 7.x, changing this behavior is a breaking change. Is my understanding correct that we've had enough users experience issues with the performance/health of their ES clusters that we should make this breaking change in a minor? @lizozom, I really like the detailed analysis that you performed in #46054 (comment). Is my understanding correct that without these changes, we're in the "TERMS w/totals, wo/timerange, wo/sort" column? Is there a column that represents the performance with only these changes? I don't think it's absolutely pertinent for us to benchmark this if we have the expert guidance from the ES team, I'm primarily asking out of curiosity. |
@kobelb the nearest benchmark would be the second column. Adding sorting didn't improve the results much. Since this is a serious problem, I guess fixing it in a minor would make sense, but I do think we need to at least find a way to resolve the inconsistencies I mentioned above as well as the memoization problem. |
Thanks so much for being so explicit with the corner cases of this approach Liza! @elastic-jb - do you know what the original reason was for removing the time filter from auto complete results? I hope we aren't repeating history, if there were legit reasons to remove it. I think there are two ways people use autocomplete. One is for fast type-ahead. I start typing, I see what I want, I click it or press tab. The other reason I personally use type ahead is for searching. For example, I'm in Discover and I have an I wouldn't expect the results to be filtered by time range and I would think something is broken. If we want to filter by time range to optimize for the fast typeahead use case, I think that's okay, but I think ideally it will be made clear in the UI. Like "results limited to time x - y". Also to the point about:
💯 I literally was just trying this out and thought my index pattern didn't have a time range and was confused why I wasn't seeing any results. I didn't realize it was limited to last 15 minutes. |
Update autocomplete tests and docs
Copying here the original PR text, as I update it with the chosen solutions: Potential issues with this approachUnavailable resultsOn Inconsistent results between various screensThis may occur under a few different cases, with similar effects:
For example, if we restrict the Alternatively, if we choose to apply the time range filtering to both the Relative time range memoizationOn |
uptime already does this by passing bool , range filter https://github.com/elastic/kibana/blob/master/x-pack/plugins/uptime/public/components/overview/kuery_bar/kuery_bar.tsx#L108 |
src/plugins/data/public/autocomplete/providers/value_suggestion_provider.ts
Show resolved
Hide resolved
useTimeFilter in uptime
x-pack/plugins/uptime/public/components/overview/kuery_bar/kuery_bar.tsx
Show resolved
Hide resolved
…r-in-autocomplete
@elasticmachine merge upstream |
…r-in-autocomplete
…ibana into fix/timefilter-in-autocomplete
…r-in-autocomplete
Co-authored-by: Lukas Olson <olson.lukas@gmail.com>
@elasticmachine merge upstream |
…r-in-autocomplete
…ibana into fix/timefilter-in-autocomplete
💚 Build SucceededMetrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
* pass timefilter to autocomplete * ignoreTimeRange advanced setting * Show all results in filter bar autocomplete * Round timerange to minute for autocomplete memoization * Change useTimeFilter param name Update autocomplete tests and docs * Fix maps test useTimeFilter in uptime * docs * useTimeRange in apm * remove date validation * Update src/plugins/data/common/constants.ts Co-authored-by: Lukas Olson <olson.lukas@gmail.com> * docs Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Lukas Olson <olson.lukas@gmail.com>
Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync. |
1 similar comment
Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync. |
* pass timefilter to autocomplete * ignoreTimeRange advanced setting * Show all results in filter bar autocomplete * Round timerange to minute for autocomplete memoization * Change useTimeFilter param name Update autocomplete tests and docs * Fix maps test useTimeFilter in uptime * docs * useTimeRange in apm * remove date validation * Update src/plugins/data/common/constants.ts Co-authored-by: Lukas Olson <olson.lukas@gmail.com> * docs Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Lukas Olson <olson.lukas@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Lukas Olson <olson.lukas@gmail.com>
Summary
Temporarily patches #46054
Related to #15887
This PR applies the active
timefilter
onto theautocomplete
requests, following a request from the Elasticsearch team, to fix the problem mentioned in #46054.This solution is temporary, as Elasticsearch plans to introduce a new and more efficient
autocomplete
API. Open question: Will the new ES API use the entire timerange? Or will it be also limited to the timerange?While this approach reduces the time needed to return autocomplete results for shorter time ranges, when the time range is long or if an index is not time based, it approaches the same performance of the current behavior on master.
It's worth mentioning here, that even on
master
frozen indices are not being queried for autocomplete suggestions.Changes with this approach
Unavailable results
On
master
, autocomplete results are constructed from the entire time range. Inthis branch
only a subset of the results is returned to the user. This is emphasized by the fact that the current UI\UX, doesn't allow the user to see the current time range, while editing the query.If an organization is unhappy with this behavior they can turn off the
autocomplete:useTimerange
advanced setting to revert to the previous behavior.master
![Discover - Elastic (2)](https://user-images.githubusercontent.com/3016806/97586965-8c89d780-1a03-11eb-984c-a6d4ec4dcb72.gif)
New behavior
![Discover - Elastic (5)](https://user-images.githubusercontent.com/3016806/97592569-7848d900-1a09-11eb-9f3e-6d310c4068f7.gif)
Inconsistent results between components \ solutions
After much deliberation with @elastic-jb , we decided to keep the drop-downs in the
FilterBar
populated with the full list of results (i.e. not filter by time) while theQuery
autocomplete suggestions will be filtered. So a user might see different suggestions in both places.Relative time range memoization
Relative time ranges will be rounded to the nearest minute, in order to avoid memoization issues.
Impact on other solutions
The autocomplete suggestions API is used by multiple consumers:
I think it's worth hearing their opinions about this change as well, and whether it has any implications on the solutions.
Release Notes
This PR applies the active
timefilter
onto theautocomplete
requests, to fix performance issues mentioned in #46054. This behavior will be enabled by default, but can be turned off by disabling theautocomplete:useTimerange
advanced settings.This solution is temporary, as Elasticsearch plans to introduce a new and more efficient
autocomplete
API.