[AO] Compile the KQL expression on the server (#162701)

Closes #159017 ## Summary This PR is related to this [comment](#158665 (comment)). Previously, we had both `fitlerQueryText` (unparsed KQL expression) and `filterQuery` (stringified JSON representation of the KQL expression). This makes the JSON body for creating a rule via the API confusing, as @simianhacker pointed out. In this PR, I've removed `filterQuery` and renamed `fitlerQueryText` to `filterQuery` to avoid confusion and parsed the `filterQuery` when needed. After this change, you should only see `filterQuery` in the AAD and the rule definition with the user's value. |Rule definition|Alert document| |---|---| |![image](https://github.com/elastic/kibana/assets/12370520/05fb04c1-4f5a-4587-b7ac-a90a088b5f26)|![image](https://github.com/elastic/kibana/assets/12370520/2471d144-f284-4721-94ec-cf1b95029786)| ## 🧪 How to test - Create a new threshold rule and add a filter for it - Make sure that all the rule functionalities that use filter work as expected - Preview - Rule execution - Saved data in rule definition and alert document - Add an invalid filter query by editing the rule and make sure the validation works as expected (filter should be invalid state and save button should not work)
elastic · Aug 2, 2023 · d0d3127 · d0d3127
1 parent d7e6f07
commit d0d3127
Show file tree

Hide file tree

Showing 22 changed files with 116 additions and 122 deletions.
diff --git a/x-pack/plugins/observability/common/threshold_rule/types.ts b/x-pack/plugins/observability/common/threshold_rule/types.ts
@@ -204,7 +204,6 @@ export interface MetricAnomalyParams {
 export interface ThresholdParams {
   criteria: MetricExpressionParams[];
   filterQuery?: string;
-  filterQueryText?: string;
   sourceId?: string;
   alertOnNoData?: boolean;
   alertOnGroupDisappear?: boolean;

diff --git a/...lic/components/threshold/components/__snapshots__/alert_details_app_section.test.tsx.snap b/...lic/components/threshold/components/__snapshots__/alert_details_app_section.test.tsx.snap
diff --git a/...lugins/observability/public/components/threshold/components/alert_details_app_section.tsx b/...lugins/observability/public/components/threshold/components/alert_details_app_section.tsx
@@ -41,7 +41,7 @@ import { MetricThresholdRuleTypeParams } from '../types';
 // TODO Use a generic props for app sections https://github.com/elastic/kibana/issues/152690
 export type MetricThresholdRule = Rule<
   MetricThresholdRuleTypeParams & {
-    filterQueryText?: string;
+    filterQuery?: string;
     groupBy?: string | string[];
   }
 >;
@@ -163,7 +163,7 @@ export default function AlertDetailsAppSection({
                   chartType={MetricsExplorerChartType.line}
                   derivedIndexPattern={derivedIndexPattern}
                   expression={criterion}
-                  filterQuery={rule.params.filterQueryText}
+                  filterQuery={rule.params.filterQuery}
                   groupBy={rule.params.groupBy}
                   hideTitle
                   timeRange={timeRange}

diff --git a/...public/components/threshold/components/custom_equation/custom_equation_editor.stories.tsx b/...public/components/threshold/components/custom_equation/custom_equation_editor.stories.tsx
@@ -22,7 +22,7 @@ import { MetricExpression } from '../../types';
 import { validateMetricThreshold } from '../validation';
 
 export default {
-  title: 'infra/alerting/CustomEquationEditor',
+  title: 'app/Alerts/CustomEquationEditor',
   decorators: [
     (wrappedStory) => <div style={{ width: 550 }}>{wrappedStory()}</div>,
     decorateWithGlobalStorybookThemeProviders,
@@ -69,6 +69,7 @@ const CustomEquationEditorTemplate: Story<CustomEquationEditorProps> = (args) =>
   useEffect(() => {
     const validationObject = validateMetricThreshold({
       criteria: [expression as MetricExpressionParams],
+      searchConfiguration: {},
     });
     setErrors(validationObject.errors[0]);
   }, [expression]);

diff --git a/x-pack/plugins/observability/public/components/threshold/components/threshold.stories.tsx b/x-pack/plugins/observability/public/components/threshold/components/threshold.stories.tsx
@@ -15,7 +15,7 @@ import { Props, Threshold as Component } from './threshold';
 
 export default {
   component: Component,
-  title: 'infra/alerting/Threshold',
+  title: 'app/Alerts/Threshold',
   decorators: [
     (Story) => (
       <div

diff --git a/x-pack/plugins/observability/public/components/threshold/components/validation.tsx b/x-pack/plugins/observability/public/components/threshold/components/validation.tsx
@@ -5,17 +5,16 @@
  * 2.0.
  */
 
-import { fromKueryExpression } from '@kbn/es-query';
+import { SerializedSearchSourceFields } from '@kbn/data-plugin/common';
+import { buildEsQuery, fromKueryExpression } from '@kbn/es-query';
 import { i18n } from '@kbn/i18n';
 import { ValidationResult } from '@kbn/triggers-actions-ui-plugin/public';
 import { isEmpty } from 'lodash';
 import {
   Aggregators,
   Comparator,
   CustomMetricExpressionParams,
-  FilterQuery,
   MetricExpressionParams,
-  QUERY_INVALID,
 } from '../../../../common/threshold_rule/types';
 
 export const EQUATION_REGEX = /[^A-Z|+|\-|\s|\d+|\.|\(|\)|\/|\*|>|<|=|\?|\:|&|\!|\|]+/g;
@@ -28,10 +27,12 @@ const isCustomMetricExpressionParams = (
 
 export function validateMetricThreshold({
   criteria,
+  searchConfiguration,
   filterQuery,
 }: {
   criteria: MetricExpressionParams[];
-  filterQuery?: FilterQuery;
+  searchConfiguration: SerializedSearchSourceFields;
+  filterQuery?: string;
 }): ValidationResult {
   const validationResult = { errors: {} };
   const errors: {
@@ -52,17 +53,32 @@ export function validateMetricThreshold({
       customMetrics: Record<string, { aggType?: string; field?: string; filter?: string }>;
       equation?: string;
     };
-  } & { filterQuery?: string[] } = {};
+  } & { filterQuery?: string[]; searchConfiguration?: string[] } = {};
   validationResult.errors = errors;
 
-  if (filterQuery === QUERY_INVALID) {
-    errors.filterQuery = [
-      i18n.translate('xpack.observability.threshold.rule.alertFlyout.error.invalidFilterQuery', {
-        defaultMessage: 'Filter query is invalid.',
-      }),
+  if (!searchConfiguration) {
+    errors.searchConfiguration = [
+      i18n.translate(
+        'xpack.observability.threshold.rule.alertFlyout.error.invalidSearchConfiguration',
+        {
+          defaultMessage: 'Data view is required.',
+        }
+      ),
     ];
   }
 
+  if (filterQuery) {
+    try {
+      buildEsQuery(undefined, [{ query: filterQuery, language: 'kuery' }], []);
+    } catch (e) {
+      errors.filterQuery = [
+        i18n.translate('xpack.observability.threshold.rule.alertFlyout.error.invalidFilterQuery', {
+          defaultMessage: 'Filter query is invalid.',
+        }),
+      ];
+    }
+  }
+
   if (!criteria || !criteria.length) {
     return validationResult;
   }
@@ -84,7 +100,6 @@ export function validateMetricThreshold({
         threshold1: [],
       },
       metric: [],
-      filterQuery: [],
       customMetrics: {},
     };
     if (!c.aggType) {

diff --git a/x-pack/plugins/observability/public/components/threshold/mocks/metric_threshold_rule.ts b/x-pack/plugins/observability/public/components/threshold/mocks/metric_threshold_rule.ts
@@ -84,8 +84,7 @@ export const buildMetricThresholdRule = (
           metric: 'system.memory.used.pct',
         },
       ],
-      filterQuery:
-        '{"bool":{"filter":[{"bool":{"should":[{"term":{"host.hostname":{"value":"Users-System.local"}}}],"minimum_should_match":1}},{"bool":{"should":[{"term":{"service.type":{"value":"system"}}}],"minimum_should_match":1}}]}}',
+      filterQuery: 'host.hostname: Users-System.local and service.type: system',
       groupBy: ['host.hostname'],
     },
     monitoring: {

diff --git a/x-pack/plugins/observability/public/components/threshold/threshold_rule_expression.test.tsx b/x-pack/plugins/observability/public/components/threshold/threshold_rule_expression.test.tsx
@@ -42,7 +42,7 @@ describe('Expression', () => {
     const ruleParams = {
       criteria: [],
       groupBy: undefined,
-      filterQueryText: '',
+      filterQuery: '',
       sourceId: 'default',
       searchConfiguration: {},
     };
@@ -86,7 +86,7 @@ describe('Expression', () => {
     };
     const { ruleParams } = await setup(currentOptions);
     expect(ruleParams.groupBy).toBe('host.hostname');
-    expect(ruleParams.filterQueryText).toBe('foo');
+    expect(ruleParams.filterQuery).toBe('foo');
     expect(ruleParams.criteria).toEqual([
       {
         metric: 'system.load.1',

diff --git a/x-pack/plugins/observability/public/components/threshold/threshold_rule_expression.tsx b/x-pack/plugins/observability/public/components/threshold/threshold_rule_expression.tsx
@@ -35,15 +35,14 @@ import {
 } from '@kbn/triggers-actions-ui-plugin/public';
 
 import { useKibana } from '../../utils/kibana_react';
-import { Aggregators, Comparator, QUERY_INVALID } from '../../../common/threshold_rule/types';
+import { Aggregators, Comparator } from '../../../common/threshold_rule/types';
 import { TimeUnitChar } from '../../../common/utils/formatters/duration';
 import { AlertContextMeta, AlertParams, MetricExpression } from './types';
 import { ExpressionChart } from './components/expression_chart';
 import { ExpressionRow } from './components/expression_row';
 import { MetricsExplorerKueryBar } from './components/kuery_bar';
 import { MetricsExplorerGroupBy } from './components/group_by';
 import { MetricsExplorerOptions } from './hooks/use_metrics_explorer_options';
-import { convertKueryToElasticSearchQuery } from './helpers/kuery';
 
 const FILTER_TYPING_DEBOUNCE_MS = 500;
 
@@ -170,17 +169,9 @@ export default function Expressions(props: Props) {
 
   const onFilterChange = useCallback(
     (filter: any) => {
-      setRuleParams('filterQueryText', filter);
-      try {
-        setRuleParams(
-          'filterQuery',
-          convertKueryToElasticSearchQuery(filter, derivedIndexPattern, false) || ''
-        );
-      } catch (e) {
-        setRuleParams('filterQuery', QUERY_INVALID);
-      }
+      setRuleParams('filterQuery', filter);
     },
-    [setRuleParams, derivedIndexPattern]
+    [setRuleParams]
   );
 
   /* eslint-disable-next-line react-hooks/exhaustive-deps */
@@ -250,23 +241,15 @@ export default function Expressions(props: Props) {
   const preFillAlertFilter = useCallback(() => {
     const md = metadata;
     if (md && md.currentOptions?.filterQuery) {
-      setRuleParams('filterQueryText', md.currentOptions.filterQuery);
-      setRuleParams(
-        'filterQuery',
-        convertKueryToElasticSearchQuery(md.currentOptions.filterQuery, derivedIndexPattern) || ''
-      );
+      setRuleParams('filterQuery', md.currentOptions.filterQuery);
     } else if (md && md.currentOptions?.groupBy && md.series) {
       const { groupBy } = md.currentOptions;
       const filter = Array.isArray(groupBy)
         ? groupBy.map((field, index) => `${field}: "${md.series?.keys?.[index]}"`).join(' and ')
         : `${groupBy}: "${md.series.id}"`;
-      setRuleParams('filterQueryText', filter);
-      setRuleParams(
-        'filterQuery',
-        convertKueryToElasticSearchQuery(filter, derivedIndexPattern) || ''
-      );
+      setRuleParams('filterQuery', filter);
     }
-  }, [metadata, derivedIndexPattern, setRuleParams]);
+  }, [metadata, setRuleParams]);
 
   const preFillAlertGroupBy = useCallback(() => {
     const md = metadata;
@@ -398,7 +381,7 @@ export default function Expressions(props: Props) {
               <ExpressionChart
                 expression={e}
                 derivedIndexPattern={derivedIndexPattern}
-                filterQuery={ruleParams.filterQueryText}
+                filterQuery={ruleParams.filterQuery}
                 groupBy={ruleParams.groupBy}
                 timeFieldName={dataView?.timeFieldName}
               />
@@ -440,20 +423,22 @@ export default function Expressions(props: Props) {
         })}
         fullWidth
         display="rowCompressed"
+        isInvalid={!!errors.filterQuery}
       >
         {(metadata && derivedIndexPattern && (
           <MetricsExplorerKueryBar
             derivedIndexPattern={derivedIndexPattern}
             onChange={debouncedOnFilterChange}
             onSubmit={onFilterChange}
-            value={ruleParams.filterQueryText}
+            value={ruleParams.filterQuery}
           />
         )) || (
           <EuiFieldSearch
             data-test-subj="thresholdRuleExpressionsFieldSearch"
             onChange={handleFieldSearchChange}
-            value={ruleParams.filterQueryText}
+            value={ruleParams.filterQuery}
             fullWidth
+            isInvalid={!!errors.filterQuery}
           />
         )}
       </EuiFormRow>

diff --git a/x-pack/plugins/observability/public/components/threshold/types.ts b/x-pack/plugins/observability/public/components/threshold/types.ts
@@ -4,6 +4,7 @@
  * 2.0; you may not use this file except in compliance with the Elastic License
  * 2.0.
  */
+
 import * as rt from 'io-ts';
 import { CasesUiStart } from '@kbn/cases-plugin/public';
 import { ChartsPluginStart } from '@kbn/charts-plugin/public';
@@ -28,7 +29,6 @@ import { MetricsExplorerSeries } from '../../../common/threshold_rule/metrics_ex
 import {
   Comparator,
   CustomMetricExpressionParams,
-  FilterQuery,
   MetricExpressionParams,
   MetricsSourceStatus,
   NonCountMetricExpressionParams,
@@ -90,9 +90,8 @@ export interface TimeRange {
 export interface AlertParams {
   criteria: MetricExpression[];
   groupBy?: string | string[];
-  filterQuery?: FilterQuery;
   sourceId: string;
-  filterQueryText?: string;
+  filterQuery?: string;
   alertOnNoData?: boolean;
   alertOnGroupDisappear?: boolean;
   searchConfiguration: SerializedSearchSourceFields;

diff --git a/x-pack/plugins/observability/server/lib/rules/threshold/lib/evaluate_rule.ts b/x-pack/plugins/observability/server/lib/rules/threshold/lib/evaluate_rule.ts
@@ -21,7 +21,6 @@ export interface EvaluatedRuleParams {
   criteria: MetricExpressionParams[];
   groupBy: string | undefined | string[];
   filterQuery?: string;
-  filterQueryText?: string;
 }
 
 export type Evaluation = Omit<MetricExpressionParams, 'metric'> & {

diff --git a/x-pack/plugins/observability/server/lib/rules/threshold/lib/metric_query.test.ts b/x-pack/plugins/observability/server/lib/rules/threshold/lib/metric_query.test.ts
@@ -54,10 +54,10 @@ describe("The Metric Threshold Alert's getElasticsearchMetricQuery", () => {
   });
 
   describe('when passed a filterQuery', () => {
-    const filterQuery =
-      // This is adapted from a real-world query that previously broke alerts
-      // We want to make sure it doesn't override any existing filters
-      '{"bool":{"filter":[{"bool":{"filter":[{"bool":{"must_not":[{"bool":{"should":[{"query_string":{"query":"bark*","fields":["host.name^1.0"],"type":"best_fields","default_operator":"or","max_determinized_states":10000,"enable_position_increments":true,"fuzziness":"AUTO","fuzzy_prefix_length":0,"fuzzy_max_expansions":50,"phrase_slop":0,"escape":false,"auto_generate_synonyms_phrase_query":true,"fuzzy_transpositions":true,"boost":1}}],"adjust_pure_negative":true,"minimum_should_match":"1","boost":1}}],"adjust_pure_negative":true,"boost":1}},{"bool":{"must_not":[{"bool":{"should":[{"query_string":{"query":"woof*","fields":["host.name^1.0"],"type":"best_fields","default_operator":"or","max_determinized_states":10000,"enable_position_increments":true,"fuzziness":"AUTO","fuzzy_prefix_length":0,"fuzzy_max_expansions":50,"phrase_slop":0,"escape":false,"auto_generate_synonyms_phrase_query":true,"fuzzy_transpositions":true,"boost":1}}],"adjust_pure_negative":true,"minimum_should_match":"1","boost":1}}],"adjust_pure_negative":true,"boost":1}}],"adjust_pure_negative":true,"boost":1}}],"adjust_pure_negative":true,"boost":1}}';
+    // This is adapted from a real-world query that previously broke alerts
+    // We want to make sure it doesn't override any existing filters
+    // https://github.com/elastic/kibana/issues/68492
+    const filterQuery = 'NOT host.name:dv* and NOT host.name:ts*';
 
     const searchBody = getElasticsearchMetricQuery(
       expressionParams,
@@ -80,6 +80,32 @@ describe("The Metric Threshold Alert's getElasticsearchMetricQuery", () => {
         expect.arrayContaining([
           { range: { mockedTimeFieldName: expect.any(Object) } },
           { exists: { field: 'system.is.a.good.puppy.dog' } },
+          {
+            bool: {
+              filter: [
+                {
+                  bool: {
+                    must_not: {
+                      bool: {
+                        should: [{ query_string: { fields: ['host.name'], query: 'dv*' } }],
+                        minimum_should_match: 1,
+                      },
+                    },
+                  },
+                },
+                {
+                  bool: {
+                    must_not: {
+                      bool: {
+                        should: [{ query_string: { fields: ['host.name'], query: 'ts*' } }],
+                        minimum_should_match: 1,
+                      },
+                    },
+                  },
+                },
+              ],
+            },
+          },
         ])
       );
     });

diff --git a/x-pack/plugins/observability/server/lib/rules/threshold/lib/metric_query.ts b/x-pack/plugins/observability/server/lib/rules/threshold/lib/metric_query.ts
@@ -5,6 +5,7 @@
  * 2.0.
  */
 
+import { fromKueryExpression, toElasticsearchQuery } from '@kbn/es-query';
 import moment from 'moment';
 import { Aggregators, MetricExpressionParams } from '../../../../../common/threshold_rule/types';
 import { isCustom, isNotCountOrCustom } from './metric_expression_params';
@@ -25,7 +26,13 @@ const getParsedFilterQuery: (filterQuery: string | undefined) => Array<Record<st
   filterQuery
 ) => {
   if (!filterQuery) return [];
-  return [JSON.parse(filterQuery)];
+
+  try {
+    const parsedQuery = toElasticsearchQuery(fromKueryExpression(filterQuery));
+    return [parsedQuery];
+  } catch (error) {
+    return [];
+  }
 };
 
 export const calculateCurrentTimeframe = (

diff --git a/x-pack/plugins/observability/server/lib/rules/threshold/messages.ts b/x-pack/plugins/observability/server/lib/rules/threshold/messages.ts
@@ -133,14 +133,6 @@ export const buildErrorAlertReason = (metric: string) =>
     },
   });
 
-export const buildInvalidQueryAlertReason = (filterQueryText: string) =>
-  i18n.translate('xpack.observability.threshold.rule.threshold.queryErrorAlertReason', {
-    defaultMessage: 'Alert is using a malformed KQL query: {filterQueryText}',
-    values: {
-      filterQueryText,
-    },
-  });
-
 export const groupByKeysActionVariableDescription = i18n.translate(
   'xpack.observability.threshold.rule.groupByKeysActionVariableDescription',
   {