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

[Search] Remove long-running query pop-up #75385

Merged
merged 115 commits into from
Sep 13, 2020

Conversation

lukasolson
Copy link
Member

Summary

Resolves #75356.

This PR removes the long-running query pop-up that shows up after 15 seconds and prompts the user to run beyond timeout. This will be replaced by a configuration value that administrators can control for async search requests explained here: #75321

Checklist

  • Functional tests

(Release notes will be added in the PR that adds the new advanced setting.)

@lukasolson lukasolson added Feature:Search Querying infrastructure in Kibana v8.0.0 Team:AppArch release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Aug 19, 2020
@lukasolson lukasolson requested a review from a team as a code owner August 19, 2020 02:11
@lukasolson lukasolson self-assigned this Aug 19, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@lizozom
Copy link
Contributor

lizozom commented Aug 20, 2020

@elasticmachine merge upstream

@lizozom lizozom closed this Aug 20, 2020
@elasticmachine
Copy link
Contributor

ignoring request to update branch, pull request is closed

@lizozom lizozom reopened this Aug 20, 2020
@lizozom
Copy link
Contributor

lizozom commented Aug 20, 2020

@elasticmachine merge upstream

@lizozom
Copy link
Contributor

lizozom commented Aug 23, 2020

@elasticmachine merge upstream

