make timeout and terminate_after configurable #17423
Conversation
nreese
added
release_note:enhancement
:Sharing
v7.0.0
Feature:Input Control
💔 Build Failed |
💚 Build Succeeded |
|
Might be an EUI thing (?) but the icons in your screenshots seem crammed right up to the text:
Maybe you should put a |
| <EuiIconTip | ||
| content={`Terms search timeout (seconds), | ||
| bounding the terms request to be executed within the specified time value and | ||
| bail with the hits accumulated up to that point when expired.`} |
There was a problem hiding this comment.
I'm an Elasticsearch n00b, so this is probably my fault, but I find this verbiage a bit confusing. I'm happy to kick around ideas for how to clear it up.
| label={( | ||
| <Fragment> | ||
| Terminate After | ||
| <EuiIconTip |
There was a problem hiding this comment.
This is fine. But "Terminate After" might be clearer if it was "Max Number of Documents" or something like that. To me, "Terminate After" implies a duration, such as "Terminate after so many seconds of execution" or something like that.
| @@ -16,9 +17,24 @@ export function FormRow(props) { | |||
| ); | |||
| } | |||
|
|
|||
| let label = props.label; | |||
There was a problem hiding this comment.
Might consider moving the conditional out into a helper function:
function renderLabel({ label, control }) {
if (!control.warning || control.warning.length === 0) {
return label;
}
return (
<Fragment>
{label}
<EuiIconTip
content={control.warning}
position="right"
type="alert"
aria-label="Warning"
/>
</Fragment>
);
}So this would become:
return (
<EuiFormRow
label={renderLabel(props)}
id={props.id}
data-test-subj={'inputControl' + props.controlIndex}
>
{control}
</EuiFormRow>
);Same goes for the let control = props.children section.
| timeout: '1s', | ||
| terminate_after: 100000 | ||
| timeout: `${_.get(this.options, 'timeout', DEFAULT_TIMEOUT)}s`, | ||
| terminate_after: _.get(this.options, 'terminateAfter', DEFAULT_TERMINATE_AFTER) |
There was a problem hiding this comment.
Same comment here, I think terminate_after is a bit misleading. max_result_set_size or max_num_docs or something might be clearer. Also, should it be camelCased instead of snake_cased?
There was a problem hiding this comment.
I have to use terminate_after here because it is an Elasticsearch parameter, https://www.elastic.co/guide/en/elasticsearch/reference/current/search-request-body.html#_parameters_4.
But I can change the label and description in the UI. Although, it might make sense to stay with Elasticsearch naming schemes since there are really just ways to allow users to configure elasticsearh parameters.
There was a problem hiding this comment.
Yup. Agreed that it's best to stick w/ the Elasticsearch terminology. I wasn't aware of that!
💚 Build Succeeded |
There was a problem hiding this comment.
I’m concerned with giving every single kibana user the power to turn off the only safeguards we have preventing them from accidentally taking down a cluster.
I quickly scanned the linked issues, it looked to me like the main issue was the return of partial results, is that correct? If so, we could take advantage of elasticsearch’s new capabilities to return an error instead of partial results when a timeout is hit. We could put these settings in kibana.yml, and if a user needs it they could ask their admin to increase the limit.
Perhaps there are better solutions too, my primary concern is with putting these settings in the hands of users who do not fully understand them.
|
@stacey-gammon Is this possible to get into 6.3 as is? @lukasolson posted a comment about using the suggestions API for fetching the terms and maybe adding support for dynamic Regardless of underlying implementation, users need to able to configure |
How is this any different than letting a user create a visualization with a terms aggregation over a large time range? |
That request is still constrained by the admin configured Even if that weren't the case, we're trying to reduce the number of ways users can take down a cluster, not increase them. |
|
I don't think we should rush this into 6.3 because of the concerns mentioned. What do you think about solving this issue by letting user type in a value that doesn't appear in the drop down like the discover input text box does? I don't think we have to rush a fix for this into 6.3 that could have some pretty bad side affects.
As soon as we expose these options, we can't get rid of them without BWC issues. So even if we later solve the problem by letting users type in a value that doesn't show up in the drop down, we still can't easily remove these settings with first deprecating them, then waiting for a major version release to get rid of completely. |
|
Closing, #18985 updates dropdown suggestions when filtered. This should help avoid the problem of early termination. |
fixes #17388 and #17156
Add inputs to configure timeout and terminate_after parameters used when fetching terms for the options list.
Generated search request
PR also adds visual notification when terms search terminated early.
