feat(alarm): add support for creating nested composite alarms #642
Conversation
lib/common/alarm/AlarmFactory.ts
Outdated
| * @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?
lib/facade/MonitoringFacade.ts
Outdated
| @@ -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.
There was a problem hiding this comment.
Okay, I've tried a few different approaches ( createdAlarm should still return AlarmWithAnnotation to maintain backward compatibility, as I'm not convinced changing that is a good solution):
- I tried to move the new methods on creating composite alarms into
Monitoringand then created a facade method to filter them. While this approach works, it feels a bit awkward. It seems to add complexity without significantly improving readability or anything really. - I also tried creating a new subclass of
Monitoringto house the new methods. This is more readable than the first option. However, as you pointed out, it results in an empty, purposely built segment that solely captures the composite alarm and doesn't render any widgets.
So, my thought is that creating a separate Monitoring is overkill in this case.
|
Aww, I accidentally reverted the origin, which caused the PR to be closed. need to fix that. |
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