@@ -124,7 +132,8 @@ export class SearchInterceptor {
// Schedule this request to automatically timeout after some interval
const timeoutController = new AbortController();
const { signal: timeoutSignal } = timeoutController;
const timeout$ = timer(this.requestTimeout);
const timeout$ =
this.requestTimeout != null && this.requestTimeout > 0 ? timer(this.requestTimeout) : NEVER;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

//nit

this.requestTimeout != null && this.requestTimeout > 0 ➡️ !this.requestTimeout ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand, could you provide the code snippet?

Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can go ahead and merge it, adding tests once the delayed agg is available.
LGTM

@lizozom
Copy link
Contributor

lizozom commented Aug 23, 2020

After thinking some more about it, I think that the search interceptor should still be responsible for showing the timeout toast.
The reasoning behind is that we're going to have different types of notifications per-license and permissions, and I think it could be convenient if those were handled in one place.

Alternatively, we could provide a component that is called by the solution and shows the correct message.

Lets talk about it.

@lizozom lizozom removed the request for review from a team September 8, 2020 17:08
@lizozom
Copy link
Contributor

lizozom commented Sep 9, 2020

@elasticmachine merge upstream

@lizozom lizozom removed the request for review from a team September 10, 2020 07:42
// error notification per session.
protected showTimeoutError = debounce(
(e: Error) => {
const message = this.application.capabilities.advancedSettings?.save
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All strings need to be translated

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
data 541 -1 542
dataEnhanced 20 -1 21
total -2

page load bundle size

id value diff baseline
data 1.4MB -5.0KB 1.4MB
dataEnhanced 177.1KB -1.6KB 178.8KB
total -6.6KB

History

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

@lukasolson lukasolson merged commit b99d8af into elastic:master Sep 13, 2020
lukasolson added a commit to lukasolson/kibana that referenced this pull request Sep 13, 2020
* [Search] Remove long-running query pop-up

* Don't timeout if requestTimeout isn't configured

* Remove unused kibanaUtils

* Remove unused kibanaReact

* Re-add reference to kibanaUtils

* Remove unused translations and update documentation

* Add new x-pack advanced setting searchTimeout and use it in the EnhancedSearchInterceptor

* docs

* Re-add toast when queries time out

* Fix types

* Update error message with capabilities

* Update docs

* Update docs

* Move search server routes into a directory.

* Add internal/_msearch route.

* Remove legacy search API, rewrite default search strategy to use internal route.

* Remove legacy es_client code.

* Handle msearch options on server.

* Remove elasticsearch-browser dependency.

* Update generated docs.

* Rely on server timeout in OSS (?)
Use UI setting in xpack.

* Rename function

* Add features to dependencies

* Undefined check

* doc

* Code review fixes

* code review

* doc

* loading count

* simplify code review and fix jest tets

* type check

* Remove esShard from client

* cleanup request parameters from FE

* doc

* doc

* Align request parameters on server,
Remove leftover parameters from client
Shim responses for search and msearch routes

* docs
Stop using toSnakeCase
Updates jest tests

* add management docs

* docs

* Remove import

* Break circular dep + fix msearch test

* Remove deleted type

* Fix jest

* Bring toSnakeCase back

* docs

* fix jest

* Add new x-pack advanced setting searchTimeout and use it in the EnhancedSearchInterceptor

* docs

* Rely on server timeout in OSS (?)
Use UI setting in xpack.

* Rename function

* doc

* Remove esShard from client

* cleanup request parameters from FE

* doc

* doc

* Align request parameters on server,
Remove leftover parameters from client
Shim responses for search and msearch routes

* docs
Stop using toSnakeCase
Updates jest tests

* add management docs

* docs

* Remove import

* Break circular dep + fix msearch test

* Remove deleted type

* Fix jest

* Bring toSnakeCase back

* docs

* fix jest

* Fix merge

* Fix types

* Allow timeout to be undefined

* Fix jest test

* Upldate docs

* Fix msearch jest

* Merge correction

* docs

* Fix rollup search merge

* Fix merge

* Use i18n

Co-authored-by: Liza K <liza.katz@elastic.co>
Co-authored-by: Luke Elmers <luke.elmers@elastic.co>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
lukasolson added a commit that referenced this pull request Sep 13, 2020
* [Search] Remove long-running query pop-up

* Don't timeout if requestTimeout isn't configured

* Remove unused kibanaUtils

* Remove unused kibanaReact

* Re-add reference to kibanaUtils

* Remove unused translations and update documentation

* Add new x-pack advanced setting searchTimeout and use it in the EnhancedSearchInterceptor

* docs

* Re-add toast when queries time out

* Fix types

* Update error message with capabilities

* Update docs

* Update docs

* Move search server routes into a directory.

* Add internal/_msearch route.

* Remove legacy search API, rewrite default search strategy to use internal route.

* Remove legacy es_client code.

* Handle msearch options on server.

* Remove elasticsearch-browser dependency.

* Update generated docs.

* Rely on server timeout in OSS (?)
Use UI setting in xpack.

* Rename function

* Add features to dependencies

* Undefined check

* doc

* Code review fixes

* code review

* doc

* loading count

* simplify code review and fix jest tets

* type check

* Remove esShard from client

* cleanup request parameters from FE

* doc

* doc

* Align request parameters on server,
Remove leftover parameters from client
Shim responses for search and msearch routes

* docs
Stop using toSnakeCase
Updates jest tests

* add management docs

* docs

* Remove import

* Break circular dep + fix msearch test

* Remove deleted type

* Fix jest

* Bring toSnakeCase back

* docs

* fix jest

* Add new x-pack advanced setting searchTimeout and use it in the EnhancedSearchInterceptor

* docs

* Rely on server timeout in OSS (?)
Use UI setting in xpack.

* Rename function

* doc

* Remove esShard from client

* cleanup request parameters from FE

* doc

* doc

* Align request parameters on server,
Remove leftover parameters from client
Shim responses for search and msearch routes

* docs
Stop using toSnakeCase
Updates jest tests

* add management docs

* docs

* Remove import

* Break circular dep + fix msearch test

* Remove deleted type

* Fix jest

* Bring toSnakeCase back

* docs

* fix jest

* Fix merge

* Fix types

* Allow timeout to be undefined

* Fix jest test

* Upldate docs

* Fix msearch jest

* Merge correction

* docs

* Fix rollup search merge

* Fix merge

* Use i18n

Co-authored-by: Liza K <liza.katz@elastic.co>
Co-authored-by: Luke Elmers <luke.elmers@elastic.co>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Liza K <liza.katz@elastic.co>
Co-authored-by: Luke Elmers <luke.elmers@elastic.co>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 14, 2020
…s-for-710

* 'master' of github.com:elastic/kibana: (65 commits)
  Separate url forwarding logic and legacy services (elastic#76892)
  Bump yargs-parser to v13.1.2+ (elastic#77009)
  [Ingest Manager] Shared Fleet agent policy action (elastic#76013)
  [Search] Re-add support for aborting when a connection is closed (elastic#76470)
  [Search] Remove long-running query pop-up (elastic#75385)
  [Monitoring] Fix UI error when alerting is not available (elastic#77179)
  do not log plugin id format warning in dist mode (elastic#77134)
  [ML] Improving client side error handling (elastic#76743)
  [Alerting][Connectors] Refactor IBM Resilient: Generic Implementation (phase one) (elastic#74357)
  [Docs] some basic searchsource api docs (elastic#77038)
  add  cGroupOverrides to the legacy config (elastic#77180)
  Change saved object bulkUpdate to work across multiple namespaces (elastic#75478)
  [Security Solution][Resolver] Replace Selectable popover with badges (elastic#76997)
  Removing ml-state index from archive (elastic#77143)
  [Security Solution] Add unit tests for histograms (elastic#77081)
  [Lens] Filters aggregation  (elastic#75635)
  [Enterprise Search] Update WS Overview logic to use new config data (elastic#77122)
  Cleanup type output before building new types (elastic#77211)
  [Security Solution] Use safe type in resolver backend (elastic#76969)
  Use proper lodash syntax (elastic#77105)
  ...

# Conflicts:
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/node_allocation.tsx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Search Querying infrastructure in Kibana release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Search] Remove run beyond timeout toast
7 participants