Add support for correlation periods for alerts #65

sfc-gh-nlele · 2023-06-09T12:51:33Z

This PR adds support for specifying custom correlation periods for individual alerts, extending having a single correlation period for all of the alerts.

This correlation period will be specified in the alert object as a negative number indicating the duration prior to the current time, for which the alerts would be correlated. If it's absent, the default correlation period set for the alert processor will be used.

E.g.

.
.
.
, 'q9u9z3b' AS query_id
  , 'USING CRON 0 0 * * * UTC' AS schedule
  , '1d' AS lookback
  , ARRAY_CONSTRUCT(
      OBJECT_CONSTRUCT(
        'type', 'ef-slack',
        'message',  ' test',
        'channel', 'test'
      )
    ) AS handlers
   , -180 as correlation_period
.
.
.

Test:

Correlation period = -10

sfc-gh-pkommini · 2023-06-20T17:14:01Z

@sfc-gh-nlele can correlation period be +ve? What would that mean? Do we check for that?

sfc-gh-pkommini

lgtm. Just a question to cover my gap in knowledge. (re: -ve vs +ve correlation_period)

sfc-gh-nlele · 2023-06-21T16:27:11Z

@sfc-gh-pkommini positive correlation periods can be entered, but they wouldn't really be fruitful I think. The two scenarios I can think of with them are:

There are, e.g., three un-correlated alerts during a run, and the alert that was most recent gets processed first. That alert will get a new correlation ID and the rest of the 2 alerts will then receive this same ID, if they're in this positive window.
There are no alerts in the future, thus every alert gets its own correlation ID.

sfc-gh-pkommini · 2023-06-21T19:47:07Z

@sfc-gh-pkommini positive correlation periods can be entered, but they wouldn't really be fruitful I think. The two scenarios I can think of with them are:

There are, e.g., three un-correlated alerts during a run, and the alert that was most recent gets processed first. That alert will get a new correlation ID and the rest of the 2 alerts will then receive this same ID, if they're in this positive window.

There are no alerts in the future, thus every alert gets its own correlation ID.

So CORRELATION_PERIOD being negative means look back in last n minutes if there is an alert with the same fingerprint then reuse tthat existing correlation ID. It being positive means look forward if there is any alert with the fingerprint. Hence when it is positive since the current batch of alerts which have same fingerprint within the window will get correlated but won't get correlated with past batches making it not as effective as when used with a negative value.

Makes sense to me. Please merge. This was not meant to be a blocker.

sfc-gh-pkommini · 2023-06-21T19:47:16Z

@sfc-gh-pkommini positive correlation periods can be entered, but they wouldn't really be fruitful I think. The two scenarios I can think of with them are:

There are, e.g., three un-correlated alerts during a run, and the alert that was most recent gets processed first. That alert will get a new correlation ID and the rest of the 2 alerts will then receive this same ID, if they're in this positive window.

There are no alerts in the future, thus every alert gets its own correlation ID.

So CORRELATION_PERIOD being negative means look back in last n minutes if there is an alert with the same fingerprint then reuse tthat existing correlation ID. It being positive means look forward if there is any alert with the fingerprint. Hence when it is positive, only alerts within the current batch which have same fingerprint will get correlated but won't get correlated with past batches making it not as effective or useful.

Makes sense to me. Please merge. This was not meant to be a blocker.

sfc-gh-afedorov · 2023-06-23T15:47:18Z

Instead of -180 as the value, could we have it be '-180s', '-180m', or '-180d' to specify second, minutes, or days?

sfc-gh-nlele · 2023-06-26T12:30:02Z

@sfc-gh-afedorov Sure 👍, but maybe positive values could indicate looking back (the usual case I think, it seems implied that we usually look back), and negative values could indicate looking forward? Either way works!

Altho it seems like we wouldn't be able to use something like this:

  AND event_time > ? - INTERVAL COALESCE(alert:CORRELATION_PERIOD, $${CORRELATION_PERIOD_MINUTES})

and will have to use CASE and manually parse the periods, since INTERVAL doesn't seem to support expressions within it.

sfc-gh-afedorov · 2023-06-29T16:27:47Z

procedures_js/alert_processor.js

