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

Fix "Dashboard doesn't display results if user navigates to open dashboard from discover" - Option 2 #64999

Merged
merged 6 commits into from
May 4, 2020
Merged

Fix "Dashboard doesn't display results if user navigates to open dashboard from discover" - Option 2 #64999

merged 6 commits into from
May 4, 2020

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented May 1, 2020 •
edited
Loading

Summary

This fixes: #64709
by a tiny workaround of underlying issue. Option 1 would a proper fix, but it touches a dozen of files.
and functional test from: #64964

Explanation:

In 7.7 branch both courier:enableBatch: true / false are going through this code path:
https://github.com/elastic/kibana/blob/7.7/src/plugins/data/public/search/fetch/fetch_soon.ts#L64

In case of courier:enableBatch: false ms = 0. The assumption was that request grouping could not happen when ms==0, but if requests were fired in scope of one micro task, then we those requests were still grouped. (because delay used setTimeout).

This was causing problems when canceling such requests. If 1 request in group was canceled, other requests were canceled too.

For example. in this dashboard, https://github.com/elastic/kibana/files/4558779/minimal.reproduction.zip, bar chart canceled pie chart's requests.

This fix makes sure, that if 0ms is passed in, then we don't delay the execution and such requests are not grouped.

This isn't happening in master, because there was a pr which made courier:enableBatch: false search don't get into fetchSoon at all. #63320

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@Dosant Dosant mentioned this pull request May 1, 2020
Closed
7 tasks
@Dosant Dosant added v7.0.0 Feature:Search Querying infrastructure in Kibana Team:AppArch v7.7.0 and removed v7.0.0 labels May 1, 2020
@elasticmachine
Copy link
Contributor

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

@Dosant Dosant added release_note:skip Skip the PR/issue when compiling release notes bug Fixes for quality problems that affect the customer experience labels May 1, 2020
@Dosant Dosant requested review from lukasolson, flash1293, lizozom and LeeDr and removed request for flash1293 May 1, 2020 11:07
This was referenced May 1, 2020
Closed
Closed
Closed
@Dosant Dosant marked this pull request as ready for review May 4, 2020 11:01
@Dosant
Copy link
Contributor Author

Dosant commented May 4, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@Dosant Dosant requested a review from ppisljar May 4, 2020 13:00
LeeDr
LeeDr approved these changes May 4, 2020
View reviewed changes
Copy link

@LeeDr LeeDr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM - I didn't pull and test it locally, but the test I created passed. I'm also going to use the flaky test runner on it, but let's not wait for that to merge.

UPDATE: The flaky test runner job ran 20 iterations of the test and all passed.

