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

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

Merged
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,6 @@ export interface MetricAnomalyParams {
export interface ThresholdParams {
criteria: MetricExpressionParams[];
filterQuery?: string;
filterQueryText?: string;
sourceId?: string;
alertOnNoData?: boolean;
alertOnGroupDisappear?: boolean;
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -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[];
}
>;
Expand Down Expand Up @@ -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}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { MetricExpression } from '../../types';
import { validateMetricThreshold } from '../validation';

export default {
title: 'infra/alerting/CustomEquationEditor',
title: 'app/Alerts/CustomEquationEditor',
Copy link
Member Author

Choose a reason for hiding this comment

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

Storybook is not working properly, created a separate ticket for fixing it (#162720)

decorators: [
(wrappedStory) => <div style={{ width: 550 }}>{wrappedStory()}</div>,
decorateWithGlobalStorybookThemeProviders,
Expand Down Expand Up @@ -69,6 +69,7 @@ const CustomEquationEditorTemplate: Story<CustomEquationEditorProps> = (args) =>
useEffect(() => {
const validationObject = validateMetricThreshold({
criteria: [expression as MetricExpressionParams],
searchConfiguration: {},
});
setErrors(validationObject.errors[0]);
}, [expression]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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: {
Expand All @@ -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;
}
Expand All @@ -84,7 +100,6 @@ export function validateMetricThreshold({
threshold1: [],
},
metric: [],
filterQuery: [],
customMetrics: {},
};
if (!c.aggType) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ describe('Expression', () => {
const ruleParams = {
criteria: [],
groupBy: undefined,
filterQueryText: '',
filterQuery: '',
sourceId: 'default',
searchConfiguration: {},
};
Expand Down Expand Up @@ -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',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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 */
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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}
/>
Expand Down Expand Up @@ -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>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -28,7 +29,6 @@ import { MetricsExplorerSeries } from '../../../common/threshold_rule/metrics_ex
import {
Comparator,
CustomMetricExpressionParams,
FilterQuery,
MetricExpressionParams,
MetricsSourceStatus,
NonCountMetricExpressionParams,
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ export interface EvaluatedRuleParams {
criteria: MetricExpressionParams[];
groupBy: string | undefined | string[];
filterQuery?: string;
filterQueryText?: string;
}

export type Evaluation = Omit<MetricExpressionParams, 'metric'> & {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
},
},
},
},
],
},
},
])
);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -25,7 +26,13 @@ const getParsedFilterQuery: (filterQuery: string | undefined) => Array<Record<st
filterQuery
) => {
if (!filterQuery) return [];
return [JSON.parse(filterQuery)];

try {
Copy link
Contributor

Choose a reason for hiding this comment

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

@maryam-saeidi, this is where compile the filterQuery on the server side instead of convertKueryToElasticSearchQuery in the frontend, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right

const parsedQuery = toElasticsearchQuery(fromKueryExpression(filterQuery));
return [parsedQuery];
} catch (error) {
return [];
}
};

export const calculateCurrentTimeframe = (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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',
{
Expand Down
Loading