From 126da30398220f41f8235faf49e3ec4b335f36d5 Mon Sep 17 00:00:00 2001 From: Ryland Herrick Date: Thu, 2 Jan 2020 20:23:22 -0600 Subject: [PATCH] [SIEM][Detection Engine] Silence 409 errors on signal creation (#53859) (#53903) * Remove punctuation from translation We already had a colon on both uses of this key, resulting in '::' on the page. * Ignore 409 errors from our signal creation 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. * Hide 409 errors during signal creation These are expected and potentially confusing to the user. Instead, we only show unexpected errors. Co-authored-by: Elastic Machine Co-authored-by: Elastic Machine --- .../pages/detection_engine/translations.ts | 2 +- .../signals/__mocks__/es_results.ts | 22 ++++++++ .../signals/single_bulk_create.test.ts | 28 ++++++++++- .../signals/single_bulk_create.ts | 50 ++++++++----------- 4 files changed, 71 insertions(+), 31 deletions(-) diff --git a/x-pack/legacy/plugins/siem/public/pages/detection_engine/translations.ts b/x-pack/legacy/plugins/siem/public/pages/detection_engine/translations.ts index 94adbfcde1e53..11b3fb11f6eee 100644 --- a/x-pack/legacy/plugins/siem/public/pages/detection_engine/translations.ts +++ b/x-pack/legacy/plugins/siem/public/pages/detection_engine/translations.ts @@ -11,7 +11,7 @@ export const PAGE_TITLE = i18n.translate('xpack.siem.detectionEngine.pageTitle', }); export const LAST_SIGNAL = i18n.translate('xpack.siem.detectionEngine.lastSignalTitle', { - defaultMessage: 'Last signal:', + defaultMessage: 'Last signal', }); export const TOTAL_SIGNAL = i18n.translate('xpack.siem.detectionEngine.totalSignalTitle', { diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/signals/__mocks__/es_results.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/signals/__mocks__/es_results.ts index db5b3701a4fc2..5e50b65b51717 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/signals/__mocks__/es_results.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/signals/__mocks__/es_results.ts @@ -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 => ({ diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/signals/single_bulk_create.test.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/signals/single_bulk_create.test.ts index d58f0a22b763d..ca4b1d1e8e84a 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/signals/single_bulk_create.test.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/signals/single_bulk_create.test.ts @@ -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'; @@ -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); @@ -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); }); diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/signals/single_bulk_create.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/signals/single_bulk_create.ts index 40b2eeab938dc..a6290e57eb225 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/signals/single_bulk_create.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/signals/single_bulk_create.ts @@ -4,6 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ +import { countBy, isEmpty } from 'lodash'; import { performance } from 'perf_hooks'; import { AlertServices } from '../../../../../alerting/server/types'; import { SignalSearchResponse, BulkResponse } from './types'; @@ -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); + delete errorCountsByStatus['409']; // Duplicate signals are expected + + if (!isEmpty(errorCountsByStatus)) { + logger.error( + `[-] bulkResponse had errors with response statuses:counts of...\n${JSON.stringify( + errorCountsByStatus, + null, + 2 + )}` + ); + } } return true; };