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.

Sign up for GitHub

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

[Response Ops][Alerting] Refactor ExecutionHandler stage 2 #193807

Merged
merged 7 commits into from
Oct 9, 2024
Merged

[Response Ops][Alerting] Refactor ExecutionHandler stage 2 #193807

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d3e831f
Breaking up ActionScheduler.run into subclasses
ymao1 Sep 26, 2024
cfb357c
Merge branch 'main' into refactor-execution-handler-stage-2
elasticmachine Oct 1, 2024
71fea9e
Merge branch 'main' into refactor-execution-handler-stage-2
elasticmachine Oct 3, 2024
3195eec
Merge branch 'main' of github.com:elastic/kibana into refactor-execut…
ymao1 Oct 7, 2024
32beee0
Adding comment about optional uuid
ymao1 Oct 7, 2024
321d58c
Merge branch 'main' into refactor-execution-handler-stage-2
elasticmachine Oct 9, 2024
680d6ff
Merge branch 'main' into refactor-execution-handler-stage-2
elasticmachine Oct 9, 2024
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
90 changes: 90 additions & 0 deletions x-pack/plugins/actions/server/create_execute_function.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1088,6 +1088,7 @@ describe('bulkExecute()', () => {
"actionTypeId": "mock-action",
"id": "123",
"response": "queuedActionsLimitError",
"uuid": undefined,
},
],
}
Expand All @@ -1099,4 +1100,93 @@ describe('bulkExecute()', () => {
]
`);
});

test('passes through action uuid if provided', async () => {
mockTaskManager.aggregate.mockResolvedValue({
took: 1,
timed_out: false,
_shards: { total: 1, successful: 1, skipped: 0, failed: 0 },
hits: { total: { value: 2, relation: 'eq' }, max_score: null, hits: [] },
aggregations: {},
});
mockActionsConfig.getMaxQueued.mockReturnValueOnce(3);
const executeFn = createBulkExecutionEnqueuerFunction({
taskManager: mockTaskManager,
actionTypeRegistry: actionTypeRegistryMock.create(),
isESOCanEncrypt: true,
inMemoryConnectors: [],
configurationUtilities: mockActionsConfig,
logger: mockLogger,
});
savedObjectsClient.bulkGet.mockResolvedValueOnce({
saved_objects: [
{ id: '123', type: 'action', attributes: { actionTypeId: 'mock-action' }, references: [] },
],
});
savedObjectsClient.bulkCreate.mockResolvedValueOnce({
saved_objects: [
{ id: '234', type: 'action_task_params', attributes: { actionId: '123' }, references: [] },
],
});
expect(
await executeFn(savedObjectsClient, [
{
id: '123',
params: { baz: false },
spaceId: 'default',
executionId: '123abc',
apiKey: null,
source: asHttpRequestExecutionSource(request),
actionTypeId: 'mock-action',
uuid: 'aaa',
},
{
id: '123',
params: { baz: false },
spaceId: 'default',
executionId: '456xyz',
apiKey: null,
source: asHttpRequestExecutionSource(request),
actionTypeId: 'mock-action',
uuid: 'bbb',
},
])
).toMatchInlineSnapshot(`
Object {
"errors": true,
"items": Array [
Object {
"actionTypeId": "mock-action",
"id": "123",
"response": "success",
"uuid": "aaa",
},
Object {
"actionTypeId": "mock-action",
"id": "123",
"response": "queuedActionsLimitError",
"uuid": "bbb",
},
],
}
`);
expect(mockTaskManager.bulkSchedule).toHaveBeenCalledTimes(1);
expect(mockTaskManager.bulkSchedule.mock.calls[0]).toMatchInlineSnapshot(`
Array [
Array [
Object {
"params": Object {
"actionTaskParamsId": "234",
"spaceId": "default",
},
"scope": Array [
"actions",
],
"state": Object {},
"taskType": "actions:mock-action",
},
],
]
`);
});
});
4 changes: 4 additions & 0 deletions x-pack/plugins/actions/server/create_execute_function.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ interface CreateExecuteFunctionOptions {
export interface ExecuteOptions
extends Pick<ActionExecutorOptions, 'params' | 'source' | 'relatedSavedObjects' | 'consumer'> {
id: string;
uuid?: string;
Copy link
Member

Choose a reason for hiding this comment

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

Curious why this is optional. I think it's because for really old rules that haven't been updated since we added the uuid to the actions, it won't be there, and we don't have a migration. But double-checking in case it's BWC or something.

Just from a quick scan of the code, it doesn't seem like we added many tests in this PR that would have a non-existing uuid. Maybe some of the tests already deal with this, that I wouldn't have noticed?

I see one reference to a uuid!, which was just refactor/move from old code - but ... scares me :-). As I take a closer look at this PR, maybe I'll figure out it's ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is typed as optional even though it should always exist on the action :(. I think because we use a single schema in the API and the persistence layer and we don't expect the UUID to be passed into the API but we create one before it reaches the persistence layer.

Copy link
Member

Choose a reason for hiding this comment

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

I found the PR where we added the uuid. For some reason I thought we hadn't done a migration to add to old rules, but we did! Add uuid to rule actions. My fear was we didn't, so an upgrade from an old version wouldn't have uuids, which got me wondering about the cases where you check it.

Seems like something we should fix. Open an issue? No need to fix in this PR (and may be nasty, presumably another type to deal with!).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created an issue: #195255

spaceId: string;
apiKey: string | null;
executionId: string;
Expand Down Expand Up @@ -71,6 +72,7 @@ export interface ExecutionResponse {

export interface ExecutionResponseItem {
id: string;
uuid?: string;
actionTypeId: string;
response: ExecutionResponseType;
}
Expand Down Expand Up @@ -197,12 +199,14 @@ export function createBulkExecutionEnqueuerFunction({
items: runnableActions
.map((a) => ({
id: a.id,
uuid: a.uuid,
actionTypeId: a.actionTypeId,
response: ExecutionResponseType.SUCCESS,
}))
.concat(
actionsOverLimit.map((a) => ({
id: a.id,
uuid: a.uuid,
actionTypeId: a.actionTypeId,
response: ExecutionResponseType.QUEUED_ACTIONS_LIMIT_ERROR,
}))
Expand Down
9 changes: 9 additions & 0 deletions x-pack/plugins/alerting/server/lib/alerting_event_logger/alerting_event_logger.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -807,6 +807,15 @@ describe('AlertingEventLogger', () => {

expect(eventLogger.logEvent).toHaveBeenCalledWith(event);
});

test('should log action event with uuid', () => {
alertingEventLogger.initialize({ context: ruleContext, runDate, ruleData });
alertingEventLogger.logAction({ ...action, uuid: 'abcdefg' });

const event = createActionExecuteRecord(ruleContext, ruleData, [alertSO], action);

expect(eventLogger.logEvent).toHaveBeenCalledWith(event);
});
});

describe('done()', () => {
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/alerting/server/lib/alerting_event_logger/alerting_event_logger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ interface AlertOpts {

export interface ActionOpts {
id: string;
uuid?: string;
typeId: string;
alertId?: string;
alertGroup?: string;
Expand Down
Loading