@aws-cdk/cloudwatch: bugfixes, small changes changes and workaround #194
Conversation
- bugfix: label/color were ignored when passed directly to the Metric constructor. - BREAKING CHANGE: ChangeMetricProps renamed to MetricCustomizations. - change: onAlarm()/onOk() now take variadic arguments - workaround: generate a dashboard name if user did not supply one. This fixes #192.
| @@ -175,38 +175,38 @@ export class Alarm extends Construct { | |||
| * | |||
| * Typically the ARN of an SNS topic or ARN of an AutoScaling policy. | |||
| */ | |||
| public onAlarm(action: IAlarmAction) { | |||
| public onAlarm(...actions: IAlarmAction[]) { | |||
| if (this.alarmActions === undefined) { | |||
There was a problem hiding this comment.
I don't believe this can happen, since it's a variadic method.
There was a problem hiding this comment.
You're thinking of the parameter. This is about the instance member.
| } | ||
|
|
||
| /** | ||
| * Trigger this action if there is insufficient data to evaluate the alarm | ||
| * | ||
| * Typically the ARN of an SNS topic or ARN of an AutoScaling policy. | ||
| */ | ||
| public onInsufficientData(action: IAlarmAction) { | ||
| public onInsufficientData(...actions: IAlarmAction[]) { | ||
| if (this.insufficientDataActions === undefined) { |
| } | ||
|
|
||
| /** | ||
| * Trigger this action if the alarm returns from breaching state into ok state | ||
| * | ||
| * Typically the ARN of an SNS topic or ARN of an AutoScaling policy. | ||
| */ | ||
| public onOk(action: IAlarmAction) { | ||
| public onOk(...actions: IAlarmAction[]) { | ||
| if (this.okActions === undefined) { |
| // WORKAROUND -- Dashboard cannot be updated if the DashboardName is missing. | ||
| // This is a bug in CloudFormation, but we don't want CDK users to have a bad | ||
| // experience. We'll generate a name here if you did not supply one. | ||
| const dashboardName = (props && props.dashboardName) || this.generateDashboardName(); |
There was a problem hiding this comment.
Why not make dashboard naming mandatory? It's one of those things that are expressly targeting human interactions, so I think it's reasonable to target an optimum console experience here.
There was a problem hiding this comment.
I considered that as well, but the rationale is to not make it required for all intermediate constructs that might contain the dashboard.
There was a problem hiding this comment.
In this case I would expect the Construct to have a dashboardName input of some sort. But okay I get your point.
| */ | ||
| private generateDashboardName(): string { | ||
| // This will include the Stack name and is hence unique | ||
| return this.path.replace('/', '-'); |
There was a problem hiding this comment.
Above comment about making the name required in props stands even more true here as:
- This will not be a console-friendly string
- This might very well be longer than the maximum dashboard name size (255 characters)
There was a problem hiding this comment.
Might be. Unlikely though, and in that case you can still pass a proper name.
There was a problem hiding this comment.
Unless someone hid it somewhere in their construct and gave you no way to name it yourself?
| @@ -21,8 +21,13 @@ export class Dashboard extends Construct { | |||
| constructor(parent: Construct, name: string, props?: DashboardProps) { | |||
| super(parent, name); | |||
|
|
|||
| // WORKAROUND -- Dashboard cannot be updated if the DashboardName is missing. | |||
| // This is a bug in CloudFormation, but we don't want CDK users to have a bad | |||
| // experience. We'll generate a name here if you did not supply one. | |||
There was a problem hiding this comment.
Let's open an issue to track the fix and revert the workaround (and reference from here)
| */ | ||
| private generateDashboardName(): string { | ||
| // This will include the Stack name and is hence unique | ||
| return this.path.replace('/', '-'); |
There was a problem hiding this comment.
Do dashboard names have a length limit? Path can be quite long. I would use the the logical ID we generate for the resource prefixed by the stack name.
There was a problem hiding this comment.
Can't. That id is only available after the DashboardResource is created, at which point the dashboardName is locked in.
There was a problem hiding this comment.
Ah, of course. It's going to be ugly, but I guess...
| */ | ||
| public toAlarmJson(): MetricAlarmJson { | ||
| public alarmInfo(): AlarmMetricInfo { |
There was a problem hiding this comment.
Why not just AlarmMetric? Whenever I see "Info" in the name of a class I die a little from the inside.
There was a problem hiding this comment.
I know.
The reason I'm dancing around it is because it's a fraction of the structure required to represent an Alarm. An AlarmMetric is not really a concept that we have, this is half of the properties of AlarmResourceProps.
There was a problem hiding this comment.
Does it have to be a public API? If there's a way to hide it, name is not that critical.
Another option is to move this logic be part of Alarm instead of the Metric. The metric should be able to expose all this info for the Alarm to be able to use it (and not vice versa).
| @@ -295,7 +304,7 @@ export enum Unit { | |||
| /** | |||
| * Properties of a metric that can be changed | |||
| */ | |||
| export interface ChangeMetricProps { | |||
| export interface MetricCustomizations { | |||
There was a problem hiding this comment.
I'd use singular form MetricCustomization
| @@ -94,6 +94,22 @@ export = { | |||
|
|
|||
| test.done(); | |||
| }, | |||
|
|
|||
| 'work around CloudFormation bug'(test: Test) { | |||
There was a problem hiding this comment.
Make sure issue is referenced here as well
constructor.
This fixes Handle DashboardName missing in CloudWatch #192.
By submitting this pull request, I confirm that my contribution is made under
the terms of the beta license.