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

* [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>

x-pack/plugins/infra/common/alerting/metrics/types.ts

@@ -75,6 +75,7 @@ export interface InventoryMetricConditions {
 export interface InventoryMetricThresholdParams {
   criteria: InventoryMetricConditions[];
   filterQuery?: string;
+  filterQueryText?: string;
   nodeType: InventoryItemType;
   sourceId?: string;
   alertOnNoData?: boolean;

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

@@ -70,6 +70,7 @@ import { useKibanaContextForPlugin } from '../../../hooks/use_kibana';
 
 import { ExpressionChart } from './expression_chart';
 const FILTER_TYPING_DEBOUNCE_MS = 500;
+export const QUERY_INVALID = Symbol('QUERY_INVALID');
 
 export interface AlertContextMeta {
   options?: Partial<InfraWaffleMapOptions>;
@@ -84,7 +85,7 @@ type Props = Omit<
     {
       criteria: Criteria;
       nodeType: InventoryItemType;
-      filterQuery?: string;
+      filterQuery?: string | symbol;
       filterQueryText?: string;
       sourceId: string;
       alertOnNoData?: boolean;
@@ -157,10 +158,14 @@ export const Expressions: React.FC<Props> = (props) => {
   const onFilterChange = useCallback(
     (filter: any) => {
       setAlertParams('filterQueryText', filter || '');
-      setAlertParams(
-        'filterQuery',
-        convertKueryToElasticSearchQuery(filter, derivedIndexPattern) || ''
-      );
+      try {
+        setAlertParams(
+          'filterQuery',
+          convertKueryToElasticSearchQuery(filter, derivedIndexPattern, false) || ''
+        );
+      } catch (e) {
+        setAlertParams('filterQuery', QUERY_INVALID);
+      }
     },
     [derivedIndexPattern, setAlertParams]
   );

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

@@ -36,7 +36,7 @@ import { useWaffleOptionsContext } from '../../../pages/metrics/inventory_view/h
 
 interface Props {
   expression: InventoryMetricConditions;
-  filterQuery?: string;
+  filterQuery?: string | symbol;
   nodeType: InventoryItemType;
   sourceId: string;
 }

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

@@ -13,11 +13,14 @@ import {
 } from '../../../../server/lib/alerting/inventory_metric_threshold/types';
 // eslint-disable-next-line @kbn/eslint/no-restricted-paths
 import { ValidationResult } from '../../../../../triggers_actions_ui/public/types';
+import { QUERY_INVALID } from './expression';
 
 export function validateMetricThreshold({
   criteria,
+  filterQuery,
 }: {
   criteria: InventoryMetricConditions[];
+  filterQuery?: string | symbol;
 }): ValidationResult {
   const validationResult = { errors: {} };
   const errors: {
@@ -34,9 +37,17 @@ export function validateMetricThreshold({
       };
       metric: string[];
     };
-  } = {};
+  } & { filterQuery?: string[] } = {};
   validationResult.errors = errors;
 
+  if (filterQuery === QUERY_INVALID) {
+    errors.filterQuery = [
+      i18n.translate('xpack.infra.metrics.alertFlyout.error.invalidFilterQuery', {
+        defaultMessage: 'Filter query is invalid.',
+      }),
+    ];
+  }
+
   if (!criteria || !criteria.length) {
     return validationResult;
   }

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

@@ -42,6 +42,7 @@ import { ExpressionChart } from './expression_chart';
 import { useKibanaContextForPlugin } from '../../../hooks/use_kibana';
 
 const FILTER_TYPING_DEBOUNCE_MS = 500;
+export const QUERY_INVALID = Symbol('QUERY_INVALID');
 
 type Props = Omit<
   AlertTypeParamsExpressionProps<AlertTypeParams & AlertParams, AlertContextMeta>,
@@ -117,10 +118,14 @@ export const Expressions: React.FC<Props> = (props) => {
   const onFilterChange = useCallback(
     (filter: any) => {
       setAlertParams('filterQueryText', filter);
-      setAlertParams(
-        'filterQuery',
-        convertKueryToElasticSearchQuery(filter, derivedIndexPattern) || ''
-      );
+      try {
+        setAlertParams(
+          'filterQuery',
+          convertKueryToElasticSearchQuery(filter, derivedIndexPattern, false) || ''
+        );
+      } catch (e) {
+        setAlertParams('filterQuery', QUERY_INVALID);
+      }
     },
     [setAlertParams, derivedIndexPattern]
   );
@@ -281,15 +286,16 @@ export const Expressions: React.FC<Props> = (props) => {
   }, [alertParams.groupBy]);
 
   const redundantFilterGroupBy = useMemo(() => {
-    if (!alertParams.filterQuery || !groupByFilterTestPatterns) return [];
+    const { filterQuery } = alertParams;
+    if (typeof filterQuery !== 'string' || !groupByFilterTestPatterns) return [];
     return groupByFilterTestPatterns
       .map(({ groupName, pattern }) => {
-        if (pattern.test(alertParams.filterQuery!)) {
+        if (pattern.test(filterQuery)) {
           return groupName;
         }
       })
       .filter((g) => typeof g === 'string') as string[];
-  }, [alertParams.filterQuery, groupByFilterTestPatterns]);
+  }, [alertParams, groupByFilterTestPatterns]);
 
   return (
     <>

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

@@ -13,11 +13,14 @@ import {
 } from '../../../../server/lib/alerting/metric_threshold/types';
 // eslint-disable-next-line @kbn/eslint/no-restricted-paths
 import { ValidationResult } from '../../../../../triggers_actions_ui/public/types';
+import { QUERY_INVALID } from './expression';
 
 export function validateMetricThreshold({
   criteria,
+  filterQuery,
 }: {
   criteria: MetricExpressionParams[];
+  filterQuery?: string | symbol;
 }): ValidationResult {
   const validationResult = { errors: {} };
   const errors: {
@@ -35,9 +38,17 @@ export function validateMetricThreshold({
       };
       metric: string[];
     };
-  } = {};
+  } & { filterQuery?: string[] } = {};
   validationResult.errors = errors;
 
+  if (filterQuery === QUERY_INVALID) {
+    errors.filterQuery = [
+      i18n.translate('xpack.infra.metrics.alertFlyout.error.invalidFilterQuery', {
+        defaultMessage: 'Filter query is invalid.',
+      }),
+    ];
+  }
+
   if (!criteria || !criteria.length) {
     return validationResult;
   }
@@ -59,6 +70,7 @@ export function validateMetricThreshold({
         threshold1: [],
       },
       metric: [],
+      filterQuery: [],
     };
     if (!c.aggType) {
       errors[id].aggField.push(

x-pack/plugins/infra/public/alerting/metric_threshold/types.ts

@@ -57,7 +57,7 @@ export interface ExpressionChartData {
 export interface AlertParams {
   criteria: MetricExpression[];
   groupBy?: string | string[];
-  filterQuery?: string;
+  filterQuery?: string | symbol;
   sourceId: string;
   filterQueryText?: string;
   alertOnNoData?: boolean;

x-pack/plugins/infra/public/pages/metrics/inventory_view/hooks/use_snaphot.ts

@@ -24,7 +24,7 @@ import {
 } from '../../../../../common/inventory_models/types';
 
 export function useSnapshot(
-  filterQuery: string | null | undefined,
+  filterQuery: string | null | symbol | undefined,
   metrics: Array<{ type: SnapshotMetricType }>,
   groupBy: SnapshotGroupBy,
   nodeType: InventoryItemType,

x-pack/plugins/infra/public/plugin.ts

@@ -10,9 +10,6 @@ import { AppMountParameters, PluginInitializerContext } from 'kibana/public';
 import { from } from 'rxjs';
 import { map } from 'rxjs/operators';
 import { DEFAULT_APP_CATEGORIES } from '../../../../src/core/public';
-import { createInventoryMetricAlertType } from './alerting/inventory';
-import { createLogThresholdAlertType } from './alerting/log_threshold';
-import { createMetricThresholdAlertType } from './alerting/metric_threshold';
 import { LOG_STREAM_EMBEDDABLE } from './components/log_stream/log_stream_embeddable';
 import { LogStreamEmbeddableFactoryDefinition } from './components/log_stream/log_stream_embeddable_factory';
 import { createMetricsFetchData, createMetricsHasData } from './metrics_overview_fetchers';
@@ -29,11 +26,15 @@ import { getLogsHasDataFetcher, getLogsOverviewDataFetcher } from './utils/logs_
 export class Plugin implements InfraClientPluginClass {
   constructor(_context: PluginInitializerContext) {}
 
-  setup(core: InfraClientCoreSetup, pluginsSetup: InfraClientSetupDeps) {
+  async setup(core: InfraClientCoreSetup, pluginsSetup: InfraClientSetupDeps) {
     if (pluginsSetup.home) {
       registerFeatures(pluginsSetup.home);
     }
 
+    const { createInventoryMetricAlertType } = await import('./alerting/inventory');
+    const { createLogThresholdAlertType } = await import('./alerting/log_threshold');
+    const { createMetricThresholdAlertType } = await import('./alerting/metric_threshold');
+
     pluginsSetup.observability.observabilityRuleTypeRegistry.register(
       createInventoryMetricAlertType()
     );

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

@@ -10,7 +10,8 @@ import { esKuery } from '../../../../../src/plugins/data/public';
 
 export const convertKueryToElasticSearchQuery = (
   kueryExpression: string,
-  indexPattern: DataViewBase
+  indexPattern: DataViewBase,
+  swallowErrors: boolean = true
 ) => {
   try {
     return kueryExpression
@@ -19,6 +20,8 @@ export const convertKueryToElasticSearchQuery = (
         )
       : '';
   } catch (err) {
-    return '';
+    if (swallowErrors) {
+      return '';
+    } else throw err;
   }
 };

x-pack/plugins/infra/server/lib/alerting/common/messages.ts

@@ -173,6 +173,14 @@ export const buildErrorAlertReason = (metric: string) =>
     },
   });
 
+export const buildInvalidQueryAlertReason = (filterQueryText: string) =>
+  i18n.translate('xpack.infra.metrics.alerting.threshold.queryErrorAlertReason', {
+    defaultMessage: 'Alert is using a malformed KQL query: {filterQueryText}',
+    values: {
+      filterQueryText,
+    },
+  });
+
 export const groupActionVariableDescription = i18n.translate(
   'xpack.infra.metrics.alerting.groupActionVariableDescription',
   {

x-pack/plugins/infra/server/lib/alerting/inventory_metric_threshold/inventory_metric_threshold_executor.ts

@@ -31,6 +31,7 @@ import {
   buildNoDataAlertReason,
   // buildRecoveredAlertReason,
   stateToAlertMessage,
+  buildInvalidQueryAlertReason,
 } from '../common/messages';
 import { evaluateCondition } from './evaluate_condition';
 
@@ -74,6 +75,25 @@ export const createInventoryMetricThresholdExecutor = (libs: InfraBackendLibs) =
         },
       });
 
+    if (!params.filterQuery && params.filterQueryText) {
+      try {
+        const { fromKueryExpression } = await import('@kbn/es-query');
+        fromKueryExpression(params.filterQueryText);
+      } catch (e) {
+        const actionGroupId = FIRED_ACTIONS.id; // Change this to an Error action group when able
+        const reason = buildInvalidQueryAlertReason(params.filterQueryText);
+        const alertInstance = alertInstanceFactory('*', reason);
+        alertInstance.scheduleActions(actionGroupId, {
+          group: '*',
+          alertState: stateToAlertMessage[AlertStates.ERROR],
+          reason,
+          timestamp: moment().toISOString(),
+          value: null,
+          metric: mapToConditionsLookup(criteria, (c) => c.metric),
+        });
+        return {};
+      }
+    }
     const source = await libs.sources.getSourceConfiguration(
       savedObjectsClient,
       sourceId || 'default'

x-pack/plugins/infra/server/lib/alerting/metric_threshold/lib/evaluate_alert.ts

@@ -56,7 +56,8 @@ interface CompositeAggregationsResponse {
 export interface EvaluatedAlertParams {
   criteria: MetricExpressionParams[];
   groupBy: string | undefined | string[];
-  filterQuery: string | undefined;
+  filterQuery?: string;
+  filterQueryText?: string;
   shouldDropPartialBuckets?: boolean;
 }
 
@@ -68,6 +69,7 @@ export const evaluateAlert = <Params extends EvaluatedAlertParams = EvaluatedAle
   timeframe?: { start?: number; end: number }
 ) => {
   const { criteria, groupBy, filterQuery, shouldDropPartialBuckets } = params;
+
   return Promise.all(
     criteria.map(async (criterion) => {
       const currentValues = await getMetric(

x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.test.ts

@@ -253,16 +253,22 @@ describe('The metric threshold alert type', () => {
               metric: metric ?? baseNonCountCriterion.metric,
             },
           ],
+          filterQuery,
         },
         state: state ?? mockOptions.state.wrapped,
       });
     test('persists previous groups that go missing, until the filterQuery param changes', async () => {
-      const stateResult1 = await executeWithFilter(Comparator.GT, [0.75], 'query', 'test.metric.2');
+      const stateResult1 = await executeWithFilter(
+        Comparator.GT,
+        [0.75],
+        JSON.stringify({ query: 'q' }),
+        'test.metric.2'
+      );
       expect(stateResult1.groups).toEqual(expect.arrayContaining(['a', 'b', 'c']));
       const stateResult2 = await executeWithFilter(
         Comparator.GT,
         [0.75],
-        'query',
+        JSON.stringify({ query: 'q' }),
         'test.metric.1',
         stateResult1
       );
@@ -270,7 +276,7 @@ describe('The metric threshold alert type', () => {
       const stateResult3 = await executeWithFilter(
         Comparator.GT,
         [0.75],
-        'different query',
+        JSON.stringify({ query: 'different' }),
         'test.metric.1',
         stateResult2
       );
@@ -710,6 +716,31 @@ describe('The metric threshold alert type', () => {
       expect(action.value.condition0).toBe('100%');
     });
   });
+
+  describe('attempting to use a malformed filterQuery', () => {
+    afterAll(() => clearInstances());
+    const instanceID = '*';
+    const execute = () =>
+      executor({
+        ...mockOptions,
+        services,
+        params: {
+          criteria: [
+            {
+              ...baseNonCountCriterion,
+            },
+          ],
+          sourceId: 'default',
+          filterQuery: '',
+          filterQueryText:
+            'host.name:(look.there.is.no.space.after.these.parentheses)and uh.oh: "wow that is bad"',
+        },
+      });
+    test('reports an error', async () => {
+      await execute();
+      expect(mostRecentAction(instanceID)).toBeErrorAction();
+    });
+  });
 });
 
 const createMockStaticConfiguration = (sources: any) => ({
@@ -847,6 +878,14 @@ expect.extend({
       pass,
     };
   },
+  toBeErrorAction(action?: Action) {
+    const pass = action?.id === FIRED_ACTIONS.id && action?.action.alertState === 'ERROR';
+    const message = () => `expected ${action} to be an ERROR action`;
+    return {
+      message,
+      pass,
+    };
+  },
 });
 
 declare global {
@@ -855,6 +894,7 @@ declare global {
     interface Matchers<R> {
       toBeAlertAction(action?: Action): R;
       toBeNoDataAction(action?: Action): R;
+      toBeErrorAction(action?: Action): R;
     }
   }
 }

x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.ts

@@ -23,6 +23,7 @@ import {
   buildNoDataAlertReason,
   // buildRecoveredAlertReason,
   stateToAlertMessage,
+  buildInvalidQueryAlertReason,
 } from '../common/messages';
 import { UNGROUPED_FACTORY_KEY } from '../common/utils';
 import { createFormatter } from '../../../../common/formatters';
@@ -85,6 +86,27 @@ export const createMetricThresholdExecutor = (libs: InfraBackendLibs) =>
       alertOnGroupDisappear: boolean | undefined;
     };
 
+    if (!params.filterQuery && params.filterQueryText) {
+      try {
+        const { fromKueryExpression } = await import('@kbn/es-query');
+        fromKueryExpression(params.filterQueryText);
+      } catch (e) {
+        const timestamp = moment().toISOString();
+        const actionGroupId = FIRED_ACTIONS.id; // Change this to an Error action group when able
+        const reason = buildInvalidQueryAlertReason(params.filterQueryText);
+        const alertInstance = alertInstanceFactory(UNGROUPED_FACTORY_KEY, reason);
+        alertInstance.scheduleActions(actionGroupId, {
+          group: UNGROUPED_FACTORY_KEY,
+          alertState: stateToAlertMessage[AlertStates.ERROR],
+          reason,
+          timestamp,
+          value: null,
+          metric: mapToConditionsLookup(criteria, (c) => c.metric),
+        });
+        return { groups: [], groupBy: params.groupBy, filterQuery: params.filterQuery };
+      }
+    }
+
     // For backwards-compatibility, interpret undefined alertOnGroupDisappear as true
     const alertOnGroupDisappear = _alertOnGroupDisappear !== false;