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.

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

Merged

Conversation

Heenawter
Copy link
Contributor

@Heenawter Heenawter commented Jan 3, 2023

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 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:

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:

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:

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 - ⭐ 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:

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

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:

{
    "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:

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 ⚠️
    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

Checklist

For maintainers

@Heenawter Heenawter added Feature:Input Control Input controls visualization Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:large Large Level of Effort impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. release_note:feature Makes this part of the condensed release notes Project:Controls v8.7.0 labels Jan 3, 2023
@Heenawter Heenawter self-assigned this Jan 3, 2023
@Heenawter Heenawter force-pushed the add-pagination-to-options-list_2022-12-30 branch from a3c4090 to 478c302 Compare January 3, 2023 17:32
@Heenawter Heenawter changed the title [Dashboard][Controls] Add pagination to options list suggestions [Dashboard] [Controls] Add pagination to options list suggestions Jan 3, 2023
@Heenawter Heenawter force-pushed the add-pagination-to-options-list_2022-12-30 branch from 176d16a to e0ba9cd Compare January 9, 2023 16:23
@Heenawter Heenawter force-pushed the add-pagination-to-options-list_2022-12-30 branch 11 times, most recently from dde3d65 to b92e307 Compare January 23, 2023 17:39
@Heenawter Heenawter changed the title [Dashboard] [Controls] Add pagination to options list suggestions [Dashboard] [Controls] Load more suggestions on scroll Jan 23, 2023
@Heenawter Heenawter changed the title [Dashboard] [Controls] Load more suggestions on scroll [Dashboard] [Controls] Load more options list suggestions on scroll Jan 23, 2023
@Heenawter Heenawter force-pushed the add-pagination-to-options-list_2022-12-30 branch 6 times, most recently from 4f45b76 to f0e2018 Compare January 25, 2023 20:20
@Heenawter Heenawter force-pushed the add-pagination-to-options-list_2022-12-30 branch from b3df88d to eaa32bc Compare February 2, 2023 21:20
Copy link
Contributor

@ThomThomson ThomThomson left a 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

Screen Shot 2023-02-03 at 10 32 53 AM

  • Instead of hiding the cardinality when expensive queries are off, it shows 0 options

Screen Shot 2023-02-03 at 10 32 34 AM

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/common/options_list/types.ts Outdated 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';
Copy link
Contributor

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?

@Heenawter
Copy link
Contributor Author

Heenawter commented Feb 6, 2023

@ThomThomson

The sort button is no longer disabled when show only selected is true

Totally right! Last minute UI changes always bite me, hahaha :) Fixed in 3485cb9

Instead of hiding the cardinality when expensive queries are off, it shows 0 options

Woops, good catch. Fixed in 01b35d5 🤦

Copy link
Contributor

@andreadelrio andreadelrio left a 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:
image

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
controls 148 150 +2
embeddable 78 83 +5
total +7

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
controls 264 266 +2
dashboard 163 164 +1
total +3

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
controls 171.2KB 175.2KB +4.0KB
dashboard 364.6KB 364.8KB +176.0B
total +4.2KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
controls 31.6KB 32.1KB +494.0B
embeddable 73.4KB 74.6KB +1.1KB
total +1.6KB
Unknown metric groups

API count

id before after diff
controls 268 270 +2
dashboard 171 172 +1
total +3

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @Heenawter

@Heenawter Heenawter merged commit 55b66e2 into elastic:main Feb 6, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Feb 6, 2023
darnautov pushed a commit to darnautov/kibana that referenced this pull request Feb 7, 2023
…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>
@Heenawter Heenawter deleted the add-pagination-to-options-list_2022-12-30 branch February 7, 2023 17:41
benakansara pushed a commit to benakansara/kibana that referenced this pull request Feb 7, 2023
…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>
kibanamachine added a commit that referenced this pull request Mar 8, 2023
# 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Dashboard Dashboard related features Feature:Embedding Embedding content via iFrame Feature:Input Control Input controls visualization impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:x-large Extra Large Level of Effort Project:Controls release_note:feature Makes this part of the condensed release notes Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Controls] Show Search Cardinality in Options List [Controls] Pagination for Options List Results
7 participants