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

[Alerting] Skip writing alerts when rule execution times out #114518

Merged
merged 25 commits into from
Dec 3, 2021
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
793b2ac
Added cancel() to alerting task runner and writing event log document
ymao1 Oct 6, 2021
5bccd7c
Updating rule saved object with timeout execution status
ymao1 Oct 6, 2021
b7d4590
Skip scheduling actions and logging event log for alerts if rule exec…
ymao1 Oct 6, 2021
56e960b
Adding config for disabling skipping actions
ymao1 Oct 6, 2021
226be69
Merge branch 'master' of https://github.com/elastic/kibana into alert…
ymao1 Oct 7, 2021
c2a8602
Fixing types
ymao1 Oct 7, 2021
e376c56
Adding flag for rule types to opt out of skipping acitons
ymao1 Oct 8, 2021
1ed70c5
Merge branch 'master' of https://github.com/elastic/kibana into alert…
ymao1 Oct 8, 2021
658ec46
Merge branch 'master' into alerting/cancel-alerting-tasks
kibanamachine Oct 11, 2021
7fac6a2
Passing function into rule executor to determine whether to write AAD
ymao1 Oct 11, 2021
36a9558
Merge branch 'master' of https://github.com/elastic/kibana into alert…
ymao1 Oct 12, 2021
44db452
Using task runner uuid to differentiate between task instances
ymao1 Oct 12, 2021
11a9cde
Merge branch 'master' of https://github.com/elastic/kibana into alert…
ymao1 Oct 12, 2021
7c6db75
Adding functional test
ymao1 Oct 12, 2021
cff64f1
Merge branch 'master' of https://github.com/elastic/kibana into alert…
ymao1 Oct 12, 2021
90ab82e
Merge branch 'alerting/cancel-alerting-tasks' into alerting/skip-writ…
ymao1 Oct 12, 2021
473c4d6
Merging in main
ymao1 Nov 16, 2021
8f46123
Fixing types
ymao1 Nov 23, 2021
5b60af9
Merge branch 'main' of https://github.com/elastic/kibana into alertin…
ymao1 Nov 23, 2021
ac2e94f
Merge branch 'main' of https://github.com/elastic/kibana into alertin…
ymao1 Nov 29, 2021
f514450
Updating service function name and adding logic to persistence rule type
ymao1 Nov 30, 2021
05bd054
Merge branch 'main' of https://github.com/elastic/kibana into alertin…
ymao1 Nov 30, 2021
522a940
Merge branch 'main' into alerting/skip-writing-aad-on-timeout-2
kibanamachine Nov 30, 2021
80c5edd
Merge branch 'main' into alerting/skip-writing-aad-on-timeout-2
kibanamachine Dec 3, 2021
98ddea7
Merge branch 'main' into alerting/skip-writing-aad-on-timeout-2
kibanamachine Dec 3, 2021
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
1 change: 1 addition & 0 deletions x-pack/plugins/alerting/server/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ const createAlertServicesMock = <
.mockReturnValue(alertInstanceFactoryMock),
savedObjectsClient: savedObjectsClientMock.create(),
scopedClusterClient: elasticsearchServiceMock.createScopedClusterClient(),
shouldWriteAlerts: () => true,
};
};
export type AlertServicesMock = ReturnType<typeof createAlertServicesMock>;
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/alerting/server/task_runner/task_runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,7 @@ export class TaskRunner<
InstanceContext,
WithoutReservedActionGroups<ActionGroupIds, RecoveryActionGroupId>
>(alertInstances),
shouldWriteAlerts: () => this.shouldLogAndScheduleActionsForAlerts(),
},
params,
state: alertTypeState as State,
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/alerting/server/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ export interface AlertServices<
alertInstanceFactory: (
id: string
) => PublicAlertInstance<InstanceState, InstanceContext, ActionGroupIds>;
shouldWriteAlerts: () => boolean;
}

export interface AlertExecutorOptions<
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ export const createRuleTypeMocks = () => {
alertInstanceFactory: jest.fn(() => ({ scheduleActions })),
alertWithLifecycle: jest.fn(),
logger: loggerMock,
shouldWriteAlerts: () => true,
};

return {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,33 @@ describe('createLifecycleExecutor', () => {
})
);
});

it('does not write alert documents when rule execution is cancelled and feature flags indicate to skip', async () => {
const logger = loggerMock.create();
const ruleDataClientMock = createRuleDataClientMock();
const executor = createLifecycleExecutor(
logger,
ruleDataClientMock
)<{}, TestRuleState, never, never, never>(async (options) => {
expect(options.state).toEqual(initialRuleState);

const nextRuleState: TestRuleState = {
aRuleStateKey: 'NEXT_RULE_STATE_VALUE',
};

return nextRuleState;
});

await executor(
createDefaultAlertExecutorOptions({
params: {},
state: { wrapped: initialRuleState, trackedAlerts: {} },
shouldWriteAlerts: false,
})
);

expect(ruleDataClientMock.getWriter).not.toHaveBeenCalled();
});
});

