[SIEM] [Detections] Fixes faulty circuit breaker #71999
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dhurley14
commented
Jul 16, 2020
| @@ -118,7 +107,6 @@ export const searchAfterAndBulkCreate = async ({ | |||
| interval, | |||
| buildRuleMessage, | |||
| }); | |||
| const useSortIds = totalToFromTuples.length <= 1; | |||
There was a problem hiding this comment.
This line was the culprit of the bug that caused this revert #71956 .
useSortIds was only > 1 when there was a gap, which would mean boolean useSortIds was false. This bug showed up now because I modified the count we used to determine when we hit max signals from the number of search results to the number of bulk create results (signals indexed) with this pr #71768. When I did that, one of the circuit breakers for jumping out of the loop relied on checking the next sort id would fail because useSortIds was false. I’ve removed the variable and simplified the logic for breaking out of the while loop. That variable was leftover from a different implementation I attempted while doing the gap detection fixes and I missed deleting it and updating the logic.
Again, this bug stayed hidden for so long after all the gap detection code was added because I never changed the count used to determine when we hit max signals until this commit went in 56de45d right before FF.
dhurley14
added
release_note:skip
dhurley14
force-pushed
the
never-ending-loop
branch
2 times, most recently
from
91af03c
to
01b450c
Compare
…ementing gap detection mitigation code. This only showed up because I modified the count variable used to determine when we hit maxSignals from utilizing the searchResult hits length to using the count of bulk created items (signals indexed) in this commit 56de45d
…function, updates log statements
dhurley14
force-pushed
the
never-ending-loop
branch
from
01b450c
to
2f0f43d
Compare
dhurley14
added
the
bug
💚 Build SucceededBuild metrics
History
To update your PR or re-run it, just comment with: |
FrankHassanabad
approved these changes
Jul 20, 2020
dhurley14
added a commit
to dhurley14/kibana
that referenced
this pull request
Jul 20, 2020
* removes useSortIds which was leftover from a previous attempt at implementing gap detection mitigation code. This only showed up because I modified the count variable used to determine when we hit maxSignals from utilizing the searchResult hits length to using the count of bulk created items (signals indexed) in this commit 56de45d * removes logs and fixes if statement ordering * adds tests, increases code coverage for search after and bulk create function, updates log statements * update tests after rebase onto master * clean up if statements * fix test data * merge conflicts are hard
dhurley14
added a commit
to dhurley14/kibana
that referenced
this pull request
Jul 20, 2020
* removes useSortIds which was leftover from a previous attempt at implementing gap detection mitigation code. This only showed up because I modified the count variable used to determine when we hit maxSignals from utilizing the searchResult hits length to using the count of bulk created items (signals indexed) in this commit 56de45d * removes logs and fixes if statement ordering * adds tests, increases code coverage for search after and bulk create function, updates log statements * update tests after rebase onto master * clean up if statements * fix test data * merge conflicts are hard
dhurley14
added a commit
that referenced
this pull request
Jul 20, 2020
* removes useSortIds which was leftover from a previous attempt at implementing gap detection mitigation code. This only showed up because I modified the count variable used to determine when we hit maxSignals from utilizing the searchResult hits length to using the count of bulk created items (signals indexed) in this commit 56de45d * removes logs and fixes if statement ordering * adds tests, increases code coverage for search after and bulk create function, updates log statements * update tests after rebase onto master * clean up if statements * fix test data * merge conflicts are hard
dhurley14
added a commit
that referenced
this pull request
Jul 20, 2020
* removes useSortIds which was leftover from a previous attempt at implementing gap detection mitigation code. This only showed up because I modified the count variable used to determine when we hit maxSignals from utilizing the searchResult hits length to using the count of bulk created items (signals indexed) in this commit 56de45d * removes logs and fixes if statement ordering * adds tests, increases code coverage for search after and bulk create function, updates log statements * update tests after rebase onto master * clean up if statements * fix test data * merge conflicts are hard
MindyRS
added
the
Team: SecuritySolution
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
One of the circuit breakers used for exiting out of the search after and bulk create function revealed itself as broken after introducing #71768 (commit 56de45d in master, reverted here aee2a12 with this pr #71956). After changing how we count the number of signals created to ensure we don't go beyond
maxSignalsin #71768, the circuit breaker started to get triggered when a gap appeared when it never was previously during the development of gap detection mitigation (#68339) because of how we were counting up tomaxSignals. This resulted in a never ending loop of searching and bulk creating the same signals when there was a gap. The boolean variable (useSortIds) was leftover during development of the gap detection mitigation code and should not have been a part of the boolean logic for the circuit breaker. I missed deleting it when I opened the gap detection mitigation pr.This PR removes that boolean, adds additional logic to ensure we are only calling bulk create when we have candidate signals, and adds more comments and some log statements.
TL;DR - faulty circuit breaker created in #68339 started to show up after merging #71768.
Checklist
Delete any items that are not applicable to this PR.
For maintainers