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

Fix suggestions dropdown for query input #80990

Merged
merged 7 commits into from
Oct 26, 2020

Conversation

sulemanof
Copy link
Contributor

@sulemanof sulemanof commented Oct 19, 2020

Summary

Fixes #80831

Initial issue: When changing an index pattern in TSVB under panel options from the default one, the KQL autocompletion under "Panel filter" will still show the fields of the default index pattern until you switched away from the Panel Options tab and back to it (unmount - mount the filter component).

It seems the problem was in order of calling updateSuggestions method. Here are logs of async fetchIndexPatterns calls:

image

Steps:

  • typing the kibana_sample_data_logs index pattern;
  • the first call used kibana_sample_data_log and freezed until fetchIndexPatterns completed;
  • right after the second call with kibana_sample_data_logs - then the resolved fetchIndexPatterns immediately updates the suggestions - and then the first call takes precedence and overrides the suggestions value

The fix bring in the AbortController to abort the previous fetchIndexPatterns.
Also, the debounce was added to reduce the amount of calls.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@sulemanof sulemanof added release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.11.0 v8.0.0 labels Oct 19, 2020
@sulemanof sulemanof marked this pull request as ready for review October 19, 2020 21:25
@sulemanof sulemanof requested a review from a team as a code owner October 19, 2020 21:25
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

indexPatterns: [...objectPatterns, ...objectPatternsFromStrings],
});
};
if (!this.fetchIndexPatternsAbortController.signal.aborted) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't test but, if I understood this correctly, this doesn't seem to be working as intended 🤔
Currently every time we are checking the LATEST abort controller, but we should instead check to the
abort controller which was created when we fired a corresponding fetch:

I think it should look something like this instead:

if (this.lastAbortController) this.lastAbortController.abort(); // make sure any older requests are canceled.
this.lastAbortController = new AbortController();
const currentAbortController = this.lastAbortController;
await fetch();
if (!currentAbortController.signal.aborted) {
 // 
 this.setState()
 this.updateSuggestions
}

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 @Dosant , updated

@sulemanof sulemanof requested a review from a team October 20, 2020 15:11
@sulemanof sulemanof requested a review from a team as a code owner October 20, 2020 15:11
@sulemanof sulemanof force-pushed the fix/query_suggestions branch from 0e8cd20 to 9ad9334 Compare October 21, 2020 07:44
@sulemanof
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Code is LGTM, I can't replicate the bug always, but I managed to replicate it on master. I can't replicate it with the new changes so it seems fine to me 🙂

@cchaos cchaos removed the request for review from a team October 22, 2020 14:59
@sulemanof
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
data 286.8KB 287.2KB +394.0B

History

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

Copy link
Contributor

@Dosant Dosant 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, didn't test

@sulemanof sulemanof merged commit b04066b into elastic:master Oct 26, 2020
@sulemanof sulemanof deleted the fix/query_suggestions branch October 26, 2020 11:09
sulemanof added a commit to sulemanof/kibana that referenced this pull request Oct 26, 2020
* Fix suggestions

* Increase timeout

* Use abort controller

* Update abort controller

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
sulemanof added a commit that referenced this pull request Oct 26, 2020
* Fix suggestions

* Increase timeout

* Use abort controller

* Update abort controller

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

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Oct 26, 2020
…arm-phase-to-formlib

* 'master' of github.com:elastic/kibana:
  [Trigger Actions UI] Properly unmount app (elastic#81436)
  skip flaky suite (elastic#81576)
  skip flaky suite (elastic#78373)
  [Security Solution] Fix styling of EditDataProvider content (elastic#81456)
  [Search] Error Alignment 2 (elastic#80965)
  [APM] Unskip test (elastic#81574)
  [ML] Fix partition value selection on the Single Metric Viewer (elastic#81585)
  cleaning up expression service types (elastic#80643)
  Fix suggestions dropdown for query input (elastic#80990)
  [Usage collection] Make `schema` mandatory (elastic#79999)
  [ILM] Update show/hide data tier logic on cloud (elastic#81455)
  added brace import to advanced settings (elastic#81458)
  chore(NA): add spec-to-console and plugin-helpers as devOnly dependencies
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TSVB] Panel filter is showing KQL autocomplete for default index pattern before switching tabs
6 participants