Skip to content

feat(alarm): add support for creating nested composite alarms #642

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(alarm): add support for creating nested composite alarms #642

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
162 changes: 154 additions & 8 deletions API.md
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

50 changes: 48 additions & 2 deletions lib/common/alarm/AlarmFactory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -821,9 +821,37 @@ export class AlarmFactory {
};
}

/**
* Creates a composite alarm from a collection of alarms and composite alarms.
*
* @param alarms Array of individual alarms to be composed
* @param props Customization options for the composite alarm
* @param compositeAlarms Optional array of composite alarms to be composed together with regular alarms
* @returns Newly created composite alarm
*/
addCompositeAlarm(
alarms: AlarmWithAnnotation[],
props: AddCompositeAlarmProps,
compositeAlarms: CompositeAlarm[] = [],
): CompositeAlarm {
const alarmBases = [
...alarms.map((alarm) => alarm.alarm),
...compositeAlarms,
] as AlarmBase[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this cast necessary? I thought CompositeAlarm was already an AlarmBase.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, it's not needed, it was mostly just a visual cue. I can remove it.

return this.addCompositeAlarmFromAlarmBases(alarmBases, props);
}

/**
* Creates a composite alarm from a collection of alarm base objects.
* This allows creation of composite alarms that include both metric alarms and other composite alarms.
*
* @param alarms Array of AlarmBase objects (can include both Alarm and CompositeAlarm)
* @param props Customization options for the composite alarm
* @returns Newly created composite alarm
*/
addCompositeAlarmFromAlarmBases(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the new method should also return an AlarmWithAnnotation like addAlarm does for better consistency?

Also not a fan of this method name, but nothing better immediately comes to mind. :\

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm not a fan of the names either. I wish I could just overload the method, but that's not an option here.

I wonder if the new method should also return an AlarmWithAnnotation, like addAlarm does, for better consistency?

I'm not sure I follow. The existing method addCompositeAlarm() already returns a CompositeAlarm. Wouldn't we want the new method to align with that?

alarms: AlarmBase[],
props: AddCompositeAlarmProps,
): CompositeAlarm {
const actionsEnabled = this.determineActionsEnabled(
props?.actionsEnabled,
Expand All @@ -840,7 +868,10 @@ export class AlarmFactory {
props?.documentationLink,
);
const dedupeString = this.alarmNamingStrategy.getDedupeString(namingInput);
const alarmRule = this.determineCompositeAlarmRule(alarms, props);
const alarmRule = this.determineCompositeAlarmRuleFromAlarmBases(
alarms,
props,
);

const alarm = new CompositeAlarm(this.alarmScope, alarmName, {
compositeAlarmName: alarmName,
Expand All @@ -867,8 +898,23 @@ export class AlarmFactory {
protected determineCompositeAlarmRule(
alarms: AlarmWithAnnotation[],
props: AddCompositeAlarmProps,
compositeAlarms: CompositeAlarm[] = [],
): IAlarmRule {
const alarmRules = alarms.map((alarm) => alarm.alarmRuleWhenAlarming);
const alarmBases = [
...alarms.map((alarm) => alarm.alarm),
...compositeAlarms,
] as AlarmBase[];
return this.determineCompositeAlarmRuleFromAlarmBases(alarmBases, props);
}

protected determineCompositeAlarmRuleFromAlarmBases(
alarms: AlarmBase[],
props: AddCompositeAlarmProps,
): IAlarmRule {
const alarmRules = alarms.map((alarm) =>
AlarmRule.fromAlarm(alarm, AlarmState.ALARM),
);

const operator = props.compositeOperator ?? CompositeAlarmOperator.OR;
switch (operator) {
case CompositeAlarmOperator.AND:
Expand Down
Loading