-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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.
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
[SIEM][Detection Engine] Silence 409 errors on signal creation #53859
Conversation
We already had a colon on both uses of this key, resulting in '::' on the page.
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.
Pinging @elastic/siem (Team:SIEM) |
@elasticmachine merge upstream |
const errorCountsByStatus = countBy(itemsWithErrors, item => item.create.status); | ||
const hasNonDuplicateError = Object.keys(errorCountsByStatus).some(status => status !== '409'); | ||
|
||
if (hasNonDuplicateError) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't decide whether or not to hide these 409 errors in all cases. That seems more consistent with the idea that these are expected errors and could potentially be confusing to the user, but as the logic stands we will show all errors (including 409s) if there are non-409 errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: 1bcf978
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Thanks for making these changes.
These are expected and potentially confusing to the user. Instead, we only show unexpected errors.
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
… (#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 <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
* master: Rename `/api/security/oidc` to `/api/security/oidc/callback`. (elastic#53886) Updating transitive dependencies to use [email protected] (elastic#53899) [Reporting/Tests] consolidate functional test configs (elastic#52671) [Reporting] Correct the docvalue_fields params in the search query Download CSV from Dashboard Panel (elastic#52833) [Test/Newsfeed] Re-enable test and add news item to be filtered (elastic#53905) cleanup server-log action (elastic#53326) [Uptime] Delete uptime eslint rule skip (elastic#50912) [skip-ci] Expression Lifecycle Docs (elastic#51494) [Endpoint] add react router to endpoint app (elastic#53808) [SIEM][Detection Engine] Silence 409 errors on signal creation (elastic#53859) [Maps] get max_result_window and max_inner_result_window from index settings (elastic#53500) [ML] New Platform server shim: update analytics routes to use new platform router (elastic#53521) fixes typo on engine detection page (elastic#53877)
* master: Rename `/api/security/oidc` to `/api/security/oidc/callback`. (elastic#53886) Updating transitive dependencies to use [email protected] (elastic#53899) [Reporting/Tests] consolidate functional test configs (elastic#52671) [Reporting] Correct the docvalue_fields params in the search query Download CSV from Dashboard Panel (elastic#52833) [Test/Newsfeed] Re-enable test and add news item to be filtered (elastic#53905) cleanup server-log action (elastic#53326) [Uptime] Delete uptime eslint rule skip (elastic#50912) [skip-ci] Expression Lifecycle Docs (elastic#51494) [Endpoint] add react router to endpoint app (elastic#53808) [SIEM][Detection Engine] Silence 409 errors on signal creation (elastic#53859) [Maps] get max_result_window and max_inner_result_window from index settings (elastic#53500) [ML] New Platform server shim: update analytics routes to use new platform router (elastic#53521) fixes typo on engine detection page (elastic#53877) [Maps] push mapbox value extraction from VectorStyle and into DynamicStyleProperty (elastic#53806) Fix suggested value for time_zone in range query (elastic#53841) Clean up generic hooks, use react-use instead (elastic#53822)
…ris/kibana into alerting/created_at-and-updated_at * 'alerting/created_at-and-updated_at' of github.com:gmmorris/kibana: Rename `/api/security/oidc` to `/api/security/oidc/callback`. (elastic#53886) Updating transitive dependencies to use [email protected] (elastic#53899) [Reporting/Tests] consolidate functional test configs (elastic#52671) [Reporting] Correct the docvalue_fields params in the search query Download CSV from Dashboard Panel (elastic#52833) [Test/Newsfeed] Re-enable test and add news item to be filtered (elastic#53905) cleanup server-log action (elastic#53326) [Uptime] Delete uptime eslint rule skip (elastic#50912) [skip-ci] Expression Lifecycle Docs (elastic#51494) [Endpoint] add react router to endpoint app (elastic#53808) [SIEM][Detection Engine] Silence 409 errors on signal creation (elastic#53859) [Maps] get max_result_window and max_inner_result_window from index settings (elastic#53500) [ML] New Platform server shim: update analytics routes to use new platform router (elastic#53521) fixes typo on engine detection page (elastic#53877) [Maps] push mapbox value extraction from VectorStyle and into DynamicStyleProperty (elastic#53806) Fix suggested value for time_zone in range query (elastic#53841) Clean up generic hooks, use react-use instead (elastic#53822)
…t-types * alerting/created_at-and-updated_at: updatedAt should equal createdAt on creation Move index patterns: src/legacy/core_plugins/data 👉 src/plugins/data (elastic#53794) moved Task Manager server code under "server" directory (elastic#53777) Rename `/api/security/oidc` to `/api/security/oidc/callback`. (elastic#53886) Updating transitive dependencies to use [email protected] (elastic#53899) [Reporting/Tests] consolidate functional test configs (elastic#52671) [Reporting] Correct the docvalue_fields params in the search query Download CSV from Dashboard Panel (elastic#52833) [Test/Newsfeed] Re-enable test and add news item to be filtered (elastic#53905) cleanup server-log action (elastic#53326) [Uptime] Delete uptime eslint rule skip (elastic#50912) [skip-ci] Expression Lifecycle Docs (elastic#51494) [Endpoint] add react router to endpoint app (elastic#53808) [SIEM][Detection Engine] Silence 409 errors on signal creation (elastic#53859) [Maps] get max_result_window and max_inner_result_window from index settings (elastic#53500) [ML] New Platform server shim: update analytics routes to use new platform router (elastic#53521) fixes typo on engine detection page (elastic#53877) [Maps] push mapbox value extraction from VectorStyle and into DynamicStyleProperty (elastic#53806) Fix suggested value for time_zone in range query (elastic#53841) Clean up generic hooks, use react-use instead (elastic#53822)
Summary
Most of our rules run every five minutes on the last six minutes of data. If a signal-generating event occurs in that overlapping minute, we will attempt to create the same signal twice, and receive a 409 response from elasticsearch on the second attempt.
In my exploration of this issue, every 409 response I encountered was due to this behavior. While that does not eliminate the possibility that other circumstances could generate a 409, I was unable to reproduce/imagine such a circumstance, and I think that we can safely hide these errors from the user as it is the expected behavior.
While there's also the option to use bulk
index
to upsert these signals and create new versions of the same signal, the only value I see in doing that would be the elimination of said errors: the duplicated signal should have no additional information, and the upsert logic has performance (and other) concerns as well.Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.[ ] This was checked for cross-browser compatibility, including a check against IE11[ ] Documentation was added for features that require explanation or tutorials[ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers