Skip to content
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

Merged
merged 2 commits into from
Jul 2, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/@aws-cdk/assert/lib/assertions/have-resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { StackInspector } from "../inspector";
* Properties can be:
*
* - An object, in which case its properties will be compared to those of the actual resource found
* - A callablage, in which case it will be treated as a predicate that is applied to the Properties of the found resources.
* - A callable, in which case it will be treated as a predicate that is applied to the Properties of the found resources.
*/
export function haveResource(resourceType: string, properties?: any): Assertion<StackInspector> {
return new HaveResourceAssertion(resourceType, properties);
Expand Down
77 changes: 68 additions & 9 deletions packages/@aws-cdk/cloudwatch/lib/alarm.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Arn, Construct, Token } from '@aws-cdk/core';
import { cloudwatch } from '@aws-cdk/resources';
import { HorizontalAnnotation } from './graph';
import { Metric } from './metric';
import { Dimension, Metric, parseStatistic, Statistic, Unit } from './metric';

/**
* Properties for Alarms
Expand Down Expand Up @@ -158,7 +158,7 @@ export class Alarm extends Construct {
okActions: new Token(() => this.okActions),

// Metric
...props.metric.toAlarmJson()
...metricJson(props.metric)
});

this.alarmArn = alarm.alarmArn;
Expand All @@ -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) {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh 🤦🏻‍♂️

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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));
}

/**
Expand Down Expand Up @@ -256,4 +256,63 @@ export interface IAlarmAction {
* Return the ARN that should be used for a CloudWatch Alarm action
*/
readonly alarmActionArn: Arn;
}
}

/**
* Return the JSON structure which represents the given metric in an alarm.
*/
function metricJson(metric: Metric): AlarmMetricJson {
const stat = parseStatistic(metric.statistic);

const dims = metric.dimensionsAsList();

return {
dimensions: dims.length > 0 ? dims : undefined,
namespace: metric.namespace,
metricName: metric.metricName,
period: metric.periodSec,
statistic: stat.type === 'simple' ? stat.statistic : undefined,
extendedStatistic: stat.type === 'percentile' ? 'p' + stat.percentile : undefined,
unit: metric.unit
};
}

/**
* Properties used to construct the Metric identifying part of an Alarm
*/
export interface AlarmMetricJson {
/**
* The dimensions to apply to the alarm
*/
dimensions?: Dimension[];

/**
* Namespace of the metric
*/
namespace: string;

/**
* Name of the metric
*/
metricName: string;

/**
* How many seconds to aggregate over
*/
period: number;

/**
* Simple aggregation function to use
*/
statistic?: Statistic;

/**
* Percentile aggregation function to use
*/
extendedStatistic?: string;

/**
* The unit of the alarm
*/
unit?: Unit;
}
22 changes: 19 additions & 3 deletions packages/@aws-cdk/cloudwatch/lib/dashboard.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Construct, Token, tokenAwareJsonify } from "@aws-cdk/core";
import { Construct, Stack, Token, tokenAwareJsonify } from "@aws-cdk/core";
import { cloudwatch } from "@aws-cdk/resources";
import { Column, Row } from "./layout";
import { IWidget } from "./widget";
Expand All @@ -17,12 +17,19 @@ export interface DashboardProps {
*/
export class Dashboard extends Construct {
private readonly rows: IWidget[] = [];
private readonly dashboard: cloudwatch.DashboardResource;

constructor(parent: Construct, name: string, props?: DashboardProps) {
super(parent, name);

new cloudwatch.DashboardResource(this, 'Resource', {
dashboardName: props && props.dashboardName,
// 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.
// See: https://github.com/awslabs/aws-cdk/issues/213
const dashboardName = (props && props.dashboardName) || new Token(() => this.generateDashboardName());

this.dashboard = new cloudwatch.DashboardResource(this, 'Resource', {
dashboardName,
dashboardBody: new Token(() => {
const column = new Column(...this.rows);
column.position(0, 0);
Expand All @@ -48,4 +55,13 @@ 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 {
// Combination of stack name and LogicalID, which are guaranteed to be unique.
const stack = Stack.find(this);
return stack.name + '-' + this.dashboard.logicalId;
}
}
38 changes: 34 additions & 4 deletions packages/@aws-cdk/cloudwatch/lib/graph.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { AwsRegion, Token } from "@aws-cdk/core";
import { Alarm } from "./alarm";
import { Metric } from "./metric";
import { Metric, parseStatistic } from "./metric";
import { ConcreteWidget } from "./widget";

/**
Expand Down Expand Up @@ -156,8 +156,8 @@ export class GraphWidget extends ConcreteWidget {
view: 'timeSeries',
title: this.props.title,
region: this.props.region || new AwsRegion(),
metrics: (this.props.left || []).map(m => m.toGraphJson('left')).concat(
(this.props.right || []).map(m => m.toGraphJson('right'))),
metrics: (this.props.left || []).map(m => metricJson(m, 'left')).concat(
(this.props.right || []).map(m => metricJson(m, 'right'))),
annotations: {
horizontal: (this.props.leftAnnotations || []).map(mapAnnotation('left')).concat(
(this.props.rightAnnotations || []).map(mapAnnotation('right')))
Expand Down Expand Up @@ -203,7 +203,7 @@ export class SingleValueWidget extends ConcreteWidget {
view: 'singleValue',
title: this.props.title,
region: this.props.region || new AwsRegion(),
metrics: this.props.metrics.map(m => m.toGraphJson('left'))
metrics: this.props.metrics.map(m => metricJson(m, 'left'))
}
}];
}
Expand Down Expand Up @@ -287,4 +287,34 @@ function mapAnnotation(yAxis: string): ((x: HorizontalAnnotation) => any) {
return (a: HorizontalAnnotation) => {
return { ...a, yAxis };
};
}

/**
* Return the JSON structure which represents this metric in a graph
*
* This will be called by GraphWidget, no need for clients to call this.
*/
function metricJson(metric: Metric, yAxis: string): any[] {
// Namespace and metric Name
const ret: any[] = [
metric.namespace,
metric.metricName,
];

// Dimensions
for (const dim of metric.dimensionsAsList()) {
ret.push(dim.name, dim.value);
}

// Options
const stat = parseStatistic(metric.statistic);
ret.push({
yAxis,
label: metric.label,
color: metric.color,
period: metric.periodSec,
stat: stat.type === 'simple' ? stat.statistic : 'p' + stat.percentile.toString(),
});

return ret;
}
Loading