Skip to content

Commit 7ca3a6f

Browse files
mikecotemikecote
committed
Increase alerting test stability and reduce flakiness (#50246)
* Increase alerting test stability * More changes to test methodology + bug fix in throttling * Fix comments * Cleanup * Typo * Fix broken tests * Fix integration tests * Fix typo
1 parent 63b5fbe commit 7ca3a6f

16 files changed

Lines changed: 226 additions & 95 deletions

File tree

‎x-pack/legacy/plugins/alerting/server/lib/alert_instance.test.ts‎

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,17 @@ afterAll(() => clock.restore());
1818
describe('hasScheduledActions()', () => {
1919
test('defaults to false', () => {
2020
const alertInstance = new AlertInstance();
21-
expect(alertInstance.hasScheduledActions(null)).toEqual(false);
21+
expect(alertInstance.hasScheduledActions()).toEqual(false);
2222
});
2323

24+
test('returns true when scheduleActions is called', () => {
25+
const alertInstance = new AlertInstance();
26+
alertInstance.scheduleActions('default');
27+
expect(alertInstance.hasScheduledActions()).toEqual(true);
28+
});
29+
});
30+
31+
describe('isThrottled', () => {
2432
test(`should throttle when group didn't change and throttle period is still active`, () => {
2533
const alertInstance = new AlertInstance({
2634
meta: {
@@ -32,7 +40,7 @@ describe('hasScheduledActions()', () => {
3240
});
3341
clock.tick(30000);
3442
alertInstance.scheduleActions('default');
35-
expect(alertInstance.hasScheduledActions('1m')).toEqual(false);
43+
expect(alertInstance.isThrottled('1m')).toEqual(true);
3644
});
3745

3846
test(`shouldn't throttle when group didn't change and throttle period expired`, () => {
@@ -46,7 +54,7 @@ describe('hasScheduledActions()', () => {
4654
});
4755
clock.tick(30000);
4856
alertInstance.scheduleActions('default');
49-
expect(alertInstance.hasScheduledActions('15s')).toEqual(true);
57+
expect(alertInstance.isThrottled('15s')).toEqual(false);
5058
});
5159

5260
test(`shouldn't throttle when group changes`, () => {
@@ -60,7 +68,7 @@ describe('hasScheduledActions()', () => {
6068
});
6169
clock.tick(5000);
6270
alertInstance.scheduleActions('other-group');
63-
expect(alertInstance.hasScheduledActions('1m')).toEqual(true);
71+
expect(alertInstance.isThrottled('1m')).toEqual(false);
6472
});
6573
});
6674

@@ -75,9 +83,9 @@ describe('unscheduleActions()', () => {
7583
test('makes hasScheduledActions() return false', () => {
7684
const alertInstance = new AlertInstance();
7785
alertInstance.scheduleActions('default');
78-
expect(alertInstance.hasScheduledActions(null)).toEqual(true);
86+
expect(alertInstance.hasScheduledActions()).toEqual(true);
7987
alertInstance.unscheduleActions();
80-
expect(alertInstance.hasScheduledActions(null)).toEqual(false);
88+
expect(alertInstance.hasScheduledActions()).toEqual(false);
8189
});
8290

8391
test('makes getScheduledActionOptions() return undefined', () => {
@@ -113,10 +121,10 @@ describe('scheduleActions()', () => {
113121
},
114122
});
115123
alertInstance.replaceState({ otherField: true }).scheduleActions('default', { field: true });
116-
expect(alertInstance.hasScheduledActions(null)).toEqual(true);
124+
expect(alertInstance.hasScheduledActions()).toEqual(true);
117125
});
118126

119-
test('makes hasScheduledActions() return false when throttled', () => {
127+
test('makes isThrottled() return true when throttled', () => {
120128
const alertInstance = new AlertInstance({
121129
state: { foo: true },
122130
meta: {
@@ -127,10 +135,10 @@ describe('scheduleActions()', () => {
127135
},
128136
});
129137
alertInstance.replaceState({ otherField: true }).scheduleActions('default', { field: true });
130-
expect(alertInstance.hasScheduledActions('1m')).toEqual(false);
138+
expect(alertInstance.isThrottled('1m')).toEqual(true);
131139
});
132140

133-
test('make hasScheduledActions() return true when throttled expired', () => {
141+
test('make isThrottled() return false when throttled expired', () => {
134142
const alertInstance = new AlertInstance({
135143
state: { foo: true },
136144
meta: {
@@ -142,7 +150,7 @@ describe('scheduleActions()', () => {
142150
});
143151
clock.tick(120000);
144152
alertInstance.replaceState({ otherField: true }).scheduleActions('default', { field: true });
145-
expect(alertInstance.hasScheduledActions('1m')).toEqual(true);
153+
expect(alertInstance.isThrottled('1m')).toEqual(false);
146154
});
147155

148156
test('makes getScheduledActionOptions() return given options', () => {

‎x-pack/legacy/plugins/alerting/server/lib/alert_instance.ts‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -35,23 +35,24 @@ export class AlertInstance {
3535
this.meta = meta;
3636
}
3737

38-
hasScheduledActions(throttle: string | null) {
39-
// scheduleActions function wasn't called
38+
hasScheduledActions() {
39+
return this.scheduledExecutionOptions !== undefined;
40+
}
41+
42+
isThrottled(throttle: string | null) {
4043
if (this.scheduledExecutionOptions === undefined) {
4144
return false;
4245
}
43-
// Shouldn't schedule actions if still within throttling window
44-
// Reset if actionGroup changes
4546
const throttleMills = throttle ? parseDuration(throttle) : 0;
4647
const actionGroup = this.scheduledExecutionOptions.actionGroup;
4748
if (
4849
this.meta.lastScheduledActions &&
4950
this.meta.lastScheduledActions.group === actionGroup &&
5051
new Date(this.meta.lastScheduledActions.date).getTime() + throttleMills > Date.now()
5152
) {
52-
return false;
53+
return true;
5354
}
54-
return true;
55+
return false;
5556
}
5657

5758
getScheduledActionOptions() {
@@ -68,7 +69,7 @@ export class AlertInstance {
6869
}
6970

7071
scheduleActions(actionGroup: string, context: Context = {}) {
71-
if (this.hasScheduledActions(null)) {
72+
if (this.hasScheduledActions()) {
7273
throw new Error('Alert instance execution has already been scheduled, cannot schedule twice');
7374
}
7475
this.scheduledExecutionOptions = { actionGroup, context, state: this.state };

‎x-pack/legacy/plugins/alerting/server/lib/task_runner_factory.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -148,8 +148,12 @@ export class TaskRunnerFactory {
148148
await Promise.all(
149149
Object.keys(alertInstances).map(alertInstanceId => {
150150
const alertInstance = alertInstances[alertInstanceId];
151-
if (alertInstance.hasScheduledActions(throttle)) {
152-
if (muteAll || mutedInstanceIds.includes(alertInstanceId)) {
151+
if (alertInstance.hasScheduledActions()) {
152+
if (
153+
alertInstance.isThrottled(throttle) ||
154+
muteAll ||
155+
mutedInstanceIds.includes(alertInstanceId)
156+
) {
153157
return;
154158
}
155159
const { actionGroup, context, state } = alertInstance.getScheduledActionOptions()!;

‎x-pack/scripts/functional_tests.js‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,7 @@ require('@kbn/test').runTestsCli([
1313
require.resolve('../test/api_integration/config_security_basic.js'),
1414
require.resolve('../test/api_integration/config.js'),
1515
require.resolve('../test/alerting_api_integration/spaces_only/config.ts'),
16-
// FLAKY: https://github.com/elastic/kibana/issues/50079
17-
// FLAKY: https://github.com/elastic/kibana/issues/50074
18-
// FLAKY: https://github.com/elastic/kibana/issues/48709
19-
// FLAKY: https://github.com/elastic/kibana/issues/50078
20-
// require.resolve('../test/alerting_api_integration/security_and_spaces/config.ts'),
16+
require.resolve('../test/alerting_api_integration/security_and_spaces/config.ts'),
2117
require.resolve('../test/plugin_api_integration/config.js'),
2218
require.resolve('../test/kerberos_api_integration/config'),
2319
require.resolve('../test/kerberos_api_integration/anonymous_access.config'),

‎x-pack/test/alerting_api_integration/common/lib/get_test_alert_data.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ export function getTestAlertData(overwrites = {}) {
99
enabled: true,
1010
name: 'abc',
1111
alertTypeId: 'test.noop',
12-
interval: '10s',
12+
interval: '1m',
1313
throttle: '1m',
1414
actions: [],
1515
alertTypeParams: {},

‎x-pack/test/alerting_api_integration/common/lib/index.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,3 +9,4 @@ export { getUrlPrefix } from './space_test_utils';
99
export { ES_TEST_INDEX_NAME, ESTestIndexTool } from './es_test_index_tool';
1010
export { getTestAlertData } from './get_test_alert_data';
1111
export { AlertUtils } from './alert_utils';
12+
export { TaskManagerUtils } from './task_manager_utils';

‎x-pack/test/alerting_api_integration/common/lib/task_manager_utils.ts‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License;
4+
* you may not use this file except in compliance with the Elastic License.
5+
*/
6+
7+
export class TaskManagerUtils {
8+
private readonly es: any;
9+
private readonly retry: any;
10+
11+
constructor(es: any, retry: any) {
12+
this.es = es;
13+
this.retry = retry;
14+
}
15+
16+
async waitForIdle(taskRunAtFilter: Date) {
17+
return await this.retry.try(async () => {
18+
const searchResult = await this.es.search({
19+
index: '.kibana_task_manager',
20+
body: {
21+
query: {
22+
bool: {
23+
must: [
24+
{
25+
terms: {
26+
'task.scope': ['actions', 'alerting'],
27+
},
28+
},
29+
{
30+
range: {
31+
'task.scheduledAt': {
32+
gte: taskRunAtFilter,
33+
},
34+
},
35+
},
36+
],
37+
},
38+
},
39+
},
40+
});
41+
if (searchResult.hits.total.value) {
42+
throw new Error(`Expected 0 tasks but received ${searchResult.hits.total.value}`);
43+
}
44+
});
45+
}
46+
}

0 commit comments

Comments
 (0)