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.

Sign up for GitHub

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

Jump to bottom

[Discover] fix auto-refresh #80635

Merged
merged 4 commits into from
Oct 16, 2020
Merged

[Discover] fix auto-refresh #80635

merged 4 commits into from
Oct 16, 2020

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Oct 15, 2020 •
edited
Loading

Summary

fixes #80586
Also implicitly fixes a subtle bug with excessive re-fetch after deleting already disabled filter

Functional test note

I found this test in visualize which uses inspector view to check that auto-refresh is working and used similar approach in discover

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@Dosant Dosant added bug Fixes for quality problems that affect the customer experience regression release_note:skip Skip the PR/issue when compiling release notes Team:AppArch v7.10.0 v7.11.0 v8.0.0 labels Oct 15, 2020
@Dosant Dosant marked this pull request as ready for review October 15, 2020 14:15
@Dosant Dosant requested a review from a team October 15, 2020 14:15
@Dosant Dosant requested a review from a team as a code owner October 15, 2020 14:15
@elasticmachine
Copy link
Contributor

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

LeeDr
LeeDr approved these changes Oct 15, 2020
View reviewed changes
Copy link

@LeeDr LeeDr left a comment

Choose a reason for hiding this comment

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

LGTM - I only reviewed that a new test was added and passed

image

Also, while I thought Dashboard refresh wasn't working on 7.10.0, after double-checking I see that it is indeed working now.

lizozom
lizozom reviewed Oct 16, 2020
View reviewed changes
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 added a patch file with a small modification to this fix.

