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

Jump to bottom

[7.7] [SIEM] print detailed bulk response error message #63327

Closed
wants to merge 2 commits into from
Closed

[7.7] [SIEM] print detailed bulk response error message #63327

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d101baa
print detailed builk response error message
Apr 13, 2020
0a10508
filter out noisy and expected version_conflict_engine_exception 409 e…
Apr 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 7 additions & 2 deletions x-pack/legacy/plugins/siem/server/lib/detection_engine/signals/single_bulk_create.ts
View file
Open in desktop
Expand Up @@ -62,6 +62,10 @@ export interface SingleBulkCreateResponse {
createdItemsCount: number;
}

function stringBreviate(text, count, insertDots){
return text.slice(0, count) + (((text.length > count) && insertDots) ? "..." : "");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than use this function you would be better off using the lodash one:
https://lodash.com/docs/4.17.15#truncate

Copy link
Author

Choose a reason for hiding this comment

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

How to use this?
image
the error says that _ is not defined


// Bulk Index documents.
export const singleBulkCreate = async ({
someResult,
Expand Down Expand Up @@ -134,13 +138,14 @@ export const singleBulkCreate = async ({
logger.debug(`took property says bulk took: ${response.took} milliseconds`);

if (response.errors) {
const itemsWithErrors = response.items.filter(item => item.create.error);
const itemsWithErrors = response.items.filter(item => item.create.error).filter(item => item.create.status !== 409);
const errorCountsByStatus = countBy(itemsWithErrors, item => item.create.status);
delete errorCountsByStatus['409']; // Duplicate signals are expected

if (!isEmpty(errorCountsByStatus)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Right now you're outputting a lot of 409 conflict messages which are not what we are interested in as 409 conflicts are ok because we do a de-duplication.

What you want to do is report per each line of your log file is any non-409 messages such as the 400 errors you are getting.

I would remove this and instead add these lines of code right before the if (!isEmpty(errorCountsByStatus)) { like so:

    itemsWithErrors
      .filter(item => item.create.status !== 409) // remove any 409 conflict error codes as those are expected due to de-duplication
      .forEach(item => {
        if (item.create.error != null) {
          logger.error(
            `Rule id: "${id}" rule_id: "${ruleParams.ruleId}" name: "${name}" has an error response with the reason of: ${item.create.error.reason}`
          );
        } else {
          // return the full item if there is no reason but typically Elastic Search should always return a reason
          logger.error(
            `Rule id: "${id}" rule_id: "${
              ruleParams.ruleId
            }" name: "${name}" has an error response with the reason of: ${JSON.stringify(item)}`
          );
        }
      });

if (!isEmpty(errorCountsByStatus)) {

I don't see anything wrong with logging out all of the errors you are seeing as we are interested in seeing them all and fixing them.

Copy link
Author

Choose a reason for hiding this comment

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

it makes sense to filter out 409. I just pushed a commit to include that. Thanks.

There is no need to print out the individual errors. Let's just think the scenarios we add a noisy rule that generates 1000s of bad requests and this would lead to a lot loggings that can cause the server running out of local disk space quickly.
I think it is ok to fix a few errors in each iteration. We do not need to fix all the errors at once, and also in the most common cases, the error messages are the same so no need to print them in a loop and repeat the same for 1000s time, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree and talked with team members so we are going to aggregate on the message reason and output that instead of the status code.

That should keep logs from filling up but also give us more robust error reporting for logs when responses are coming back unexpected with errors such as networking errors, gateway errors, or other misc things.

The PR is this one if you want to watch for it:
#63513

Copy link
Author

Choose a reason for hiding this comment

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

looks good to me. thanks

const errmsg = stringBreviate(JSON.stringify(itemsWithErrors), 1000, true);
logger.error(
`[-] bulkResponse had errors with response statuses:counts of...\n${JSON.stringify(
`[-] bulkResponse had errors ${errmsg} with response statuses:counts of...\n${JSON.stringify(
errorCountsByStatus,
null,
2
Expand Down