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

Reconsider allow expensive queries setting #90898

Open
javanna opened this issue Oct 14, 2022 · 9 comments
Open

Reconsider allow expensive queries setting #90898

javanna opened this issue Oct 14, 2022 · 9 comments
Assignees
Labels
>enhancement feedback_needed :Search Foundations/Search Catch all for Search Foundations Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch

Comments

@javanna
Copy link
Member

javanna commented Oct 14, 2022

We have not so long ago introduced a setting that allows users to disable expensive queries, which makes Elasticsearch reject some type of queries that are known to be potentially slow, like scripted queries (or based on runtime fields), wildcard queries, regex queries etc.

While we have recently added new potentially slow queries to this existing mechanism, like doc_value only queries, the setting does not cover all ways to run slow queries and may give users a false sense of security.

Additionally, we've been adding more and more ways to run slow queries, Elasticsearch is able to support them much better than it did in the past, thanks to the Async Search API. We've observed that our Solutions rely on expensive queries for core functionalities, and their UI may not even load when users disable the ability to run expensive queries, which makes the setting problematic and mush less useful.

Given that we got requests to make the setting configurable per role (#53607), we should discuss if this is a reliable mechanism that we want to build on top of, or consider other options. Some thoughts to get the discussion started:

  • Is it a reasonable expectation for our Solutions to not rely on expensive queries?
  • Should Solutions instead have a preemptive check that expensive queries are allowed, otherwise give a clear error message or expose degraded functionalities?
  • Should Elasticsearch document that Solutions may rely on expensive queries, hence disallowing them may prevent Solutions from working at all?
  • Should Solutions have some kind of mechanism that prevents users from disabling running expensive queries as they rely on them?
  • How is the setting used in the wild and is it reliable in those scenarios?
  • Is disallowing expensive queries to be considered a security feature?
  • Do we have other options when it comes to supporting slow queries, is there still a need to disable them entirely?
@javanna javanna added :Search/Search Search-related issues that do not fall into other categories team-discuss labels Oct 14, 2022
@elasticsearchmachine elasticsearchmachine added the Team:Search Meta label for search team label Oct 14, 2022
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

@joshdover
Copy link
Contributor

In terms of Solution's usage, I wonder if allowing system indices to opt out of this behavior so that we can rely on these "expensive" features in some cases where we know the impact is limited. This won't solve all potential usages but it could prevent some breakages of functionality when this setting is enabled.

@ywangd
Copy link
Member

ywangd commented Oct 17, 2022

I wonder if allowing system indices to opt out of this behavior so that we can rely on these "expensive" features in some cases where we know the impact is limited.

For the existing bug of threadpool usage of system indices (#89073), one idea is to attach the conditional behaviour to the user (e.g. elastic/fleet-server) instead of the target resource. I think makes sense in a service point of view. It likely requires core to be aware of users which is not an easy change. But our decision should be consistent between these two issues.

@javanna
Copy link
Member Author

javanna commented Nov 17, 2022

We discussed this with the team. We were rather aligned on the current challenges with the allow expensive queries setting: it gives false sense of security, it may prevent non expensive queries while at the same time allowing alternative ways to run expensive queries. We said that often times slow is confused with expensive and we should clarify that distinction: slow is not necessarily bad, especially when async search is used. When it comes to resource usage, some queries may require more memory or CPU than others, and possibly end up crashing data nodes: these problems could be solved assigning priorities to search requests (#37867), improving resiliency and/or our current timeout mechanisms.

It is clear that there is a need to protect clusters from potentially falling apart due to bad queries, yet the allow expensive queries is not a complete solution to address that. It is not necessarily reasonable that our Solutions should fully work without relying on what we currently declare "expensive". We said we would like to collect more data points on users that use this setting and whether it does help in practice addressing the need around securing clusters. We will resume this discussion once we have more information.

@ThomThomson
Copy link
Contributor

Has there been any movement on this? Currently the new Controls in Dashboard work well with allow_expensive_queries off, but we're considering migrating to use prefix queries instead of terms include for prefix searching within Keyword mapped fields. I'm getting the sense that terms include is roughly as expensive as prefix, and that it can be a little arbitrary which query types require that setting to be on.

Switching to prefix queries would greatly reduce our code complexity. The only risk is that this might prevent the Dashboard UI from loading when the allow_expensive_queries is off - just like the many of the solutions fail to load. Are we aware of many user complaints resulting in these apps not loading? Is turning off this setting common at all?

Additionally, I've been unable to find any methods which can tell Kibana plugins the value of this setting - is it even possible to check whether or not it's on in order to change pieces of the UI?

@ywangd
Copy link
Member

ywangd commented Jan 16, 2023

I've been unable to find any methods which can tell Kibana plugins the value of this setting

It is just a cluster setting, so you can check its value with the following request

GET _cluster/settings?include_defaults&filter_path=**.allow_expensive_queries

@ThomThomson
Copy link
Contributor

@ywangd, that's very helpful! As a cluster setting, do we track it in any way? For instance, could we answer "what percent of clusters have allow_expensive_queries turned off"?.

Now that we know it's possible to detect if the setting is on or off we need to decide how much effort we should put into making features work well with the setting off, and the answer to that will partially depend on the number of folks who have the setting off.

@ywangd
Copy link
Member

ywangd commented Jan 16, 2023

could we answer "what percent of clusters have allow_expensive_queries turned off"?.

It does not seem to be reported in telemetry. @javanna Do you know whether there is a way to get this type of data? Since this setting is not part of xpack, my knowledge is limited. Thanks!

Heenawter added a commit to elastic/kibana that referenced this issue Feb 6, 2023
…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>
darnautov pushed a commit to darnautov/kibana that referenced this issue 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>
benakansara pushed a commit to benakansara/kibana that referenced this issue 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>
@javanna javanna added :Search Foundations/Search Catch all for Search Foundations and removed :Search/Search Search-related issues that do not fall into other categories labels Jul 17, 2024
@elasticsearchmachine elasticsearchmachine added Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch and removed Team:Search Meta label for search team labels Jul 17, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search-foundations (Team:Search Foundations)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement feedback_needed :Search Foundations/Search Catch all for Search Foundations Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch
Projects
None yet
Development

No branches or pull requests

6 participants