src/plugins/data/public/query/query_string/query_string_manager.ts Show resolved Hide resolved
src/plugins/discover/public/application/angular/discover.js
@@ -633,12 +633,18 @@ function discoverController($element, $route, $scope, $timeout, $window, Promise

const init = _.once(() => {
$scope.updateDataSource().then(async () => {
const searchBarChanges = merge(data.query.state$, $fetchObservable).pipe(debounceTime(100));
const fetch$ = merge(
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of reverting to using individual observables and relying on the handleRefresh (which can actually be removed) I'm suggesting the fix in the attached patch file.

80635.txt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the patch, but I think it would cause issue with excessive refetch on removing disabled filter which I mentioned in the description:

Also implicitly fixes a subtle bug with excessive re-fetch after deleting already disabled filter

Also some thoughts on query.state$ vs fetch$: #80586 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. Thanks for the feedback.

lizozom
lizozom approved these changes Oct 16, 2020
View reviewed changes
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.

LGTM

download (3)

kertal
kertal approved these changes Oct 16, 2020
View reviewed changes
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 in Chrome, Firefox, Safari, Auto-Refresh works again 👍

@Dosant Dosant changed the title fix auto-refresh in discover [Discover] fix auto-refresh Oct 16, 2020
@Dosant
Copy link
Contributor Author

Dosant commented Oct 16, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
discover 439.3KB 439.3KB +43.0B

page load bundle size

id before after diff
data 1.1MB 1.1MB +63.0B

History

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

@Dosant Dosant merged commit a23b423 into elastic:master Oct 16, 2020
@Dosant Dosant mentioned this pull request Oct 16, 2020
Merged
Dosant added a commit to Dosant/kibana that referenced this pull request Oct 16, 2020
@Dosant @kibanamachine
[Discover] fix auto-refresh (elastic#80635)
7f5c656 
* fix refresh interval in discover
* Also implicitly fixes a subtle bug with excessive re-fetch after deleting already disabled filter

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@Dosant Dosant mentioned this pull request Oct 16, 2020
Merged
Dosant added a commit to Dosant/kibana that referenced this pull request Oct 16, 2020
@Dosant @kibanamachine
[Discover] fix auto-refresh (elastic#80635)
3467075 
* fix refresh interval in discover
* Also implicitly fixes a subtle bug with excessive re-fetch after deleting already disabled filter

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Dosant added a commit that referenced this pull request Oct 16, 2020
@Dosant @kibanamachine
[Discover] fix auto-refresh (#80635) (#80863)
7934ccc 
* fix refresh interval in discover
* Also implicitly fixes a subtle bug with excessive re-fetch after deleting already disabled filter

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Dosant added a commit that referenced this pull request Oct 16, 2020
@Dosant @kibanamachine
[Discover] fix auto-refresh (#80635) (#80864)
59d3e06 
* fix refresh interval in discover
* Also implicitly fixes a subtle bug with excessive re-fetch after deleting already disabled filter

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 19, 2020
@gmmorris
Merge branch 'master' into task-manager/refactor-oversized-class
c30d71b 
* master: (43 commits)
  [ML] Transforms: Fix tab ids for expanded row. (elastic#80666)
  server logs config paths to use for runner (elastic#52980)
  Fix audit logger logging to console even when disabled (elastic#80928)
  skip flaky suite (elastic#80929)
  Added Enterprise Search config to kibana-docker (elastic#80872)
  skip flaky suite (elastic#80914)
  [keystore_cli] parse values as JSON before adding to keystore (elastic#80848)
  [Ingest Manager] Fix for comparing versions with -SNAPSHOT suffix (elastic#80742)
  ECS audit logging (elastic#74640)
  [Uptime] Add client-side unit tests for remaining synthetics code (elastic#80215)
  [Security_Solution][Resolver] Promote z-index on node labels (elastic#80854)
  Move renderHeaderActions back into mount useEffect + update tests (elastic#80861)
  [Reporting] Document Network Policy configuration (elastic#80431)
  [Reporting] Add contextual documentation for CSV Max Bytes setting (elastic#80782)
  Add catch for Enterprise Search sending back a 401 response instead of redirect (elastic#80757)
  [Actions] Back Button on Add Connector Flyout (elastic#80160)
  removing `kibana_datatable` in favor of `datatable`  (elastic#80548)
  [Alerting UI] Updating 'Add new' wording (elastic#80509)
  [Docs] Document Encrypted Saved Objects functionality. (elastic#80183)
  [Discover] fix auto-refresh (elastic#80635)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 19, 2020
@gmmorris
Merge branch 'master' into task-manager/health
12e52e6 
* master: (23 commits)
  [ML] Transforms: Fix tab ids for expanded row. (elastic#80666)
  server logs config paths to use for runner (elastic#52980)
  Fix audit logger logging to console even when disabled (elastic#80928)
  skip flaky suite (elastic#80929)
  Added Enterprise Search config to kibana-docker (elastic#80872)
  skip flaky suite (elastic#80914)
  [keystore_cli] parse values as JSON before adding to keystore (elastic#80848)
  [Ingest Manager] Fix for comparing versions with -SNAPSHOT suffix (elastic#80742)
  ECS audit logging (elastic#74640)
  [Uptime] Add client-side unit tests for remaining synthetics code (elastic#80215)
  [Security_Solution][Resolver] Promote z-index on node labels (elastic#80854)
  Move renderHeaderActions back into mount useEffect + update tests (elastic#80861)
  [Reporting] Document Network Policy configuration (elastic#80431)
  [Reporting] Add contextual documentation for CSV Max Bytes setting (elastic#80782)
  Add catch for Enterprise Search sending back a 401 response instead of redirect (elastic#80757)
  [Actions] Back Button on Add Connector Flyout (elastic#80160)
  removing `kibana_datatable` in favor of `datatable`  (elastic#80548)
  [Alerting UI] Updating 'Add new' wording (elastic#80509)
  [Docs] Document Encrypted Saved Objects functionality. (elastic#80183)
  [Discover] fix auto-refresh (elastic#80635)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LeeDr LeeDr LeeDr approved these changes

@lizozom lizozom lizozom approved these changes

@kertal kertal kertal approved these changes

Assignees
No one assigned
Labels
bug Fixes for quality problems that affect the customer experience regression release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v7.11.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Auto-Refresh isn't working in 7.10.0
6 participants
@Dosant @elasticmachine @kibanamachine @kertal @lizozom @LeeDr