Skip to content

Commit

Permalink
[ResponseOps] Errors during marking tasks as running are not shown in…
Browse files Browse the repository at this point in the history
… metrics (#191300)

Resolves #184171

## Summary

Errors are not shown in metrics when Elasticsearch returns an error
during `markAsRunning` (changes status from claiming to running)
operation in TaskManager. This PR updates the TaskManager to throw an
error instead of just logging it.

### Checklist

- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios


### To verify

1. Create an Always Firing rule.
2. Put the below code in the [try block of TaskStore.bulkUpdate
method](https://github.com/elastic/kibana/blob/692b2285176afc75ff5019e5ed651024d9374f91/x-pack/plugins/task_manager/server/task_store.ts#L304)
to mimic markAsRunning
```
      const isMarkAsRunning = docs.some(
        (doc) =>
          doc.taskType === 'alerting:example.always-firing' &&
          doc.status === 'running' &&
          doc.retryAt !== null
      );
      if (isMarkAsRunning) {
        throw SavedObjectsErrorHelpers.decorateEsUnavailableError(new Error('test'));
      }
```
3. Verify that when the above error is thrown, it is reflected in
[metrics
endpoint](http://localhost:5601/api/task_manager/metrics?reset=false)
results.
  • Loading branch information
doakalexi authored Sep 4, 2024
1 parent 772655a commit 866a6c9
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 13 deletions.
24 changes: 12 additions & 12 deletions x-pack/plugins/task_manager/server/task_pool/task_pool.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ describe('TaskPool', () => {
expect(shouldNotRun).not.toHaveBeenCalled();
});

test('should log when marking a Task as running fails', async () => {
test('should log and throw an error when marking a Task as running fails', async () => {
const pool = new TaskPool({
capacity$: of(3),
definitions,
Expand All @@ -180,15 +180,15 @@ describe('TaskPool', () => {
throw new Error(`Mark Task as running has failed miserably`);
});

const result = await pool.run([mockTask(), taskFailedToMarkAsRunning, mockTask()]);
await expect(
pool.run([mockTask(), taskFailedToMarkAsRunning, mockTask()])
).rejects.toThrowError('Mark Task as running has failed miserably');

expect((logger as jest.Mocked<Logger>).error.mock.calls[0]).toMatchInlineSnapshot(`
Array [
"Failed to mark Task TaskType \\"shooooo\\" as running: Mark Task as running has failed miserably",
]
`);

expect(result).toEqual(TaskPoolRunResult.RunningAllClaimedTasks);
});

test('should log when running a Task fails', async () => {
Expand Down Expand Up @@ -549,7 +549,7 @@ describe('TaskPool', () => {
expect(shouldNotRun).not.toHaveBeenCalled();
});

test('should log when marking a Task as running fails', async () => {
test('should log and throw an error when marking a Task as running fails', async () => {
const pool = new TaskPool({
capacity$: of(6),
definitions,
Expand All @@ -562,15 +562,15 @@ describe('TaskPool', () => {
throw new Error(`Mark Task as running has failed miserably`);
});

const result = await pool.run([mockTask(), taskFailedToMarkAsRunning, mockTask()]);
await expect(
pool.run([mockTask(), taskFailedToMarkAsRunning, mockTask()])
).rejects.toThrowError('Mark Task as running has failed miserably');

expect((logger as jest.Mocked<Logger>).error.mock.calls[0]).toMatchInlineSnapshot(`
Array [
"Failed to mark Task TaskType \\"shooooo\\" as running: Mark Task as running has failed miserably",
]
`);

expect(result).toEqual(TaskPoolRunResult.RunningAllClaimedTasks);
Array [
"Failed to mark Task TaskType \\"shooooo\\" as running: Mark Task as running has failed miserably",
]
`);
});

test('should log when running a Task fails', async () => {
Expand Down
5 changes: 4 additions & 1 deletion x-pack/plugins/task_manager/server/task_pool/task_pool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,10 @@ export class TaskPool {
message: VERSION_CONFLICT_MESSAGE,
})
)
.catch((err) => this.handleFailureOfMarkAsRunning(taskRunner, err));
.catch((err) => {
this.handleFailureOfMarkAsRunning(taskRunner, err);
throw err;
});
})
);
}
Expand Down

0 comments on commit 866a6c9

Please sign in to comment.