-  AND event_time > DATEADD(minutes, $${CORRELATION_PERIOD_MINUTES}, ?)
+  AND event_time >  
+      CASE 
+        WHEN REGEXP_SUBSTR(LOWER(COALESCE(alert:CORRELATION_PERIOD, $${CORRELATION_PERIOD_MINUTES})), '[a-z]') = 's' THEN DATEADD(seconds, - TO_NUMBER(REGEXP_SUBSTR(COALESCE(alert:CORRELATION_PERIOD, $${CORRELATION_PERIOD_MINUTES})), '\\d+'),?)


this seems like a good place to use the alternative syntax for CASE

Oo right! This will be better 👍

sfc-gh-afedorov · 2023-06-29T16:28:06Z

procedures_js/alert_processor.js

-WHERE alert:EVENT_TIME > DATEADD(minutes, $${CORRELATION_PERIOD_MINUTES}, ?)
+WHERE alert:EVENT_TIME >
+      CASE 
+        WHEN REGEXP_SUBSTR(LOWER(COALESCE(alert:CORRELATION_PERIOD, $${CORRELATION_PERIOD_MINUTES})), '[a-z]') = 's' THEN DATEADD(seconds, - TO_NUMBER(REGEXP_SUBSTR(COALESCE(alert:CORRELATION_PERIOD, $${CORRELATION_PERIOD_MINUTES})), '\\d+'),?)


sfc-gh-nlele · 2023-06-30T15:12:45Z

procedures_js/alert_queries_runner.js

@@ -34,6 +34,21 @@ function fillArray(value, len) {
  return arr
 }