type TestRuleState = Record<string, unknown> & {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ export const createLifecycleExecutor =
>
): Promise<WrappedLifecycleRuleState<State>> => {
const {
services: { alertInstanceFactory },
services: { alertInstanceFactory, shouldWriteAlerts },
state: previousState,
} = options;

Expand Down Expand Up @@ -281,7 +281,15 @@ export const createLifecycleExecutor =
const newEventsToIndex = makeEventsDataMapFor(newAlertIds);
const allEventsToIndex = [...trackedEventsToIndex, ...newEventsToIndex];

if (allEventsToIndex.length > 0 && ruleDataClient.isWriteEnabled()) {
// Only write alerts if:
// - writing is enabled
// AND
// - rule execution has not been cancelled due to timeout
// OR
// - if execution has been cancelled due to timeout, if feature flags are configured to write alerts anyway
const writeAlerts = ruleDataClient.isWriteEnabled() && shouldWriteAlerts();

if (allEventsToIndex.length > 0 && writeAlerts) {
logger.debug(`Preparing to index ${allEventsToIndex.length} alerts.`);

await ruleDataClient.getWriter().bulk({
Expand All @@ -307,6 +315,6 @@ export const createLifecycleExecutor =

return {
wrapped: nextWrappedState ?? ({} as State),
trackedAlerts: ruleDataClient.isWriteEnabled() ? nextTrackedAlerts : {},
trackedAlerts: writeAlerts ? nextTrackedAlerts : {},
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import { createLifecycleRuleTypeFactory } from './create_lifecycle_rule_type_fac

type RuleTestHelpers = ReturnType<typeof createRule>;

function createRule() {
function createRule(shouldWriteAlerts: boolean = true) {
const ruleDataClientMock = createRuleDataClientMock();

const factory = createLifecycleRuleTypeFactory({
Expand Down Expand Up @@ -110,6 +110,7 @@ function createRule() {
alertInstanceFactory,
savedObjectsClient: {} as any,
scopedClusterClient: {} as any,
shouldWriteAlerts: () => shouldWriteAlerts,
},
spaceId: 'spaceId',
state,
Expand Down Expand Up @@ -152,6 +153,26 @@ describe('createLifecycleRuleTypeFactory', () => {
});
});

describe('when rule is cancelled due to timeout and config flags indicate to skip actions', () => {
beforeEach(() => {
helpers = createRule(false);
helpers.ruleDataClientMock.isWriteEnabled.mockReturnValue(true);
});

it("doesn't persist anything", async () => {
await helpers.alertWithLifecycle([
{
id: 'opbeans-java',
fields: {
'service.name': 'opbeans-java',
},
},
]);

expect(helpers.ruleDataClientMock.getWriter().bulk).toHaveBeenCalledTimes(0);
});
});

describe('when alerts are new', () => {
beforeEach(async () => {
await helpers.alertWithLifecycle([
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,16 @@ export const createPersistenceRuleTypeWrapper: CreatePersistenceRuleTypeWrapper
const numAlerts = alerts.length;
logger.debug(`Found ${numAlerts} alerts.`);

if (ruleDataClient.isWriteEnabled() && numAlerts) {
// Only write alerts if:
// - writing is enabled
// AND
// - rule execution has not been cancelled due to timeout
// OR
// - if execution has been cancelled due to timeout, if feature flags are configured to write alerts anyway
const writeAlerts =
ruleDataClient.isWriteEnabled() && options.services.shouldWriteAlerts();

if (writeAlerts && numAlerts) {
const commonRuleFields = getCommonAlertFields(options);

const CHUNK_SIZE = 10000;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export const createDefaultAlertExecutorOptions = <
createdAt = new Date(),
startedAt = new Date(),
updatedAt = new Date(),
shouldWriteAlerts = true,
}: {
alertId?: string;
ruleName?: string;
Expand All @@ -39,6 +40,7 @@ export const createDefaultAlertExecutorOptions = <
createdAt?: Date;
startedAt?: Date;
updatedAt?: Date;
shouldWriteAlerts?: boolean;
}): AlertExecutorOptions<Params, State, InstanceState, InstanceContext, ActionGroupIds> => ({
alertId,
createdBy: 'CREATED_BY',
Expand Down Expand Up @@ -69,6 +71,7 @@ export const createDefaultAlertExecutorOptions = <
.alertInstanceFactory,
savedObjectsClient: savedObjectsClientMock.create(),
scopedClusterClient: elasticsearchServiceMock.createScopedClusterClient(),
shouldWriteAlerts: () => shouldWriteAlerts,
},
state,
updatedBy: null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ export const previewRulesRoute = async (
ruleTypeId: string,
ruleTypeName: string,
params: TParams,
shouldWriteAlerts: () => boolean,
alertInstanceFactory: (
id: string
) => Pick<
Expand Down Expand Up @@ -157,6 +158,7 @@ export const previewRulesRoute = async (
previousStartedAt,
rule,
services: {
shouldWriteAlerts,
alertInstanceFactory,
savedObjectsClient: context.core.savedObjects.client,
scopedClusterClient: context.core.elasticsearch.client,
Expand Down Expand Up @@ -191,6 +193,7 @@ export const previewRulesRoute = async (
signalRuleAlertType.id,
signalRuleAlertType.name,
previewRuleParams,
() => true,
alertInstanceFactoryStub
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ export const createRuleTypeMocks = (
findAlerts: jest.fn(), // TODO: does this stay?
alertWithPersistence: jest.fn(),
logger: loggerMock,
shouldWriteAlerts: () => true,
};

return {
Expand Down