@Dosant Dosant merged commit 7535d01 into elastic:7.7 May 4, 2020
Dosant added a commit to Dosant/kibana that referenced this pull request May 4, 2020
@Dosant
forwardport elastic#64999
31561a9
@Dosant Dosant mentioned this pull request May 4, 2020
Merged
7 tasks
Dosant added a commit that referenced this pull request May 8, 2020
@Dosant @dmlemeshko
fix fetchSoon delay in case of 0ms. Add async_dashboard test (#65083)
7cf7c26 
* forwardport #64999

* Add additional verifications on dashboard

I hope you don't mind me updating the test directly.
I *thought* the other dashboard tests required a consistent set of exact documents so that the count would always be the same.  Since the test uses sample data, I added a new timepicker:quickRanges so that the test can just select it.  Test FTR isn't set up to do relative time ranges right now.
But it looks like the dashboard checks aren't that specific to the data.  The dashboard seems to have `Last 24 hours` saved in it.  And when I don't change it to the whole sample data time range the test still passes.

* fix eslint error

* [page_objects/time_picker] allow any string in setCommonlyUsedTime

Co-authored-by: Lee Drengenberg <lee.drengenberg@elastic.co>
Co-authored-by: Dzmitry Lemechko <dzmitry.lemechko@elastic.co>
Dosant added a commit to Dosant/kibana that referenced this pull request May 8, 2020
@Dosant @dmlemeshko
fix fetchSoon delay in case of 0ms. Add async_dashboard test (elastic…
adc63a1 
…#65083)

* forwardport elastic#64999

* Add additional verifications on dashboard

I hope you don't mind me updating the test directly.
I *thought* the other dashboard tests required a consistent set of exact documents so that the count would always be the same.  Since the test uses sample data, I added a new timepicker:quickRanges so that the test can just select it.  Test FTR isn't set up to do relative time ranges right now.
But it looks like the dashboard checks aren't that specific to the data.  The dashboard seems to have `Last 24 hours` saved in it.  And when I don't change it to the whole sample data time range the test still passes.

* fix eslint error

* [page_objects/time_picker] allow any string in setCommonlyUsedTime

Co-authored-by: Lee Drengenberg <lee.drengenberg@elastic.co>
Co-authored-by: Dzmitry Lemechko <dzmitry.lemechko@elastic.co>
Dosant added a commit to Dosant/kibana that referenced this pull request May 8, 2020
@Dosant @dmlemeshko
fix fetchSoon delay in case of 0ms. Add async_dashboard test (elastic…
18f619e 
…#65083)

* forwardport elastic#64999

* Add additional verifications on dashboard

I hope you don't mind me updating the test directly.
I *thought* the other dashboard tests required a consistent set of exact documents so that the count would always be the same.  Since the test uses sample data, I added a new timepicker:quickRanges so that the test can just select it.  Test FTR isn't set up to do relative time ranges right now.
But it looks like the dashboard checks aren't that specific to the data.  The dashboard seems to have `Last 24 hours` saved in it.  And when I don't change it to the whole sample data time range the test still passes.

* fix eslint error

* [page_objects/time_picker] allow any string in setCommonlyUsedTime

Co-authored-by: Lee Drengenberg <lee.drengenberg@elastic.co>
Co-authored-by: Dzmitry Lemechko <dzmitry.lemechko@elastic.co>
Dosant added a commit that referenced this pull request May 8, 2020
@Dosant @dmlemeshko
fix fetchSoon delay in case of 0ms. Add async_dashboard test (#65083) (…
33041e5 
…#65827)

* forwardport #64999

* Add additional verifications on dashboard

I hope you don't mind me updating the test directly.
I *thought* the other dashboard tests required a consistent set of exact documents so that the count would always be the same.  Since the test uses sample data, I added a new timepicker:quickRanges so that the test can just select it.  Test FTR isn't set up to do relative time ranges right now.
But it looks like the dashboard checks aren't that specific to the data.  The dashboard seems to have `Last 24 hours` saved in it.  And when I don't change it to the whole sample data time range the test still passes.

* fix eslint error

* [page_objects/time_picker] allow any string in setCommonlyUsedTime

Co-authored-by: Lee Drengenberg <lee.drengenberg@elastic.co>
Co-authored-by: Dzmitry Lemechko <dzmitry.lemechko@elastic.co>

Co-authored-by: Lee Drengenberg <lee.drengenberg@elastic.co>
Co-authored-by: Dzmitry Lemechko <dzmitry.lemechko@elastic.co>
Dosant added a commit that referenced this pull request May 11, 2020
@Dosant @dmlemeshko @elasticmachine
fix fetchSoon delay in case of 0ms. Add async_dashboard test (#65083) (…
92d94bf 
…#65826)

* forwardport #64999

* Add additional verifications on dashboard

I hope you don't mind me updating the test directly.
I *thought* the other dashboard tests required a consistent set of exact documents so that the count would always be the same.  Since the test uses sample data, I added a new timepicker:quickRanges so that the test can just select it.  Test FTR isn't set up to do relative time ranges right now.
But it looks like the dashboard checks aren't that specific to the data.  The dashboard seems to have `Last 24 hours` saved in it.  And when I don't change it to the whole sample data time range the test still passes.

* fix eslint error

* [page_objects/time_picker] allow any string in setCommonlyUsedTime

Co-authored-by: Lee Drengenberg <lee.drengenberg@elastic.co>
Co-authored-by: Dzmitry Lemechko <dzmitry.lemechko@elastic.co>

Co-authored-by: Lee Drengenberg <lee.drengenberg@elastic.co>
Co-authored-by: Dzmitry Lemechko <dzmitry.lemechko@elastic.co>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
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

@lukasolson lukasolson Awaiting requested review from lukasolson

@lizozom lizozom Awaiting requested review from lizozom

@ppisljar ppisljar Awaiting requested review from ppisljar

Assignees
No one assigned
Labels
bug Fixes for quality problems that affect the customer experience Feature:Search Querying infrastructure in Kibana release_note:skip Skip the PR/issue when compiling release notes v7.7.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Dosant @elasticmachine @kibanamachine @LeeDr