From 64d5602387969fbf2958a8b9102d84c63e187cbb Mon Sep 17 00:00:00 2001 From: Ersin Erdal Date: Thu, 4 Apr 2024 15:17:33 +0200 Subject: [PATCH 1/3] Let addLastRunError to report user errors --- .../server/lib/rule_execution_status.ts | 2 +- .../monitoring/rule_result_service.test.ts | 29 +++++++++++++++-- .../server/monitoring/rule_result_service.ts | 15 ++++++--- .../server/task_runner/task_runner.test.ts | 32 +++++++++++++++++-- .../server/task_runner/task_runner.ts | 3 +- x-pack/plugins/alerting/server/types.ts | 2 +- 6 files changed, 70 insertions(+), 13 deletions(-) diff --git a/x-pack/plugins/alerting/server/lib/rule_execution_status.ts b/x-pack/plugins/alerting/server/lib/rule_execution_status.ts index 4a8869fac452e..7af7cf49f0324 100644 --- a/x-pack/plugins/alerting/server/lib/rule_execution_status.ts +++ b/x-pack/plugins/alerting/server/lib/rule_execution_status.ts @@ -76,7 +76,7 @@ export function executionStatusFromState({ // These errors are reported by ruleResultService.addLastRunError, therefore they are landed in successful execution map error = { reason: RuleExecutionStatusErrorReasons.Unknown, - message: errorsFromLastRun.join(','), + message: errorsFromLastRun.map((lastRunError) => lastRunError.message).join(','), }; } diff --git a/x-pack/plugins/alerting/server/monitoring/rule_result_service.test.ts b/x-pack/plugins/alerting/server/monitoring/rule_result_service.test.ts index 5012c9902faca..bb4168b7d162a 100644 --- a/x-pack/plugins/alerting/server/monitoring/rule_result_service.test.ts +++ b/x-pack/plugins/alerting/server/monitoring/rule_result_service.test.ts @@ -31,7 +31,16 @@ describe('RuleResultService', () => { test('should return errors array with added error', () => { lastRunSetters.addLastRunError('First error'); - expect(ruleResultService.getLastRunErrors()).toEqual(['First error']); + expect(ruleResultService.getLastRunErrors()).toEqual([ + { message: 'First error', userError: false }, + ]); + }); + + test('should return errors array with added user error', () => { + lastRunSetters.addLastRunError('First error', true); + expect(ruleResultService.getLastRunErrors()).toEqual([ + { message: 'First error', userError: true }, + ]); }); test('should return warnings array with added warning', () => { @@ -49,7 +58,12 @@ describe('RuleResultService', () => { lastRunSetters.addLastRunWarning('warning'); lastRunSetters.setLastRunOutcomeMessage('outcome message'); const expectedLastRun: RuleResultServiceResults = { - errors: ['error'], + errors: [ + { + message: 'error', + userError: false, + }, + ], warnings: ['warning'], outcomeMessage: 'outcome message', }; @@ -63,7 +77,16 @@ describe('RuleResultService', () => { lastRunSetters.setLastRunOutcomeMessage('second outcome message'); lastRunSetters.setLastRunOutcomeMessage('last outcome message'); const expectedLastRun = { - errors: ['first error', 'second error'], + errors: [ + { + message: 'first error', + userError: false, + }, + { + message: 'second error', + userError: false, + }, + ], warnings: [], outcomeMessage: 'last outcome message', }; diff --git a/x-pack/plugins/alerting/server/monitoring/rule_result_service.ts b/x-pack/plugins/alerting/server/monitoring/rule_result_service.ts index 54ac2240c662e..f196cdd665f35 100644 --- a/x-pack/plugins/alerting/server/monitoring/rule_result_service.ts +++ b/x-pack/plugins/alerting/server/monitoring/rule_result_service.ts @@ -8,17 +8,22 @@ import { PublicLastRunSetters } from '../types'; export interface RuleResultServiceResults { - errors: string[]; + errors: LastRunError[]; warnings: string[]; outcomeMessage: string; } +interface LastRunError { + message: string; + userError?: boolean; +} + export class RuleResultService { - private errors: string[] = []; + private errors: LastRunError[] = []; private warnings: string[] = []; private outcomeMessage: string = ''; - public getLastRunErrors(): string[] { + public getLastRunErrors(): LastRunError[] { return this.errors; } @@ -46,8 +51,8 @@ export class RuleResultService { }; } - private addLastRunError(error: string) { - this.errors.push(error); + private addLastRunError(message: string, userError: boolean = false) { + this.errors.push({ message, userError }); } private addLastRunWarning(warning: string) { diff --git a/x-pack/plugins/alerting/server/task_runner/task_runner.test.ts b/x-pack/plugins/alerting/server/task_runner/task_runner.test.ts index 172c490cdbc8c..22777453c573b 100644 --- a/x-pack/plugins/alerting/server/task_runner/task_runner.test.ts +++ b/x-pack/plugins/alerting/server/task_runner/task_runner.test.ts @@ -3311,7 +3311,10 @@ describe('Task Runner', () => { }); ruleResultService.getLastRunResults.mockImplementation(() => ({ - errors: ['an error occurred'], + errors: [ + { message: 'an error occurred', userError: false }, + { message: 'second error occurred', userError: true }, + ], warnings: [], outcomeMessage: '', })); @@ -3325,13 +3328,38 @@ describe('Task Runner', () => { testAlertingEventLogCalls({ status: 'error', softErrorFromLastRun: true, - errorMessage: 'an error occurred', + errorMessage: 'an error occurred,second error occurred', errorReason: 'unknown', }); expect(getErrorSource(runnerResult.taskRunError as Error)).toBe(TaskErrorSource.FRAMEWORK); }); + test('returns user error if all the errors are user error', async () => { + rulesClient.getAlertFromRaw.mockReturnValue(mockedRuleTypeSavedObject as Rule); + encryptedSavedObjectsClient.getDecryptedAsInternalUser.mockResolvedValue(mockedRawRuleSO); + + const taskRunner = new TaskRunner({ + ruleType, + taskInstance: mockedTaskInstance, + context: taskRunnerFactoryInitializerParams, + inMemoryMetrics, + }); + + ruleResultService.getLastRunResults.mockImplementation(() => ({ + errors: [ + { message: 'an error occurred', userError: true }, + { message: 'second error occurred', userError: true }, + ], + warnings: [], + outcomeMessage: '', + })); + + const runnerResult = await taskRunner.run(); + + expect(getErrorSource(runnerResult.taskRunError as Error)).toBe(TaskErrorSource.USER); + }); + function testAlertingEventLogCalls({ ruleContext = alertingEventLoggerInitializer, activeAlerts = 0, diff --git a/x-pack/plugins/alerting/server/task_runner/task_runner.ts b/x-pack/plugins/alerting/server/task_runner/task_runner.ts index 28922187f902c..17cdefc4855c4 100644 --- a/x-pack/plugins/alerting/server/task_runner/task_runner.ts +++ b/x-pack/plugins/alerting/server/task_runner/task_runner.ts @@ -715,10 +715,11 @@ export class TaskRunner< const { errors: errorsFromLastRun } = this.ruleResult.getLastRunResults(); if (errorsFromLastRun.length > 0) { + const isUserError = !errorsFromLastRun.some((lastRunError) => !lastRunError.userError); return { taskRunError: createTaskRunError( new Error(errorsFromLastRun.join(',')), - TaskErrorSource.FRAMEWORK + isUserError ? TaskErrorSource.USER : TaskErrorSource.FRAMEWORK ), }; } diff --git a/x-pack/plugins/alerting/server/types.ts b/x-pack/plugins/alerting/server/types.ts index 5316bcaa1a5ae..858ca01c75976 100644 --- a/x-pack/plugins/alerting/server/types.ts +++ b/x-pack/plugins/alerting/server/types.ts @@ -414,7 +414,7 @@ export interface PublicMetricsSetters { } export interface PublicLastRunSetters { - addLastRunError: (outcome: string) => void; + addLastRunError: (message: string, userError?: boolean) => void; addLastRunWarning: (outcomeMsg: string) => void; setLastRunOutcomeMessage: (warning: string) => void; } From 0628e42209bda20784bfa6c7d91d3d89cd49a963 Mon Sep 17 00:00:00 2001 From: Ersin Erdal Date: Fri, 5 Apr 2024 13:19:15 +0200 Subject: [PATCH 2/3] fix check type --- x-pack/plugins/alerting/server/lib/last_run_status.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/alerting/server/lib/last_run_status.test.ts b/x-pack/plugins/alerting/server/lib/last_run_status.test.ts index 97c47d5294a30..954054fa46c0c 100644 --- a/x-pack/plugins/alerting/server/lib/last_run_status.test.ts +++ b/x-pack/plugins/alerting/server/lib/last_run_status.test.ts @@ -39,7 +39,7 @@ const getRuleResultService = ({ const ruleResultService = new RuleResultService(); const { addLastRunError, addLastRunWarning, setLastRunOutcomeMessage } = ruleResultService.getLastRunSetters(); - errors.forEach((error) => addLastRunError(error)); + errors.forEach((error) => addLastRunError(error.message)); warnings.forEach((warning) => addLastRunWarning(warning)); setLastRunOutcomeMessage(outcomeMessage); return ruleResultService; @@ -250,7 +250,7 @@ describe('lastRunFromState', () => { metrics: getMetrics({ hasReachedAlertLimit: true }), }, getRuleResultService({ - errors: ['MOCK_ERROR'], + errors: [{ message: 'MOCK_ERROR', userError: false }], outcomeMessage: 'Rule execution reported an error', }) ); From 84beb254636f0d88adff7c762e5b2c5d0b95e836 Mon Sep 17 00:00:00 2001 From: Ersin Erdal Date: Fri, 5 Apr 2024 14:41:04 +0200 Subject: [PATCH 3/3] fix error message mapping in result --- .../plugins/alerting/server/monitoring/rule_result_service.ts | 2 +- x-pack/plugins/alerting/server/task_runner/task_runner.test.ts | 1 + x-pack/plugins/alerting/server/task_runner/task_runner.ts | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/alerting/server/monitoring/rule_result_service.ts b/x-pack/plugins/alerting/server/monitoring/rule_result_service.ts index f196cdd665f35..1646efb29797a 100644 --- a/x-pack/plugins/alerting/server/monitoring/rule_result_service.ts +++ b/x-pack/plugins/alerting/server/monitoring/rule_result_service.ts @@ -15,7 +15,7 @@ export interface RuleResultServiceResults { interface LastRunError { message: string; - userError?: boolean; + userError: boolean; } export class RuleResultService { diff --git a/x-pack/plugins/alerting/server/task_runner/task_runner.test.ts b/x-pack/plugins/alerting/server/task_runner/task_runner.test.ts index 22777453c573b..893670d906cea 100644 --- a/x-pack/plugins/alerting/server/task_runner/task_runner.test.ts +++ b/x-pack/plugins/alerting/server/task_runner/task_runner.test.ts @@ -3333,6 +3333,7 @@ describe('Task Runner', () => { }); expect(getErrorSource(runnerResult.taskRunError as Error)).toBe(TaskErrorSource.FRAMEWORK); + expect(runnerResult.taskRunError).toEqual(new Error('an error occurred,second error occurred')); }); test('returns user error if all the errors are user error', async () => { diff --git a/x-pack/plugins/alerting/server/task_runner/task_runner.ts b/x-pack/plugins/alerting/server/task_runner/task_runner.ts index 17cdefc4855c4..edfe77e1502c7 100644 --- a/x-pack/plugins/alerting/server/task_runner/task_runner.ts +++ b/x-pack/plugins/alerting/server/task_runner/task_runner.ts @@ -718,7 +718,7 @@ export class TaskRunner< const isUserError = !errorsFromLastRun.some((lastRunError) => !lastRunError.userError); return { taskRunError: createTaskRunError( - new Error(errorsFromLastRun.join(',')), + new Error(errorsFromLastRun.map((lastRunError) => lastRunError.message).join(',')), isUserError ? TaskErrorSource.USER : TaskErrorSource.FRAMEWORK ), };