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

Improve kql error message handling and avoid fetcihng twice #54239

Merged
merged 1 commit into from
Jan 10, 2020

Conversation

flash1293
Copy link
Contributor

Fixes #48392

The code building the errors message for a KQL parsing exception assumes the expected property is an array - in some cases (e.g. when using a leading wildcard) this is not the case.

This PR makes sure in these cases a meaningful error message is built.

While working on this problem I noticed that Discover sends two requests when updating the query because there is a watcher on the query state that triggers an additional fetch. In this PR it is made sure that the fetch is only triggered when necessary.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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

@flash1293 flash1293 marked this pull request as ready for review January 8, 2020 13:41
@flash1293 flash1293 requested a review from a team January 8, 2020 13:41
@flash1293 flash1293 requested a review from a team as a code owner January 8, 2020 13:41
@flash1293 flash1293 requested a review from Bargs January 8, 2020 13:42
@flash1293 flash1293 added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Jan 8, 2020
@elasticmachine
Copy link
Contributor

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

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

Copy link
Contributor

@Bargs Bargs left a comment

Choose a reason for hiding this comment

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

Error message fix looks good 👍

The change to the watcher does fix the double request, though I'm a bit worried we're missing whatever change caused this to happen in the first place. That watcher has been around for a long time but the double request seems to be a recent problem. Also Visualize and Dashboard have similar watchers, but don't have this problem. They've diverged enough that I can't immediately see what the difference is though. I thought courier had some deduplication logic in the past, I wonder if anything changed there @lukasolson ?

In any case I suppose this fix works fine for the problem at hand in Discover, I'm just nervous that we don't know why it broke.

@flash1293
Copy link
Contributor Author

flash1293 commented Jan 10, 2020

@Bargs I can reproduce the problem on upstream/7.5 and on a 7.5.0 cloud instance - seems like it has been a problem for a while. The implementation in visualize has the check whether the migrated query has changed (which would prevent the double fetch: https://github.com/elastic/kibana/blob/master/src/legacy/core_plugins/kibana/public/visualize/np_ready/editor/editor.js#L374).

fyi @rayafratkina

Merging this despite missing code ownership review from Kibana App (come back home @Bargs , we miss you).

@Bargs
Copy link
Contributor

Bargs commented Jan 10, 2020

@flash1293 the $scope.fetch happens outside of the conditional you linked to, so that doesn't explain that lack of a double fetch.

I dunno how long this problem has existed, but that watcher has been around since 6.x and I don't think we've been doing a double fetch for that long.

@flash1293
Copy link
Contributor Author

flash1293 commented Jan 11, 2020

Ah, you are right, the story is more complicated than that. There is also a double fetch on changing the time filter, so it might be an underlying problem. I will keep digging and test some older releases as well to pin-point it. #54526

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Discover Discover Application Feature:KQL KQL release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wildcard search disable and error message
5 participants