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

[input controls] update dropdown suggestions when filtered #18985

Merged
merged 15 commits into from Jul 10, 2018

Conversation

Projects
None yet
6 participants
@nreese
Copy link
Contributor

nreese commented May 10, 2018

fixes #17758 and #14529

PR adds new configuration Dynamic Options. When set to true, the dropdown options list is updated and filtered by the user input.

screen shot 2018-05-10 at 2 38 00 pm

@nreese

This comment has been minimized.

Copy link
Contributor Author

nreese commented May 10, 2018

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented May 10, 2018

@nreese nreese requested review from chrisdavies and stacey-gammon May 10, 2018

@chrisdavies
Copy link
Contributor

chrisdavies left a comment

LGTM w/ possible tweaks

controlIndex={index}
stageFilter={this.props.stageFilter}
fetchOptions={async (query) => { return await this.props.refreshControl(index, query); }}
/>

This comment has been minimized.

Copy link
@chrisdavies

chrisdavies May 11, 2018

Contributor

In this case, the caller of fetchOptions is doing an await, so you can make this query => this.props.refreshControl(index, query) since the caller is handling a promise anyway.

This comment has been minimized.

Copy link
@chrisdavies

chrisdavies May 11, 2018

Contributor

Er. Also, I wonder if we can get rid of props.refreshControl altogether, and just do query => control.fetch(query) here, and allow the base Control to intelligently refresh when a query completes? Not sure. I don't know enough about this.

This comment has been minimized.

Copy link
@nreese

nreese May 11, 2018

Author Contributor

There is no good way to update control in this component since it's state is passed in as props.

