-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
@aws-cdk/cloudwatch: bugfixes, small changes changes and workaround #194
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -158,7 +158,7 @@ export class Alarm extends Construct { | |
okActions: new Token(() => this.okActions), | ||
|
||
// Metric | ||
...props.metric.toAlarmJson() | ||
...props.metric.alarmInfo() | ||
}); | ||
|
||
this.alarmArn = alarm.alarmArn; | ||
|
@@ -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) { | ||
this.alarmActions = []; | ||
} | ||
|
||
this.alarmActions.push(action.alarmActionArn); | ||
this.alarmActions.push(...actions.map(a => a.alarmActionArn)); | ||
} | ||
|
||
/** | ||
* 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here |
||
this.insufficientDataActions = []; | ||
} | ||
|
||
this.insufficientDataActions.push(action.alarmActionArn); | ||
this.insufficientDataActions.push(...actions.map(a => a.alarmActionArn)); | ||
} | ||
|
||
/** | ||
* 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And also here |
||
this.okActions = []; | ||
} | ||
|
||
this.okActions.push(action.alarmActionArn); | ||
this.okActions.push(...actions.map(a => a.alarmActionArn)); | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's open an issue to track the fix and revert the workaround (and reference from here) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
const dashboardName = (props && props.dashboardName) || this.generateDashboardName(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case I would expect the |
||
|
||
new cloudwatch.DashboardResource(this, 'Resource', { | ||
dashboardName: props && props.dashboardName, | ||
dashboardName, | ||
dashboardBody: new Token(() => { | ||
const column = new Column(...this.rows); | ||
column.position(0, 0); | ||
|
@@ -48,4 +53,12 @@ export class Dashboard extends Construct { | |
const w = widgets.length > 1 ? new Row(...widgets) : widgets[0]; | ||
this.rows.push(w); | ||
} | ||
|
||
/** | ||
* Generate a unique dashboard name in case the user didn't supply one | ||
*/ | ||
private generateDashboardName(): string { | ||
// This will include the Stack name and is hence unique | ||
return this.path.replace('/', '-'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't. That There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, of course. It's going to be ugly, but I guess... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Above comment about making the name required in
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be. Unlikely though, and in that case you can still pass a proper name. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unless someone hid it somewhere in their construct and gave you no way to name it yourself? |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -101,17 +101,22 @@ export class Metric { | |
this.metricName = props.metricName; | ||
this.periodSec = props.periodSec !== undefined ? props.periodSec : 300; | ||
this.statistic = props.statistic || "Average"; | ||
this.label = props.label; | ||
this.color = props.color; | ||
this.unit = props.unit; | ||
|
||
// Try parsing, this will throw if it's not a valid stat | ||
parseStatistic(this.statistic); | ||
} | ||
|
||
/** | ||
* Return a copy of Metric with properties changed | ||
* @param props Re | ||
* Return a copy of Metric with properties changed. | ||
* | ||
* All properties except namespace and metricName can be changed. | ||
* | ||
* @param props The set of properties to change. | ||
*/ | ||
public with(props: ChangeMetricProps): Metric { | ||
public with(props: MetricCustomizations): Metric { | ||
return new Metric({ | ||
dimensions: ifUndefined(props.dimensions, this.dimensions), | ||
namespace: this.namespace, | ||
|
@@ -149,8 +154,10 @@ export class Metric { | |
|
||
/** | ||
* Return the JSON structure which represents this metric in an alarm | ||
* | ||
* This will be called by Alarm, no need for clients to call this. | ||
*/ | ||
public toAlarmJson(): MetricAlarmJson { | ||
public alarmInfo(): AlarmMetricInfo { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know. The reason I'm dancing around it is because it's a fraction of the structure required to represent an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
const stat = parseStatistic(this.statistic); | ||
|
||
return { | ||
|
@@ -166,8 +173,10 @@ export class Metric { | |
|
||
/** | ||
* Return the JSON structure which represents this metric in a graph | ||
* | ||
* This will be called by GraphWidget, no need for clients to call this. | ||
*/ | ||
public toGraphJson(yAxis: string): any[] { | ||
public graphJson(yAxis: string): any[] { | ||
// Namespace and metric Name | ||
const ret: any[] = [ | ||
this.namespace, | ||
|
@@ -196,7 +205,7 @@ export class Metric { | |
/** | ||
* Properties used to construct the Metric identifying part of an Alarm | ||
*/ | ||
export interface MetricAlarmJson { | ||
export interface AlarmMetricInfo { | ||
/** | ||
* The dimensions to apply to the alarm | ||
*/ | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd use singular form |
||
/** | ||
* Dimensions of the metric | ||
* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
import { expect, haveResource, isSuperObject } from '@aws-cdk/assert'; | ||
import { Stack } from '@aws-cdk/core'; | ||
import { App, Stack } from '@aws-cdk/core'; | ||
import { Test } from 'nodeunit'; | ||
import { Dashboard, GraphWidget, TextWidget } from '../lib'; | ||
|
||
|
@@ -94,6 +94,22 @@ export = { | |
|
||
test.done(); | ||
}, | ||
|
||
'work around CloudFormation bug'(test: Test) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make sure issue is referenced here as well |
||
// GIVEN | ||
const app = new App(); | ||
const stack = new Stack(app, 'MyStack'); | ||
|
||
// WHEN | ||
new Dashboard(stack, 'MyDashboard'); | ||
|
||
// THEN | ||
expect(stack).to(haveResource('AWS::CloudWatch::Dashboard', { | ||
DashboardName: 'MyStack-MyDashboard' | ||
})); | ||
|
||
test.done(); | ||
} | ||
}; | ||
|
||
/** | ||
|
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.
I don't believe this can happen, since it's a variadic method.
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.
You're thinking of the parameter. This is about the instance member.
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.
Oh 🤦🏻♂️