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

Add advanced setting to turn off search on Discover's initial page load #42036

Merged
merged 8 commits into from
Jul 31, 2019

Conversation

Bargs
Copy link
Contributor

@Bargs Bargs commented Jul 25, 2019

Summary

Related to #7238

Discover currently executes a search as soon as it loads. For some users this is useful. But for others it may return worthless results at the expense of extra load on their ES cluster and increased page load times, making it harder to get to the data they actually want. This PR adds an advanced setting allowing users to turn off the "search on page load" functionality in Discover.

One thing to note is that I chose to continue loading results immediately when loading a saved search (for example, from a bookmark). If you're loading a specific saved search, I think that's an indication that you're interested in seeing its results immediately.

Screen Shot 2019-07-25 at 7 42 03 PM

Screen Shot 2019-07-25 at 7 41 43 PM

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

- [ ] This was checked for cross-browser compatibility, including a check against IE11

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately

@Bargs Bargs added release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 v7.4.0 labels Jul 25, 2019
@Bargs Bargs requested review from kertal and TinaHeiligers July 25, 2019 23:56
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@elasticmachine
Copy link
Contributor

💔 Build Failed

@kertal
Copy link
Member

kertal commented Jul 26, 2019

jenkins test this

@kertal kertal added the Feature:Discover Discover Application label Jul 26, 2019
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Code LGTM, tested locally with Chrome, I found one thing to discuss:
When you set a refresh interval, reload discover, initially the unintializedText is displayed until the first interval period is over. I think it would be better to also start the reloading after the first click on refresh

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

Code LGTM.
I tested the implementation and setting the searchOnPageLoad to false prevents the initial query being sent. The setting also prevents a query being sent on page load if a filter or a query or a date filter has already been applied when navigating away from Discover to another application (Visualize for example) and then navigating back to Discover if no action was taken in the other app.
The steps I took are:

  1. Add a filter and issue the request in Discover. We get the results from the search:
    Screen Shot 2019-07-26 at 8 17 14 AM
  2. Navigate away from Discover to another app but don't load anything:
    Screen Shot 2019-07-26 at 8 18 48 AM
  3. Go back to Discover and the filter/query/date filters are still present but the search request was not issued.
    Screen Shot 2019-07-26 at 8 19 39 AM
    The implementation satisfies the intent of preventing requests on every initial load if the setting is applied and a saved search is not being loaded.

Bargs added 2 commits July 26, 2019 12:32
* Search on page load if refresh interval is set
* Search as soon as auto refresh is turned on
@Bargs
Copy link
Contributor Author

Bargs commented Jul 26, 2019

@kertal I agree with you, the interaction with the auto refresh was a bit weird. I've made two changes:

  • A search will now execute as soon as you turn on auto refresh
  • A search will execute on page load if auto refresh is enabled, regardless of the new setting's value

@TinaHeiligers The scenario you outlined is one I thought about a bit. If a user is bouncing between apps and they've already applied a query and/or filters in Discover, I'm not sure if they would want to automatically load results every time they bounce to Discover or not. On the one hand, since they entered the query then maybe they want to see those results. On the other hand the old query may be stale and they may be coming back to Discover to execute a new query. Since the primary use of this setting is to reduce load I erred on the side of not automatically loading results in this situation. I figure if someone wants to reference back to one specific query frequently, they can keep it open in a separate tab.

@Bargs
Copy link
Contributor Author

Bargs commented Jul 26, 2019

@AlonaNadler would you like to take a look at this before it is merged?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@AlonaNadler
Copy link

@Bargs haven't checked it out yet just by the screen I have a few suggestions:

  • How can we make this to not feel like an empty screen? Maybe @elastic/kibana-design will have a few ideas. We can do something fun here instead of leaving it empty.
  • Can we add a few examples for search queries? and work on the wording, maybe @gchaps will have ideas

@AlonaNadler AlonaNadler reopened this Jul 26, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Code LGTM, now the refresh interval behaviour is fine, tested with Chrome

@cchaos
Copy link
Contributor

cchaos commented Jul 30, 2019

Hey @Bargs I'm gonna push you a PR soon that will fill in that empty state. cc @AlonaNadler

@cchaos
Copy link
Contributor

cchaos commented Jul 30, 2019

PR4U: Bargs#8

@Bargs Bargs requested a review from a team as a code owner July 31, 2019 17:14
@Bargs
Copy link
Contributor Author

Bargs commented Jul 31, 2019

Thanks for the PR @cchaos. The only thing I noticed is that the discover results also have a border around them now. Is that intentional?

Screen Shot 2019-07-31 at 1 43 09 PM

@elasticmachine
Copy link
Contributor

💔 Build Failed

@cchaos
Copy link
Contributor

cchaos commented Jul 31, 2019

Yes the main results now live in a panel, similar to how we handle other pages within Kibana.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Bargs Bargs merged commit 90ec9bc into elastic:master Jul 31, 2019
Bargs added a commit to Bargs/kibana that referenced this pull request Jul 31, 2019
…ad (elastic#42036)

Discover currently executes a search as soon as it loads. For some users this is useful. But for others it may return worthless results at the expense of extra load on their ES cluster and increased page load times, making it harder to get to the data they actually want. This PR adds an advanced setting allowing users to turn off the "search on page load" functionality in Discover.
Bargs added a commit that referenced this pull request Aug 1, 2019
…ad (#42036) (#42408)

Discover currently executes a search as soon as it loads. For some users this is useful. But for others it may return worthless results at the expense of extra load on their ES cluster and increased page load times, making it harder to get to the data they actually want. This PR adds an advanced setting allowing users to turn off the "search on page load" functionality in Discover.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Discover Discover Application release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.4.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants