Skip to content

Commit

Permalink
Ignore 409 errors from our signal creation
Browse files Browse the repository at this point in the history
In my experience these are always due to a rule being run multiple times
on the same document, generating a duplicate signal with a (correctly)
duplicate id. Only if we encounter non-409 errors do we log a message to
the user.
  • Loading branch information
rylnd committed Dec 30, 2019
1 parent 31deb25 commit 3b9c26e
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 30 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,28 @@ export const sampleBulkCreateDuplicateResult = {
],
};

export const sampleBulkCreateErrorResult = {
...sampleBulkCreateDuplicateResult,
items: [
...sampleBulkCreateDuplicateResult.items,
{
create: {
_index: 'test',
_type: '_doc',
_id: '5',
status: 500,
error: {
type: 'internal_server_error',
reason: '[4]: internal server error',
index_uuid: 'cXmq4Rt3RGGswDTTwZFzvA',
shard: '0',
index: 'test',
},
},
},
],
};

export const sampleDocSearchResultsNoSortId = (
someUuid: string = sampleIdGuid
): SignalSearchResponse => ({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
sampleDocSearchResultsNoSortIdNoVersion,
sampleEmptyDocSearchResults,
sampleBulkCreateDuplicateResult,
sampleBulkCreateErrorResult,
} from './__mocks__/es_results';
import { savedObjectsClientMock } from 'src/core/server/mocks';
import { DEFAULT_SIGNALS_INDEX } from '../../../../common/constants';
Expand Down Expand Up @@ -206,7 +207,8 @@ describe('singleBulkCreate', () => {
});
expect(successfulsingleBulkCreate).toEqual(true);
});
test('create successful bulk create when bulk create has errors', async () => {

test('create successful bulk create when bulk create has duplicate errors', async () => {
const sampleParams = sampleRuleAlertParams();
const sampleSearchResult = sampleDocSearchResultsNoSortId;
mockService.callCluster.mockReturnValue(sampleBulkCreateDuplicateResult);
Expand All @@ -224,6 +226,30 @@ describe('singleBulkCreate', () => {
enabled: true,
tags: ['some fake tag 1', 'some fake tag 2'],
});

expect(mockLogger.error).not.toHaveBeenCalled();
expect(successfulsingleBulkCreate).toEqual(true);
});

test('create successful bulk create when bulk create has multiple error statuses', async () => {
const sampleParams = sampleRuleAlertParams();
const sampleSearchResult = sampleDocSearchResultsNoSortId;
mockService.callCluster.mockReturnValue(sampleBulkCreateErrorResult);
const successfulsingleBulkCreate = await singleBulkCreate({
someResult: sampleSearchResult(),
ruleParams: sampleParams,
services: mockService,
logger: mockLogger,
id: sampleRuleGuid,
signalsIndex: DEFAULT_SIGNALS_INDEX,
name: 'rule-name',
createdBy: 'elastic',
updatedBy: 'elastic',
interval: '5m',
enabled: true,
tags: ['some fake tag 1', 'some fake tag 2'],
});

expect(mockLogger.error).toHaveBeenCalled();
expect(successfulsingleBulkCreate).toEqual(true);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { countBy } from 'lodash';
import { performance } from 'perf_hooks';
import { AlertServices } from '../../../../../alerting/server/types';
import { SignalSearchResponse, BulkResponse } from './types';
Expand Down Expand Up @@ -68,39 +69,30 @@ export const singleBulkCreate = async ({
},
buildBulkBody({ doc, ruleParams, id, name, createdBy, updatedBy, interval, enabled, tags }),
]);
const time1 = performance.now();
const firstResult: BulkResponse = await services.callCluster('bulk', {
const start = performance.now();
const response: BulkResponse = await services.callCluster('bulk', {
index: signalsIndex,
refresh: false,
body: bulkBody,
});
const time2 = performance.now();
logger.debug(
`individual bulk process time took: ${Number(time2 - time1).toFixed(2)} milliseconds`
);
logger.debug(`took property says bulk took: ${firstResult.took} milliseconds`);
if (firstResult.errors) {
// go through the response status errors and see what
// types of errors they are, count them up, and log them.
const errorCountMap = firstResult.items.reduce((acc: { [key: string]: number }, item) => {
if (item.create.error) {
const responseStatusKey = item.create.status.toString();
acc[responseStatusKey] = acc[responseStatusKey] ? acc[responseStatusKey] + 1 : 1;
}
return acc;
}, {});
/*
the logging output below should look like
{'409': 55}
which is read as "there were 55 counts of 409 errors returned from bulk create"
*/
logger.error(
`[-] bulkResponse had errors with response statuses:counts of...\n${JSON.stringify(
errorCountMap,
null,
2
)}`
);
const end = performance.now();
logger.debug(`individual bulk process time took: ${Number(end - start).toFixed(2)} milliseconds`);
logger.debug(`took property says bulk took: ${response.took} milliseconds`);

if (response.errors) {
const itemsWithErrors = response.items.filter(item => item.create.error);
const errorCountsByStatus = countBy(itemsWithErrors, item => item.create.status);
const hasNonDuplicateError = Object.keys(errorCountsByStatus).some(status => status !== '409');

if (hasNonDuplicateError) {
logger.error(
`[-] bulkResponse had errors with response statuses:counts of...\n${JSON.stringify(
errorCountsByStatus,
null,
2
)}`
);
}
}
return true;
};

0 comments on commit 3b9c26e

Please sign in to comment.