feat(alarm): add support for creating nested composite alarms #642
Conversation
| * @param props Customization options for the composite alarm | ||
| * @returns Newly created composite alarm | ||
| */ | ||
| addCompositeAlarmFromAlarmBases( |
There was a problem hiding this comment.
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. :\
There was a problem hiding this comment.
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, likeaddAlarmdoes, 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?
| const alarmBases = [ | ||
| ...alarms.map((alarm) => alarm.alarm), | ||
| ...compositeAlarms, | ||
| ] as AlarmBase[]; |
There was a problem hiding this comment.
Is this cast necessary? I thought CompositeAlarm was already an AlarmBase.
There was a problem hiding this comment.
yeah, it's not needed, it was mostly just a visual cue. I can remove it.
| @@ -178,7 +192,7 @@ export class MonitoringFacade extends MonitoringScope { | |||
| | IDashboardSegment | |||
| | IDynamicDashboardSegment | |||
| )[]; | |||
| protected readonly createdComposites: CompositeAlarm[]; | |||
| protected readonly createdComposites: CompositeAlarmWithMetadata[]; | |||
There was a problem hiding this comment.
Spitballing here, but I wonder if it'd be possible to have a dummy Monitoring segment that just acts as a composite alarm factory that the facade delegates to?
Things like createdAlarms() would "just work" by returning everything, although things like createdCompositeAlarms() would need to filter first. (That said, I wonder if createdAlarms() should return created composite alarms too? It may be more intuitive to do so.)
Not sure how weird it is to have a segment that doesn't actually do any widget things though.
There was a problem hiding this comment.
I'll comment back on the dummy segment, I need to think about whether it makes sense.
To answer your question about whether createdAlarms() should return the created composite alarm: I don't think that's feasible if we want to maintain backward compatibility. Changing the return value of the method could break some downstream callers, especially if they're casting the return value for whatever reason. I'm inclined to avoid opening that can of worms for now.
Fixes #237
Fixes #618
Fixes #640
This commit adds support for creating nested composite alarms (i.e., including composite alarms within another composite alarm when using
createCompositeAlarmUsingTag).To avoid breaking the existing API or changing the current logic for how alarms are looked up during composite alarm creation, the feature is gated behind a flag, which is disabled by default.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license