Skip to content

Commit

Permalink
Fix undefined error source in alerting log tags (#182352)
Browse files Browse the repository at this point in the history
We add the error resource type (USER or FRAMEWORK) to the error logs in
alerting plugin, but it misses some unhandled edge cases and adds the
tag as `undefined-error`.

This PR fixes that bug by setting all the non-user-error sources as
FRAMEWORK.
  • Loading branch information
ersin-erdal committed May 14, 2024
1 parent 0df3e6d commit 8477740
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 6 deletions.
32 changes: 32 additions & 0 deletions x-pack/plugins/alerting/server/task_runner/task_runner.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ import { RuleResultService } from '../monitoring/rule_result_service';
import { ruleResultServiceMock } from '../monitoring/rule_result_service.mock';
import { backfillClientMock } from '../backfill_client/backfill_client.mock';
import { UntypedNormalizedRuleType } from '../rule_type_registry';
import * as getExecutorServicesModule from './get_executor_services';

jest.mock('uuid', () => ({
v4: () => '5f6aa57d-3e22-484e-bae8-cbed868f4d28',
Expand All @@ -106,6 +107,8 @@ jest.mock('../lib/alerting_event_logger/alerting_event_logger');

jest.mock('../monitoring/rule_result_service');

jest.spyOn(getExecutorServicesModule, 'getExecutorServices');

let fakeTimer: sinon.SinonFakeTimers;
const logger: ReturnType<typeof loggingSystemMock.createLogger> = loggingSystemMock.createLogger();

Expand Down Expand Up @@ -1924,6 +1927,35 @@ describe('Task Runner', () => {
expect(mockUsageCounter.incrementCounter).not.toHaveBeenCalled();
});

test('should set unexpected errors as framework-error', async () => {
(getExecutorServicesModule.getExecutorServices as jest.Mock).mockRejectedValue(
new Error('test')
);

const taskRunner = new TaskRunner({
ruleType,
internalSavedObjectsRepository,
taskInstance: mockedTaskInstance,
context: taskRunnerFactoryInitializerParams,
inMemoryMetrics,
});
expect(AlertingEventLogger).toHaveBeenCalled();
rulesClient.getAlertFromRaw.mockReturnValue(mockedRuleTypeSavedObject as Rule);
encryptedSavedObjectsClient.getDecryptedAsInternalUser.mockResolvedValue(mockedRawRuleSO);

await taskRunner.run();

expect(logger.error).toBeCalledTimes(1);

const loggerCall = logger.error.mock.calls[0][0];
const loggerMeta = logger.error.mock.calls[0][1];
const loggerCallPrefix = (loggerCall as string).split('-');
expect(loggerCallPrefix[0].trim()).toMatchInlineSnapshot(
`"Executing Rule default:test:1 has resulted in Error: test"`
);
expect(loggerMeta?.tags).toEqual(['test', '1', 'rule-run-failed', 'framework-error']);
});

test('recovers gracefully when the RuleType executor throws an exception', async () => {
const taskRunError = new Error(GENERIC_ERROR_MESSAGE);

Expand Down
15 changes: 9 additions & 6 deletions x-pack/plugins/alerting/server/task_runner/task_runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
throwUnrecoverableError,
} from '@kbn/task-manager-plugin/server';
import { nanosToMillis } from '@kbn/event-log-plugin/server';
import { getErrorSource } from '@kbn/task-manager-plugin/server/task_running';
import { getErrorSource, isUserError } from '@kbn/task-manager-plugin/server/task_running';
import { ExecutionHandler, RunResult } from './execution_handler';
import {
RuleRunnerErrorStackTraceLog,
Expand Down Expand Up @@ -686,8 +686,10 @@ export class TaskRunner<

const { errors: errorsFromLastRun } = this.ruleResult.getLastRunResults();
if (errorsFromLastRun.length > 0) {
const isUserError = !errorsFromLastRun.some((lastRunError) => !lastRunError.userError);
const errorSource = isUserError ? TaskErrorSource.USER : TaskErrorSource.FRAMEWORK;
const isLastRunUserError = !errorsFromLastRun.some(
(lastRunError) => !lastRunError.userError
);
const errorSource = isLastRunUserError ? TaskErrorSource.USER : TaskErrorSource.FRAMEWORK;
const lasRunErrorMessages = errorsFromLastRun
.map((lastRunError) => lastRunError.message)
.join(',');
Expand All @@ -709,14 +711,15 @@ export class TaskRunner<
(ruleRunStateWithMetrics: RuleTaskStateAndMetrics) =>
transformRunStateToTaskState(ruleRunStateWithMetrics),
(err: ElasticsearchError) => {
const errorSource = `${getErrorSource(err)}-error`;
const errorSource = isUserError(err) ? TaskErrorSource.USER : TaskErrorSource.FRAMEWORK;
const errorSourceTag = `${errorSource}-error`;

if (isAlertSavedObjectNotFoundError(err, ruleId)) {
const message = `Executing Rule ${spaceId}:${
this.ruleType.id
}:${ruleId} has resulted in Error: ${getEsErrorMessage(err)}`;
this.logger.debug(message, {
tags: [this.ruleType.id, ruleId, 'rule-run-failed', errorSource],
tags: [this.ruleType.id, ruleId, 'rule-run-failed', errorSourceTag],
});
} else {
const error = this.stackTraceLog ? this.stackTraceLog.message : err;
Expand All @@ -725,7 +728,7 @@ export class TaskRunner<
this.ruleType.id
}:${ruleId} has resulted in Error: ${getEsErrorMessage(error)} - ${stack ?? ''}`;
this.logger.error(message, {
tags: [this.ruleType.id, ruleId, 'rule-run-failed', errorSource],
tags: [this.ruleType.id, ruleId, 'rule-run-failed', errorSourceTag],
error: { stack_trace: stack },
});
}
Expand Down

0 comments on commit 8477740

Please sign in to comment.