-
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
[Dashboard] [Controls] Load more options list suggestions on scroll #148331
[Dashboard] [Controls] Load more options list suggestions on scroll #148331
Conversation
a3c4090
to
478c302
Compare
176d16a
to
e0ba9cd
Compare
dde3d65
to
b92e307
Compare
4f45b76
to
f0e2018
Compare
b3df88d
to
eaa32bc
Compare
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.
This is indeed a huge PR, and it's also hugely nice to see it completed!
Reviewed this locally, with expensive queries on and off. Everything looks and works great. These controls are getting more and more robust and feature complete, great work!
There are a few small UX nits caused by the recent design change:
- The sort button is no longer disabled when
show only selected
is true
- Instead of hiding the cardinality when expensive queries are off, it shows 0 options
Additionally, I found a bit of a problem with how we fetch the expensive query setting. If I'm not mistaken we might have to make a small change to include all ways that it can be set.
I also left a few small comments & questions. Approving so we can unblock and hopefully get this in before FF!
src/plugins/controls/public/options_list/components/options_list_popover.test.tsx
Show resolved
Hide resolved
src/plugins/controls/public/options_list/components/options_list_popover_suggestions.tsx
Show resolved
Hide resolved
src/plugins/controls/server/options_list/options_list_cheap_suggestion_queries.ts
Outdated
Show resolved
Hide resolved
src/plugins/controls/server/options_list/options_list_cluster_settings_route.ts
Outdated
Show resolved
Hide resolved
src/plugins/controls/server/options_list/options_list_expensive_suggestion_queries.test.ts
Show resolved
Hide resolved
src/plugins/controls/server/options_list/options_list_validation_queries.ts
Show resolved
Hide resolved
@@ -11,6 +11,7 @@ import { EmbeddablePanelError } from '../panel/embeddable_panel_error'; | |||
import { Embeddable } from './embeddable'; | |||
import { EmbeddableInput, EmbeddableOutput, IEmbeddable } from './i_embeddable'; | |||
import { IContainer } from '../containers'; | |||
import './error_embeddable.scss'; |
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.
Have we checked if this changes anything in the way that the normal error embeddables on a dashboard render?
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.
Amazing work implementing this feature and nice improvements to the previous UI for displaying counters.
Shoutout to DeFazio for bringing a relevant UI pattern to our attention. @mdefazio Check out the end result:
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
History
To update your PR or re-run it, just comment with: cc @Heenawter |
## Summary - #148331: [Updated screenshots](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html) - #146335: [Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#search-or-filter-your-data) - #146363: [Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#edit-panels) - #144867: [Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html#edit-controls)
## Summary - elastic#148331: [Updated screenshots](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html) - elastic#146335: [Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#search-or-filter-your-data) - elastic#146363: [Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#edit-panels) - elastic#144867: [Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html#edit-controls) (cherry picked from commit e57883f)
# Backport This will backport the following commits from `main` to `8.7`: - [[DOCS] 8.7 Presentation docs (#151797)](#151797) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Kaarina Tungseth","email":"kaarina.tungseth@elastic.co"},"sourceCommit":{"committedDate":"2023-03-08T22:09:43Z","message":"[DOCS] 8.7 Presentation docs (#151797)\n\n## Summary\r\n\r\n- #148331: [Updated\r\nscreenshots](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html)\r\n- #146335:\r\n[Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#search-or-filter-your-data)\r\n- #146363:\r\n[Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#edit-panels)\r\n- #144867:\r\n[Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html#edit-controls)","sha":"e57883f3be8772c39cce0b6901a19f3aaf55d2d3","branchLabelMapping":{"^v8.8.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Docs","Team:Presentation","release_note:skip","v8.7.0","v8.8.0"],"number":151797,"url":"https://github.com/elastic/kibana/pull/151797","mergeCommit":{"message":"[DOCS] 8.7 Presentation docs (#151797)\n\n## Summary\r\n\r\n- #148331: [Updated\r\nscreenshots](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html)\r\n- #146335:\r\n[Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#search-or-filter-your-data)\r\n- #146363:\r\n[Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#edit-panels)\r\n- #144867:\r\n[Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html#edit-controls)","sha":"e57883f3be8772c39cce0b6901a19f3aaf55d2d3"}},"sourceBranch":"main","suggestedTargetBranches":["8.7"],"targetPullRequestStates":[{"branch":"8.7","label":"v8.7.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.8.0","labelRegex":"^v8.8.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/151797","number":151797,"mergeCommit":{"message":"[DOCS] 8.7 Presentation docs (#151797)\n\n## Summary\r\n\r\n- #148331: [Updated\r\nscreenshots](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html)\r\n- #146335:\r\n[Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#search-or-filter-your-data)\r\n- #146363:\r\n[Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#edit-panels)\r\n- #144867:\r\n[Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html#edit-controls)","sha":"e57883f3be8772c39cce0b6901a19f3aaf55d2d3"}}]}] BACKPORT--> Co-authored-by: Kaarina Tungseth <kaarina.tungseth@elastic.co>
## Summary - elastic#148331: [Updated screenshots](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html) - elastic#146335: [Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#search-or-filter-your-data) - elastic#146363: [Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#edit-panels) - elastic#144867: [Docs](https://kibana_151797.docs-preview.app.elstc.co/guide/en/kibana/master/add-controls.html#edit-controls)
Closes #140175
Closes #143580
Summary
Oh, boy! Get ready for a doozy of a PR, folks! Let's talk about the three major things that were accomplished here:
1) Pagination
Originally, this PR was meant to add traditional pagination to the options list control. However, after implementing a version of this, it became apparent that, not only was UI becoming uncomfortably messy, it also had some UX concerns because we were deviating from the usual pagination pattern by showing the cardinality rather than the number of pages:
So, instead of traditional pagination, we decided to take a different approach (which was made possible by #148420) - load more options when the user scrolls to the bottom! Here it is in action:
It is important that the first query remains fast - that is why we still only request the top 10 options when the control first loads. So, having a "load more" is the best approach that allows users to see more suggestions while also ensuring that the performance of options lists (especially with respect to chaining) is not impacted.
Note that it is not possible to grab every single value of a field - the limit is
10,000
. However, since it is impractical that a user would want to scroll through10,000
suggestions (and potentially very slow to fetch), we have instead made the limit of this "show more" functionality1,000
. To make this clear, if the field has more than1,000
values and the user scrolls all the way to the bottom, they will get the following message:2) Cardinality
Previously, the cardinality of the options list control was only shown as part of the control placeholder text - this meant that, once the user entered their search term, they could no longer see the cardinality of the returned options. This PR changes this functionality by placing the cardinality in a badge beside the search bar - this value now changes as the user types, so they can very clearly see how many options match their search:
3) Changes to Queries
This is where things get.... messy! Essentially, our previous queries were all built with the expectation that the Elasticsearch setting
search.allow_expensive_queries
was off - this meant that they worked regardless of the value of this setting. However, when trying to get the cardinality to update based on a search term, it became apparent that this was not possible if we kept the same assumptions - specifically, ifsearch.allow_expensive_queries
is off, there is absolutely no way for the cardinality of keyword only fields to respond to a search term.After a whole lot of discussion, we decided that the updating cardinality was a feature important enough to justify having two separate versions of the queries:
Queries for when
search.allow_expensive_queries
is off:These are essentially the same as our old queries - however, since we can safely assume that this setting is usually on (it defaults on, and there is no UI to easily change it), we opted to simplify them a bit.
First of all, we used to create a special object for tracking the parent/child relationship of fields that are mapped as keyword+text - this was so that, if a user created a control on these fields, we could support case-insensitive search. We no longer do this - if
search.allow_expensive_queries
is off and you create a control on a text+keyword field, the search will be case sensitive. This helps clean up our code quite a bit.Second, we are no longer returning any cardinality. Since the cardinality is now displayed as a badge beside the search bar, users would expect that this value would change as they type - however, since it's impossible to make this happen for keyword-only fields and to keep behaviour consistent, we have opted to simply remove this badge when
search.allow_expensive_queries
is off regardless of the field type. So, there is no longer a need to include thecardinality
query when grabbing the suggestions.Finally, we do not support "load more" when
search.allow_expensive_queries
is off. While this would theoretically be possible, because we are no longer grabbing the cardinality, we would have to always fetch1,000
results when the user loads more, even if the true cardinality is much smaller. Again, we are pretty confident that more often than not, thesearch.allow_expensive_queries
is on; therefore, we are choosing to favour developer experience in this instance because the impact should be quite small.Queries for when
search.allow_expensive_queries
is on:When this setting is on, we now have access to the prefix query, which greatly simplifies how our queries are handled - now, rather than having separate queries for keyword-only, keyword+text, and nested fields, these have all been combined into a single query! And even better - ⭐ now all string-based fields support case-insensitive search! ⭐ Yup, that's right - even keyword-only fields 💃
There has been discussion on the Elasticsearch side about whether or not this setting is even practical, and so it is possible that, in the near future, this distinction will no longer be necessary. With this in mind, I have made these two versions of our queries completely separate from each other - while this introduces some code duplication, it makes the cleanup that may follow much, much easier.
Well, that was sure fun, hey?
How to Test
I've created a quick little Python program to ingest some good testing data for this PR:
However, if you don't have Python up and running, here's a CSV with a smaller version of this data: testNewQueriesData.csv
Testing Notes
Because there are now two versions of the queries, thorough testing should be done for both when
search.allow_expensive_queries
istrue
and when it isfalse
for every single field type that is currently supported. Use the following call to the cluster settings API to toggle this value back and forth:You should pay super special attention to the behaviour that happens when toggling this value from
true
tofalse
- for example, consider the following:search.allow_expensive_queries
is eithertrue
orundefined
search.allow_expensive_queries
tofalse
- DO NOT REFRESHThe Elasticsearch server knows that
search.allow_expensive_queries
is nowfalse
but, because we only fetch this value on the first load on the client side, it has not yet been updated - this means the options list service still tries to fetch the suggestions using the expensive version of the queries despite the fact that Elasticsearch will now reject this request. The most graceful way to handle this is to simply throw a fatal error.Flaky Test Runner
Checklist
For maintainers