-
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
[Security Solutions] Fixes the newer notification system throttle resets and enabling immediate execution on first detection of a signal #114214
[Security Solutions] Fixes the newer notification system throttle resets and enabling immediate execution on first detection of a signal #114214
Conversation
…ndition and fixes bug where it did not fire immediately
@elasticmachine merge upstream |
merge conflict between base and head |
@elasticmachine merge upstream |
...ion/server/lib/detection_engine/notifications/schedule_throttle_notification_actions.test.ts
Outdated
Show resolved
Hide resolved
esClient: ElasticsearchClient; | ||
alertInstance: AlertInstance; | ||
notificationRuleParams: NotificationRuleTypeParams; | ||
signals: unknown[]; |
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.
Does SignalSearchResponse
work here as a type?
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.
They're not exactly the same type from looking at things. It's legacy and confusing to have the unknown
here. We might be able to change it later but I'm not going to for this PR for right now.
...solution/server/lib/detection_engine/notifications/schedule_throttle_notification_actions.ts
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/lib/detection_engine/notifications/utils.test.ts
Show resolved
Hide resolved
It's looking great! I only tested with a custom query rule and it wasn't exhaustive. But did verify the following behaviors:
Some test data for sanity check:Run 0 (activation) - 🔔 Notified - `NEWWWWW Rule test new generated 200 alerts - 2021-10-12T04:57:13.909Z` (says 200 because I had disabled/enabled the rule to test initial run notification, also note that the clock between my logger and what's showing up on slack notification differs) Run 1 - No notification - Run 2 - No notification - Run 3 - No notification - [...] Run 7 - No notification - Run 8 - No notification - [...] Run 13 - 🔔 Notified - I am a bit confused about that second notification up there that notifies the user of Went ahead and did another round this time noting the logs and rule running every 10 minutes:Run 0 (activation) - 🔔 Notified - `Testing total signals count. Rule test new generated 200 alerts - 2021-10-12T06:12:41.325Z` (says 200 because I had disabled/enabled the rule to test initial run notification).
Run 1 - No notification -
Run 2 - No notification -
Run 3 - No notification -
Run 4 - No notification -
Run 5 - No notification -
Run 6 - No notification -
Run 7 - 🔔 Notified -
I'm thinking maybe it's in the dedupe? I'll take a closer look tomorrow. |
@elasticmachine merge upstream |
...solution/server/lib/detection_engine/notifications/schedule_throttle_notification_actions.ts
Show resolved
Hide resolved
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.
Just adding a few comments that I forgot to submit the other day; I can do another pass later if necessary.
...solution/server/lib/detection_engine/notifications/schedule_throttle_notification_actions.ts
Outdated
Show resolved
Hide resolved
...solution/server/lib/detection_engine/notifications/schedule_throttle_notification_actions.ts
Outdated
Show resolved
Hide resolved
…hat does not break later.
…into fix-throttle-bug
@elasticmachine merge upstream |
@elasticmachine merge upstream |
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.
Reviewed fix. LGTM! Thanks so much 🚀
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/search/session·ts.apis search search session touched time updates when you poll on an searchStandard Out
Stack Trace
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
…ets and enabling immediate execution on first detection of a signal (elastic#114214) ## Summary Fixes: * Resets happening by adding the throttle to the else switches and error catching. We have to call throttle on every rule execution or we will cause a reset. * Fixes a case where we were not firing the signal immediately by pushing down the alerts detected. This can cause a reset or a delay of MTTD. * Adds unit tests for the conditions * Changes some of the logic to clean things up. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
…ets and enabling immediate execution on first detection of a signal (#114214) (#115292) ## Summary Fixes: * Resets happening by adding the throttle to the else switches and error catching. We have to call throttle on every rule execution or we will cause a reset. * Fixes a case where we were not firing the signal immediately by pushing down the alerts detected. This can cause a reset or a delay of MTTD. * Adds unit tests for the conditions * Changes some of the logic to clean things up. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios Co-authored-by: Frank Hassanabad <frank.hassanabad@elastic.co>
…-migrate-away-from-injected-css-js * 'master' of github.com:elastic/kibana: (237 commits) [Uptime] Added uptime query inspector panel (elastic#115170) [Osquery] Add packs (elastic#107345) [App Search] Allow for query parameter to indicate ingestion mechanism for new engines (elastic#115188) [Alerting] Active alerts do not recover after re-enabling a rule (elastic#111671) skip flaky tests. elastic#115308, elastic#115313 [Breaking] Remove deprecated `enabled` settings from plugins. (elastic#113495) skip flaky suite. elastic#107057 skip flaky tests. elastic#89052, elastic#113418, elastic#115304 skip flaky test. elastic#113892 Bump node to 16.11.1 (elastic#110684) [Security Solution] Restores Alerts table local storage persistence and the Remove Column action (elastic#114742) skip flaky suite. elastic#115130 one line remove assert (elastic#115127) Fixes migration bug where I was deleting attributes (elastic#115098) [Security Solutions] Fixes the newer notification system throttle resets and enabling immediate execution on first detection of a signal (elastic#114214) [build] Dockerfile update (elastic#115237) Fixes Cypress flake cypress test (elastic#115270) Disable APM e2e tests log an invalid type for SO (elastic#115175) [Fleet] Don't auto upgrade policies for AUTO_UPDATE packages (elastic#115199) ... # Conflicts: # src/plugins/dashboard/public/application/dashboard_app.tsx # src/plugins/dashboard/public/types.ts # x-pack/plugins/reporting/server/lib/layouts/print_layout.ts
…-link-to-kibana-app * 'master' of github.com:elastic/kibana: (287 commits) [Security Solution][Endpoint] Change `trustedAppByPolicyEnabled` flag to `true` by default (elastic#115264) [APM] generator: support error events and application metrics (elastic#115311) [kibanaUtils] Don't import full `semver` client side (elastic#114986) [RAC] Link inventory alerts to the right inventory view (elastic#113553) [Uptime] Added uptime query inspector panel (elastic#115170) [Osquery] Add packs (elastic#107345) [App Search] Allow for query parameter to indicate ingestion mechanism for new engines (elastic#115188) [Alerting] Active alerts do not recover after re-enabling a rule (elastic#111671) skip flaky tests. elastic#115308, elastic#115313 [Breaking] Remove deprecated `enabled` settings from plugins. (elastic#113495) skip flaky suite. elastic#107057 skip flaky tests. elastic#89052, elastic#113418, elastic#115304 skip flaky test. elastic#113892 Bump node to 16.11.1 (elastic#110684) [Security Solution] Restores Alerts table local storage persistence and the Remove Column action (elastic#114742) skip flaky suite. elastic#115130 one line remove assert (elastic#115127) Fixes migration bug where I was deleting attributes (elastic#115098) [Security Solutions] Fixes the newer notification system throttle resets and enabling immediate execution on first detection of a signal (elastic#114214) [build] Dockerfile update (elastic#115237) ... # Conflicts: # x-pack/plugins/reporting/public/management/__snapshots__/report_listing.test.tsx.snap
Summary
Fixes:
Checklist