-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Controls] Pagination for Options List Results #140175
Comments
Pinging @elastic/kibana-presentation (Team:Presentation) |
I just wanted to note that in #140026 you said: we decided to add an option to configure the number of values displayed in the filter … which is missing here, just for completeness' sake 🙂 I think it would make sense to still have the maximum number be configurable by the user, even if it incurs a performance penalty. Maybe it won't even affect some customers with performant nodes. Anyway, this feature seems very useful! (We have a list of devices with ascending IDs, and the order is … weird for our customers, to say the least.) |
I've been thinking through how we could accomplish this, and it seems like we might be able to add pagination to the Options List Control using a collapse query. We would display a |
I've done a little initial design exploration for this + #143580. Something to note here, is that I haven't used the EUI pagination component, instead just using a badge and eui icon buttons. This is due to the fact that we can't put a In the badge, I've also used the format Tagging @andreadelrio for visibility and potential design feedback! |
@ThomThomson Nice designs! That looks really clean with the I wonder if replacing the |
I'm down for that change! It also looks nicer to have |
@ThomThomson This looks really good. I think it's a smart way to handle the fact we can't link to a last page. +1 on Hannah's suggestion to use |
Based on my research in to this problem so far, it seems as though pagination will not be possible while also supporting ordering by document count - only alphabetical sorting is supported. First of all, I explored Devon’s solution of using the collapse query. By default without any pre-sorting, the collapse query seems to return results simply by document order (i.e. if you were collapsing on The best solution I could find was composite aggregations, which is the typical query that the Elasticsearch documentation recommends for pagination. However, similar to using the collapse query, only alphabetical sorting would be supported if this is the path that we chose - sorting by document count is, to my knowledge, not possible. |
…148331) 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: <p align="center"><img src="https://user-images.githubusercontent.com/8698078/214687041-f8950d3a-2b29-41d5-b656-c79d9575d744.gif"/></p> 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: <p align="center"><img src="https://user-images.githubusercontent.com/8698078/214688854-06c7e8a9-7b8c-4dc0-9846-00ccf5e5f771.gif"/></p> 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 through `10,000` suggestions (and potentially very slow to fetch), we have instead made the limit of this "show more" functionality `1,000`. To make this clear, if the field has more than `1,000` values and the user scrolls all the way to the bottom, they will get the following message: <p align="center"><img src="https://user-images.githubusercontent.com/8698078/214920302-1e3574dc-f2b6-4845-be69-f9ba04177e7f.png"/></p> ### 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: <p align="center"><img src="https://user-images.githubusercontent.com/8698078/214689739-9670719c-5878-4e8b-806c-0b5a6f6f907f.gif"/></p> > **Note** > After some initial feedback, we have removed both the cardinality and invalid selections badges in favour of displaying the cardinality below the search bar, like so: > > <p align="center"><img src="https://user-images.githubusercontent.com/8698078/216473930-e99366a3-86df-4777-a3d8-cf2d41e550fb.gif"/></p> > > So, please be aware that the screenshots above are outdated. ### 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, if `search.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: 1. **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 the `cardinality` 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 fetch `1,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**, the `search.allow_expensive_queries` is on; therefore, we are choosing to favour developer experience in this instance because the impact should be quite small. 2. **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 - :star: now **all** string-based fields support case-insensitive search! :star: Yup, that's right - even keyword-only fields 💃 There has been [discussion on the Elasticsearch side ](elastic/elasticsearch#90898) 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? <p align="center"><img src="https://user-images.githubusercontent.com/8698078/214921985-49058ff0-42f2-4b01-8ae3-0a4d259d1075.gif"/></p> ## How to Test I've created a quick little Python program to ingest some good testing data for this PR: ```python import random import time import pandas as pd from faker import Faker from elasticsearch import Elasticsearch SIZE = 10000 ELASTIC_PASSWORD = "changeme" INDEX_NAME = 'test_large_index' Faker.seed(time.time()) faker = Faker() hundredRandomSentences = [faker.sentence(random.randint(5, 35)) for _ in range(100)] thousandRandomIps = [faker.ipv4() if random.randint(0, 99) < 50 else faker.ipv6() for _ in range(1000)] client = Elasticsearch( "http://localhost:9200", basic_auth=("elastic", ELASTIC_PASSWORD), ) if(client.indices.exists(index=INDEX_NAME)): client.indices.delete(index=INDEX_NAME) client.indices.create(index=INDEX_NAME, mappings={"properties":{"keyword_field":{"type":"keyword"},"id":{"type":"long"},"ip_field":{"type":"ip"},"boolean_field":{"type":"boolean"},"keyword_text_field":{"type":"text","fields":{"keyword":{"type":"keyword"}}},"nested_field":{"type":"nested","properties":{"first":{"type":"text","fields":{"keyword":{"type":"keyword"}}},"last":{"type":"text","fields":{"keyword":{"type":"keyword"}}}}},"long_keyword_text_field":{"type":"text","fields":{"keyword":{"type":"keyword"}}}}}) print('Generating data', end='') for i in range(SIZE): name1 = faker.name(); [first_name1, last_name1] = name1.split(' ', 1) name2 = faker.name(); [first_name2, last_name2] = name2.split(' ', 1) response = client.create(index=INDEX_NAME, id=i, document={ 'keyword_field': faker.country(), 'id': i, 'boolean_field': faker.boolean(), 'ip_field': thousandRandomIps[random.randint(0, 999)], 'keyword_text_field': faker.name(), 'nested_field': [ { 'first': first_name1, 'last': last_name1}, { 'first': first_name2, 'last': last_name2} ], 'long_keyword_text_field': hundredRandomSentences[random.randint(0, 99)] }) print('.', end='') print(' Done!') ``` However, if you don't have Python up and running, here's a CSV with a smaller version of this data: [testNewQueriesData.csv](https://github.com/elastic/kibana/files/10538537/testNewQueriesData.csv) > **Warning** > When uploading, make sure to update the mappings of the CSV data to the mappings included as part of the Python script above (which you can find as part of the `client.indices.create` call). You'll notice, however, that **none of the CSV documents have a nested field**. Unfortunately, there doesn't seem to be a way to able to ingest nested data through uploading a CSV, so the above data does not include one - in order to test the nested data type, you'd have to add some of your own documents > > Here's a sample nested field document, for your convenience: > ```json > { > "keyword_field": "Russian Federation", > "id": 0, > "boolean_field": true, > "ip_field": "121.149.70.251", > "keyword_text_field": "Michael Foster", > "nested_field": [ > { > "first": "Rachel", > "last": "Wright" > }, > { > "first": "Gary", > "last": "Reyes" > } > ], > "long_keyword_text_field": "Color hotel indicate appear since well sure right yet individual easy often test enough left a usually attention." > } > ``` > ### Testing Notes Because there are now two versions of the queries, thorough testing should be done for both when `search.allow_expensive_queries` is `true` and when it is `false` 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: ```php PUT _cluster/settings { "transient": { "search.allow_expensive_queries": <value> // true or false } } ``` You should pay super special attention to the behaviour that happens when toggling this value from `true` to `false` - for example, consider the following: 1. Ensure `search.allow_expensive_queries` is either `true` or `undefined` 2. Create and save a dashboard with at least one options list control 3. Navigate to the console and set `search.allow_expensive_queries` to `false` - **DO NOT REFRESH** 4. Go back to the dashboard 5. Open up the options list control you created in step 2 6. Fetch a new, uncached request, either by scrolling to the bottom and fetching more (assuming these values aren't already in the cache) or by performing a search with a string you haven't tried before 7.⚠️ **The options list control _should_ have a fatal error**⚠️ <br>The Elasticsearch server knows that `search.allow_expensive_queries` is now `false` 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. 8. Refreshing the browser will make things sync up again and you should now get the expected results when opening the options list control. ### Flaky Test Runner <a href="https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1845"><img src="https://user-images.githubusercontent.com/8698078/215894267-97f07e59-6660-4117-bda7-18f63cb19af6.png"/></a> ### Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [x] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) > **Note** > Technically, it actually does - however, it is due to an [EUI bug](elastic/eui#6565) from adding the group label to the bottom of the list. - [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
…lastic#148331) Closes elastic#140175 Closes elastic#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: <p align="center"><img src="https://user-images.githubusercontent.com/8698078/214687041-f8950d3a-2b29-41d5-b656-c79d9575d744.gif"/></p> So, instead of traditional pagination, we decided to take a different approach (which was made possible by elastic#148420) - **load more options when the user scrolls to the bottom!** Here it is in action: <p align="center"><img src="https://user-images.githubusercontent.com/8698078/214688854-06c7e8a9-7b8c-4dc0-9846-00ccf5e5f771.gif"/></p> 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 through `10,000` suggestions (and potentially very slow to fetch), we have instead made the limit of this "show more" functionality `1,000`. To make this clear, if the field has more than `1,000` values and the user scrolls all the way to the bottom, they will get the following message: <p align="center"><img src="https://user-images.githubusercontent.com/8698078/214920302-1e3574dc-f2b6-4845-be69-f9ba04177e7f.png"/></p> ### 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: <p align="center"><img src="https://user-images.githubusercontent.com/8698078/214689739-9670719c-5878-4e8b-806c-0b5a6f6f907f.gif"/></p> > **Note** > After some initial feedback, we have removed both the cardinality and invalid selections badges in favour of displaying the cardinality below the search bar, like so: > > <p align="center"><img src="https://user-images.githubusercontent.com/8698078/216473930-e99366a3-86df-4777-a3d8-cf2d41e550fb.gif"/></p> > > So, please be aware that the screenshots above are outdated. ### 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, if `search.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: 1. **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 the `cardinality` 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 fetch `1,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**, the `search.allow_expensive_queries` is on; therefore, we are choosing to favour developer experience in this instance because the impact should be quite small. 2. **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 - :star: now **all** string-based fields support case-insensitive search! :star: Yup, that's right - even keyword-only fields 💃 There has been [discussion on the Elasticsearch side ](elastic/elasticsearch#90898) 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? <p align="center"><img src="https://user-images.githubusercontent.com/8698078/214921985-49058ff0-42f2-4b01-8ae3-0a4d259d1075.gif"/></p> ## How to Test I've created a quick little Python program to ingest some good testing data for this PR: ```python import random import time import pandas as pd from faker import Faker from elasticsearch import Elasticsearch SIZE = 10000 ELASTIC_PASSWORD = "changeme" INDEX_NAME = 'test_large_index' Faker.seed(time.time()) faker = Faker() hundredRandomSentences = [faker.sentence(random.randint(5, 35)) for _ in range(100)] thousandRandomIps = [faker.ipv4() if random.randint(0, 99) < 50 else faker.ipv6() for _ in range(1000)] client = Elasticsearch( "http://localhost:9200", basic_auth=("elastic", ELASTIC_PASSWORD), ) if(client.indices.exists(index=INDEX_NAME)): client.indices.delete(index=INDEX_NAME) client.indices.create(index=INDEX_NAME, mappings={"properties":{"keyword_field":{"type":"keyword"},"id":{"type":"long"},"ip_field":{"type":"ip"},"boolean_field":{"type":"boolean"},"keyword_text_field":{"type":"text","fields":{"keyword":{"type":"keyword"}}},"nested_field":{"type":"nested","properties":{"first":{"type":"text","fields":{"keyword":{"type":"keyword"}}},"last":{"type":"text","fields":{"keyword":{"type":"keyword"}}}}},"long_keyword_text_field":{"type":"text","fields":{"keyword":{"type":"keyword"}}}}}) print('Generating data', end='') for i in range(SIZE): name1 = faker.name(); [first_name1, last_name1] = name1.split(' ', 1) name2 = faker.name(); [first_name2, last_name2] = name2.split(' ', 1) response = client.create(index=INDEX_NAME, id=i, document={ 'keyword_field': faker.country(), 'id': i, 'boolean_field': faker.boolean(), 'ip_field': thousandRandomIps[random.randint(0, 999)], 'keyword_text_field': faker.name(), 'nested_field': [ { 'first': first_name1, 'last': last_name1}, { 'first': first_name2, 'last': last_name2} ], 'long_keyword_text_field': hundredRandomSentences[random.randint(0, 99)] }) print('.', end='') print(' Done!') ``` However, if you don't have Python up and running, here's a CSV with a smaller version of this data: [testNewQueriesData.csv](https://github.com/elastic/kibana/files/10538537/testNewQueriesData.csv) > **Warning** > When uploading, make sure to update the mappings of the CSV data to the mappings included as part of the Python script above (which you can find as part of the `client.indices.create` call). You'll notice, however, that **none of the CSV documents have a nested field**. Unfortunately, there doesn't seem to be a way to able to ingest nested data through uploading a CSV, so the above data does not include one - in order to test the nested data type, you'd have to add some of your own documents > > Here's a sample nested field document, for your convenience: > ```json > { > "keyword_field": "Russian Federation", > "id": 0, > "boolean_field": true, > "ip_field": "121.149.70.251", > "keyword_text_field": "Michael Foster", > "nested_field": [ > { > "first": "Rachel", > "last": "Wright" > }, > { > "first": "Gary", > "last": "Reyes" > } > ], > "long_keyword_text_field": "Color hotel indicate appear since well sure right yet individual easy often test enough left a usually attention." > } > ``` > ### Testing Notes Because there are now two versions of the queries, thorough testing should be done for both when `search.allow_expensive_queries` is `true` and when it is `false` 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: ```php PUT _cluster/settings { "transient": { "search.allow_expensive_queries": <value> // true or false } } ``` You should pay super special attention to the behaviour that happens when toggling this value from `true` to `false` - for example, consider the following: 1. Ensure `search.allow_expensive_queries` is either `true` or `undefined` 2. Create and save a dashboard with at least one options list control 3. Navigate to the console and set `search.allow_expensive_queries` to `false` - **DO NOT REFRESH** 4. Go back to the dashboard 5. Open up the options list control you created in step 2 6. Fetch a new, uncached request, either by scrolling to the bottom and fetching more (assuming these values aren't already in the cache) or by performing a search with a string you haven't tried before 7.⚠️ **The options list control _should_ have a fatal error**⚠️ <br>The Elasticsearch server knows that `search.allow_expensive_queries` is now `false` 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. 8. Refreshing the browser will make things sync up again and you should now get the expected results when opening the options list control. ### Flaky Test Runner <a href="https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1845"><img src="https://user-images.githubusercontent.com/8698078/215894267-97f07e59-6660-4117-bda7-18f63cb19af6.png"/></a> ### Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [x] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) > **Note** > Technically, it actually does - however, it is due to an [EUI bug](elastic/eui#6565) from adding the group label to the bottom of the list. - [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
…lastic#148331) Closes elastic#140175 Closes elastic#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: <p align="center"><img src="https://user-images.githubusercontent.com/8698078/214687041-f8950d3a-2b29-41d5-b656-c79d9575d744.gif"/></p> So, instead of traditional pagination, we decided to take a different approach (which was made possible by elastic#148420) - **load more options when the user scrolls to the bottom!** Here it is in action: <p align="center"><img src="https://user-images.githubusercontent.com/8698078/214688854-06c7e8a9-7b8c-4dc0-9846-00ccf5e5f771.gif"/></p> 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 through `10,000` suggestions (and potentially very slow to fetch), we have instead made the limit of this "show more" functionality `1,000`. To make this clear, if the field has more than `1,000` values and the user scrolls all the way to the bottom, they will get the following message: <p align="center"><img src="https://user-images.githubusercontent.com/8698078/214920302-1e3574dc-f2b6-4845-be69-f9ba04177e7f.png"/></p> ### 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: <p align="center"><img src="https://user-images.githubusercontent.com/8698078/214689739-9670719c-5878-4e8b-806c-0b5a6f6f907f.gif"/></p> > **Note** > After some initial feedback, we have removed both the cardinality and invalid selections badges in favour of displaying the cardinality below the search bar, like so: > > <p align="center"><img src="https://user-images.githubusercontent.com/8698078/216473930-e99366a3-86df-4777-a3d8-cf2d41e550fb.gif"/></p> > > So, please be aware that the screenshots above are outdated. ### 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, if `search.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: 1. **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 the `cardinality` 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 fetch `1,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**, the `search.allow_expensive_queries` is on; therefore, we are choosing to favour developer experience in this instance because the impact should be quite small. 2. **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 - :star: now **all** string-based fields support case-insensitive search! :star: Yup, that's right - even keyword-only fields 💃 There has been [discussion on the Elasticsearch side ](elastic/elasticsearch#90898) 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? <p align="center"><img src="https://user-images.githubusercontent.com/8698078/214921985-49058ff0-42f2-4b01-8ae3-0a4d259d1075.gif"/></p> ## How to Test I've created a quick little Python program to ingest some good testing data for this PR: ```python import random import time import pandas as pd from faker import Faker from elasticsearch import Elasticsearch SIZE = 10000 ELASTIC_PASSWORD = "changeme" INDEX_NAME = 'test_large_index' Faker.seed(time.time()) faker = Faker() hundredRandomSentences = [faker.sentence(random.randint(5, 35)) for _ in range(100)] thousandRandomIps = [faker.ipv4() if random.randint(0, 99) < 50 else faker.ipv6() for _ in range(1000)] client = Elasticsearch( "http://localhost:9200", basic_auth=("elastic", ELASTIC_PASSWORD), ) if(client.indices.exists(index=INDEX_NAME)): client.indices.delete(index=INDEX_NAME) client.indices.create(index=INDEX_NAME, mappings={"properties":{"keyword_field":{"type":"keyword"},"id":{"type":"long"},"ip_field":{"type":"ip"},"boolean_field":{"type":"boolean"},"keyword_text_field":{"type":"text","fields":{"keyword":{"type":"keyword"}}},"nested_field":{"type":"nested","properties":{"first":{"type":"text","fields":{"keyword":{"type":"keyword"}}},"last":{"type":"text","fields":{"keyword":{"type":"keyword"}}}}},"long_keyword_text_field":{"type":"text","fields":{"keyword":{"type":"keyword"}}}}}) print('Generating data', end='') for i in range(SIZE): name1 = faker.name(); [first_name1, last_name1] = name1.split(' ', 1) name2 = faker.name(); [first_name2, last_name2] = name2.split(' ', 1) response = client.create(index=INDEX_NAME, id=i, document={ 'keyword_field': faker.country(), 'id': i, 'boolean_field': faker.boolean(), 'ip_field': thousandRandomIps[random.randint(0, 999)], 'keyword_text_field': faker.name(), 'nested_field': [ { 'first': first_name1, 'last': last_name1}, { 'first': first_name2, 'last': last_name2} ], 'long_keyword_text_field': hundredRandomSentences[random.randint(0, 99)] }) print('.', end='') print(' Done!') ``` However, if you don't have Python up and running, here's a CSV with a smaller version of this data: [testNewQueriesData.csv](https://github.com/elastic/kibana/files/10538537/testNewQueriesData.csv) > **Warning** > When uploading, make sure to update the mappings of the CSV data to the mappings included as part of the Python script above (which you can find as part of the `client.indices.create` call). You'll notice, however, that **none of the CSV documents have a nested field**. Unfortunately, there doesn't seem to be a way to able to ingest nested data through uploading a CSV, so the above data does not include one - in order to test the nested data type, you'd have to add some of your own documents > > Here's a sample nested field document, for your convenience: > ```json > { > "keyword_field": "Russian Federation", > "id": 0, > "boolean_field": true, > "ip_field": "121.149.70.251", > "keyword_text_field": "Michael Foster", > "nested_field": [ > { > "first": "Rachel", > "last": "Wright" > }, > { > "first": "Gary", > "last": "Reyes" > } > ], > "long_keyword_text_field": "Color hotel indicate appear since well sure right yet individual easy often test enough left a usually attention." > } > ``` > ### Testing Notes Because there are now two versions of the queries, thorough testing should be done for both when `search.allow_expensive_queries` is `true` and when it is `false` 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: ```php PUT _cluster/settings { "transient": { "search.allow_expensive_queries": <value> // true or false } } ``` You should pay super special attention to the behaviour that happens when toggling this value from `true` to `false` - for example, consider the following: 1. Ensure `search.allow_expensive_queries` is either `true` or `undefined` 2. Create and save a dashboard with at least one options list control 3. Navigate to the console and set `search.allow_expensive_queries` to `false` - **DO NOT REFRESH** 4. Go back to the dashboard 5. Open up the options list control you created in step 2 6. Fetch a new, uncached request, either by scrolling to the bottom and fetching more (assuming these values aren't already in the cache) or by performing a search with a string you haven't tried before 7.⚠️ **The options list control _should_ have a fatal error**⚠️ <br>The Elasticsearch server knows that `search.allow_expensive_queries` is now `false` 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. 8. Refreshing the browser will make things sync up again and you should now get the expected results when opening the options list control. ### Flaky Test Runner <a href="https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1845"><img src="https://user-images.githubusercontent.com/8698078/215894267-97f07e59-6660-4117-bda7-18f63cb19af6.png"/></a> ### Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [x] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) > **Note** > Technically, it actually does - however, it is due to an [EUI bug](elastic/eui#6565) from adding the group label to the bottom of the list. - [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Describe the feature:
Controls are displaying the first top 10 values by default. Users would like to configure that number. We agreed the upper limit should be 50 values to avoid performance issues.
We will implement this in the context of merging Controls with Unified Search.
Needs design @andreadelrio
Update:
It might be possible to add server-side pagination to the Options List dropdown rather than allowing the user to configure the number of results. This will allow the user to page through the available options, up to 10000 without affecting the speed of the initial search term query.
The text was updated successfully, but these errors were encountered: