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

[Discover] Improve warn feedback when data view has changed after rule creation #144908

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { useEffect, useMemo } from 'react';
import { useHistory, useLocation, useParams } from 'react-router-dom';
import { sha256 } from 'js-sha256';
import type { Rule } from '@kbn/alerting-plugin/common';
import { getTime } from '@kbn/data-plugin/common';
import { type DataViewSpec, getTime } from '@kbn/data-plugin/common';
import type { DataView } from '@kbn/data-views-plugin/public';
import type { Filter } from '@kbn/es-query';
import { DiscoverAppLocatorParams } from '../../locator';
Expand All @@ -19,11 +19,34 @@ import { getAlertUtils, QueryParams, SearchThresholdAlertParams } from './view_a

type NonNullableEntry<T> = { [K in keyof T]: NonNullable<T[keyof T]> };

const getCurrentChecksum = (params: SearchThresholdAlertParams) =>
sha256.create().update(JSON.stringify(params)).hex();
const getDataViewParamsChecksum = (dataViewSpec: DataViewSpec) => {
const { title, timeFieldName, sourceFilters, runtimeFieldMap } = dataViewSpec;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think changing the source filters changes the output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually we can filter out some field, then it will be unavailable on Discover. It first was found here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but it won't alter the output of the alert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but here we are deciding whether to show Data View has changed toast message. When he/she opens the alert results and data view has changed in the meantime, we detect it by comparing checksum.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dimaanj

When he/she opens the alert results and data view has changed in the meantime, we detect it by comparing checksum.

Yes, I understand that, but maybe there's something else I'm missing. Can you describe the steps a user might take where a change to source filters deserves this toast?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I've updated description

const orderedParams = Object.values({ title, timeFieldName, sourceFilters, runtimeFieldMap });
return sha256.create().update(JSON.stringify(orderedParams)).hex();
};

/**
* Get rule params checksum skipping serialized data view object
*/
const getRuleParamsChecksum = (params: SearchThresholdAlertParams) => {
const orderedParams = Object.values(params);
return sha256
.create()
.update(
JSON.stringify(orderedParams, (key: string, value: string) =>
key === 'index' ? undefined : value
)
)
.hex();
};

const isActualAlert = (queryParams: QueryParams): queryParams is NonNullableEntry<QueryParams> => {
return Boolean(queryParams.from && queryParams.to && queryParams.checksum);
return Boolean(
queryParams.from &&
queryParams.to &&
queryParams.ruleParamsChecksum &&
queryParams.dataViewChecksum
);
};

