-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(cloudwatch): support AT_LEAST wrapper function of comopsite alarm
#36100
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.
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
base: main
Are you sure you want to change the base?
feat(cloudwatch): support AT_LEAST wrapper function of comopsite alarm
#36100
Conversation
go-to-k
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice PR! I left some comments.
| * | ||
| * @param options options for creating a new AlarmRule. | ||
| */ | ||
| public static hasAtLeastAlarm(options: HasAtLeastOptions): IAlarmRule { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not atLeastXxx without has? I think the has methods are generally used to return boolean value.
| if (options.count !== undefined && (options.count < 1 || options.operands.length < options.count | ||
| || !Number.isInteger(options.count))) { | ||
| throw new UnscopedValidationError(`count must be between 1 and alarm length(${options.operands.length}) integer, got ${options.count}`); | ||
| } | ||
|
|
||
| if (options.percentage !== undefined && (options.percentage < 1 | ||
| || 100 < options.percentage || !Number.isInteger(options.percentage))) { | ||
| throw new UnscopedValidationError(`percentage must be between 1 and 100, got ${options.percentage}`); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to check whether it is a token using Token.isUnresolved.
packages/aws-cdk-lib/aws-cloudwatch/test/composite-alarm.test.ts
Outdated
Show resolved
Hide resolved
| export interface HasAtLeastOptions { | ||
|
|
||
| /** | ||
| * function to wrap provided AlarmRule in AT_LEAST expression. | ||
| */ | ||
| readonly operands: IAlarm[]; | ||
|
|
||
| /** | ||
| * minimum number of specified alarms | ||
| * | ||
| * Units: Count | ||
| * | ||
| * @default - Exactly one of `count`, `percentage` is required. | ||
| */ | ||
| readonly count?: number; | ||
| /** | ||
| * minimum percentage of specified alarms | ||
| * | ||
| * Units: Percentage | ||
| * | ||
| * @default - Exactly one of `count`, `percentage` is required. | ||
| */ | ||
| readonly percentage?: number; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about creating union-like classes for the options instead of the interface, including static factory methods with count() and percentage()?
For example:
- abstract
AtLeastOptions- static
count(operands: IAlarm[], count: number) - static
percentage(operands: IAlarm[], percentage: number) - It would also be good to create new props for extensibility, including that properties.
- props interfaces:
{operands: IAlarm[], count: number}and{operands: IAlarm[], percentage: number} - or common props (
{operands}) and sub props ({count}and{percentage}) extending the common props
- props interfaces:
- static
- Sub classes
AtLeastOptionsWithCountextendsAtLeastOptionsAtLeastOptionsWithPercentageextendsAtLeastOptions- constructor for both of them is private
- How to use:
AlarmRule.atLeastAlarm(AtLeastOptions.count(alarms, 3))AlarmRule.atLeastAlarm(AtLeastOptions.percentage(alarms, 30))- or
AlarmRule.atLeastAlarm(AtLeastOptions.percentage({alarms, 30}))- see above:
It would also be good to create new props for extensibility, including that properties.
- see above:
Since we can guarantee exclusivity, we can remove the validation in renderAlarmRule and move some of the validation logic to these classes, which will likely make renderAlarmRule simpler.
Additionally, it might be good to create an internal abstract getter method that returns the threshold as string in the abstract class. The method in each subclass would then wrap its respective format (count or ${percentage}%).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your proposal.
I will accept it overall, but there are a few changes.
Instead of passing all properties of AtLeastOptions to the Union-like class, I decided to pass only the properties related to count and percentage thresholds.
This is because I felt the role of the Union-like class passed to the AtLeastXxx method for AtLeastOptions was unclear. AlarmRule.atLeastAlarm(AtLeastOptions.count(alarms, 3))
However, I have a concern.
Since operands are necessary for the validation of AtLeastThreshold.count, they are passed on the bind method. This necessitates passing unnecessary operands to AtLeastThreshold.percentage.
I would like to hear your thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, it's good too.
And see: #36100 (comment)
And it would be good to define the arg with operands, the arg could be operands in the AtLeastThresholdCount, and could be _operands in the AtLeastThresholdPercentage. Because name for args can be changed in each method. Also these are internal methods, so there is no impact on users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
I didn’t know you can change the parameter name when implementing an abstract method.
Co-authored-by: Kenta Goto <[email protected]>
Co-authored-by: Kenta Goto <[email protected]>
| /** | ||
| * configuration for creating a threshold for AT_LEAST expression | ||
| */ | ||
| export interface AtLeastThresholdConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is export needed? (I'm not sure if rosetta will warn us even if it is for an internal method...)
for example: https://github.com/aws/aws-cdk/blob/v2.227.0/packages/aws-cdk-lib/aws-lambda/lib/adot-layers.ts#L47
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AtLeastThresholdConfig is only used internally, so it didn’t need to be exported.
The implementing classes of AtLeastThreshold are exported because we want to support use cases where they are treated as types, as shown below.
class AtLeastCountSampleClass {
readonly rule;
constructor(alarms: IAlarm[], count: AtLeastThresholdCount) {
this.rule = AlarmRule.atLeastAlarm({
operands: alarms,
threshold: count,
})
}
}
declare const alarms: IAlarm[]
const count = AtLeastThreshold.count(2)
new AtLeastCountSampleClass(alarms, count)Co-authored-by: Kenta Goto <[email protected]>
Co-authored-by: Kenta Goto <[email protected]>
go-to-k
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Issue # (if applicable)
Closes #.
Reason for this change
Amazon CloudWatch Composite Alarms adds threshold-based alerting
Description of changes
Added several wrapper functions for the AT_LEAST expression to AlarmRule.
hasAtLeastAlarmhasAtLeastOkhasAtLeastInsuffisienthasAtLeastNotAlarmhasAtLeastNotOkhasAtLeastNotInsuffisientI will explain the reason for separating the AT_LEAST expression into these functions.
First, the AlarmState available in the AT_LEAST expression and the AlarmState defined in the Enum-like class may have different lifecycles. For this reason, I did not think it was appropriate to partially extract the Enum-like class.
Second, the handling of the NOT operator. When using the NOT operator in this function, it is always considered in conjunction with AlarmState. For example, when considering a situation where it is not at least OK, I did not think that AlarmState and the NOT operator should be separate properties. Here too, I did not think it was appropriate to create an Enum-like class that extends AlarmState with the NOT operator.
Describe any new or updated permissions being added
Description of how you validated changes
Both unit and integ tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license