+function ifColumnExists(column_name) {


@sfc-gh-afedorov would something like this be okay for backward compatibility?

Checking if CORRELATION_PERIOD exists in the rule definition, and then adding it to the insert statement body if it does.

sfc-gh-afedorov · 2023-07-12T04:58:49Z

procedures_js/alert_queries_runner.js

@@ -62,6 +77,7 @@ SELECT '$${RUN_ID}' run_id
      'EVENT_DATA', IFNULL(EVENT_DATA::VARIANT, PARSE_JSON('null')),
      'SEVERITY', IFNULL(SEVERITY::VARIANT, PARSE_JSON('null')),
      'HANDLERS', IFNULL(OBJECT_CONSTRUCT(*):HANDLERS::VARIANT, PARSE_JSON('null'))
+      $${ifColumnExists('CORRELATION_PERIOD')}


it seems better to repeat the pattern above --

'CORRELATION_PERIOD', IFNULL(OBJECT_CONSTRUCT(*):CORRELATION_PERIOD::VARIANT, PARSE_JSON('null'))

or break it into a helper that applies this pattern like

'CORRELATION_PERIOD', $${defaultNullReference('CORRELATION_PERIOD::VARIANT')}

sfc-gh-afedorov · 2023-07-12T05:00:20Z

procedures_js/alert_processor.js

+SET correlation_id = COALESCE(:1, UUID_STRING())
+WHERE alert:EVENT_TIME >
+      CASE REGEXP_SUBSTR(LOWER(COALESCE(alert:CORRELATION_PERIOD, '$${CORRELATION_PERIOD_MINUTES}')), '[a-z]')
+        WHEN 's' THEN DATEADD(seconds, - TO_NUMBER(REGEXP_SUBSTR(COALESCE(alert:CORRELATION_PERIOD, '$${CORRELATION_PERIOD_MINUTES}'), '\\\\d+')), :2)


since these are repeated eight times, maybe another approach is better, e.g. putting the number of seconds into JS and then the SQL just uses that variable

sfc-gh-afedorov

two changes pls and then feel free to merge or punt back for re-review

sfc-gh-nlele · 2023-07-17T07:39:43Z

Tested the latest changes.

sfc-gh-nlele added 3 commits June 9, 2023 16:59

Initial commit

3d70fe6

Update warehouse size

25d3763

Update correlation period default

34f993c

sfc-gh-nlele changed the title ~~Add support for correlation periods in alerts~~ Add support for correlation periods for alerts Jun 9, 2023

sfc-gh-nlele added 2 commits June 12, 2023 18:12

Update tables.tf

5ca9ba6

Update alert_processor.js

9b3c24a

sfc-gh-nlele requested a review from sfc-gh-pkommini June 13, 2023 13:34

sfc-gh-pkommini approved these changes Jun 20, 2023

View reviewed changes

sfc-gh-afedorov self-requested a review June 23, 2023 15:46

Update alert_processor.js

55c5300

sfc-gh-nlele added 6 commits June 26, 2023 18:53

Update alert_processor.js

60ce877

Use case for parsing correlation period

28f90cf

Remove unneeded trim()

466f821

Update alert_processor.js

3a3cd16

Update alert_processor.js

a083cc0

Update alert_processor.js

89fbd80

sfc-gh-afedorov reviewed Jun 29, 2023

View reviewed changes

sfc-gh-nlele added 7 commits June 29, 2023 22:50

Update alert_processor.js

c955301

Update alert_processor.js

7c3bd8a

Update alert_queries_runner.js

49098f2

Update alert_queries_runner.js

5a320bb

Update procedures.tf

7ba663f

Update alert_queries_runner.js

16c1c60

Update alert_queries_runner.js

76f4891

Update procedures.tf

4bcc747

sfc-gh-nlele commented Jun 30, 2023

View reviewed changes

sfc-gh-afedorov reviewed Jul 12, 2023

View reviewed changes

sfc-gh-afedorov approved these changes Jul 12, 2023

View reviewed changes

sfc-gh-nlele added 6 commits July 12, 2023 17:51

Use UDF to avoid repetition in statements

59edcbc

Update procedures.tf

203c678

Add missing comma

e14b539

Fix regex

47e2004

Use nullRef function for handlers

ca71a95

Improve formatting

9196468

sfc-gh-nlele requested a review from sfc-gh-afedorov July 13, 2023 10:04

Update functions.tf

bdbba6b

sfc-gh-nlele merged commit cc05344 into main Jul 17, 2023

sfc-gh-nlele deleted the add_custom_correlation_period branch July 17, 2023 07:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for correlation periods for alerts #65

Add support for correlation periods for alerts #65

sfc-gh-nlele commented Jun 9, 2023 •

edited

Loading

sfc-gh-pkommini commented Jun 20, 2023

sfc-gh-pkommini left a comment

sfc-gh-nlele commented Jun 21, 2023

sfc-gh-pkommini commented Jun 21, 2023

sfc-gh-pkommini commented Jun 21, 2023 •

edited

Loading

sfc-gh-afedorov commented Jun 23, 2023 •

edited

Loading

sfc-gh-nlele commented Jun 26, 2023 •

edited

Loading

sfc-gh-afedorov Jun 29, 2023

sfc-gh-nlele Jun 29, 2023

sfc-gh-afedorov Jun 29, 2023

sfc-gh-nlele Jun 30, 2023

sfc-gh-afedorov Jul 12, 2023

sfc-gh-afedorov Jul 12, 2023

sfc-gh-afedorov left a comment

sfc-gh-nlele commented Jul 17, 2023

Add support for correlation periods for alerts #65

Add support for correlation periods for alerts #65

Conversation

sfc-gh-nlele commented Jun 9, 2023 • edited Loading

sfc-gh-pkommini commented Jun 20, 2023

sfc-gh-pkommini left a comment

Choose a reason for hiding this comment

sfc-gh-nlele commented Jun 21, 2023

sfc-gh-pkommini commented Jun 21, 2023

sfc-gh-pkommini commented Jun 21, 2023 • edited Loading

sfc-gh-afedorov commented Jun 23, 2023 • edited Loading

sfc-gh-nlele commented Jun 26, 2023 • edited Loading

sfc-gh-afedorov Jun 29, 2023

Choose a reason for hiding this comment

sfc-gh-nlele Jun 29, 2023

Choose a reason for hiding this comment

sfc-gh-afedorov Jun 29, 2023

Choose a reason for hiding this comment

sfc-gh-nlele Jun 30, 2023

Choose a reason for hiding this comment

sfc-gh-afedorov Jul 12, 2023

Choose a reason for hiding this comment

sfc-gh-afedorov Jul 12, 2023

Choose a reason for hiding this comment

sfc-gh-afedorov left a comment

Choose a reason for hiding this comment

sfc-gh-nlele commented Jul 17, 2023

sfc-gh-nlele commented Jun 9, 2023 •

edited

Loading

sfc-gh-pkommini commented Jun 21, 2023 •

edited

Loading

sfc-gh-afedorov commented Jun 23, 2023 •

edited

Loading

sfc-gh-nlele commented Jun 26, 2023 •

edited

Loading