Skip to content

Commit

Permalink
[OBX-UX-MGMT][ALERTING] Fix Metric and Custom Threshold rules time ra…
Browse files Browse the repository at this point in the history
…nge 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](#191179 (comment))
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.

<img width="1433" alt="Screenshot 2024-11-28 at 11 57 13"
src="https://github.com/user-attachments/assets/72a04b25-c7c6-4261-8fea-9fa9a1cce3a6">
<img width="1427" alt="Screenshot 2024-11-28 at 11 57 06"
src="https://github.com/user-attachments/assets/e0ee8605-fe27-4f23-bf42-4b6a9fe76e2b">
  • Loading branch information
fkanout authored Jan 15, 2025
1 parent ac76690 commit e7f0771
Show file tree
Hide file tree
Showing 4 changed files with 71 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
};
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Expand Down

0 comments on commit e7f0771

Please sign in to comment.