const buildTimeRangeFilter = (
Expand Down Expand Up @@ -55,7 +78,8 @@ export function ViewAlertRoute() {
() => ({
from: query.get('from'),
to: query.get('to'),
checksum: query.get('checksum'),
ruleParamsChecksum: query.get('ruleParamsChecksum'),
dataViewChecksum: query.get('dataViewChecksum'),
}),
[query]
);
Expand Down Expand Up @@ -92,30 +116,21 @@ export function ViewAlertRoute() {
return;
}

if (dataView.isPersisted()) {
const dataViewSavedObject = await core.savedObjects.client.get(
'index-pattern',
dataView.id!
);

const alertUpdatedAt = fetchedAlert.updatedAt;
const dataViewUpdatedAt = dataViewSavedObject.updatedAt!;
// data view updated after the last update of the alert rule
if (
openActualAlert &&
new Date(dataViewUpdatedAt).valueOf() > new Date(alertUpdatedAt).valueOf()
) {
if (openActualAlert) {
const currentDataViewChecksum = getDataViewParamsChecksum(dataView.toSpec(false));
if (currentDataViewChecksum !== queryParams.dataViewChecksum) {
// data view params which might affect the displayed results were changed
showDataViewUpdatedWarning();
}
}

const calculatedChecksum = getCurrentChecksum(fetchedAlert.params);
// rule params changed
if (openActualAlert && calculatedChecksum !== queryParams.checksum) {
displayRuleChangedWarn();
} else if (openActualAlert && calculatedChecksum === queryParams.checksum) {
// documents might be updated or deleted
displayPossibleDocsDiffInfoAlert();
const currentRuleParamsChecksum = getRuleParamsChecksum(fetchedAlert.params);
if (currentRuleParamsChecksum !== queryParams.ruleParamsChecksum) {
// rule params changed
displayRuleChangedWarn();
} else {
// documents might be updated or deleted
displayPossibleDocsDiffInfoAlert();
}
}

const timeRange = openActualAlert
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ export interface SearchThresholdAlertParams extends RuleTypeParams {
export interface QueryParams {
from: string | null;
to: string | null;
checksum: string | null;
ruleParamsChecksum: string | null;
dataViewChecksum: string | null;
}

const LEGACY_BASE_ALERT_API_PATH = '/api/alerts';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/
import { sha256 } from 'js-sha256';

import { i18n } from '@kbn/i18n';
import { CoreSetup } from '@kbn/core/server';
import { parseDuration } from '@kbn/alerting-plugin/server';
Expand All @@ -30,7 +30,8 @@ export async function executor(core: CoreSetup, options: ExecutorOptions<EsQuery
} = options;
const { alertFactory, scopedClusterClient, searchSourceClient } = services;
const currentTimestamp = new Date().toISOString();
const publicBaseUrl = core.http.basePath.publicBaseUrl ?? '';
const base = core.http.basePath.publicBaseUrl ?? '';
const spacePrefix = spaceId !== 'default' ? `/s/${spaceId}` : '';

const alertLimit = alertFactory.alertLimit.getValue();

Expand All @@ -48,26 +49,34 @@ export async function executor(core: CoreSetup, options: ExecutorOptions<EsQuery
// of the rule, the latestTimestamp will be used to gate the query in order to
// avoid counting a document multiple times.

const { numMatches, searchResult, dateStart, dateEnd } = esQueryRule
? await fetchEsQuery(ruleId, name, params as OnlyEsQueryRuleParams, latestTimestamp, {
scopedClusterClient,
logger,
})
: await fetchSearchSourceQuery(ruleId, params as OnlySearchSourceRuleParams, latestTimestamp, {
searchSourceClient,
logger,
});
const { numMatches, searchResult, dateStart, dateEnd, link } = esQueryRule
? await fetchEsQuery(
ruleId,
name,
params as OnlyEsQueryRuleParams,
latestTimestamp,
base,
spacePrefix,
{
scopedClusterClient,
logger,
}
)
: await fetchSearchSourceQuery(
ruleId,
params as OnlySearchSourceRuleParams,
latestTimestamp,
base,
spacePrefix,
{
searchSourceClient,
logger,
}
);

// apply the rule condition
const conditionMet = compareFn(numMatches, params.threshold);

const base = publicBaseUrl;
const spacePrefix = spaceId !== 'default' ? `/s/${spaceId}` : '';
const link = esQueryRule
? `${base}${spacePrefix}/app/management/insightsAndAlerting/triggersActions/rule/${ruleId}`
: `${base}${spacePrefix}/app/discover#/viewAlert/${ruleId}?from=${dateStart}&to=${dateEnd}&checksum=${getChecksum(
params as OnlyEsQueryRuleParams
)}`;
const baseContext: Omit<EsQueryRuleActionContext, 'conditions'> = {
title: name,
date: currentTimestamp,
Expand Down Expand Up @@ -186,10 +195,6 @@ export function tryToParseAsDate(sortValue?: string | number | null): undefined
}
}

export function getChecksum(params: OnlyEsQueryRuleParams) {
return sha256.create().update(JSON.stringify(params));
}

export function getInvalidComparatorError(comparator: string) {
return i18n.translate('xpack.stackAlerts.esQuery.invalidComparatorErrorMessage', {
defaultMessage: 'invalid thresholdComparator specified: {comparator}',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ export async function fetchEsQuery(
name: string,
params: OnlyEsQueryRuleParams,
timestamp: string | undefined,
publicBaseUrl: string,
spacePrefix: string,
services: {
scopedClusterClient: IScopedClusterClient;
logger: Logger;
Expand Down Expand Up @@ -88,7 +90,10 @@ export async function fetchEsQuery(
logger.debug(
` es query rule ${ES_QUERY_ID}:${ruleId} "${name}" result - ${JSON.stringify(searchResult)}`
);
const link = `${publicBaseUrl}${spacePrefix}/app/management/insightsAndAlerting/triggersActions/rule/${ruleId}`;

return {
link,
numMatches: (searchResult.hits.total as estypes.SearchTotalHits).value,
searchResult,
dateStart,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ describe('fetchSearchSourceQuery', () => {

const { searchSource, dateStart, dateEnd } = updateSearchSource(
searchSourceInstance,
dataViewMock,
params,
undefined
);
Expand Down Expand Up @@ -96,6 +97,7 @@ describe('fetchSearchSourceQuery', () => {

const { searchSource } = updateSearchSource(
searchSourceInstance,
dataViewMock,
params,
'2020-02-09T23:12:41.941Z'
);
Expand Down Expand Up @@ -136,6 +138,7 @@ describe('fetchSearchSourceQuery', () => {

const { searchSource } = updateSearchSource(
searchSourceInstance,
dataViewMock,
params,
'2020-01-09T22:12:41.941Z'
);
Expand Down Expand Up @@ -169,6 +172,7 @@ describe('fetchSearchSourceQuery', () => {

const { searchSource } = updateSearchSource(
searchSourceInstance,
dataViewMock,
params,
'2020-02-09T23:12:41.941Z'
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,13 @@
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { sha256 } from 'js-sha256';
import { buildRangeFilter, Filter } from '@kbn/es-query';
import { Logger } from '@kbn/core/server';
import {
DataView,
DataViewSpec,
getTime,
ISearchSource,
ISearchStartSearchSource,
Expand All @@ -18,6 +22,8 @@ export async function fetchSearchSourceQuery(
ruleId: string,
params: OnlySearchSourceRuleParams,
latestTimestamp: string | undefined,
publicBaseUrl: string,
spacePrefix: string,
services: {
logger: Logger;
searchSourceClient: ISearchStartSearchSource;
Expand All @@ -27,8 +33,14 @@ export async function fetchSearchSourceQuery(

const initialSearchSource = await searchSourceClient.create(params.searchConfiguration);

const index = initialSearchSource.getField('index') as DataView;
if (!isTimeBasedDataView(index)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there another issue to prevent rules from being created with non time based data views? Typically we like to prevent situations where users can create rules that error with every execution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there is another issue for this case #135806

throw new Error('Invalid data view without timeFieldName.');
}

const { searchSource, dateStart, dateEnd } = updateSearchSource(
initialSearchSource,
index,
params,
latestTimestamp
);
Expand All @@ -41,7 +53,12 @@ export async function fetchSearchSourceQuery(

const searchResult = await searchSource.fetch();

const dataViewChecksum = getDataViewChecksum(index.toSpec(false));
const ruleParamsChecksum = getRuleParamsChecksum(params as OnlySearchSourceRuleParams);
const link = `${publicBaseUrl}${spacePrefix}/app/discover#/viewAlert/${ruleId}?from=${dateStart}&to=${dateEnd}&ruleParamsChecksum=${ruleParamsChecksum}&dataViewChecksum=${dataViewChecksum}`;

return {
link,
numMatches: Number(searchResult.hits.total),
searchResult,
dateStart,
Expand All @@ -51,16 +68,11 @@ export async function fetchSearchSourceQuery(

export function updateSearchSource(
searchSource: ISearchSource,
index: DataView,
params: OnlySearchSourceRuleParams,
latestTimestamp: string | undefined
latestTimestamp?: string
) {
const index = searchSource.getField('index');

const timeFieldName = index?.timeFieldName;
if (!timeFieldName) {
throw new Error('Invalid data view without timeFieldName.');
}

const timeFieldName = index.timeFieldName!;
searchSource.setField('size', params.size);

const timerangeFilter = getTime(index, {
Expand All @@ -84,9 +96,35 @@ export function updateSearchSource(
const searchSourceChild = searchSource.createChild();
searchSourceChild.setField('filter', filters as Filter[]);
searchSourceChild.setField('sort', [{ [timeFieldName]: SortDirection.desc }]);

return {
searchSource: searchSourceChild,
dateStart,
dateEnd,
};
}

function isTimeBasedDataView(index?: DataView) {
return index?.timeFieldName;
}

function getDataViewChecksum(index: DataViewSpec) {
const { title, timeFieldName, sourceFilters, runtimeFieldMap } = index;
const orderedParams = Object.values({ title, timeFieldName, sourceFilters, runtimeFieldMap });
return sha256.create().update(JSON.stringify(orderedParams)).hex();
}

/**
* Get rule params checksum skipping serialized data view object
*/
function getRuleParamsChecksum(params: OnlySearchSourceRuleParams) {
const orderedParams = Object.values(params);
return sha256
.create()
.update(
JSON.stringify(orderedParams, (key: string, value: string) =>
key === 'index' ? undefined : value
)
)
.hex();
}
Original file line number Diff line number Diff line change
Expand Up @@ -500,6 +500,11 @@ describe('ruleType', () => {
aggregatable: false,
},
],
toSpec: () => ({
id: 'test-id',
title: 'test-title',
timeFieldName: 'time-field',
}),
};
const defaultParams: OnlySearchSourceRuleParams = {
size: 100,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,24 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
expect(await dataGrid.getDocCount()).to.be(1);
});

it('should not notify about data view update when field popularity changed', async () => {
await PageObjects.common.sleep(8000);
await testSubjects.click('field-message-showDetails');
await testSubjects.click('discoverFieldListPanelEdit-message');
await testSubjects.click('toggleAdvancedSetting');

const popularityInput = await testSubjects.find('editorFieldCount');
await popularityInput.type('5');
await testSubjects.click('fieldSaveButton');
await PageObjects.header.waitUntilLoadingHasFinished();

await openAlertResults(RULE_NAME, sourceDataViewId);

await PageObjects.common.sleep(8000);

expect(await toasts.getToastCount()).to.be.equal(1);
});

it('should display warning about recently updated data view', async () => {
await PageObjects.common.navigateToUrlWithBrowserHistory(
'management',
Expand Down