const resp = await searchSource.fetch();
if (query && this.lastQuery !== query) {
// search results returned out of order - ignore results from old query
return;

This comment has been minimized.

Copy link
@chrisdavies

chrisdavies May 11, 2018

Contributor

Seems this could accidentally blank out the results:

  • fetch is called twice ('a', 'b')
  • 'b' returns first
  • 'a' returns second, and, and fetch returns undefined

The caller of fetch needs to know that if it gets an undefined, it should do nothing (it should keep its current state). Might be better to have this return the correct results if called out of sync (e.g. in the previous example, 'a' would return 'b's results).

This comment has been minimized.

Copy link
@nreese

nreese May 11, 2018

Author Contributor

The return is not used to set anything. The resp is used to update this.selectOptions later in the function. Calling return early just short circuits so that this.selectOptions is not updated with no longer needed data.

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented May 11, 2018

@nreese nreese force-pushed the nreese:dynamicOptions branch from 34a1994 to 7a02a3a May 14, 2018

@stacey-gammon

This comment has been minimized.

Copy link
Contributor

stacey-gammon commented May 14, 2018

Chatted offline with @nreese about some feedback:

  • Initially I didn't see the use case for having Dynamic options off at all, but was convinced we should leave it in to improve performance on low cardinality fields.
  • If we do leave it as an option, I think we should default new input controls to having it on.
  • If we do leave it as an option, I don't see the use case for giving a size parameter when it's off. Why would anyone limit a low cardinality field so that not every option is shown? To me, this would just present as a bug because it's possible some field options will not be shown, and it would create confusion. My thought is that we should only offer the size option when Dynamic options is on. Agree that this PR shouldn't be blocked on that decision though since this is how it acts now, we are no worse off.
  • Lets bring in @gchaps to go over the terminology, perhaps helpful tooltip text in addition because by itself, I don't think Dynamic options makes it obvious what is going on. We could also reverse the logic and have it something you turn on for low cardinality fields, rather than something you turn off for low cardinality fields.

Some random ideas:

  • Improve performance for low cardinality fields by pre-fetching all options once
  • Add dynamic "search-as-you-type" functionality
@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented May 14, 2018

@stacey-gammon

This comment has been minimized.

Copy link
Contributor

stacey-gammon commented May 14, 2018

@nreese - going to hold off on further review until you investigate that bug with searching on ip field. Just lmk when ready to go for a second look.

@gchaps

This comment has been minimized.

Copy link
Contributor

gchaps commented May 15, 2018

@nreese @stacey-gammon

Here is some text to consider:

Multiselect -> Allow multiple selection

Dynamic Options -> Update options in response to user input
Tooltip: If enabled, add and remove options based on user input. Otherwise, use a fixed set of options.

Size -> Number of options

nreese added some commits May 10, 2018

@nreese nreese force-pushed the nreese:dynamicOptions branch from 7a02a3a to 032d042 May 16, 2018

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented May 16, 2018

@AlonaNadler

This comment has been minimized.

Copy link

AlonaNadler commented May 17, 2018

Thanks @nreese just played with that and it looks great!

nreese added some commits Jun 20, 2018

@nreese

This comment has been minimized.

Copy link
Contributor Author

nreese commented Jun 20, 2018

bug with searching on ip field

Turns out that terms aggregation includes property only works for string fields.

{
  "size": 0,
  "aggs": {
    "termsAgg": {
      "terms": {
        "size": 5,
        "order": {
          "_count": "desc"
        },
        "field": "clientip",
        "include": "1.*"
      }
    }
  },
  "query": {
    "bool": {
      "must": [],
      "filter": [],
      "should": [],
      "must_not": []
    }
  }
}

returns

{
  "took": 8,
  "responses": [
    {
      "error": {
        "root_cause": [
          {
            "type": "aggregation_execution_exception",
            "reason": "Aggregation [termsAgg] cannot support regular expression style include/exclude settings as they can only be applied to string fields. Use an array of values for include/exclude clauses"
          }
        ],
        "type": "search_phase_execution_exception",
        "reason": "all shards failed",
        "phase": "query",
        "grouped": true,
        "failed_shards": [
          {
            "shard": 0,
            "index": "logstash-0",
            "node": "IrnksBJLQ6iv4LreZEBhJg",
            "reason": {
              "type": "aggregation_execution_exception",
              "reason": "Aggregation [termsAgg] cannot support regular expression style include/exclude settings as they can only be applied to string fields. Use an array of values for include/exclude clauses"
            }
          }
        ]
      },
      "status": 500
    }
  ]
}

I tried using filters for non-string fields and that did not work either.

GET logstash-*/_search
{
  "size": 0,
  "aggs": {
    "termsAgg": {
      "terms": {
        "size": 100,
        "order": {
          "_count": "desc"
        },
        "field": "clientip"
      }
    }
  },
  "query": {
    "bool": {
      "must": [
        {
          "prefix" : { 
            "clientip" : "1" 
          }
        }
      ],
      "filter": [],
      "should": [],
      "must_not": []
    }
  }
}

just returns

{
  "error": {
    "root_cause": [
      {
        "type": "query_shard_exception",
        "reason": "Can only use prefix queries on keyword and text fields - not on [clientip] which is of type [ip]",
        "index_uuid": "KWhhNlvAQwC9MWfaL6f8sw",
        "index": "logstash-0"
      }
    ],
    "type": "search_phase_execution_exception",
    "reason": "all shards failed",
    "phase": "query",
    "grouped": true,
    "failed_shards": [
      {
        "shard": 0,
        "index": "logstash-0",
        "node": "IrnksBJLQ6iv4LreZEBhJg",
        "reason": {
          "type": "query_shard_exception",
          "reason": "Can only use prefix queries on keyword and text fields - not on [clientip] which is of type [ip]",
          "index_uuid": "KWhhNlvAQwC9MWfaL6f8sw",
          "index": "logstash-0"
        }
      }
    ]
  },
  "status": 400
}

Looks like this option will only be available for String fields.

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jun 20, 2018

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jun 25, 2018

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jun 25, 2018

@nreese

This comment has been minimized.

Copy link
Contributor Author

nreese commented Jun 26, 2018

blocked on elastic/eui#947

nreese added some commits Jul 6, 2018

clean up list control editor to only display options when field selec…
…ted and only display dynamic checkbox when field is string
@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jul 6, 2018

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jul 6, 2018

@nreese

This comment has been minimized.

Copy link
Contributor Author

nreese commented Jul 6, 2018

@stacey-gammon This is finally ready for review. Now the dynamic options checkbox is only visible for string fields. When checked, size is not allowed to be set.

@stacey-gammon

This comment has been minimized.

Copy link
Contributor

stacey-gammon commented Jul 9, 2018

The dynamic options checkbox is only visible for string fields.

What do you think about making it visible but disabled with a tooltip explanation for non-string fields? Just because I envision answering discuss tickets "why can't I see this option with this field". cc @elastic/kibana-design .

@stacey-gammon
Copy link
Contributor

stacey-gammon left a comment

small suggestion about the UI and the matching, but otherwise lgtm. Did a code review and pulled code and played around with searching the field values on the visualize app with both dynamic options turned on and off.

@@ -45,6 +51,11 @@ const termsAgg = (field, size, direction) => {
} else {
terms.field = field.name;
}

if (query) {
terms.include = `${getEscapedQuery(query)}.*`;

This comment has been minimized.

Copy link
@stacey-gammon

stacey-gammon Jul 9, 2018

Contributor

What do you think about .*${getEscapedQuery(query)}.* so it searches the middle of the word too, not just a prefix? Esp since thats how it works when you use the size option.

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jul 9, 2018

@elasticmachine

This comment has been minimized.

Copy link

elasticmachine commented Jul 10, 2018

@nreese nreese merged commit 00f81dc into elastic:master Jul 10, 2018

2 checks passed

CLA Commit author has signed the CLA
Details
kibana-ci Build finished.
Details

nreese added a commit to nreese/kibana that referenced this pull request Jul 10, 2018

[input controls] update dropdown suggestions when filtered (elastic#1…
…8985)

* input controls - re-fetch options list user input

* fix jest tests

* add functional test case

* remove unneeded async and await

* use fetchAsRejectablePromise instead of fetch to avoid kill kibana session when fetching agg results

* only show options once field is selected

* clean up list control editor to only display options when field selected and only display dynamic checkbox when field is string

* do not use size when using dynamic options

* display disabled toggle when field is not string field, allow searching in middle of word

* no tooltip for disabled dyncamic options, just change help text

* fix functional test expects since search now includes more than terms that start with

nreese added a commit that referenced this pull request Jul 10, 2018

[input controls] update dropdown suggestions when filtered (#18985) (#…
…20604)

* input controls - re-fetch options list user input

* fix jest tests

* add functional test case

* remove unneeded async and await

* use fetchAsRejectablePromise instead of fetch to avoid kill kibana session when fetching agg results

* only show options once field is selected

* clean up list control editor to only display options when field selected and only display dynamic checkbox when field is string

* do not use size when using dynamic options

* display disabled toggle when field is not string field, allow searching in middle of word

* no tooltip for disabled dyncamic options, just change help text

* fix functional test expects since search now includes more than terms that start with

@bhavyarm bhavyarm referenced this pull request Aug 22, 2018

Open

Accessibility 6.4 meta issue #22250

9 of 16 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.