From 4c8b32c442b2b371c997f1798a7535fdfc2716f8 Mon Sep 17 00:00:00 2001 From: Faisal Kanout Date: Wed, 15 Jan 2025 17:12:37 +0300 Subject: [PATCH] [OBX-UX-MGMT][ALERTING] Fix Metric and Custom Threshold rules time range extension when the rule execution fails (#202142) ## Summary It fixes #191179 It fixes #202493 By adding guard rail to limit the time range to "no more than" 3 times the execution window. As proposed [here](https://github.com/elastic/kibana/issues/191179#issuecomment-2501435071) While ensuring the rule will still be able to detect the missing groups. ### What has been done: - Fix the unlimited extension of the time range when the rule fails for both rules, the Metric and Custom Threshold - Tests have been added to cover the guardrail ### Hot to test the PR: - Create Metric and Threshold rules that fire alerts with a groupBy options, e.g., with `host.name` - From the data source/generation tool, try to remove groups, and the rule should be able to detect the missing groups. - Try to make the rule fail e.g., a typo in the rules' executors. - The rule time range would be extended up to 3x during the execution and then back to the defined time range. ### Why not ignore the `lastPeriodEnd` without using a guardrail This feature is implemented for a reason, which is to catch the missing groups and keep the rule on-sync as much as possible. Also, the Security team has an appetite to introduce a similar feature. Instead of removing it completely, we keep it with a guardrail to protect the cluster resources. And we can go back to remove it or update it anytime in the future. Screenshot 2024-11-28 at 11 57 13 Screenshot 2024-11-28 at 11 57 06 (cherry picked from commit e7f0771be7356b1c05df6992da75a9298642fe90) --- .../lib/create_timerange.test.ts | 34 +++++++++++++++++++ .../metric_threshold/lib/create_timerange.ts | 21 ++++++++---- .../lib/create_timerange.test.ts | 22 ++++++++---- .../custom_threshold/lib/create_timerange.ts | 10 ++++-- 4 files changed, 71 insertions(+), 16 deletions(-) diff --git a/x-pack/solutions/observability/plugins/infra/server/lib/alerting/metric_threshold/lib/create_timerange.test.ts b/x-pack/solutions/observability/plugins/infra/server/lib/alerting/metric_threshold/lib/create_timerange.test.ts index 6906800bf4c9d..9cd3b741a7773 100644 --- a/x-pack/solutions/observability/plugins/infra/server/lib/alerting/metric_threshold/lib/create_timerange.test.ts +++ b/x-pack/solutions/observability/plugins/infra/server/lib/alerting/metric_threshold/lib/create_timerange.test.ts @@ -129,4 +129,38 @@ describe('createTimerange(interval, aggType, timeframe)', () => { }); }); }); + describe('With lastPeriodEnd', () => { + const end = moment(); + const timeframe = { + start: end.clone().valueOf(), + end: end.valueOf(), + }; + it('should return a second range for last 1 second when the lastPeriodEnd is not less than the timeframe start', () => { + const subject = createTimerange( + 1000, + Aggregators.COUNT, + timeframe, + end.clone().add(2, 'seconds').valueOf() + ); + expect(subject.end - subject.start).toEqual(1000); + }); + it('should return a 3 minutes range for last 1 minute when the lastPeriodEnd is not more than 3x the execution window (maxAllowedLookBack)', () => { + const subject = createTimerange( + 60000, + Aggregators.COUNT, + timeframe, + end.clone().subtract(2, 'minute').valueOf() + ); + expect(subject.end - subject.start).toEqual(60000 * 3); + }); + it('should return a minute range for last 1 minute when the lastPeriodEnd is more than 3x the execution window (maxAllowedLookBack)', () => { + const subject = createTimerange( + 60000, + Aggregators.COUNT, + timeframe, + end.clone().subtract(4, 'minute').valueOf() + ); + expect(subject.end - subject.start).toEqual(60000); + }); + }); }); diff --git a/x-pack/solutions/observability/plugins/infra/server/lib/alerting/metric_threshold/lib/create_timerange.ts b/x-pack/solutions/observability/plugins/infra/server/lib/alerting/metric_threshold/lib/create_timerange.ts index 035f8dcdb88a9..fb2bf7e3fa716 100644 --- a/x-pack/solutions/observability/plugins/infra/server/lib/alerting/metric_threshold/lib/create_timerange.ts +++ b/x-pack/solutions/observability/plugins/infra/server/lib/alerting/metric_threshold/lib/create_timerange.ts @@ -14,18 +14,27 @@ export const createTimerange = ( timeframe?: { end: number; start?: number }, lastPeriodEnd?: number ) => { - const to = moment(timeframe ? timeframe.end : Date.now()).valueOf(); + const end = moment(timeframe && timeframe.end ? timeframe.end : Date.now()).valueOf(); + const start = moment(timeframe && timeframe.start ? timeframe.start : end).valueOf(); // Rate aggregations need 5 buckets worth of data const minimumBuckets = aggType === Aggregators.RATE ? 2 : 1; - const calculatedFrom = lastPeriodEnd ? lastPeriodEnd - interval : to - interval * minimumBuckets; - // Use either the timeframe.start when the start is less then calculatedFrom - // OR use the calculatedFrom + interval = interval * minimumBuckets; + + let calculatedFrom = end - interval; + + if (lastPeriodEnd && lastPeriodEnd - interval < start) { + const maxAllowedLookBack = moment(start).subtract(3 * interval, 'ms'); + // Calculate the maximum allowable look-back time (3 intervals before the current 'from' time). + if (moment(lastPeriodEnd).isAfter(maxAllowedLookBack)) { + // Ensure lastPeriodEnd is within the allowable look-back range. + calculatedFrom = lastPeriodEnd - interval; + } + } const from = timeframe && timeframe.start && timeframe.start <= calculatedFrom ? timeframe.start : calculatedFrom; - - return { start: from, end: to }; + return { start: from, end }; }; diff --git a/x-pack/solutions/observability/plugins/observability/server/lib/rules/custom_threshold/lib/create_timerange.test.ts b/x-pack/solutions/observability/plugins/observability/server/lib/rules/custom_threshold/lib/create_timerange.test.ts index c82f07594bf83..b73c9771d7e99 100644 --- a/x-pack/solutions/observability/plugins/observability/server/lib/rules/custom_threshold/lib/create_timerange.test.ts +++ b/x-pack/solutions/observability/plugins/observability/server/lib/rules/custom_threshold/lib/create_timerange.test.ts @@ -38,17 +38,25 @@ describe('createTimerange(interval, aggType, timeframe)', () => { }); }); describe('With lastPeriodEnd', () => { - it('should return a minute and 1 second range for last 1 second when the lastPeriodEnd is less than the timeframe start', () => { + it('should return a second range for last 1 second when the lastPeriodEnd is not less than the timeframe start', () => { + const subject = createTimerange(1000, timeframe, end.clone().add(2, 'seconds').valueOf()); + expect(subject.end - subject.start).toEqual(1000); + }); + it('should return a 3 minutes range for last 1 minute when the lastPeriodEnd is not more than 3x the execution window (maxAllowedLookBack)', () => { const subject = createTimerange( - 1000, + 60000, timeframe, - end.clone().subtract(1, 'minutes').valueOf() + end.clone().subtract(2, 'minute').valueOf() ); - expect(subject.end - subject.start).toEqual(61000); + expect(subject.end - subject.start).toEqual(60000 * 3); }); - it('should return a second range for last 1 second when the lastPeriodEnd is not less than the timeframe start', () => { - const subject = createTimerange(1000, timeframe, end.clone().add(2, 'seconds').valueOf()); - expect(subject.end - subject.start).toEqual(1000); + it('should return a minute range for last 1 minute when the lastPeriodEnd is more than 3x the execution window (maxAllowedLookBack)', () => { + const subject = createTimerange( + 60000, + timeframe, + end.clone().subtract(4, 'minute').valueOf() + ); + expect(subject.end - subject.start).toEqual(60000); }); }); }); diff --git a/x-pack/solutions/observability/plugins/observability/server/lib/rules/custom_threshold/lib/create_timerange.ts b/x-pack/solutions/observability/plugins/observability/server/lib/rules/custom_threshold/lib/create_timerange.ts index 0d82b5df637b8..760d294298c5c 100644 --- a/x-pack/solutions/observability/plugins/observability/server/lib/rules/custom_threshold/lib/create_timerange.ts +++ b/x-pack/solutions/observability/plugins/observability/server/lib/rules/custom_threshold/lib/create_timerange.ts @@ -19,11 +19,15 @@ export const createTimerange = ( const minimumBuckets = isRateAgg ? 2 : 1; interval = interval * minimumBuckets; - start = start - interval; + start = end - interval; - // Use lastPeriodEnd - interval when it's less than start if (lastPeriodEnd && lastPeriodEnd - interval < start) { - start = lastPeriodEnd - interval; + const maxAllowedLookBack = moment(start).subtract(3 * interval, 'ms'); + // Calculate the maximum allowable look-back time (3 intervals before the current 'start' time). + if (moment(lastPeriodEnd).isAfter(maxAllowedLookBack)) { + // Ensure lastPeriodEnd is within the allowable look-back range. + start = lastPeriodEnd - interval; + } } return { start, end };