[Metrics Alerts] Handle invalid KQL in filterQuery by Zacqary · Pull Request #119557 · elastic/kibana

Zacqary · 2021-11-23T23:44:40Z

Summary

Closes #119416

This PR will:

Add filterQuery validation to Metric Threshold and Inventory alerts on the frontend. If the KQL query bar is invalid, the alert cannot be saved.

Handle any invalid filterQuery that makes it to the backend, for backwards compatibility. The frontend previously sent an empty filterQuery to the backend, but because the alert executor can also access the filterQueryText, it will validate the filterQueryText before evaluating anything and send an error alert if the query is invalid.

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios

Zacqary · 2021-11-24T20:22:16Z

Reverting a change where I imported some esKuery logic from @kbn/es-query instead of from the data plugin. Importing from the data plugin is deprecated and we're supposed to remove it by 8.1, but changing this ballooned the infra plugin bundle size to the point where it wouldn't pass CI.

We can avoid this by using async imports, but converting all the uses of convertKueryToElasticSearchQuery to async is kind of an invasive project, so I'm punting on this for now.

Zacqary · 2021-11-24T22:40:42Z

@elasticmachine merge upstream

Zacqary · 2021-11-24T23:32:46Z

I have no idea why the page load bundle size is ballooning. I moved all new imports to async and it's still doing this. Very confused. If someone else can help figure this out I'd appreciate it.

miltonhultgren · 2021-11-25T09:15:25Z

@elastic/kibana-operations Can you see anything in this PR that would cause the bundle to blow up?

miltonhultgren

LGTM, just a few code clarity questions 👍🏼

miltonhultgren · 2021-11-25T10:26:50Z

x-pack/plugins/infra/public/alerting/inventory/components/expression.tsx


 import { ExpressionChart } from './expression_chart';
 const FILTER_TYPING_DEBOUNCE_MS = 500;
+export const QUERY_INVALID = Symbol('QUERY_INVALID');


Nit: For me a name like INVALID_QUERY_MARKER would be more clear.

Would it make sense to use unique symbol type and export that type to the other places where this is the value being passed in? So it's not "any symbol"?

miltonhultgren · 2021-11-25T10:27:15Z

x-pack/plugins/infra/public/utils/kuery.ts

  kueryExpression: string,
-  indexPattern: DataViewBase
+  indexPattern: DataViewBase,
+  swallowErrors: boolean = true


Maybe we can convert this to a small helper instead and use composition for the same effect?
I think it's more clear at call site to read something like this instead:
tryOrFallback(convertKueryToElasticSearchQuery(kuery, indexPatterns), '')
This leaves the places that should throw more clear also by not sending the magic boolean into them.

miltonhultgren · 2021-11-25T10:31:00Z

x-pack/plugins/infra/public/alerting/inventory/components/validation.tsx

      metric: string[];
    };
-  } = {};
+  } & { filterQuery?: string[] } = {};


NIt: Can this type live as part of the ValidationResult type?

miltonhultgren · 2021-11-25T10:37:22Z

x-pack/plugins/infra/public/alerting/metric_threshold/components/expression.tsx

 import { useKibanaContextForPlugin } from '../../../hooks/use_kibana';

 const FILTER_TYPING_DEBOUNCE_MS = 500;
+export const QUERY_INVALID = Symbol('QUERY_INVALID');


I'm wondering if there is any benefit to having two symbols for this?

miltonhultgren · 2021-11-25T10:39:06Z

x-pack/plugins/infra/public/alerting/metric_threshold/components/expression.tsx

  const redundantFilterGroupBy = useMemo(() => {
-    if (!alertParams.filterQuery || !groupByFilterTestPatterns) return [];
+    const { filterQuery } = alertParams;
+    if (typeof filterQuery !== 'string' || !groupByFilterTestPatterns) return [];


Nit: I'd expect to compare filterQuery to QUERY_INVALID here instead (TypeScript should ensure it's a string in any other case)

miltonhultgren · 2021-11-25T10:50:11Z

.../infra/server/lib/alerting/inventory_metric_threshold/inventory_metric_threshold_executor.ts

        },
      });

+    if (!params.filterQuery && params.filterQueryText) {


Might be worth wrapping this in a function that calls out it's handling old alerts that didn't have the new guard

mgiota · 2021-11-29T19:39:18Z

@elasticmachine merge upstream

kibana-ci · 2021-11-30T17:58:15Z

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`infra`	928.7KB	991.0KB	+62.3KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`infra`	88.3KB	62.2KB	-26.1KB

Unknown metric groups

async chunk count

id	before	after	diff
`infra`	13	14	+1

History

💔 Build #9795 failed 10e7bc1
💔 Build #9175 failed 95a5628
💔 Build #9146 failed f7087cd
💔 Build #9095 failed acb90e2
💔 Build #8915 failed 7339e3b

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

* [Metrics Alerts] Handle invalid KQL in filterQuery * Add test for malformed KQL, fix existing KQL tests * Revert @kbn/es-query imports

kibanamachine · 2021-11-30T18:01:35Z

💚 Backport successful

Status	Branch	Result
✅	8.0
✅	7.16

The backport PRs will be merged automatically after passing CI.

* [Metrics Alerts] Handle invalid KQL in filterQuery * Add test for malformed KQL, fix existing KQL tests * Revert @kbn/es-query imports Co-authored-by: Zacqary Adam Xeper <Zacqary@users.noreply.github.com>

mgiota · 2021-12-01T08:28:29Z

@Zacqary I was looking into this PR and I need a few clarifications.

In the first bullet of the PR description you say If the KQL query bar is invalid, the alert cannot be saved.. Do you refer to the rule instead? I guess you mean that if the KQL query is not valid, when user clicks the Create rule button, then user gets a validation error and so the Rule can not be saved, right? Let's clarify this
Your screenshot shows two generated alerts, so this screenshot is an example of the second bullet, where an invalid filterQuery made it to the backend, for backwards compatibility, right? The message that appears in the reason column says Alert is using a malformed KQL query... I guess it should be Rule instead. Am I right?

Let's clarify the correct terminology here, because it gets a bit confusing. @jasonrhodes What are your thoughts?

* [Metrics Alerts] Handle invalid KQL in filterQuery * Add test for malformed KQL, fix existing KQL tests * Revert @kbn/es-query imports

[Metrics Alerts] Handle invalid KQL in filterQuery

7339e3b

Add test for malformed KQL, fix existing KQL tests

09e3569

Zacqary marked this pull request as ready for review November 24, 2021 16:55

Zacqary requested a review from a team as a code owner November 24, 2021 16:55

Zacqary added 2 commits November 24, 2021 11:10

Merge remote-tracking branch 'upstream/main' into 119416-kql-validation

acb90e2

Revert @kbn/es-query imports

f7087cd

Zacqary enabled auto-merge (squash) November 24, 2021 21:48

Async import kql

2489138

Merge branch 'main' into 119416-kql-validation

95a5628

miltonhultgren approved these changes Nov 25, 2021

View reviewed changes

kibanamachine and others added 2 commits November 29, 2021 14:39

Merge branch 'main' into 119416-kql-validation

10e7bc1

use async imports for the rule types

1ee8a85

Zacqary merged commit 611da24 into elastic:main Nov 30, 2021

kibanamachine mentioned this pull request Nov 30, 2021

[8.0] [Metrics Alerts] Handle invalid KQL in filterQuery (#119557) #119998

Merged

kibanamachine mentioned this pull request Nov 30, 2021

[7.16] [Metrics Alerts] Handle invalid KQL in filterQuery (#119557) #119999

Merged

This was referenced Jan 13, 2022

[Infra UI] Plugin registrations pull in UI components too eagerly #122941

Closed

[Infra UI] Avoid eager async imports in metric alert registrations #123285

Merged

Comments

Conversation

Zacqary commented Nov 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

Zacqary commented Nov 24, 2021

Uh oh!

Zacqary commented Nov 24, 2021

Uh oh!

Zacqary commented Nov 24, 2021

Uh oh!

miltonhultgren commented Nov 25, 2021

Uh oh!

miltonhultgren left a comment

Choose a reason for hiding this comment

Uh oh!

miltonhultgren Nov 25, 2021

Choose a reason for hiding this comment

Uh oh!

miltonhultgren Nov 25, 2021

Choose a reason for hiding this comment

Uh oh!

miltonhultgren Nov 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miltonhultgren Nov 25, 2021

Choose a reason for hiding this comment

Uh oh!

miltonhultgren Nov 25, 2021

Choose a reason for hiding this comment

Uh oh!

miltonhultgren Nov 25, 2021

Choose a reason for hiding this comment

Uh oh!

miltonhultgren Nov 25, 2021

Choose a reason for hiding this comment

Uh oh!

mgiota commented Nov 29, 2021

Uh oh!

kibana-ci commented Nov 30, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

async chunk count

History

Uh oh!

kibanamachine commented Nov 30, 2021

💚 Backport successful

Uh oh!

mgiota commented Dec 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Zacqary commented Nov 23, 2021 •

edited

Loading

miltonhultgren Nov 25, 2021 •

edited

Loading

mgiota commented Dec 1, 2021 •

edited

Loading