From 4f102bfd15008841a0de8aa75a2b94eaa666feae Mon Sep 17 00:00:00 2001 From: Ahmed Sedek Date: Mon, 30 Dec 2019 13:42:02 +0100 Subject: [PATCH 01/38] Dummy change --- .../aws-cloudwatch/lib/metric-types.ts | 66 +++++++++---------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts index e09c6a5abbe76..f6da5e9535c68 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts @@ -1,4 +1,3 @@ - /** * Interface for metrics */ @@ -16,6 +15,7 @@ export interface IMetric { /** * Metric dimension + * */ export interface Dimension { /** @@ -33,44 +33,44 @@ export interface Dimension { * Statistic to use over the aggregation period */ export enum Statistic { - SAMPLE_COUNT = 'SampleCount', - AVERAGE = 'Average', - SUM = 'Sum', - MINIMUM = 'Minimum', - MAXIMUM = 'Maximum', + SAMPLE_COUNT = "SampleCount", + AVERAGE = "Average", + SUM = "Sum", + MINIMUM = "Minimum", + MAXIMUM = "Maximum" } /** * Unit for metric */ export enum Unit { - SECONDS = 'Seconds', - MICROSECONDS = 'Microseconds', - MILLISECONDS = 'Milliseconds', - BYTES = 'Bytes', - KILOBYTES = 'Kilobytes', - MEGABYTES = 'Megabytes', - GIGABYTES = 'Gigabytes', - TERABYTES = 'Terabytes', - BITS = 'Bits', - KILOBITS = 'Kilobits', - MEGABITS = 'Megabits', - GIGABITS = 'Gigabits', - TERABITS = 'Terabits', - PERCENT = 'Percent', - COUNT = 'Count', - BYTES_PER_SECOND = 'Bytes/Second', - KILOBYTES_PER_SECOND = 'Kilobytes/Second', - MEGABYTES_PER_SECOND = 'Megabytes/Second', - GIGABYTES_PER_SECOND = 'Gigabytes/Second', - TERABYTES_PER_SECOND = 'Terabytes/Second', - BITS_PER_SECOND = 'Bits/Second', - KILOBITS_PER_SECOND = 'Kilobits/Second', - MEGABITS_PER_SECOND = 'Megabits/Second', - GIGABITS_PER_SECOND = 'Gigabits/Second', - TERABITS_PER_SECOND = 'Terabits/Second', - COUNT_PER_SECOND = 'Count/Second', - NONE = 'None' + SECONDS = "Seconds", + MICROSECONDS = "Microseconds", + MILLISECONDS = "Milliseconds", + BYTES = "Bytes", + KILOBYTES = "Kilobytes", + MEGABYTES = "Megabytes", + GIGABYTES = "Gigabytes", + TERABYTES = "Terabytes", + BITS = "Bits", + KILOBITS = "Kilobits", + MEGABITS = "Megabits", + GIGABITS = "Gigabits", + TERABITS = "Terabits", + PERCENT = "Percent", + COUNT = "Count", + BYTES_PER_SECOND = "Bytes/Second", + KILOBYTES_PER_SECOND = "Kilobytes/Second", + MEGABYTES_PER_SECOND = "Megabytes/Second", + GIGABYTES_PER_SECOND = "Gigabytes/Second", + TERABYTES_PER_SECOND = "Terabytes/Second", + BITS_PER_SECOND = "Bits/Second", + KILOBITS_PER_SECOND = "Kilobits/Second", + MEGABITS_PER_SECOND = "Megabits/Second", + GIGABITS_PER_SECOND = "Gigabits/Second", + TERABITS_PER_SECOND = "Terabits/Second", + COUNT_PER_SECOND = "Count/Second", + NONE = "None" } /** From ff022bd03cc7c4bb4b764fa5887b41eaab85cd2f Mon Sep 17 00:00:00 2001 From: Ahmed Sedek Date: Mon, 30 Dec 2019 15:10:03 +0100 Subject: [PATCH 02/38] Created base interfaces --- .../aws-cloudwatch/lib/metric-types.ts | 96 +++++++++++++++++++ 1 file changed, 96 insertions(+) diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts index f6da5e9535c68..631d0549452e0 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts @@ -2,13 +2,22 @@ * Interface for metrics */ export interface IMetric { + /** + * Inspect the details of the metric object + */ + toMetricConfig(): MetricConfig; + /** * Turn this metric object into an alarm configuration + * + * @deprecated Use `toMetricsConfig()` instead. */ toAlarmConfig(): MetricAlarmConfig; /** * Turn this metric object into a graph configuration + * + * @deprecated Use `toMetricsConfig()` instead. */ toGraphConfig(): MetricGraphConfig; } @@ -73,8 +82,91 @@ export enum Unit { NONE = "None" } +/** + * Properties of a rendered metric + */ +export interface MetricConfig { + /** + * In case the metric represents a query, the details of the query + */ + readonly metricStat?: MetricStatConfig; + + /** + * In case the metric is a math expression, the details of the math expression + */ + readonly mathExpression?: MetricExpressionConfig; + + /** + * Additional properties which will be rendered if the metric is used in a dashboard + * + * Examples are 'label' and 'color', but any key in here will be + * added to dashboard graphs. + */ + readonly renderingProperties?: Record; +} + +/** + * Properties for a concrete metric + * + * NOTE: `unit` is no longer on this object since it is only used for `Alarms`, and doesn't mean what one + * would expect it to mean there anyway. It is most likely to be misused. + */ +export interface MetricStatConfig { + /** + * The dimensions to apply to the alarm + */ + readonly dimensions?: Dimension[]; + + /** + * Namespace of the metric + */ + readonly namespace: string; + + /** + * Name of the metric + */ + readonly metricName: string; + + /** + * How many seconds to aggregate over + */ + readonly period?: number; + + /** + * Aggregation function to use (can be either simple or a percentile) + */ + readonly statistic?: string; + + /** + * Region of the metric + */ + readonly region?: string; + + /** + * Account of the metric + */ + readonly account?: string; +} + +/** + * Properties for a concrete metric + */ +export interface MetricExpressionConfig { + /** + * Math expression for the metric. + */ + readonly expression: string; + + /** + * Metrics used in the math expression + */ + readonly expressionMetrics: Record; +} + /** * Properties used to construct the Metric identifying part of an Alarm + * + * @deprecated Replaced by MetricConfig */ export interface MetricAlarmConfig { /** @@ -115,6 +207,8 @@ export interface MetricAlarmConfig { /** * Properties used to construct the Metric identifying part of a Graph + * * + * @deprecated Replaced by MetricConfig */ export interface MetricGraphConfig { /** @@ -175,6 +269,8 @@ export interface MetricGraphConfig { /** * Custom rendering properties that override the default rendering properties specified in the yAxis parameter of the widget object. + * + * @deprecated Replaced by `MetricConfig`. */ export interface MetricRenderingProperties { /** From d2a143837574fdf247dc40812fc86d7e80243b0c Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 30 Dec 2019 16:29:04 +0100 Subject: [PATCH 03/38] Consume new toMetricConfig() API for dashboard widgets --- packages/@aws-cdk/aws-cloudwatch/lib/graph.ts | 33 +-- .../aws-cloudwatch/lib/metric-util.ts | 189 ++++++++++++++++++ .../@aws-cdk/aws-cloudwatch/lib/metric.ts | 2 +- 3 files changed, 194 insertions(+), 30 deletions(-) create mode 100644 packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/graph.ts b/packages/@aws-cdk/aws-cloudwatch/lib/graph.ts index d9ca31aa49494..f33cdd473327b 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/graph.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/graph.ts @@ -1,6 +1,7 @@ import * as cdk from '@aws-cdk/core'; import { IAlarm } from "./alarm"; import { IMetric } from "./metric-types"; +import { metricGraphJson } from './metric-util'; import { ConcreteWidget } from "./widget"; /** @@ -169,8 +170,7 @@ export class GraphWidget extends ConcreteWidget { public toJson(): any[] { const horizontalAnnoations = (this.props.leftAnnotations || []).map(mapAnnotation('left')).concat( (this.props.rightAnnotations || []).map(mapAnnotation('right'))); - const metrics = (this.props.left || []).map(m => metricJson(m, 'left')).concat( - (this.props.right || []).map(m => metricJson(m, 'right'))); + const metrics = metricGraphJson(this.props.left || [], this.props.right || []); return [{ type: 'metric', width: this.width, @@ -232,7 +232,7 @@ export class SingleValueWidget extends ConcreteWidget { view: 'singleValue', title: this.props.title, region: this.props.region || cdk.Aws.REGION, - metrics: this.props.metrics.map(m => metricJson(m, 'left')), + metrics: metricGraphJson(this.props.metrics, []), setPeriodToTimeRange: this.props.setPeriodToTimeRange } }]; @@ -298,29 +298,4 @@ 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: IMetric, yAxis: string): any[] { - const config = metric.toGraphConfig(); - - // Namespace and metric Name - const ret: any[] = [ - config.namespace, - config.metricName, - ]; - - // Dimensions - for (const dim of (config.dimensions || [])) { - ret.push(dim.name, dim.value); - } - - // Options - ret.push({ yAxis, ...config.renderingProperties }); - - return ret; -} +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts new file mode 100644 index 0000000000000..f824706b255b5 --- /dev/null +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts @@ -0,0 +1,189 @@ +import { IMetric } from "./metric-types"; + +/** + * Return the JSON structure which represents these metrics in a graph. + * + * Depending on the metric type (stat or expression), one `Metric` object + * can render to multiple time series. + * + * - Top-level metrics will be rendered visibly, additionally added metrics will + * be rendered invisibly. + * - IDs used in math expressions need to be either globally unique, or refer to the same + * metric object. + * + * This will be called by GraphWidget, no need for clients to call this. + */ +export function metricGraphJson(left: IMetric[], right: IMetric[]): any[][] { + // Add metrics to a set which will automatically expand them recursively, + // making sure to retain conflicting the visible one on conflicting metrics objects. + const mset = new MetricSet(); + mset.addVisible('left', ...left); + mset.addVisible('right', ...right); + + // Render all metrics from the set. + return mset.entries.map(entry => metricJson(entry.metric, entry.yAxis, entry.id)); +} + +function metricJson(metric: IMetric, yAxis?: string, id?: string) { + const config = metric.toMetricConfig(); + + const ret: any[] = []; + const options: any = { ...config.renderingProperties || {} }; + + if (config.metricStat) { + // Namespace and metric Name + ret.push( + config.metricStat.namespace, + config.metricStat.metricName, + ); + + // Dimensions + for (const dim of (config.metricStat.dimensions || [])) { + ret.push(dim.name, dim.value); + } + + // Metric attributes that are rendered to graph options + if (config.metricStat.account) { options.accountId = config.metricStat.account; } + if (config.metricStat.region) { options.region = config.metricStat.region; } + } else if (config.mathExpression) { + // Everything goes into the options object + options.expression = config.mathExpression.expression; + } else { + throw new Error(`Metric object must have either 'metricStat' or 'mathExpression'`); + } + + // Options + if (!yAxis) { options.visible = false; } + if (yAxis !== 'left') { options.yAxis = yAxis; } + if (id) { options.id = id; } + + if (Object.keys(options).length !== 0) { + ret.push(options); + } + return ret; +} + +interface MetricEntry { + metric: IMetric; + yAxis?: string; + id?: string; +} + +/** + * Maintain a set of metrics, expanding math expressions + */ +class MetricSet { + private readonly metricKeys = new Map(); + private readonly metricMap = new Map(); + + /** + * Add the given set of metrics to this set + */ + public addVisible(yAxis: string, ...metrics: IMetric[]) { + for (const metric of metrics) { + this.addOne(metric, yAxis); + } + } + + public get entries(): MetricEntry[] { + return Array.from(this.metricMap.values()); + } + + /** + * Add a metric into the set + * + * If it already exists, either it must be getting a new id or the id must be + * the same as before. + * + * It can be made visible, in which case the new "metric" object replaces the old + * one (and the new ones "renderingPropertieS" will be honored instead of the old + * one's). + */ + private addOne(metric: IMetric, yAxis?: string, id?: string) { + const key = this.keyOf(metric); + + // Record this one + const existing = this.metricMap.get(key); + if (existing) { + // Set id on previously unnamed object + if (id) { + if (existing.id && existing.id !== id) { + throw new Error(`Same id used for two metrics in the same graph: '${metric}' and '${existing.metric}'. Rename one.`); + } + existing.id = id; + } + // Replace object if invisible metric is being made visible (to use new one's renderingProperties). + if (yAxis) { + if (existing.yAxis && existing.yAxis !== yAxis) { + throw new Error(`Same metric added on both axes: '${metric}' and '${existing.metric}'. Remove one.`); + } + // Replace object + existing.yAxis = yAxis; + existing.metric = metric; + } + } else { + this.metricMap.set(key, { metric, yAxis, id }); + } + + // Recurse and add children + const conf = metric.toMetricConfig(); + if (conf.mathExpression) { + for (const [subId, subMetric] of Object.entries(conf.mathExpression.expressionMetrics)) { + this.addOne(subMetric, undefined, subId); + } + } + } + + /** + * Cached version of metricKey + */ + private keyOf(metric: IMetric) { + const existing = this.metricKeys.get(metric); + if (existing) { return existing; } + + const key = metricKey(metric); + this.metricKeys.set(metric, key); + return key; + } +} + +/** + * Return a unique string representation for this metric. + * + * Can be used to determine as a hash key to determine if 2 Metric objects + * represent the same metric. Excludes rendering properties. + */ +function metricKey(metric: IMetric) { + const parts = new Array(); + + const conf = metric.toMetricConfig(); + if (conf.mathExpression) { + parts.push(conf.mathExpression.expression); + for (const id of Object.keys(conf.mathExpression.expressionMetrics).sort()) { + parts.push(id); + parts.push(metricKey(conf.mathExpression.expressionMetrics[id])); + } + } + if (conf.metricStat) { + parts.push(conf.metricStat.namespace); + parts.push(conf.metricStat.metricName); + for (const dim of conf.metricStat.dimensions || []) { + parts.push(dim.name); + parts.push(dim.value); + } + if (conf.metricStat.statistic) { + parts.push(conf.metricStat.statistic); + } + if (conf.metricStat.period) { + parts.push(`${conf.metricStat.period}`); + } + if (conf.metricStat.region) { + parts.push(conf.metricStat.region); + } + if (conf.metricStat.account) { + parts.push(conf.metricStat.account); + } + } + + return parts.join('|'); +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts index b75c967cbb00d..ab4802a2c0d56 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts @@ -224,7 +224,7 @@ export class Metric implements IMetric { return []; } - const list = Object.keys(dims).map(key => ({ name: key, value: dims[key] })); + const list = Object.keys(dims).sort().map(key => ({ name: key, value: dims[key] })); return list; } From 2b08eec5a17952f772239341f124f533aa3e644b Mon Sep 17 00:00:00 2001 From: Ahmed Sedek Date: Mon, 30 Dec 2019 16:39:30 +0100 Subject: [PATCH 04/38] Added MathExpression --- .../aws-cloudwatch/lib/metric-types.ts | 4 +- .../@aws-cdk/aws-cloudwatch/lib/metric.ts | 205 ++++++++++++++++-- 2 files changed, 187 insertions(+), 22 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts index 631d0549452e0..76aa37c9183ee 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts @@ -130,12 +130,12 @@ export interface MetricStatConfig { /** * How many seconds to aggregate over */ - readonly period?: number; + readonly period: number; /** * Aggregation function to use (can be either simple or a percentile) */ - readonly statistic?: string; + readonly statistic: string; /** * Region of the metric diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts index ab4802a2c0d56..8828b00609990 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts @@ -1,7 +1,7 @@ import * as iam from '@aws-cdk/aws-iam'; import * as cdk from '@aws-cdk/core'; import { Alarm, ComparisonOperator, TreatMissingData } from './alarm'; -import { Dimension, IMetric, MetricAlarmConfig, MetricGraphConfig, Unit } from './metric-types'; +import { Dimension, IMetric, MetricAlarmConfig, MetricConfig, MetricGraphConfig, Unit } from './metric-types'; import { normalizeStatistic, parseStatistic } from './util.statistic'; export type DimensionHash = {[dim: string]: any}; @@ -69,12 +69,37 @@ export interface MetricProps extends CommonMetricOptions { * Name of the metric. */ readonly metricName: string; + + /** + * Account which this metric comes from. + */ + readonly account?: string; + + /** + * Region which this metric comes from. + */ + readonly region?: string; } /** * Properties of a metric that can be changed */ export interface MetricOptions extends CommonMetricOptions { + /** + * Account which this metric comes from. + */ + readonly account?: string; + + /** + * Region which this metric comes from. + */ + readonly region?: string; +} + +export interface MathExpressionProps extends CommonMetricOptions { + readonly expression: string; + + readonly expressionMetrics: Record; } /** @@ -113,6 +138,8 @@ export class Metric implements IMetric { public readonly unit?: Unit; public readonly label?: string; public readonly color?: string; + public readonly account?: string; + public readonly region?: string; constructor(props: MetricProps) { this.period = props.period || cdk.Duration.minutes(5); @@ -129,6 +156,8 @@ export class Metric implements IMetric { this.label = props.label; this.color = props.color; this.unit = props.unit; + this.account = props.account; + this.region = props.region; } /** @@ -147,7 +176,9 @@ export class Metric implements IMetric { statistic: ifUndefined(props.statistic, this.statistic), unit: ifUndefined(props.unit, this.unit), label: ifUndefined(props.label, this.label), - color: ifUndefined(props.color, this.color) + color: ifUndefined(props.color, this.color), + account: this.account, + region: this.region }); } @@ -176,14 +207,17 @@ export class Metric implements IMetric { } public toAlarmConfig(): MetricAlarmConfig { - const stat = parseStatistic(this.statistic); - const dims = this.dimensionsAsList(); + const metricConfig = this.toMetricConfig(); + if (metricConfig.metricStat === undefined) { + throw new Error("MetricStat must be set."); + } + const stat = parseStatistic(metricConfig.metricStat.statistic); return { - dimensions: dims.length > 0 ? dims : undefined, - namespace: this.namespace, - metricName: this.metricName, - period: this.period.toSeconds(), + dimensions: metricConfig.metricStat.dimensions, + namespace: metricConfig.metricStat.namespace, + metricName: metricConfig.metricStat.metricName, + period: metricConfig.metricStat.period, statistic: stat.type === 'simple' ? stat.statistic : undefined, extendedStatistic: stat.type === 'percentile' ? 'p' + stat.percentile : undefined, unit: this.unit @@ -191,22 +225,45 @@ export class Metric implements IMetric { } public toGraphConfig(): MetricGraphConfig { + const metricConfig = this.toMetricConfig(); + if (metricConfig.metricStat === undefined) { + throw new Error("MetricStatConfig need to set"); + } + return { - dimensions: this.dimensionsAsList(), - namespace: this.namespace, - metricName: this.metricName, + dimensions: metricConfig.metricStat.dimensions, + namespace: metricConfig.metricStat.namespace, + metricName: metricConfig.metricStat.metricName, renderingProperties: { - period: this.period.toSeconds(), - stat: this.statistic, - color: this.color, - label: this.label, + period: metricConfig.metricStat.period, + stat: metricConfig.metricStat.statistic, + color: metricConfig.renderingProperties?.color, + label: metricConfig.renderingProperties?.label }, // deprecated properties for backwards compatibility - period: this.period.toSeconds(), - statistic: this.statistic, - unit: this.unit, - color: this.color, - label: this.label, + period: metricConfig.metricStat.period, + statistic: metricConfig.metricStat.statistic, + color: metricConfig.renderingProperties?.color, + label: metricConfig.renderingProperties?.label, + unit: this.unit + }; + } + + public toMetricConfig(): MetricConfig { + return { + metricStat: { + dimensions: this.dimensionsAsList(), + namespace: this.namespace, + metricName: this.metricName, + period: this.period.toSeconds(), + statistic: this.statistic, + account: this.account, + region: this.region + }, + renderingProperties: { + color: this.color, + label: this.label + } }; } @@ -230,6 +287,114 @@ export class Metric implements IMetric { } } +/** + * A math expression built with metric(s) emitted by a service + * + * The math expression is a combination of an expression (x+y) and metrics to apply expression on. + * It also contains metadata which is used only in graphs, such as color and label. + * It makes sense to embed this in here, so that compound constructs can attach + * that metadata to metrics they expose. + * + * This class does not represent a resource, so hence is not a construct. Instead, + * MathExpression is an abstraction that makes it easy to specify metrics for use in both + * alarms and graphs. + */ +export class MathExpression implements IMetric { + /** + * Grant permissions to the given identity to write metrics. + * + * @param grantee The IAM identity to give permissions to. + */ + public static grantPutMetricData(grantee: iam.IGrantable): iam.Grant { + return iam.Grant.addToPrincipal({ + grantee, + actions: ['cloudwatch:PutMetricData'], + resourceArns: ['*'] + }); + } + + public readonly expression: string; + public readonly expressionMetrics: Record; + public readonly period: cdk.Duration; + public readonly label?: string; + public readonly color?: string; + + constructor(props: MathExpressionProps) { + this.expression = props.expression; + this.expressionMetrics = props.expressionMetrics; + this.label = props.label; + this.color = props.color; + this.period = this.getPeriod(); + + const periodSec = this.period.toSeconds(); + if (periodSec !== 1 && periodSec !== 5 && periodSec !== 10 && periodSec !== 30 && periodSec % 60 !== 0) { + throw new Error(`'period' must be 1, 5, 10, 30, or a multiple of 60 seconds, received ${props.period}`); + } + } + + /** + * 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: MetricOptions): MathExpression { + return new MathExpression({ + expression: this.expression, + expressionMetrics: this.expressionMetrics, + label: ifUndefined(props.label, this.label), + color: ifUndefined(props.color, this.color) + }); + } + + /** + * Make a new Alarm for this metric + * + * Combines both properties that may adjust the metric (aggregation) as well + * as alarm properties. + */ + // public createAlarm(scope: cdk.Construct, id: string, props: CreateAlarmOptions): Alarm { + // //TODO + // } + + public toAlarmConfig(): MetricAlarmConfig { + throw new Error("This method is depricated"); + } + + public toGraphConfig(): MetricGraphConfig { + throw new Error("This method is depricated"); + } + + public toMetricConfig(): MetricConfig { + return { + mathExpression: { + expression: this.expression, + expressionMetrics: this.expressionMetrics + } + }; + } + + public toString() { + return this.label; + } + + private getPeriod(): cdk.Duration { + // TODO: Overall period must be the LCM of expressionMetrics periods. + // However, if different periods are used then most probably something was misconfigured. + let period = cdk.Duration.millis(1); + Object.keys(this.expressionMetrics).forEach(key => { + const metric = this.expressionMetrics[key]; + if (metric instanceof Metric || metric instanceof MathExpression) { + if (period.toMilliseconds() < metric.period.toMilliseconds()) { + period = metric.period; + } + } + }); + return period; + } +} + /** * Properties needed to make an alarm from a metric */ From 1eae9f2ce90cf7e68cbe4c836e1494ee20590a79 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 30 Dec 2019 16:52:49 +0100 Subject: [PATCH 05/38] Consume new API for rendering Alarms --- packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts | 55 ++++++++++++++++++- packages/@aws-cdk/aws-cloudwatch/lib/graph.ts | 6 +- .../aws-cloudwatch/lib/metric-util.ts | 54 +++++++++++------- 3 files changed, 90 insertions(+), 25 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts b/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts index b34ce68c54fc8..430f39221bc8d 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts @@ -4,6 +4,7 @@ import { CfnAlarm } from './cloudwatch.generated'; import { HorizontalAnnotation } from './graph'; import { CreateAlarmOptions } from './metric'; import { IMetric } from './metric-types'; +import { MetricSet } from './metric-util'; import { parseStatistic } from './util.statistic'; export interface IAlarm extends IResource { @@ -121,8 +122,6 @@ export class Alarm extends Resource implements IAlarm { const comparisonOperator = props.comparisonOperator || ComparisonOperator.GREATER_THAN_OR_EQUAL_TO_THRESHOLD; - const config = props.metric.toAlarmConfig(); - const alarm = new CfnAlarm(this, 'Resource', { // Meta alarmDescription: props.alarmDescription, @@ -143,7 +142,7 @@ export class Alarm extends Resource implements IAlarm { okActions: Lazy.listValue({ produce: () => this.okActionArns }), // Metric - ...dropUndef(config), + ...renderAlarmMetric(props.metric), ...dropUndef({ // Alarm overrides period: props.period && props.period.toSeconds(), @@ -228,6 +227,56 @@ export class Alarm extends Resource implements IAlarm { } } +function renderAlarmMetric(metric: IMetric) { + const config = metric.toMetricConfig(); + + if (config.metricStat) { + const st = config.metricStat; + return dropUndef({ + dimensions: st.dimensions, + namespace: st.namespace, + metricName: st.metricName, + period: st.period, + statistic: renderIfSimpleStatistic(st.statistic), + extendedStatistic: renderIfExtendedStatistic(st.statistic), + }); + } else if (config.mathExpression) { + // Expand the math expression metric into a set + const mset = new MetricSet(); + mset.addPrimary(true, metric); + + let mid = 0; + function uniqueMetricId() { + return `mid${++mid}`; + } + + return { + metrics: mset.entries.map(entry => { + const conf = entry.metric.toMetricConfig(); + + return dropUndef({ + expression: conf.mathExpression?.expression, + metricStat: conf.metricStat ? { + metric: { + metricName: conf.metricStat.metricName, + namespace: conf.metricStat.namespace, + dimensions: conf.metricStat.dimensions, + }, + period: conf.metricStat.period, + stat: conf.metricStat.statistic, + // unit: not used, on purpose. Does not do what we need it to do. + } : undefined, + id: entry.id || uniqueMetricId(), + label: conf.renderingProperties?.label, + returnData: entry.tag ? undefined : false, // Tag stores "primary" attribute, default is "true" + } as CfnAlarm.MetricDataQueryProperty); + }) + }; + } else { + throw new Error(`Metric object must have either 'metricStat' or 'mathExpression'`); + } +} + /** * Return a human readable string for this period * diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/graph.ts b/packages/@aws-cdk/aws-cloudwatch/lib/graph.ts index f33cdd473327b..708f755268fc6 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/graph.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/graph.ts @@ -1,7 +1,7 @@ import * as cdk from '@aws-cdk/core'; import { IAlarm } from "./alarm"; import { IMetric } from "./metric-types"; -import { metricGraphJson } from './metric-util'; +import { allMetricsGraphJson } from './metric-util'; import { ConcreteWidget } from "./widget"; /** @@ -170,7 +170,7 @@ export class GraphWidget extends ConcreteWidget { public toJson(): any[] { const horizontalAnnoations = (this.props.leftAnnotations || []).map(mapAnnotation('left')).concat( (this.props.rightAnnotations || []).map(mapAnnotation('right'))); - const metrics = metricGraphJson(this.props.left || [], this.props.right || []); + const metrics = allMetricsGraphJson(this.props.left || [], this.props.right || []); return [{ type: 'metric', width: this.width, @@ -232,7 +232,7 @@ export class SingleValueWidget extends ConcreteWidget { view: 'singleValue', title: this.props.title, region: this.props.region || cdk.Aws.REGION, - metrics: metricGraphJson(this.props.metrics, []), + metrics: allMetricsGraphJson(this.props.metrics, []), setPeriodToTimeRange: this.props.setPeriodToTimeRange } }]; diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts index f824706b255b5..71df50ab189a8 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts @@ -13,18 +13,18 @@ import { IMetric } from "./metric-types"; * * This will be called by GraphWidget, no need for clients to call this. */ -export function metricGraphJson(left: IMetric[], right: IMetric[]): any[][] { +export function allMetricsGraphJson(left: IMetric[], right: IMetric[]): any[][] { // Add metrics to a set which will automatically expand them recursively, // making sure to retain conflicting the visible one on conflicting metrics objects. - const mset = new MetricSet(); - mset.addVisible('left', ...left); - mset.addVisible('right', ...right); + const mset = new MetricSet(); + mset.addPrimary('left', ...left); + mset.addPrimary('right', ...right); // Render all metrics from the set. - return mset.entries.map(entry => metricJson(entry.metric, entry.yAxis, entry.id)); + return mset.entries.map(entry => metricGraphJson(entry.metric, entry.tag, entry.id)); } -function metricJson(metric: IMetric, yAxis?: string, id?: string) { +function metricGraphJson(metric: IMetric, yAxis?: string, id?: string) { const config = metric.toMetricConfig(); const ret: any[] = []; @@ -63,29 +63,45 @@ function metricJson(metric: IMetric, yAxis?: string, id?: string) { return ret; } -interface MetricEntry { +/** + * A single metric in a MetricSet + */ +export interface MetricEntry { + /** + * The metric object + */ metric: IMetric; - yAxis?: string; + + /** + * The tag, added if the object is a primary metric + */ + tag?: A; + + /** + * ID for this metric object + */ id?: string; } /** - * Maintain a set of metrics, expanding math expressions + * Contain a set of metrics, expanding math expressions + * + * "Primary" metrics (added via a top-level call) can be tagged with an additional value. */ -class MetricSet { +export class MetricSet { private readonly metricKeys = new Map(); - private readonly metricMap = new Map(); + private readonly metricMap = new Map>(); /** * Add the given set of metrics to this set */ - public addVisible(yAxis: string, ...metrics: IMetric[]) { + public addPrimary(tag: A, ...metrics: IMetric[]) { for (const metric of metrics) { - this.addOne(metric, yAxis); + this.addOne(metric, tag); } } - public get entries(): MetricEntry[] { + public get entries(): Array> { return Array.from(this.metricMap.values()); } @@ -99,7 +115,7 @@ class MetricSet { * one (and the new ones "renderingPropertieS" will be honored instead of the old * one's). */ - private addOne(metric: IMetric, yAxis?: string, id?: string) { + private addOne(metric: IMetric, tag?: A, id?: string) { const key = this.keyOf(metric); // Record this one @@ -113,16 +129,16 @@ class MetricSet { existing.id = id; } // Replace object if invisible metric is being made visible (to use new one's renderingProperties). - if (yAxis) { - if (existing.yAxis && existing.yAxis !== yAxis) { + if (tag) { + if (existing.tag && existing.tag !== tag) { throw new Error(`Same metric added on both axes: '${metric}' and '${existing.metric}'. Remove one.`); } // Replace object - existing.yAxis = yAxis; + existing.tag = tag; existing.metric = metric; } } else { - this.metricMap.set(key, { metric, yAxis, id }); + this.metricMap.set(key, { metric, tag, id }); } // Recurse and add children From 0844513c0b4b8ff3b10553d7042fa2d81e371411 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 30 Dec 2019 17:27:14 +0100 Subject: [PATCH 06/38] Make period a Duration, add a dispatch helper function, fix compilation --- packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts | 102 +++++++++--------- .../aws-cloudwatch/lib/metric-types.ts | 4 +- .../aws-cloudwatch/lib/metric-util.ts | 85 +++++++++++---- .../@aws-cdk/aws-cloudwatch/lib/metric.ts | 8 +- 4 files changed, 125 insertions(+), 74 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts b/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts index 430f39221bc8d..e394002d1b5bb 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts @@ -4,7 +4,7 @@ import { CfnAlarm } from './cloudwatch.generated'; import { HorizontalAnnotation } from './graph'; import { CreateAlarmOptions } from './metric'; import { IMetric } from './metric-types'; -import { MetricSet } from './metric-util'; +import { dispatchMetric, metricPeriod, MetricSet } from './metric-util'; import { parseStatistic } from './util.statistic'; export interface IAlarm extends IResource { @@ -162,7 +162,7 @@ export class Alarm extends Resource implements IAlarm { this.metric = props.metric; this.annotation = { // tslint:disable-next-line:max-line-length - label: `${this.metric} ${OPERATOR_SYMBOLS[comparisonOperator]} ${props.threshold} for ${props.evaluationPeriods} datapoints within ${describePeriod(props.evaluationPeriods * config.period)}`, + label: `${this.metric} ${OPERATOR_SYMBOLS[comparisonOperator]} ${props.threshold} for ${props.evaluationPeriods} datapoints within ${describePeriod(props.evaluationPeriods * metricPeriod(props.metric).toSeconds())}`, value: props.threshold, }; } @@ -228,53 +228,59 @@ export class Alarm extends Resource implements IAlarm { } function renderAlarmMetric(metric: IMetric) { - const config = metric.toMetricConfig(); - - if (config.metricStat) { - const st = config.metricStat; - return dropUndef({ - dimensions: st.dimensions, - namespace: st.namespace, - metricName: st.metricName, - period: st.period, - statistic: renderIfSimpleStatistic(st.statistic), - extendedStatistic: renderIfExtendedStatistic(st.statistic), - }); - } else if (config.mathExpression) { - // Expand the math expression metric into a set - const mset = new MetricSet(); - mset.addPrimary(true, metric); - - let mid = 0; - function uniqueMetricId() { - return `mid${++mid}`; + return dispatchMetric(metric, { + withStat(st) { + return dropUndef({ + dimensions: st.dimensions, + namespace: st.namespace, + metricName: st.metricName, + period: st.period?.toSeconds(), + statistic: renderIfSimpleStatistic(st.statistic), + extendedStatistic: renderIfExtendedStatistic(st.statistic), + }); + }, + + withExpression() { + // Expand the math expression metric into a set + const mset = new MetricSet(); + mset.addPrimary(true, metric); + + let mid = 0; + function uniqueMetricId() { + return `mid${++mid}`; + } + + return { + metrics: mset.entries.map(entry => dispatchMetric(entry.metric, { + withStat(stat, conf) { + return { + metricStat: { + metric: { + metricName: stat.metricName, + namespace: stat.namespace, + dimensions: stat.dimensions, + }, + period: stat.period.toSeconds(), + stat: stat.statistic, + // unit: not used, on purpose. Does not do what we need it to do. + }, + id: entry.id || uniqueMetricId(), + label: conf.renderingProperties?.label, + returnData: entry.tag ? undefined : false, // Tag stores "primary" attribute, default is "true" + }; + }, + withExpression(expr, conf) { + return { + expression: expr.expression, + id: entry.id || uniqueMetricId(), + label: conf.renderingProperties?.label, + returnData: entry.tag ? undefined : false, // Tag stores "primary" attribute, default is "true" + }; + }, + }) as CfnAlarm.MetricDataQueryProperty) + }; } - - return { - metrics: mset.entries.map(entry => { - const conf = entry.metric.toMetricConfig(); - - return dropUndef({ - expression: conf.mathExpression?.expression, - metricStat: conf.metricStat ? { - metric: { - metricName: conf.metricStat.metricName, - namespace: conf.metricStat.namespace, - dimensions: conf.metricStat.dimensions, - }, - period: conf.metricStat.period, - stat: conf.metricStat.statistic, - // unit: not used, on purpose. Does not do what we need it to do. - } : undefined, - id: entry.id || uniqueMetricId(), - label: conf.renderingProperties?.label, - returnData: entry.tag ? undefined : false, // Tag stores "primary" attribute, default is "true" - } as CfnAlarm.MetricDataQueryProperty); - }) - }; - } else { - throw new Error(`Metric object must have either 'metricStat' or 'mathExpression'`); - } + }); } /** diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts index 76aa37c9183ee..6c132cfa871b0 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts @@ -1,3 +1,5 @@ +import { Duration } from "@aws-cdk/core"; + /** * Interface for metrics */ @@ -130,7 +132,7 @@ export interface MetricStatConfig { /** * How many seconds to aggregate over */ - readonly period: number; + readonly period: Duration; /** * Aggregation function to use (can be either simple or a percentile) diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts index 71df50ab189a8..88026ac66d294 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts @@ -1,4 +1,5 @@ -import { IMetric } from "./metric-types"; +import { Duration } from "@aws-cdk/core"; +import { IMetric, MetricConfig, MetricExpressionConfig, MetricStatConfig } from "./metric-types"; /** * Return the JSON structure which represents these metrics in a graph. @@ -30,27 +31,27 @@ function metricGraphJson(metric: IMetric, yAxis?: string, id?: string) { const ret: any[] = []; const options: any = { ...config.renderingProperties || {} }; - if (config.metricStat) { - // Namespace and metric Name - ret.push( - config.metricStat.namespace, - config.metricStat.metricName, - ); + dispatchMetric(metric, { + withStat(stat) { + ret.push( + stat.namespace, + stat.metricName, + ); - // Dimensions - for (const dim of (config.metricStat.dimensions || [])) { - ret.push(dim.name, dim.value); - } + // Dimensions + for (const dim of (stat.dimensions || [])) { + ret.push(dim.name, dim.value); + } - // Metric attributes that are rendered to graph options - if (config.metricStat.account) { options.accountId = config.metricStat.account; } - if (config.metricStat.region) { options.region = config.metricStat.region; } - } else if (config.mathExpression) { - // Everything goes into the options object - options.expression = config.mathExpression.expression; - } else { - throw new Error(`Metric object must have either 'metricStat' or 'mathExpression'`); - } + // Metric attributes that are rendered to graph options + if (stat.account) { options.accountId = stat.account; } + if (stat.region) { options.region = stat.region; } + }, + + withExpression(expr) { + options.expression = expr.expression; + } + }); // Options if (!yAxis) { options.visible = false; } @@ -191,7 +192,7 @@ function metricKey(metric: IMetric) { parts.push(conf.metricStat.statistic); } if (conf.metricStat.period) { - parts.push(`${conf.metricStat.period}`); + parts.push(`${conf.metricStat.period.toSeconds()}`); } if (conf.metricStat.region) { parts.push(conf.metricStat.region); @@ -202,4 +203,46 @@ function metricKey(metric: IMetric) { } return parts.join('|'); +} + +/** + * Return the period of a metric + * + * For a stat metric, return the immediate period. For an expression metric, + * return the longest period of its constituent metrics (because the math expression + * will only emit a data point if all of its parts emit a data point). + * + * Formally it should be the LCM of the constituent periods, but the max is simpler, + * periods are likely to be multiples of one another anyway, and this is only used + * for display purposes. + */ +export function metricPeriod(metric: IMetric): Duration { + return dispatchMetric(metric, { + withStat(stat) { + return stat.period; + }, + withExpression(expr) { + const metrs = Object.values(expr.expressionMetrics); + let maxPeriod = Duration.seconds(0); + for (const metr of metrs) { + const p = metricPeriod(metr); + if (p.toSeconds() > maxPeriod.toSeconds()) { + maxPeriod = p; + } + } + return maxPeriod; + }, + }); +} + +// tslint:disable-next-line:max-line-length +export function dispatchMetric(metric: IMetric, fns: { withStat: (x: MetricStatConfig, c: MetricConfig) => A, withExpression: (x: MetricExpressionConfig, c: MetricConfig) => B }): A | B { + const conf = metric.toMetricConfig(); + if (conf.metricStat) { + return fns.withStat(conf.metricStat, conf); + } else if (conf.mathExpression) { + return fns.withExpression(conf.mathExpression, conf); + } else { + throw new Error(`Metric object must have either 'metricStat' or 'mathExpression'`); + } } \ No newline at end of file diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts index 8828b00609990..744b804f6a9fa 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts @@ -217,7 +217,7 @@ export class Metric implements IMetric { dimensions: metricConfig.metricStat.dimensions, namespace: metricConfig.metricStat.namespace, metricName: metricConfig.metricStat.metricName, - period: metricConfig.metricStat.period, + period: metricConfig.metricStat.period.toSeconds(), statistic: stat.type === 'simple' ? stat.statistic : undefined, extendedStatistic: stat.type === 'percentile' ? 'p' + stat.percentile : undefined, unit: this.unit @@ -235,13 +235,13 @@ export class Metric implements IMetric { namespace: metricConfig.metricStat.namespace, metricName: metricConfig.metricStat.metricName, renderingProperties: { - period: metricConfig.metricStat.period, + period: metricConfig.metricStat.period.toSeconds(), stat: metricConfig.metricStat.statistic, color: metricConfig.renderingProperties?.color, label: metricConfig.renderingProperties?.label }, // deprecated properties for backwards compatibility - period: metricConfig.metricStat.period, + period: metricConfig.metricStat.period.toSeconds(), statistic: metricConfig.metricStat.statistic, color: metricConfig.renderingProperties?.color, label: metricConfig.renderingProperties?.label, @@ -255,7 +255,7 @@ export class Metric implements IMetric { dimensions: this.dimensionsAsList(), namespace: this.namespace, metricName: this.metricName, - period: this.period.toSeconds(), + period: this.period, statistic: this.statistic, account: this.account, region: this.region From ea6d0b5d503a8ef700c9d1fc2d6c052f8e573552 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 30 Dec 2019 17:53:26 +0100 Subject: [PATCH 07/38] More properly detect empty options object, tests are still failing --- packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts | 12 +---------- .../aws-cloudwatch/lib/metric-util.ts | 20 ++++++++++++++++--- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts b/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts index e394002d1b5bb..f0f0817cffcde 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts @@ -4,7 +4,7 @@ import { CfnAlarm } from './cloudwatch.generated'; import { HorizontalAnnotation } from './graph'; import { CreateAlarmOptions } from './metric'; import { IMetric } from './metric-types'; -import { dispatchMetric, metricPeriod, MetricSet } from './metric-util'; +import { dispatchMetric, dropUndef, metricPeriod, MetricSet } from './metric-util'; import { parseStatistic } from './util.statistic'; export interface IAlarm extends IResource { @@ -295,16 +295,6 @@ function describePeriod(seconds: number) { return seconds + ' seconds'; } -function dropUndef(x: T): T { - const ret: any = {}; - for (const [key, value] of Object.entries(x)) { - if (value !== undefined) { - ret[key] = value; - } - } - return ret; -} - function renderIfSimpleStatistic(statistic?: string): string | undefined { if (statistic === undefined) { return undefined; } diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts index 88026ac66d294..37688896f3a9d 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts @@ -46,6 +46,8 @@ function metricGraphJson(metric: IMetric, yAxis?: string, id?: string) { // Metric attributes that are rendered to graph options if (stat.account) { options.accountId = stat.account; } if (stat.region) { options.region = stat.region; } + if (stat.period && stat.period.toSeconds() !== 300) { options.period = stat.period.toSeconds(); } + if (stat.statistic && stat.statistic !== 'Average') { options.stat = stat.statistic; } }, withExpression(expr) { @@ -58,8 +60,10 @@ function metricGraphJson(metric: IMetric, yAxis?: string, id?: string) { if (yAxis !== 'left') { options.yAxis = yAxis; } if (id) { options.id = id; } - if (Object.keys(options).length !== 0) { - ret.push(options); + const renderedOpts = dropUndef(options); + + if (Object.keys(renderedOpts).length !== 0) { + ret.push(renderedOpts); } return ret; } @@ -245,4 +249,14 @@ export function dispatchMetric(metric: IMetric, fns: { withStat: (x: Metri } else { throw new Error(`Metric object must have either 'metricStat' or 'mathExpression'`); } -} \ No newline at end of file +} + +export function dropUndef(x: T): T { + const ret: any = {}; + for (const [key, value] of Object.entries(x)) { + if (value !== undefined) { + ret[key] = value; + } + } + return ret; +} From 8eeac0dbb903436495dfbc1830a7bc13f6fc3652 Mon Sep 17 00:00:00 2001 From: Ahmed Sedek Date: Mon, 30 Dec 2019 18:21:13 +0100 Subject: [PATCH 08/38] Created BaseMetric --- .../@aws-cdk/aws-cloudwatch/lib/metric.ts | 198 +++++++++--------- 1 file changed, 95 insertions(+), 103 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts index 744b804f6a9fa..0f9b26c75d462 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts @@ -102,6 +102,89 @@ export interface MathExpressionProps extends CommonMetricOptions { readonly expressionMetrics: Record; } +/** + * This is a base class for metrics that has legacy code. + * This is kept only for backward compatibility and shouldn't be used. + */ +abstract class BaseMetric implements IMetric { + public readonly unit?: Unit; + + public constructor(unit?: Unit) { + this.unit = unit; + } + + public abstract toMetricConfig(): MetricConfig; + public abstract with(props: MetricOptions): IMetric; + + public toAlarmConfig(): MetricAlarmConfig { + const metricConfig = this.toMetricConfig(); + if (metricConfig.metricStat === undefined) { + throw new Error("MetricStatConfig must be set."); + } + + const stat = parseStatistic(metricConfig.metricStat.statistic); + return { + dimensions: metricConfig.metricStat.dimensions, + namespace: metricConfig.metricStat.namespace, + metricName: metricConfig.metricStat.metricName, + period: metricConfig.metricStat.period.toSeconds(), + statistic: stat.type === 'simple' ? stat.statistic : undefined, + extendedStatistic: stat.type === 'percentile' ? 'p' + stat.percentile : undefined, + unit: this.unit + }; + } + + public toGraphConfig(): MetricGraphConfig { + const metricConfig = this.toMetricConfig(); + if (metricConfig.metricStat === undefined) { + throw new Error("MetricStatConfig need to set"); + } + + return { + dimensions: metricConfig.metricStat.dimensions, + namespace: metricConfig.metricStat.namespace, + metricName: metricConfig.metricStat.metricName, + renderingProperties: { + period: metricConfig.metricStat.period.toSeconds(), + stat: metricConfig.metricStat.statistic, + color: metricConfig.renderingProperties?.color, + label: metricConfig.renderingProperties?.label + }, + // deprecated properties for backwards compatibility + period: metricConfig.metricStat.period.toSeconds(), + statistic: metricConfig.metricStat.statistic, + color: metricConfig.renderingProperties?.color, + label: metricConfig.renderingProperties?.label, + unit: this.unit + }; + } + + /** + * Make a new Alarm for this metric + * + * Combines both properties that may adjust the metric (aggregation) as well + * as alarm properties. + */ + public createAlarm(scope: cdk.Construct, id: string, props: CreateAlarmOptions): Alarm { + return new Alarm(scope, id, { + metric: this.with({ + statistic: props.statistic, + period: props.period, + }), + alarmName: props.alarmName, + alarmDescription: props.alarmDescription, + comparisonOperator: props.comparisonOperator, + datapointsToAlarm: props.datapointsToAlarm, + threshold: props.threshold, + evaluationPeriods: props.evaluationPeriods, + evaluateLowSampleCountPercentile: props.evaluateLowSampleCountPercentile, + treatMissingData: props.treatMissingData, + actionsEnabled: props.actionsEnabled, + }); + } + +} + /** * A metric emitted by a service * @@ -116,7 +199,7 @@ export interface MathExpressionProps extends CommonMetricOptions { * Metric is an abstraction that makes it easy to specify metrics for use in both * alarms and graphs. */ -export class Metric implements IMetric { +export class Metric extends BaseMetric { /** * Grant permissions to the given identity to write metrics. * @@ -135,13 +218,13 @@ export class Metric implements IMetric { public readonly metricName: string; public readonly period: cdk.Duration; public readonly statistic: string; - public readonly unit?: Unit; public readonly label?: string; public readonly color?: string; public readonly account?: string; public readonly region?: string; constructor(props: MetricProps) { + super(props.unit); this.period = props.period || cdk.Duration.minutes(5); const periodSec = this.period.toSeconds(); if (periodSec !== 1 && periodSec !== 5 && periodSec !== 10 && periodSec !== 30 && periodSec % 60 !== 0) { @@ -155,7 +238,6 @@ export class Metric implements IMetric { this.statistic = normalizeStatistic(props.statistic || "Average"); this.label = props.label; this.color = props.color; - this.unit = props.unit; this.account = props.account; this.region = props.region; } @@ -177,78 +259,11 @@ export class Metric implements IMetric { unit: ifUndefined(props.unit, this.unit), label: ifUndefined(props.label, this.label), color: ifUndefined(props.color, this.color), - account: this.account, - region: this.region + account: ifUndefined(props.account, this.account), + region: ifUndefined(props.region, this.region) }); } - /** - * Make a new Alarm for this metric - * - * Combines both properties that may adjust the metric (aggregation) as well - * as alarm properties. - */ - public createAlarm(scope: cdk.Construct, id: string, props: CreateAlarmOptions): Alarm { - return new Alarm(scope, id, { - metric: this.with({ - statistic: props.statistic, - period: props.period, - }), - alarmName: props.alarmName, - alarmDescription: props.alarmDescription, - comparisonOperator: props.comparisonOperator, - datapointsToAlarm: props.datapointsToAlarm, - threshold: props.threshold, - evaluationPeriods: props.evaluationPeriods, - evaluateLowSampleCountPercentile: props.evaluateLowSampleCountPercentile, - treatMissingData: props.treatMissingData, - actionsEnabled: props.actionsEnabled, - }); - } - - public toAlarmConfig(): MetricAlarmConfig { - const metricConfig = this.toMetricConfig(); - if (metricConfig.metricStat === undefined) { - throw new Error("MetricStat must be set."); - } - - const stat = parseStatistic(metricConfig.metricStat.statistic); - return { - dimensions: metricConfig.metricStat.dimensions, - namespace: metricConfig.metricStat.namespace, - metricName: metricConfig.metricStat.metricName, - period: metricConfig.metricStat.period.toSeconds(), - statistic: stat.type === 'simple' ? stat.statistic : undefined, - extendedStatistic: stat.type === 'percentile' ? 'p' + stat.percentile : undefined, - unit: this.unit - }; - } - - public toGraphConfig(): MetricGraphConfig { - const metricConfig = this.toMetricConfig(); - if (metricConfig.metricStat === undefined) { - throw new Error("MetricStatConfig need to set"); - } - - return { - dimensions: metricConfig.metricStat.dimensions, - namespace: metricConfig.metricStat.namespace, - metricName: metricConfig.metricStat.metricName, - renderingProperties: { - period: metricConfig.metricStat.period.toSeconds(), - stat: metricConfig.metricStat.statistic, - color: metricConfig.renderingProperties?.color, - label: metricConfig.renderingProperties?.label - }, - // deprecated properties for backwards compatibility - period: metricConfig.metricStat.period.toSeconds(), - statistic: metricConfig.metricStat.statistic, - color: metricConfig.renderingProperties?.color, - label: metricConfig.renderingProperties?.label, - unit: this.unit - }; - } - public toMetricConfig(): MetricConfig { return { metricStat: { @@ -299,20 +314,7 @@ export class Metric implements IMetric { * MathExpression is an abstraction that makes it easy to specify metrics for use in both * alarms and graphs. */ -export class MathExpression implements IMetric { - /** - * Grant permissions to the given identity to write metrics. - * - * @param grantee The IAM identity to give permissions to. - */ - public static grantPutMetricData(grantee: iam.IGrantable): iam.Grant { - return iam.Grant.addToPrincipal({ - grantee, - actions: ['cloudwatch:PutMetricData'], - resourceArns: ['*'] - }); - } - +export class MathExpression extends BaseMetric { public readonly expression: string; public readonly expressionMetrics: Record; public readonly period: cdk.Duration; @@ -320,16 +322,12 @@ export class MathExpression implements IMetric { public readonly color?: string; constructor(props: MathExpressionProps) { + super(); this.expression = props.expression; this.expressionMetrics = props.expressionMetrics; this.label = props.label; this.color = props.color; this.period = this.getPeriod(); - - const periodSec = this.period.toSeconds(); - if (periodSec !== 1 && periodSec !== 5 && periodSec !== 10 && periodSec !== 30 && periodSec % 60 !== 0) { - throw new Error(`'period' must be 1, 5, 10, 30, or a multiple of 60 seconds, received ${props.period}`); - } } /** @@ -348,22 +346,12 @@ export class MathExpression implements IMetric { }); } - /** - * Make a new Alarm for this metric - * - * Combines both properties that may adjust the metric (aggregation) as well - * as alarm properties. - */ - // public createAlarm(scope: cdk.Construct, id: string, props: CreateAlarmOptions): Alarm { - // //TODO - // } - public toAlarmConfig(): MetricAlarmConfig { - throw new Error("This method is depricated"); + throw new Error("Cannot use a MathExpression here, use a Metric instead."); } public toGraphConfig(): MetricGraphConfig { - throw new Error("This method is depricated"); + throw new Error("Cannot use a MathExpression here, use a Metric instead."); } public toMetricConfig(): MetricConfig { @@ -371,6 +359,10 @@ export class MathExpression implements IMetric { mathExpression: { expression: this.expression, expressionMetrics: this.expressionMetrics + }, + renderingProperties: { + label: this.label, + color: this.color } }; } From 271130a7da59c6b55c78184da4240d0ba0f20261 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 31 Dec 2019 10:40:35 +0100 Subject: [PATCH 09/38] Fix tests --- .../aws-cloudwatch/test/test.graphs.ts | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudwatch/test/test.graphs.ts b/packages/@aws-cdk/aws-cloudwatch/test/test.graphs.ts index df765cdf86433..24ceb4ae0801a 100644 --- a/packages/@aws-cdk/aws-cloudwatch/test/test.graphs.ts +++ b/packages/@aws-cdk/aws-cloudwatch/test/test.graphs.ts @@ -51,8 +51,8 @@ export = { title: 'My fancy graph', region: { Ref: 'AWS::Region' }, metrics: [ - ['CDK', 'Test', { yAxis: 'left', period: 300, stat: 'Average' }], - ['CDK', 'Tast', { yAxis: 'right', period: 300, stat: 'Average' }] + ['CDK', 'Test'], + ['CDK', 'Tast', { yAxis: 'right' }] ], yAxis: {} } @@ -77,7 +77,7 @@ export = { view: 'timeSeries', region: { Ref: 'AWS::Region' }, metrics: [ - ['CDK', 'Test', { yAxis: 'left', period: 300, stat: 'Average', label: 'MyMetric', color: '000000' }], + ['CDK', 'Test', { label: 'MyMetric', color: '000000' }], ], yAxis: {} } @@ -105,7 +105,7 @@ export = { view: 'singleValue', region: { Ref: 'AWS::Region' }, metrics: [ - ['CDK', 'Test', { yAxis: 'left', period: 300, stat: 'Average' }], + ['CDK', 'Test'], ] } }]); @@ -171,7 +171,7 @@ export = { title: 'My fancy graph', region: { Ref: 'AWS::Region' }, metrics: [ - ['CDK', 'Test', { yAxis: 'left', period: 300, stat: 'Average' }], + ['CDK', 'Test'], ], annotations: { horizontal: [{ yAxis: 'left', @@ -213,7 +213,7 @@ export = { view: 'timeSeries', region: { Ref: 'AWS::Region' }, metrics: [ - ['CDK', 'Test', { yAxis: 'right', period: 300, stat: 'Average' }], + ['CDK', 'Test', { yAxis: 'right' }], ], annotations: { horizontal: [{ @@ -261,8 +261,8 @@ export = { title: 'My fancy graph', region: { Ref: 'AWS::Region' }, metrics: [ - ['CDK', 'Test', { yAxis: 'left', period: 300, stat: 'Average' }], - ['CDK', 'Tast', { yAxis: 'right', period: 300, stat: 'Average' }] + ['CDK', 'Test'], + ['CDK', 'Tast', { yAxis: 'right' }] ], yAxis: { left: { label: "Left yAxis", max: 100 }, @@ -309,7 +309,7 @@ export = { view: 'singleValue', region: { Ref: 'AWS::Region' }, metrics: [ - ['CDK', 'Test', { yAxis: 'left', period: 300, stat: 'Average' }], + ['CDK', 'Test'], ], setPeriodToTimeRange: true } @@ -320,8 +320,8 @@ export = { 'allows overriding custom values of dashboard widgets'(test: Test) { class HiddenMetric extends Metric { - public toGraphConfig(): any { - const ret = super.toGraphConfig(); + public toMetricConfig() { + const ret = super.toMetricConfig(); // @ts-ignore ret.renderingProperties.visible = false; return ret; @@ -338,7 +338,7 @@ export = { // test.ok(widget.toJson()[0].properties.metrics[0].visible === false); test.deepEqual( stack.resolve(widget.toJson())[0].properties.metrics[0], - ["CDK", "Test", { yAxis: 'left', period: 300, stat: 'Average', visible: false }] + ["CDK", "Test", { visible: false }] ); test.done(); From ce6b58b54871fb4c248c376c8ed8e41e51d200bd Mon Sep 17 00:00:00 2001 From: Ahmed Sedek Date: Tue, 31 Dec 2019 11:25:42 +0100 Subject: [PATCH 10/38] Added docstrings --- .../aws-cloudwatch/lib/metric-types.ts | 16 +++- .../@aws-cdk/aws-cloudwatch/lib/metric.ts | 76 +++++++++++++------ 2 files changed, 67 insertions(+), 25 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts index 6c132cfa871b0..6dbf114634073 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts @@ -90,11 +90,15 @@ export enum Unit { export interface MetricConfig { /** * In case the metric represents a query, the details of the query + * + * @default unset */ readonly metricStat?: MetricStatConfig; /** * In case the metric is a math expression, the details of the math expression + * + * @default unset */ readonly mathExpression?: MetricExpressionConfig; @@ -103,6 +107,8 @@ export interface MetricConfig { * * Examples are 'label' and 'color', but any key in here will be * added to dashboard graphs. + * + * @default {} */ readonly renderingProperties?: Record; } @@ -116,6 +122,8 @@ export interface MetricConfig { export interface MetricStatConfig { /** * The dimensions to apply to the alarm + * + * @default [] */ readonly dimensions?: Dimension[]; @@ -140,12 +148,16 @@ export interface MetricStatConfig { readonly statistic: string; /** - * Region of the metric + * Region which this metric comes from. + * + * @default Deployment region. */ readonly region?: string; /** - * Account of the metric + * Account which this metric comes from. + * + * @default Deployment account. */ readonly account?: string; } diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts index 0f9b26c75d462..09aa82d0506a5 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts @@ -42,6 +42,9 @@ export interface CommonMetricOptions { /** * Unit for the metric that is associated with the alarm + * + * @deprecated It is just used to select the data points with this specific unit and not to scale the threshold w.r.t the data points unit. + * It is most likely to be misused. */ readonly unit?: Unit; @@ -72,11 +75,15 @@ export interface MetricProps extends CommonMetricOptions { /** * Account which this metric comes from. + * + * @default Deployment account. */ readonly account?: string; /** * Region which this metric comes from. + * + * @default Deployment region. */ readonly region?: string; } @@ -87,19 +94,31 @@ export interface MetricProps extends CommonMetricOptions { export interface MetricOptions extends CommonMetricOptions { /** * Account which this metric comes from. + * + * @default Deployment account. */ readonly account?: string; /** * Region which this metric comes from. + * + * @default Deployment region. */ readonly region?: string; } +/** + * Properties for a MathExpression + */ export interface MathExpressionProps extends CommonMetricOptions { - readonly expression: string; - - readonly expressionMetrics: Record; + /** + * The expression defining the metric. + */ + readonly expression: string; + /** + * The metrics used in the expression as KeyValuePair . + */ + readonly expressionMetrics: Record; } /** @@ -107,6 +126,12 @@ export interface MathExpressionProps extends CommonMetricOptions { * This is kept only for backward compatibility and shouldn't be used. */ abstract class BaseMetric implements IMetric { + /** + * Unit of the metric. + * + * @default None + * @deprecated Deprecated in MetricProps. + */ public readonly unit?: Unit; public constructor(unit?: Unit) { @@ -119,7 +144,7 @@ abstract class BaseMetric implements IMetric { public toAlarmConfig(): MetricAlarmConfig { const metricConfig = this.toMetricConfig(); if (metricConfig.metricStat === undefined) { - throw new Error("MetricStatConfig must be set."); + throw new Error("A `Metric` object must be used here."); } const stat = parseStatistic(metricConfig.metricStat.statistic); @@ -220,7 +245,17 @@ export class Metric extends BaseMetric { public readonly statistic: string; public readonly label?: string; public readonly color?: string; + /** + * Account which this metric comes from. + * + * @default Deployment account. + */ public readonly account?: string; + /** + * Region which this metric comes from. + * + * @default Deployment region. + */ public readonly region?: string; constructor(props: MetricProps) { @@ -315,10 +350,21 @@ export class Metric extends BaseMetric { * alarms and graphs. */ export class MathExpression extends BaseMetric { + /** + * The expression defining the metric. + */ public readonly expression: string; + /** + * The metrics used in the expression as KeyValuePair . + */ public readonly expressionMetrics: Record; - public readonly period: cdk.Duration; + /** + * Label for this metric when added to a Graph. + */ public readonly label?: string; + /** + * Color for this metric when added to a Graph. + */ public readonly color?: string; constructor(props: MathExpressionProps) { @@ -327,7 +373,6 @@ export class MathExpression extends BaseMetric { this.expressionMetrics = props.expressionMetrics; this.label = props.label; this.color = props.color; - this.period = this.getPeriod(); } /** @@ -347,11 +392,11 @@ export class MathExpression extends BaseMetric { } public toAlarmConfig(): MetricAlarmConfig { - throw new Error("Cannot use a MathExpression here, use a Metric instead."); + throw new Error("A `Metric` object must be used here."); } public toGraphConfig(): MetricGraphConfig { - throw new Error("Cannot use a MathExpression here, use a Metric instead."); + throw new Error("A `Metric` object must be used here."); } public toMetricConfig(): MetricConfig { @@ -370,21 +415,6 @@ export class MathExpression extends BaseMetric { public toString() { return this.label; } - - private getPeriod(): cdk.Duration { - // TODO: Overall period must be the LCM of expressionMetrics periods. - // However, if different periods are used then most probably something was misconfigured. - let period = cdk.Duration.millis(1); - Object.keys(this.expressionMetrics).forEach(key => { - const metric = this.expressionMetrics[key]; - if (metric instanceof Metric || metric instanceof MathExpression) { - if (period.toMilliseconds() < metric.period.toMilliseconds()) { - period = metric.period; - } - } - }); - return period; - } } /** From 63793537dc20fac850a86e8abff957226bd718e1 Mon Sep 17 00:00:00 2001 From: Ahmed Sedek Date: Tue, 31 Dec 2019 12:05:03 +0100 Subject: [PATCH 11/38] Updated autoscaling libs to use new API --- .../lib/step-scaling-policy.ts | 2 +- .../lib/target-tracking-scaling-policy.ts | 9 +- .../lib/step-scaling-policy.ts | 2 +- .../lib/target-tracking-scaling-policy.ts | 9 +- .../@aws-cdk/aws-cloudwatch/lib/metric.ts | 3 +- yarn.lock | 89 ++++++++++++++++++- 6 files changed, 103 insertions(+), 11 deletions(-) diff --git a/packages/@aws-cdk/aws-applicationautoscaling/lib/step-scaling-policy.ts b/packages/@aws-cdk/aws-applicationautoscaling/lib/step-scaling-policy.ts index 6441f3180de96..018232499f6a0 100644 --- a/packages/@aws-cdk/aws-applicationautoscaling/lib/step-scaling-policy.ts +++ b/packages/@aws-cdk/aws-applicationautoscaling/lib/step-scaling-policy.ts @@ -183,7 +183,7 @@ export interface ScalingInterval { } function aggregationTypeFromMetric(metric: cloudwatch.IMetric): MetricAggregationType { - const statistic = metric.toAlarmConfig().statistic; + const statistic = metric.toMetricConfig().metricStat?.statistic; switch (statistic) { case 'Average': return MetricAggregationType.AVERAGE; diff --git a/packages/@aws-cdk/aws-applicationautoscaling/lib/target-tracking-scaling-policy.ts b/packages/@aws-cdk/aws-applicationautoscaling/lib/target-tracking-scaling-policy.ts index 27bba82adab3a..d086f1861c2fb 100644 --- a/packages/@aws-cdk/aws-applicationautoscaling/lib/target-tracking-scaling-policy.ts +++ b/packages/@aws-cdk/aws-applicationautoscaling/lib/target-tracking-scaling-policy.ts @@ -140,7 +140,11 @@ export class TargetTrackingScalingPolicy extends cdk.Construct { function renderCustomMetric(metric?: cloudwatch.IMetric): CfnScalingPolicy.CustomizedMetricSpecificationProperty | undefined { if (!metric) { return undefined; } - const c = metric.toAlarmConfig(); + const c = metric.toMetricConfig().metricStat; + + if (c === undefined) { + throw new Error("A `Metric` object must be used here."); + } if (!c.statistic) { throw new Error('Can only use Average, Minimum, Maximum, SampleCount, Sum statistic for target tracking'); @@ -150,8 +154,7 @@ function renderCustomMetric(metric?: cloudwatch.IMetric): CfnScalingPolicy.Custo dimensions: c.dimensions, metricName: c.metricName, namespace: c.namespace, - statistic: c.statistic, - unit: c.unit + statistic: c.statistic }; } diff --git a/packages/@aws-cdk/aws-autoscaling/lib/step-scaling-policy.ts b/packages/@aws-cdk/aws-autoscaling/lib/step-scaling-policy.ts index 452c741acf570..e91fb10305427 100644 --- a/packages/@aws-cdk/aws-autoscaling/lib/step-scaling-policy.ts +++ b/packages/@aws-cdk/aws-autoscaling/lib/step-scaling-policy.ts @@ -147,7 +147,7 @@ export class StepScalingPolicy extends cdk.Construct { } function aggregationTypeFromMetric(metric: cloudwatch.IMetric): MetricAggregationType { - const statistic = metric.toAlarmConfig().statistic; + const statistic = metric.toMetricConfig().metricStat?.statistic; switch (statistic) { case 'Average': return MetricAggregationType.AVERAGE; diff --git a/packages/@aws-cdk/aws-autoscaling/lib/target-tracking-scaling-policy.ts b/packages/@aws-cdk/aws-autoscaling/lib/target-tracking-scaling-policy.ts index 5368c74f08d7f..ef126b7a2d4e5 100644 --- a/packages/@aws-cdk/aws-autoscaling/lib/target-tracking-scaling-policy.ts +++ b/packages/@aws-cdk/aws-autoscaling/lib/target-tracking-scaling-policy.ts @@ -141,7 +141,11 @@ export class TargetTrackingScalingPolicy extends cdk.Construct { function renderCustomMetric(metric?: cloudwatch.IMetric): CfnScalingPolicy.CustomizedMetricSpecificationProperty | undefined { if (!metric) { return undefined; } - const c = metric.toAlarmConfig(); + const c = metric.toMetricConfig().metricStat; + + if (c === undefined) { + throw new Error("A `Metric` object must be used here."); + } if (!c.statistic) { throw new Error('Can only use Average, Minimum, Maximum, SampleCount, Sum statistic for target tracking'); @@ -151,8 +155,7 @@ function renderCustomMetric(metric?: cloudwatch.IMetric): CfnScalingPolicy.Custo dimensions: c.dimensions, metricName: c.metricName, namespace: c.namespace, - statistic: c.statistic, - unit: c.unit + statistic: c.statistic }; } diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts index 09aa82d0506a5..08383a5b03a7b 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts @@ -300,9 +300,10 @@ export class Metric extends BaseMetric { } public toMetricConfig(): MetricConfig { + const dims = this.dimensionsAsList(); return { metricStat: { - dimensions: this.dimensionsAsList(), + dimensions: dims.length > 0 ? dims : undefined, namespace: this.namespace, metricName: this.metricName, period: this.period, diff --git a/yarn.lock b/yarn.lock index 543a82e175bfd..437937d7cab33 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1668,6 +1668,11 @@ anymatch@^2.0.0: micromatch "^3.1.4" normalize-path "^2.1.1" +app-root-path@^2.2.1: + version "2.2.1" + resolved "https://registry.yarnpkg.com/app-root-path/-/app-root-path-2.2.1.tgz#d0df4a682ee408273583d43f6f79e9892624bc9a" + integrity sha512-91IFKeKk7FjfmezPKkwtaRvSpnUc4gDwPAjA1YZ9Gn0q0PPeW+vbeUsZuyDwjI7+QTHhcLen2v25fi/AmhvbJA== + append-transform@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/append-transform/-/append-transform-1.0.0.tgz#046a52ae582a228bd72f58acfbe2967c678759ab" @@ -3179,6 +3184,16 @@ dot-prop@^4.2.0: dependencies: is-obj "^1.0.0" +dotenv-json@^1.0.0: + version "1.0.0" + resolved "https://registry.yarnpkg.com/dotenv-json/-/dotenv-json-1.0.0.tgz#fc7f672aafea04bed33818733b9f94662332815c" + integrity sha512-jAssr+6r4nKhKRudQ0HOzMskOFFi9+ubXWwmrSGJFgTvpjyPXCXsCsYbjif6mXp7uxA7xY3/LGaiTQukZzSbOQ== + +dotenv@^8.0.0: + version "8.2.0" + resolved "https://registry.yarnpkg.com/dotenv/-/dotenv-8.2.0.tgz#97e619259ada750eea3e4ea3e26bceea5424b16a" + integrity sha512-8sJ78ElpbDJBHNeBzUbUVLsqKdccaa/BXF1uPTw3GrvQTBgrQrtObr2mUrE38vzYd8cEv+m/JBfDLioYcfXoaw== + dreamopt@~0.6.0: version "0.6.0" resolved "https://registry.yarnpkg.com/dreamopt/-/dreamopt-0.6.0.tgz#d813ccdac8d39d8ad526775514a13dda664d6b4b" @@ -3351,6 +3366,11 @@ escodegen@^1.9.1: optionalDependencies: source-map "~0.6.1" +eslint-config-standard@^14.1.0: + version "14.1.0" + resolved "https://registry.yarnpkg.com/eslint-config-standard/-/eslint-config-standard-14.1.0.tgz#b23da2b76fe5a2eba668374f246454e7058f15d4" + integrity sha512-EF6XkrrGVbvv8hL/kYa/m6vnvmUT+K82pJJc4JJVMM6+Qgqh0pnwprSxdduDLB9p/7bIxD+YV5O0wfb8lmcPbA== + eslint-import-resolver-node@^0.3.2: version "0.3.2" resolved "https://registry.yarnpkg.com/eslint-import-resolver-node/-/eslint-import-resolver-node-0.3.2.tgz#58f15fb839b8d0576ca980413476aab2472db66a" @@ -3378,6 +3398,14 @@ eslint-module-utils@^2.4.1: debug "^2.6.9" pkg-dir "^2.0.0" +eslint-plugin-es@^2.0.0: + version "2.0.0" + resolved "https://registry.yarnpkg.com/eslint-plugin-es/-/eslint-plugin-es-2.0.0.tgz#0f5f5da5f18aa21989feebe8a73eadefb3432976" + integrity sha512-f6fceVtg27BR02EYnBhgWLFQfK6bN4Ll0nQFrBHOlCsAyxeZkn0NHns5O0YZOPrV1B3ramd6cgFwaoFLcSkwEQ== + dependencies: + eslint-utils "^1.4.2" + regexpp "^3.0.0" + eslint-plugin-import@^2.19.1: version "2.19.1" resolved "https://registry.yarnpkg.com/eslint-plugin-import/-/eslint-plugin-import-2.19.1.tgz#5654e10b7839d064dd0d46cd1b88ec2133a11448" @@ -3396,6 +3424,28 @@ eslint-plugin-import@^2.19.1: read-pkg-up "^2.0.0" resolve "^1.12.0" +eslint-plugin-node@^10.0.0: + version "10.0.0" + resolved "https://registry.yarnpkg.com/eslint-plugin-node/-/eslint-plugin-node-10.0.0.tgz#fd1adbc7a300cf7eb6ac55cf4b0b6fc6e577f5a6" + integrity sha512-1CSyM/QCjs6PXaT18+zuAXsjXGIGo5Rw630rSKwokSs2jrYURQc4R5JZpoanNCqwNmepg+0eZ9L7YiRUJb8jiQ== + dependencies: + eslint-plugin-es "^2.0.0" + eslint-utils "^1.4.2" + ignore "^5.1.1" + minimatch "^3.0.4" + resolve "^1.10.1" + semver "^6.1.0" + +eslint-plugin-promise@^4.2.1: + version "4.2.1" + resolved "https://registry.yarnpkg.com/eslint-plugin-promise/-/eslint-plugin-promise-4.2.1.tgz#845fd8b2260ad8f82564c1222fce44ad71d9418a" + integrity sha512-VoM09vT7bfA7D+upt+FjeBO5eHIJQBUWki1aPvB+vbNiHS3+oGIJGIeyBtKQTME6UPXXy3vV07OL1tHd3ANuDw== + +eslint-plugin-standard@^4.0.1: + version "4.0.1" + resolved "https://registry.yarnpkg.com/eslint-plugin-standard/-/eslint-plugin-standard-4.0.1.tgz#ff0519f7ffaff114f76d1bd7c3996eef0f6e20b4" + integrity sha512-v/KBnfyaOMPmZc/dmc6ozOdWqekGp7bBGq4jLAecEfPGmfKiWS4sA8sC0LqiV9w5qmXAtXVn4M3p1jSyhY85SQ== + eslint-scope@^5.0.0: version "5.0.0" resolved "https://registry.yarnpkg.com/eslint-scope/-/eslint-scope-5.0.0.tgz#e87c8887c73e8d1ec84f1ca591645c358bfc8fb9" @@ -3404,7 +3454,7 @@ eslint-scope@^5.0.0: esrecurse "^4.1.0" estraverse "^4.1.1" -eslint-utils@^1.4.3: +eslint-utils@^1.4.2, eslint-utils@^1.4.3: version "1.4.3" resolved "https://registry.yarnpkg.com/eslint-utils/-/eslint-utils-1.4.3.tgz#74fec7c54d0776b6f67e0251040b5806564e981f" integrity sha512-fbBN5W2xdY45KulGXmLHZ3c3FHfVYmKg0IrAKGOkT/464PQsx2UeIzfz1RmEci+KLm1bBaAzZAh8+/E+XAeZ8Q== @@ -4340,6 +4390,11 @@ ignore@^4.0.3, ignore@^4.0.6: resolved "https://registry.yarnpkg.com/ignore/-/ignore-4.0.6.tgz#750e3db5862087b4737ebac8207ffd1ef27b25fc" integrity sha512-cyFDKrqc/YdcWFniJhzI42+AzS+gNwmUzOSFcRCQYwySuBBBy/KjuxWLZ/FHEH6Moq1NizMOBWyTcv8O4OZIMg== +ignore@^5.1.1: + version "5.1.4" + resolved "https://registry.yarnpkg.com/ignore/-/ignore-5.1.4.tgz#84b7b3dbe64552b6ef0eca99f6743dbec6d97adf" + integrity sha512-MzbUSahkTW1u7JpKKjY7LCARd1fU5W2rLdxlM4kdkayuCwZImjkpluF9CM1aLewYJguPDqewLam18Y6AU69A8A== + immediate@~3.0.5: version "3.0.6" resolved "https://registry.yarnpkg.com/immediate/-/immediate-3.0.6.tgz#9db1dbd0faf8de6fbe0f5dd5e56bb606280de69b" @@ -5474,6 +5529,24 @@ kleur@^3.0.3: resolved "https://registry.yarnpkg.com/kleur/-/kleur-3.0.3.tgz#a79c9ecc86ee1ce3fa6206d1216c501f147fc07e" integrity sha512-eTIzlVOSUR+JxdDFepEYcBMtZ9Qqdef+rnzWdRZuMbOywu5tO2w2N7rqjoANZ5k9vywhL6Br1VRjUIgTQx4E8w== +lambda-leak@^2.0.0: + version "2.0.0" + resolved "https://registry.yarnpkg.com/lambda-leak/-/lambda-leak-2.0.0.tgz#771985d3628487f6e885afae2b54510dcfb2cd7e" + integrity sha1-dxmF02KEh/boha+uK1RRDc+yzX4= + +lambda-tester@^3.6.0: + version "3.6.0" + resolved "https://registry.yarnpkg.com/lambda-tester/-/lambda-tester-3.6.0.tgz#ceb7d4f4f0da768487a05cff37dcd088508b5247" + integrity sha512-F2ZTGWCLyIR95o/jWK46V/WnOCFAEUG/m/V7/CLhPJ7PCM+pror1rZ6ujP3TkItSGxUfpJi0kqwidw+M/nEqWw== + dependencies: + app-root-path "^2.2.1" + dotenv "^8.0.0" + dotenv-json "^1.0.0" + lambda-leak "^2.0.0" + semver "^6.1.1" + uuid "^3.3.2" + vandium-utils "^1.1.1" + lazystream@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/lazystream/-/lazystream-1.0.0.tgz#f6995fe0f820392f61396be89462407bb77168e4" @@ -7446,6 +7519,13 @@ resolve@1.x, resolve@^1.10.0, resolve@^1.11.1, resolve@^1.3.2: dependencies: path-parse "^1.0.6" +resolve@^1.10.1: + version "1.14.1" + resolved "https://registry.yarnpkg.com/resolve/-/resolve-1.14.1.tgz#9e018c540fcf0c427d678b9931cbf45e984bcaff" + integrity sha512-fn5Wobh4cxbLzuHaE+nphztHy43/b++4M6SsGFC2gB8uYwf0C8LcarfCz1un7UTW8OFQg9iNjZ4xpcFVGebDPg== + dependencies: + path-parse "^1.0.6" + resolve@^1.12.0, resolve@^1.5.0: version "1.13.1" resolved "https://registry.yarnpkg.com/resolve/-/resolve-1.13.1.tgz#be0aa4c06acd53083505abb35f4d66932ab35d16" @@ -7583,7 +7663,7 @@ sax@>=0.6.0, sax@^1.2.4: resolved "https://registry.yarnpkg.com/semver/-/semver-5.7.1.tgz#a954f931aeba508d307bbf069eff0c01c96116f7" integrity sha512-sauaDf/PZdVgrLTNYHRtpXa1iRiKcaebiKQ1BJdpQlWH2lCvexQdX55snPFyK7QzpudqbCI0qXFfOasHdyNDGQ== -semver@^6.0.0, semver@^6.1.2, semver@^6.2.0, semver@^6.3.0: +semver@^6.0.0, semver@^6.1.0, semver@^6.1.1, semver@^6.1.2, semver@^6.2.0, semver@^6.3.0: version "6.3.0" resolved "https://registry.yarnpkg.com/semver/-/semver-6.3.0.tgz#ee0a64c8af5e8ceea67687b133761e1becbd1d3d" integrity sha512-b39TBaTSfV6yBrapU89p5fKekE2m/NwnDocOVruQFS1/veMgdzuPcnOM34M6CwxW8jH/lxEa5rBoDeUwu5HHTw== @@ -8733,6 +8813,11 @@ validate-npm-package-name@^3.0.0: dependencies: builtins "^1.0.3" +vandium-utils@^1.1.1: + version "1.2.0" + resolved "https://registry.yarnpkg.com/vandium-utils/-/vandium-utils-1.2.0.tgz#44735de4b7641a05de59ebe945f174e582db4f59" + integrity sha1-RHNd5LdkGgXeWevpRfF05YLbT1k= + verror@1.10.0: version "1.10.0" resolved "https://registry.yarnpkg.com/verror/-/verror-1.10.0.tgz#3a105ca17053af55d6e270c1f8288682e18da400" From b77aefeeae578ba456f45070fd469aa4c6b08772 Mon Sep 17 00:00:00 2001 From: Ahmed Sedek Date: Tue, 31 Dec 2019 12:25:54 +0100 Subject: [PATCH 12/38] Updated README with MathExpression --- packages/@aws-cdk/aws-cloudwatch/README.md | 26 ++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/packages/@aws-cdk/aws-cloudwatch/README.md b/packages/@aws-cdk/aws-cloudwatch/README.md index 77f8f5873489e..d2734fd2eed5f 100644 --- a/packages/@aws-cdk/aws-cloudwatch/README.md +++ b/packages/@aws-cdk/aws-cloudwatch/README.md @@ -17,6 +17,8 @@ attributes. Resources that expose metrics will have functions that look like `metricXxx()` which will return a Metric object, initialized with defaults that make sense. +#### Exposed Metrics + For example, `lambda.Function` objects have the `fn.metricErrors()` method, which represents the amount of errors reported by that Lambda function: @@ -24,6 +26,30 @@ represents the amount of errors reported by that Lambda function: const errors = fn.metricErrors(); ``` +#### Constructed Metrics + +Constructed metrics could be a regular metric or a math expression. + +For example, a math expression that sums two regular metrics: +```ts +const mathExpression = new MathExpression({ + expression: "x+y", + expressionMetrics: { + x: new Metric(...metricProps), + y: new Metric(...otherMetricProps) + } +}) +``` +Moreover, math expressions could be nested: +```ts +const nestedMathExpression = new MathExpression({ + expression: "z/100", + expressionMetrics: { + z: mathExpression + } +}) +``` + ### Aggregation To graph or alarm on metrics you must aggregate them first, using a function From 02194cb9cde51d32434947b4db0b8ea7bc384469 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 31 Dec 2019 12:27:28 +0100 Subject: [PATCH 13/38] Adding tests, still breaking --- .../aws-cloudwatch/lib/metric-util.ts | 74 +++++++++++++------ .../@aws-cdk/aws-cloudwatch/lib/metric.ts | 28 +++---- 2 files changed, 61 insertions(+), 41 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts index 37688896f3a9d..047c6fc831eff 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts @@ -60,6 +60,13 @@ function metricGraphJson(metric: IMetric, yAxis?: string, id?: string) { if (yAxis !== 'left') { options.yAxis = yAxis; } if (id) { options.id = id; } + // If math expressions don't have a label (or an ID), they'll render in an ugly + // way (like "metric_alias0"). The ID may be autogenerated in a silly way, so if a + // mathexpression doesn't have a label, use its toString() as the label. + if (options.visible !== false && options.expression && !options.label) { + options.label = `${metric}`; + } + const renderedOpts = dropUndef(options); if (Object.keys(renderedOpts).length !== 0) { @@ -75,7 +82,7 @@ export interface MetricEntry { /** * The metric object */ - metric: IMetric; + readonly metric: IMetric; /** * The tag, added if the object is a primary metric @@ -95,7 +102,9 @@ export interface MetricEntry { */ export class MetricSet { private readonly metricKeys = new Map(); - private readonly metricMap = new Map>(); + private readonly metrics = new Array>(); + private readonly metricById = new Map>(); + private readonly metricByKey = new Map>(); /** * Add the given set of metrics to this set @@ -107,14 +116,13 @@ export class MetricSet { } public get entries(): Array> { - return Array.from(this.metricMap.values()); + return this.metrics; } /** * Add a metric into the set * - * If it already exists, either it must be getting a new id or the id must be - * the same as before. + * The id may not be the same as a previous metric added, unless it's the same metric. * * It can be made visible, in which case the new "metric" object replaces the old * one (and the new ones "renderingPropertieS" will be honored instead of the old @@ -123,27 +131,45 @@ export class MetricSet { private addOne(metric: IMetric, tag?: A, id?: string) { const key = this.keyOf(metric); - // Record this one - const existing = this.metricMap.get(key); - if (existing) { - // Set id on previously unnamed object - if (id) { - if (existing.id && existing.id !== id) { - throw new Error(`Same id used for two metrics in the same graph: '${metric}' and '${existing.metric}'. Rename one.`); - } - existing.id = id; - } - // Replace object if invisible metric is being made visible (to use new one's renderingProperties). - if (tag) { - if (existing.tag && existing.tag !== tag) { - throw new Error(`Same metric added on both axes: '${metric}' and '${existing.metric}'. Remove one.`); - } - // Replace object - existing.tag = tag; - existing.metric = metric; + let existingEntry: MetricEntry | undefined; + + // Try lookup existing by id if we have one + if (id) { + existingEntry = this.metricById.get(id); + if (existingEntry && this.keyOf(existingEntry.metric) !== key) { + throw new Error(`Same id used for two metrics in the same graph: '${metric}' and '${existingEntry.metric}'. Rename one.`); } + } + + if (!existingEntry) { + // Try lookup by metric if we didn't find one by id + existingEntry = this.metricByKey.get(key); + + // If the one we found already has an id, it must be different from the id + // we're trying to add and we want to add a new metric. Pretend we didn't + // find one. + if (existingEntry?.id && id) { existingEntry = undefined; } + } + + // Create a new entry if we didn't find one so far + let entry; + if (existingEntry) { + entry = existingEntry; } else { - this.metricMap.set(key, { metric, tag, id }); + entry = { metric }; + this.metrics.push(entry); + this.metricByKey.set(key, entry); + } + + // If it didn't have an id but now we do, add one + if (!entry.id && id) { + entry.id = id; + this.metricById.set(id, entry); + } + + // If it didn't have a tag but now we do, add one + if (!entry.tag && tag) { + entry.tag = tag; } // Recurse and add children diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts index 0f9b26c75d462..b6a81c2696578 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts @@ -317,7 +317,6 @@ export class Metric extends BaseMetric { export class MathExpression extends BaseMetric { public readonly expression: string; public readonly expressionMetrics: Record; - public readonly period: cdk.Duration; public readonly label?: string; public readonly color?: string; @@ -327,7 +326,11 @@ export class MathExpression extends BaseMetric { this.expressionMetrics = props.expressionMetrics; this.label = props.label; this.color = props.color; - this.period = this.getPeriod(); + + const invalidVariableNames = Object.keys(props.expressionMetrics).filter(x => !validVariableName(x)); + if (invalidVariableNames.length > 0) { + throw new Error(`Invalid variable names in expression: ${invalidVariableNames}. Must start with lowercase letter and only contain alphanumerics.`); + } } /** @@ -368,23 +371,14 @@ export class MathExpression extends BaseMetric { } public toString() { - return this.label; + return this.label || this.expression; } +} - private getPeriod(): cdk.Duration { - // TODO: Overall period must be the LCM of expressionMetrics periods. - // However, if different periods are used then most probably something was misconfigured. - let period = cdk.Duration.millis(1); - Object.keys(this.expressionMetrics).forEach(key => { - const metric = this.expressionMetrics[key]; - if (metric instanceof Metric || metric instanceof MathExpression) { - if (period.toMilliseconds() < metric.period.toMilliseconds()) { - period = metric.period; - } - } - }); - return period; - } +const VALID_VARIABLE = new RegExp('^[a-z][a-zA-Z0-9_]*$'); + +function validVariableName(x: string) { + return VALID_VARIABLE.test(x); } /** From 6a18672ccdfab3a7292e156abe83b02cb00a0003 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 31 Dec 2019 12:52:22 +0100 Subject: [PATCH 14/38] Make more tests pass --- .../aws-cloudwatch/lib/metric-util.ts | 51 +++++++++++++------ .../@aws-cdk/aws-cloudwatch/lib/metric.ts | 3 ++ 2 files changed, 38 insertions(+), 16 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts index 047c6fc831eff..3ae0cf9d98e08 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts @@ -1,4 +1,5 @@ import { Duration } from "@aws-cdk/core"; +import { MathExpression } from "./metric"; import { IMetric, MetricConfig, MetricExpressionConfig, MetricStatConfig } from "./metric-types"; /** @@ -101,7 +102,6 @@ export interface MetricEntry { * "Primary" metrics (added via a top-level call) can be tagged with an additional value. */ export class MetricSet { - private readonly metricKeys = new Map(); private readonly metrics = new Array>(); private readonly metricById = new Map>(); private readonly metricByKey = new Map>(); @@ -129,15 +129,15 @@ export class MetricSet { * one's). */ private addOne(metric: IMetric, tag?: A, id?: string) { - const key = this.keyOf(metric); + const key = metricKey(metric); let existingEntry: MetricEntry | undefined; // Try lookup existing by id if we have one if (id) { existingEntry = this.metricById.get(id); - if (existingEntry && this.keyOf(existingEntry.metric) !== key) { - throw new Error(`Same id used for two metrics in the same graph: '${metric}' and '${existingEntry.metric}'. Rename one.`); + if (existingEntry && metricKey(existingEntry.metric) !== key) { + throw new Error(`Can't happen, already checked elsewhere`); } } @@ -180,27 +180,44 @@ export class MetricSet { } } } +} - /** - * Cached version of metricKey - */ - private keyOf(metric: IMetric) { - const existing = this.metricKeys.get(metric); - if (existing) { return existing; } - - const key = metricKey(metric); - this.metricKeys.set(metric, key); - return key; +export function validateNoIdConflicts(expression: MathExpression) { + const seen = new Map(); + visit(expression); + + function visit(metric: IMetric) { + dispatchMetric(metric, { + withStat() { + // Nothing + }, + withExpression(expr) { + for (const [id, subMetric] of Object.entries(expr.expressionMetrics)) { + const existing = seen.get(id); + if (existing && metricKey(existing) !== metricKey(subMetric)) { + throw new Error(`Same id ('${id}') used for two metrics in the expression: '${subMetric}' and '${existing}'. Rename one.`); + } + seen.set(id, subMetric); + visit(subMetric); + } + } + }); } } +const METRICKEY_SYMBOL = Symbol('@aws-cdk/aws-cloudwatch.MetricKey'); + /** * Return a unique string representation for this metric. * * Can be used to determine as a hash key to determine if 2 Metric objects * represent the same metric. Excludes rendering properties. */ -function metricKey(metric: IMetric) { +function metricKey(metric: IMetric): string { + // Cache on the object itself. This is safe because Metric objects are immutable. + if ((metric as any)[METRICKEY_SYMBOL] !== undefined) { + return (metric as any)[METRICKEY_SYMBOL]; + } const parts = new Array(); const conf = metric.toMetricConfig(); @@ -232,7 +249,9 @@ function metricKey(metric: IMetric) { } } - return parts.join('|'); + const ret = parts.join('|'); + Object.defineProperty(metric, METRICKEY_SYMBOL, { value: ret }); + return ret; } /** diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts index 954f36ff867f8..cb9fff5828529 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts @@ -2,6 +2,7 @@ import * as iam from '@aws-cdk/aws-iam'; import * as cdk from '@aws-cdk/core'; import { Alarm, ComparisonOperator, TreatMissingData } from './alarm'; import { Dimension, IMetric, MetricAlarmConfig, MetricConfig, MetricGraphConfig, Unit } from './metric-types'; +import { validateNoIdConflicts } from './metric-util'; import { normalizeStatistic, parseStatistic } from './util.statistic'; export type DimensionHash = {[dim: string]: any}; @@ -382,6 +383,8 @@ export class MathExpression extends BaseMetric { if (invalidVariableNames.length > 0) { throw new Error(`Invalid variable names in expression: ${invalidVariableNames}. Must start with lowercase letter and only contain alphanumerics.`); } + + validateNoIdConflicts(this); } /** From ca15a28f4e28b75a006f5880bec669576322f1e3 Mon Sep 17 00:00:00 2001 From: Ahmed Sedek Date: Tue, 31 Dec 2019 13:03:02 +0100 Subject: [PATCH 15/38] Fixed integ test --- .../test/integ.alarm-and-dashboard.expected.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudwatch/test/integ.alarm-and-dashboard.expected.json b/packages/@aws-cdk/aws-cloudwatch/test/integ.alarm-and-dashboard.expected.json index 282ae62636a42..ae3e4a31cd67d 100644 --- a/packages/@aws-cdk/aws-cloudwatch/test/integ.alarm-and-dashboard.expected.json +++ b/packages/@aws-cdk/aws-cloudwatch/test/integ.alarm-and-dashboard.expected.json @@ -56,7 +56,7 @@ "QueueName" ] }, - "\",{\"yAxis\":\"left\",\"period\":300,\"stat\":\"Average\"}]],\"annotations\":{\"horizontal\":[{\"label\":\"ApproximateNumberOfMessagesVisible >= 100 for 3 datapoints within 15 minutes\",\"value\":100,\"yAxis\":\"left\"}]},\"yAxis\":{}}},{\"type\":\"metric\",\"width\":6,\"height\":3,\"x\":0,\"y\":14,\"properties\":{\"view\":\"singleValue\",\"title\":\"Current messages in queue\",\"region\":\"", + "\"]],\"annotations\":{\"horizontal\":[{\"label\":\"ApproximateNumberOfMessagesVisible >= 100 for 3 datapoints within 15 minutes\",\"value\":100,\"yAxis\":\"left\"}]},\"yAxis\":{}}},{\"type\":\"metric\",\"width\":6,\"height\":3,\"x\":0,\"y\":14,\"properties\":{\"view\":\"singleValue\",\"title\":\"Current messages in queue\",\"region\":\"", { "Ref": "AWS::Region" }, @@ -67,7 +67,7 @@ "QueueName" ] }, - "\",{\"yAxis\":\"left\",\"period\":300,\"stat\":\"Average\"}]]}}]}" + "\"]]}}]}" ] ] }, From 675bf854709b7ea98e54680171e0567289b45d66 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 31 Dec 2019 13:03:48 +0100 Subject: [PATCH 16/38] Properly add unit test file --- .../aws-cloudwatch/test/test.metric-math.ts | 321 ++++++++++++++++++ 1 file changed, 321 insertions(+) create mode 100644 packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts diff --git a/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts b/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts new file mode 100644 index 0000000000000..af6f77f627f9b --- /dev/null +++ b/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts @@ -0,0 +1,321 @@ +import { expect, haveResourceLike } from '@aws-cdk/assert'; +import { Duration, Stack } from '@aws-cdk/core'; +import { Test } from 'nodeunit'; +import { Alarm, GraphWidget, IWidget, MathExpression, Metric } from '../lib'; + +const a = new Metric({ namespace: 'Test', metricName: 'ACount', period: Duration.seconds(10) }); +const b = new Metric({ namespace: 'Test', metricName: 'BCount', statistic: 'Average' }); +const c = new Metric({ namespace: 'Test', metricName: 'CCount' }); +// const b99 = new Metric({ namespace: 'Test', metricName: 'BCount', statistic: 'p99' }); + +let stack: Stack; +export = { + 'setUp'(cb: () => void) { + stack = new Stack(); + cb(); + }, + + 'can not use invalid variable names in MathExpression'(test: Test) { + test.throws(() => { + new MathExpression({ + expression: 'HAPPY + JOY', + expressionMetrics: { + HAPPY: a, + JOY: b + } + }); + }, /Invalid variable names in expression/); + + test.done(); + }, + + 'cannot reuse variable names in nested MathExpressions'(test: Test) { + // WHEN + test.throws(() => { + new MathExpression({ + expression: 'a + e', + expressionMetrics: { + a, + e: new MathExpression({ + expression: 'a + c', + expressionMetrics: { a: b, c } + }) + } + }); + }, /Same id \('a'\) used for two metrics/); + + test.done(); + }, + + 'in graphs': { + 'MathExpressions can be added to a graph'(test: Test) { + // GIVEN + const graph = new GraphWidget({ + left: [ + new MathExpression({ + expression: 'a + b', + expressionMetrics: { a, b } + }) + ], + }); + + // THEN + graphMetricsAre(test, graph, [ + [ { expression: 'a + b', label: 'a + b' } ], + [ 'Test', 'ACount', { period: 10, visible: false, id: 'a' } ], + [ 'Test', 'BCount', { visible: false, id: 'b' } ], + ]); + + test.done(); + }, + + 'can nest MathExpressions in a graph'(test: Test) { + // GIVEN + const graph = new GraphWidget({ + left: [ + new MathExpression({ + expression: 'a + e', + expressionMetrics: { + a, + e: new MathExpression({ + expression: 'b + c', + expressionMetrics: { b, c } + }) + } + }) + ], + }); + + // THEN + graphMetricsAre(test, graph, [ + [ { label: 'a + e', expression: 'a + e' } ], + [ 'Test', 'ACount', { period: 10, visible: false, id: 'a' } ], + [ { expression: 'b + c', visible: false, id: 'e' } ], + [ 'Test', 'BCount', { visible: false, id: 'b' } ], + [ 'Test', 'CCount', { visible: false, id: 'c' } ] + ]); + + test.done(); + }, + + 'can add the same metric under different ids'(test: Test) { + const graph = new GraphWidget({ + left: [ + new MathExpression({ + expression: 'a + e', + expressionMetrics: { + a, + e: new MathExpression({ + expression: 'b + c', + expressionMetrics: { b: a, c } + }) + } + }) + ], + }); + + graphMetricsAre(test, graph, [ + [ { label: 'a + e', expression: 'a + e' } ], + [ 'Test', 'ACount', { period: 10, visible: false, id: 'a' } ], + [ { expression: 'b + c', visible: false, id: 'e' } ], + [ 'Test', 'ACount', { period: 10, visible: false, id: 'b' } ], + [ 'Test', 'CCount', { visible: false, id: 'c' } ] + ]); + + test.done(); + }, + + 'can reuse identifiers in MathExpressions if metrics are the same'(test: Test) { + const graph = new GraphWidget({ + left: [ + new MathExpression({ + expression: 'a + e', + expressionMetrics: { + a, + e: new MathExpression({ + expression: 'a + c', + expressionMetrics: { a, c } + }) + } + }) + ], + }); + + // THEN + graphMetricsAre(test, graph, [ + [ { label: 'a + e', expression: 'a + e' } ], + [ 'Test', 'ACount', { period: 10, visible: false, id: 'a' } ], + [ { expression: 'a + c', visible: false, id: 'e' } ], + [ 'Test', 'CCount', { visible: false, id: 'c' } ] + ]); + + test.done(); + }, + + 'MathExpression and its constituent metrics can both be added to a graph'(test: Test) { + const graph = new GraphWidget({ + left: [ + a, + new MathExpression({ + expression: 'a + b', + expressionMetrics: { a, b } + }) + ], + }); + + // THEN + graphMetricsAre(test, graph, [ + [ 'Test', 'ACount', { period: 10, id: 'a' } ], + [ { label: 'a + b', expression: 'a + b' } ], + [ 'Test', 'BCount', { visible: false, id: 'b' } ] + ]); + test.done(); + }, + + 'can use percentiles in expression metrics in graphs'(test: Test) { + test.done(); + }, + }, + + 'in alarms': { + 'MathExpressions can be used for an alarm'(test: Test) { + // GIVEN + new Alarm(stack, 'Alarm', { + threshold: 1, evaluationPeriods: 1, + metric: new MathExpression({ + expression: 'a + b', + expressionMetrics: { a, b } + }) + }); + + // THEN + alarmMetricsAre([ + { + Expression: "a + b", + Id: "mid1" + }, + { + Id: "a", + MetricStat: { + Metric: { + MetricName: "ACount", + Namespace: "Test" + }, + Period: 10, + Stat: "Average" + }, + ReturnData: false + }, + { + Id: "b", + MetricStat: { + Metric: { + MetricName: "BCount", + Namespace: "Test" + }, + Period: 300, + Stat: "Average" + }, + ReturnData: false + } + + ]); + + test.done(); + }, + + 'can nest MathExpressions in an alarm'(test: Test) { + // GIVEN + new Alarm(stack, 'Alarm', { + threshold: 1, evaluationPeriods: 1, + metric: new MathExpression({ + expression: 'a + e', + expressionMetrics: { + a, + e: new MathExpression({ + expression: 'b + c', + expressionMetrics: { b, c } + }) + } + }) + }); + + // THEN + alarmMetricsAre([ + { + Expression: "a + e", + Id: "mid1" + }, + { + Id: "a", + MetricStat: { + Metric: { + MetricName: "ACount", + Namespace: "Test" + }, + Period: 10, + Stat: "Average" + }, + ReturnData: false + }, + { + Expression: "b + c", + Id: "e", + ReturnData: false + }, + { + Id: "b", + MetricStat: { + Metric: { + MetricName: "BCount", + Namespace: "Test" + }, + Period: 300, + Stat: "Average" + }, + ReturnData: false + }, + { + Id: "c", + MetricStat: { + Metric: { + MetricName: "CCount", + Namespace: "Test" + }, + Period: 300, + Stat: "Average" + }, + ReturnData: false + } + ]); + + test.done(); + }, + + 'annotation for a mathexpression alarm is calculated based upon constituent metrics'(test: Test) { + test.done(); + }, + + 'can use percentiles in expression metrics in alarms'(test: Test) { + test.done(); + }, + } +}; + +function graphMetricsAre(test: Test, w: IWidget, metrics: any[]) { + test.deepEqual(stack.resolve(w.toJson()), [ { + type: 'metric', + width: 6, + height: 6, + properties: + { view: 'timeSeries', + region: { Ref: 'AWS::Region' }, + metrics, + yAxis: {} } }]); +} + +function alarmMetricsAre(metrics: any[]) { + expect(stack).to(haveResourceLike('AWS::CloudWatch::Alarm', { + Metrics: metrics + })); +} From f0aaabd362cf65ba7e8471fdf64f56be36c5e294 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 31 Dec 2019 13:09:47 +0100 Subject: [PATCH 17/38] Add some final unit tests --- .../aws-cloudwatch/test/test.metric-math.ts | 75 ++++++++++++++++++- 1 file changed, 74 insertions(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts b/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts index af6f77f627f9b..dcc5521e07d8a 100644 --- a/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts +++ b/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts @@ -6,7 +6,7 @@ import { Alarm, GraphWidget, IWidget, MathExpression, Metric } from '../lib'; const a = new Metric({ namespace: 'Test', metricName: 'ACount', period: Duration.seconds(10) }); const b = new Metric({ namespace: 'Test', metricName: 'BCount', statistic: 'Average' }); const c = new Metric({ namespace: 'Test', metricName: 'CCount' }); -// const b99 = new Metric({ namespace: 'Test', metricName: 'BCount', statistic: 'p99' }); +const b99 = new Metric({ namespace: 'Test', metricName: 'BCount', statistic: 'p99' }); let stack: Stack; export = { @@ -173,6 +173,23 @@ export = { }, 'can use percentiles in expression metrics in graphs'(test: Test) { + // GIVEN + const graph = new GraphWidget({ + left: [ + new MathExpression({ + expression: 'a + b99', + expressionMetrics: { a, b99 } + }) + ], + }); + + // THEN + graphMetricsAre(test, graph, [ + [ { expression: 'a + b99', label: 'a + b99' } ], + [ 'Test', 'ACount', { period: 10, visible: false, id: 'a' } ], + [ 'Test', 'BCount', { visible: false, id: 'b99', stat: 'p99' } ], + ]); + test.done(); }, }, @@ -293,10 +310,66 @@ export = { }, 'annotation for a mathexpression alarm is calculated based upon constituent metrics'(test: Test) { + // GIVEN + const alarm = new Alarm(stack, 'Alarm', { + threshold: 1, evaluationPeriods: 1, + metric: new MathExpression({ + expression: 'a + b', + expressionMetrics: { a, b: b.with({ period: Duration.minutes(10) }) } + }) + }); + + // WHEN + const alarmLabel = alarm.toAnnotation().label; + + // THEN + test.equals(alarmLabel, 'a + b >= 1 for 1 datapoints within 10 minutes'); + test.done(); }, 'can use percentiles in expression metrics in alarms'(test: Test) { + // GIVEN + new Alarm(stack, 'Alarm', { + threshold: 1, evaluationPeriods: 1, + metric: new MathExpression({ + expression: 'a + b99', + expressionMetrics: { a, b99 } + }) + }); + + // THEN + alarmMetricsAre([ + { + Expression: "a + b99", + Id: "mid1" + }, + { + Id: "a", + MetricStat: { + Metric: { + MetricName: "ACount", + Namespace: "Test" + }, + Period: 10, + Stat: "Average" + }, + ReturnData: false + }, + { + Id: "b99", + MetricStat: { + Metric: { + MetricName: "BCount", + Namespace: "Test" + }, + Period: 300, + Stat: "p99" + }, + ReturnData: false + } + ]); + test.done(); }, } From f4a701958727ffa1de38d525b28aebedde95f7e1 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 31 Dec 2019 14:04:48 +0100 Subject: [PATCH 18/38] Update expectation to swap dimensions around --- .../test/integ.alb.expected.json | 38 +++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/packages/@aws-cdk/aws-elasticloadbalancingv2/test/integ.alb.expected.json b/packages/@aws-cdk/aws-elasticloadbalancingv2/test/integ.alb.expected.json index a5fb352391516..5a8cb621b5a7c 100644 --- a/packages/@aws-cdk/aws-elasticloadbalancingv2/test/integ.alb.expected.json +++ b/packages/@aws-cdk/aws-elasticloadbalancingv2/test/integ.alb.expected.json @@ -490,15 +490,6 @@ "ComparisonOperator": "GreaterThanOrEqualToThreshold", "EvaluationPeriods": 2, "Dimensions": [ - { - "Name": "TargetGroup", - "Value": { - "Fn::GetAtt": [ - "LBListenerTargetGroupF04FCF6D", - "TargetGroupFullName" - ] - } - }, { "Name": "LoadBalancer", "Value": { @@ -549,6 +540,15 @@ ] ] } + }, + { + "Name": "TargetGroup", + "Value": { + "Fn::GetAtt": [ + "LBListenerTargetGroupF04FCF6D", + "TargetGroupFullName" + ] + } } ], "MetricName": "TargetResponseTime", @@ -564,15 +564,6 @@ "ComparisonOperator": "GreaterThanOrEqualToThreshold", "EvaluationPeriods": 2, "Dimensions": [ - { - "Name": "TargetGroup", - "Value": { - "Fn::GetAtt": [ - "LBListenerConditionalTargetGroupA75CCCD9", - "TargetGroupFullName" - ] - } - }, { "Name": "LoadBalancer", "Value": { @@ -623,6 +614,15 @@ ] ] } + }, + { + "Name": "TargetGroup", + "Value": { + "Fn::GetAtt": [ + "LBListenerConditionalTargetGroupA75CCCD9", + "TargetGroupFullName" + ] + } } ], "MetricName": "TargetResponseTime", @@ -633,4 +633,4 @@ } } } -} \ No newline at end of file +} From 03eb4a07002c86f2e10f3d8a1493c18d16db01f5 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 31 Dec 2019 14:06:42 +0100 Subject: [PATCH 19/38] Review comments --- .../lib/step-scaling-policy.ts | 4 +- .../lib/target-tracking-scaling-policy.ts | 14 +- .../lib/step-scaling-policy.ts | 4 +- .../lib/target-tracking-scaling-policy.ts | 14 +- packages/@aws-cdk/aws-cloudwatch/README.md | 63 ++++-- .../@aws-cdk/aws-cloudwatch/lib/metric.ts | 199 +++++++++--------- 6 files changed, 172 insertions(+), 126 deletions(-) diff --git a/packages/@aws-cdk/aws-applicationautoscaling/lib/step-scaling-policy.ts b/packages/@aws-cdk/aws-applicationautoscaling/lib/step-scaling-policy.ts index 018232499f6a0..f6e8bd26d9c19 100644 --- a/packages/@aws-cdk/aws-applicationautoscaling/lib/step-scaling-policy.ts +++ b/packages/@aws-cdk/aws-applicationautoscaling/lib/step-scaling-policy.ts @@ -182,8 +182,10 @@ export interface ScalingInterval { readonly change: number; } -function aggregationTypeFromMetric(metric: cloudwatch.IMetric): MetricAggregationType { +function aggregationTypeFromMetric(metric: cloudwatch.IMetric): MetricAggregationType | undefined { const statistic = metric.toMetricConfig().metricStat?.statistic; + if (statistic === undefined) { return undefined; } // Math expression, don't know aggregation, leave default + switch (statistic) { case 'Average': return MetricAggregationType.AVERAGE; diff --git a/packages/@aws-cdk/aws-applicationautoscaling/lib/target-tracking-scaling-policy.ts b/packages/@aws-cdk/aws-applicationautoscaling/lib/target-tracking-scaling-policy.ts index d086f1861c2fb..5bf93a90985df 100644 --- a/packages/@aws-cdk/aws-applicationautoscaling/lib/target-tracking-scaling-policy.ts +++ b/packages/@aws-cdk/aws-applicationautoscaling/lib/target-tracking-scaling-policy.ts @@ -115,6 +115,10 @@ export class TargetTrackingScalingPolicy extends cdk.Construct { throw new Error(`Exactly one of 'customMetric' or 'predefinedMetric' must be specified.`); } + if (props.customMetric && !props.customMetric.toMetricConfig().metricStat) { + throw new Error(`Math expressions are not supported for Target Tracking. Use Step Scaling or supply a Metric object.`); + } + super(scope, id); const resource = new CfnScalingPolicy(this, 'Resource', { @@ -140,14 +144,10 @@ export class TargetTrackingScalingPolicy extends cdk.Construct { function renderCustomMetric(metric?: cloudwatch.IMetric): CfnScalingPolicy.CustomizedMetricSpecificationProperty | undefined { if (!metric) { return undefined; } - const c = metric.toMetricConfig().metricStat; - - if (c === undefined) { - throw new Error("A `Metric` object must be used here."); - } + const c = metric.toMetricConfig().metricStat!; - if (!c.statistic) { - throw new Error('Can only use Average, Minimum, Maximum, SampleCount, Sum statistic for target tracking'); + if (c.statistic.startsWith('p')) { + throw new Error(`Can not use statistic '${c.statistic}' for Target Tracking: only Average, Minimum, Maximum, SampleCount, Sum are supported.`); } return { diff --git a/packages/@aws-cdk/aws-autoscaling/lib/step-scaling-policy.ts b/packages/@aws-cdk/aws-autoscaling/lib/step-scaling-policy.ts index e91fb10305427..a89e6813a736b 100644 --- a/packages/@aws-cdk/aws-autoscaling/lib/step-scaling-policy.ts +++ b/packages/@aws-cdk/aws-autoscaling/lib/step-scaling-policy.ts @@ -146,8 +146,10 @@ export class StepScalingPolicy extends cdk.Construct { } } -function aggregationTypeFromMetric(metric: cloudwatch.IMetric): MetricAggregationType { +function aggregationTypeFromMetric(metric: cloudwatch.IMetric): MetricAggregationType | undefined { const statistic = metric.toMetricConfig().metricStat?.statistic; + if (statistic === undefined) { return undefined; } // Math expression, don't know aggregation, leave default + switch (statistic) { case 'Average': return MetricAggregationType.AVERAGE; diff --git a/packages/@aws-cdk/aws-autoscaling/lib/target-tracking-scaling-policy.ts b/packages/@aws-cdk/aws-autoscaling/lib/target-tracking-scaling-policy.ts index ef126b7a2d4e5..9dacc293ce862 100644 --- a/packages/@aws-cdk/aws-autoscaling/lib/target-tracking-scaling-policy.ts +++ b/packages/@aws-cdk/aws-autoscaling/lib/target-tracking-scaling-policy.ts @@ -117,6 +117,10 @@ export class TargetTrackingScalingPolicy extends cdk.Construct { throw new Error('When tracking the ALBRequestCountPerTarget metric, the ALB identifier must be supplied in resourceLabel'); } + if (props.customMetric && !props.customMetric.toMetricConfig().metricStat) { + throw new Error(`Math expressions are not supported for Target Tracking. Use Step Scaling or supply a Metric object.`); + } + super(scope, id); this.resource = new CfnScalingPolicy(this, 'Resource', { @@ -141,15 +145,7 @@ export class TargetTrackingScalingPolicy extends cdk.Construct { function renderCustomMetric(metric?: cloudwatch.IMetric): CfnScalingPolicy.CustomizedMetricSpecificationProperty | undefined { if (!metric) { return undefined; } - const c = metric.toMetricConfig().metricStat; - - if (c === undefined) { - throw new Error("A `Metric` object must be used here."); - } - - if (!c.statistic) { - throw new Error('Can only use Average, Minimum, Maximum, SampleCount, Sum statistic for target tracking'); - } + const c = metric.toMetricConfig().metricStat!; return { dimensions: c.dimensions, diff --git a/packages/@aws-cdk/aws-cloudwatch/README.md b/packages/@aws-cdk/aws-cloudwatch/README.md index d2734fd2eed5f..aa4dc460ffdc0 100644 --- a/packages/@aws-cdk/aws-cloudwatch/README.md +++ b/packages/@aws-cdk/aws-cloudwatch/README.md @@ -9,6 +9,8 @@ --- +## Metric objects + Metric objects represent a metric that is emitted by AWS services or your own application, such as `CPUUsage`, `FailureCount` or `Bandwidth`. @@ -17,8 +19,6 @@ attributes. Resources that expose metrics will have functions that look like `metricXxx()` which will return a Metric object, initialized with defaults that make sense. -#### Exposed Metrics - For example, `lambda.Function` objects have the `fn.metricErrors()` method, which represents the amount of errors reported by that Lambda function: @@ -26,26 +26,45 @@ represents the amount of errors reported by that Lambda function: const errors = fn.metricErrors(); ``` -#### Constructed Metrics +### Instantiating a new Metric object + +If you want to reference a metric that is not yet exposed by an existing construct, +you can instantiate a `Metric` object to represent it. For example: + +```ts +const metric = new Metric({ + namespace: 'MyNamespace', + metricName: 'MyMetric', + dimensions: { + ProcessingStep: 'Download' + } +}); +``` + +### Metric Math -Constructed metrics could be a regular metric or a math expression. +Math expressions are supported by instantiating the `MathExpression` class. +For example, a math expression that sums two other metrics looks like this: -For example, a math expression that sums two regular metrics: ```ts -const mathExpression = new MathExpression({ - expression: "x+y", +const allProblems = new MathExpression({ + expression: "errors + faults", expressionMetrics: { - x: new Metric(...metricProps), - y: new Metric(...otherMetricProps) + errors: myConstruct.metricErrors(), + faults: myConstruct.metricFaults(), } }) ``` -Moreover, math expressions could be nested: + +You can use `MathExpression` objects like any other metric, including using +them in other math expressions: + ```ts -const nestedMathExpression = new MathExpression({ - expression: "z/100", +const problemPercentage = new MathExpression({ + expression: "(problems / invocations) * 100", expressionMetrics: { - z: mathExpression + problems: allProblems, + invocations: myConstruct.metricInvocations() } }) ``` @@ -123,6 +142,24 @@ The most important properties to set while creating an Alarms are: - `evaluationPeriods`: how many consecutive periods the metric has to be breaching the the threshold for the alarm to trigger. +### A note on units + +Metrics values are emitted with units, such as `seconds` or `bytes`. In +CloudWatch, `Alarm`s do have a `unit` attribute, but that it is not used to +scale the threshold value but it is scaled to filter metric values inside the +same metric, considering only the metric values emitted under the given unit. + +For example, if a metric value is emitted as `1000 bytes`, and an alarm +threshold is set to `1 kilobyte` (including the unit), the alarm will *not* +trigger, as the units of the values and the alarm are different. + +It is therefore recommended to set the alarm (in this case) to `1000` and +leave out the unit. + +It is still possible to specify `unit` as a property when creating `Metric` +objects, but for safety that property will be ignored, because it will rarely +do what you want. + ## Dashboards Dashboards are set of Widgets stored server-side which can be accessed quickly diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts index cb9fff5828529..02b8cf7fa2be9 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts @@ -122,95 +122,6 @@ export interface MathExpressionProps extends CommonMetricOptions { readonly expressionMetrics: Record; } -/** - * This is a base class for metrics that has legacy code. - * This is kept only for backward compatibility and shouldn't be used. - */ -abstract class BaseMetric implements IMetric { - /** - * Unit of the metric. - * - * @default None - * @deprecated Deprecated in MetricProps. - */ - public readonly unit?: Unit; - - public constructor(unit?: Unit) { - this.unit = unit; - } - - public abstract toMetricConfig(): MetricConfig; - public abstract with(props: MetricOptions): IMetric; - - public toAlarmConfig(): MetricAlarmConfig { - const metricConfig = this.toMetricConfig(); - if (metricConfig.metricStat === undefined) { - throw new Error("A `Metric` object must be used here."); - } - - const stat = parseStatistic(metricConfig.metricStat.statistic); - return { - dimensions: metricConfig.metricStat.dimensions, - namespace: metricConfig.metricStat.namespace, - metricName: metricConfig.metricStat.metricName, - period: metricConfig.metricStat.period.toSeconds(), - statistic: stat.type === 'simple' ? stat.statistic : undefined, - extendedStatistic: stat.type === 'percentile' ? 'p' + stat.percentile : undefined, - unit: this.unit - }; - } - - public toGraphConfig(): MetricGraphConfig { - const metricConfig = this.toMetricConfig(); - if (metricConfig.metricStat === undefined) { - throw new Error("MetricStatConfig need to set"); - } - - return { - dimensions: metricConfig.metricStat.dimensions, - namespace: metricConfig.metricStat.namespace, - metricName: metricConfig.metricStat.metricName, - renderingProperties: { - period: metricConfig.metricStat.period.toSeconds(), - stat: metricConfig.metricStat.statistic, - color: metricConfig.renderingProperties?.color, - label: metricConfig.renderingProperties?.label - }, - // deprecated properties for backwards compatibility - period: metricConfig.metricStat.period.toSeconds(), - statistic: metricConfig.metricStat.statistic, - color: metricConfig.renderingProperties?.color, - label: metricConfig.renderingProperties?.label, - unit: this.unit - }; - } - - /** - * Make a new Alarm for this metric - * - * Combines both properties that may adjust the metric (aggregation) as well - * as alarm properties. - */ - public createAlarm(scope: cdk.Construct, id: string, props: CreateAlarmOptions): Alarm { - return new Alarm(scope, id, { - metric: this.with({ - statistic: props.statistic, - period: props.period, - }), - alarmName: props.alarmName, - alarmDescription: props.alarmDescription, - comparisonOperator: props.comparisonOperator, - datapointsToAlarm: props.datapointsToAlarm, - threshold: props.threshold, - evaluationPeriods: props.evaluationPeriods, - evaluateLowSampleCountPercentile: props.evaluateLowSampleCountPercentile, - treatMissingData: props.treatMissingData, - actionsEnabled: props.actionsEnabled, - }); - } - -} - /** * A metric emitted by a service * @@ -225,7 +136,7 @@ abstract class BaseMetric implements IMetric { * Metric is an abstraction that makes it easy to specify metrics for use in both * alarms and graphs. */ -export class Metric extends BaseMetric { +export class Metric implements IMetric { /** * Grant permissions to the given identity to write metrics. * @@ -246,12 +157,21 @@ export class Metric extends BaseMetric { public readonly statistic: string; public readonly label?: string; public readonly color?: string; + + /** + * Unit of the metric. + * + * @default None + */ + public readonly unit?: Unit; + /** * Account which this metric comes from. * * @default Deployment account. */ public readonly account?: string; + /** * Region which this metric comes from. * @@ -260,7 +180,6 @@ export class Metric extends BaseMetric { public readonly region?: string; constructor(props: MetricProps) { - super(props.unit); this.period = props.period || cdk.Duration.minutes(5); const periodSec = this.period.toSeconds(); if (periodSec !== 1 && periodSec !== 5 && periodSec !== 10 && periodSec !== 30 && periodSec % 60 !== 0) { @@ -319,6 +238,73 @@ export class Metric extends BaseMetric { }; } + public toAlarmConfig(): MetricAlarmConfig { + const metricConfig = this.toMetricConfig(); + if (metricConfig.metricStat === undefined) { + throw new Error("A `Metric` object must be used here."); + } + + const stat = parseStatistic(metricConfig.metricStat.statistic); + return { + dimensions: metricConfig.metricStat.dimensions, + namespace: metricConfig.metricStat.namespace, + metricName: metricConfig.metricStat.metricName, + period: metricConfig.metricStat.period.toSeconds(), + statistic: stat.type === 'simple' ? stat.statistic : undefined, + extendedStatistic: stat.type === 'percentile' ? 'p' + stat.percentile : undefined, + unit: this.unit + }; + } + + public toGraphConfig(): MetricGraphConfig { + const metricConfig = this.toMetricConfig(); + if (metricConfig.metricStat === undefined) { + throw new Error("MetricStatConfig need to set"); + } + + return { + dimensions: metricConfig.metricStat.dimensions, + namespace: metricConfig.metricStat.namespace, + metricName: metricConfig.metricStat.metricName, + renderingProperties: { + period: metricConfig.metricStat.period.toSeconds(), + stat: metricConfig.metricStat.statistic, + color: metricConfig.renderingProperties?.color, + label: metricConfig.renderingProperties?.label + }, + // deprecated properties for backwards compatibility + period: metricConfig.metricStat.period.toSeconds(), + statistic: metricConfig.metricStat.statistic, + color: metricConfig.renderingProperties?.color, + label: metricConfig.renderingProperties?.label, + unit: this.unit + }; + } + + /** + * Make a new Alarm for this metric + * + * Combines both properties that may adjust the metric (aggregation) as well + * as alarm properties. + */ + public createAlarm(scope: cdk.Construct, id: string, props: CreateAlarmOptions): Alarm { + return new Alarm(scope, id, { + metric: this.with({ + statistic: props.statistic, + period: props.period, + }), + alarmName: props.alarmName, + alarmDescription: props.alarmDescription, + comparisonOperator: props.comparisonOperator, + datapointsToAlarm: props.datapointsToAlarm, + threshold: props.threshold, + evaluationPeriods: props.evaluationPeriods, + evaluateLowSampleCountPercentile: props.evaluateLowSampleCountPercentile, + treatMissingData: props.treatMissingData, + actionsEnabled: props.actionsEnabled, + }); + } + public toString() { return this.label || this.metricName; } @@ -351,7 +337,7 @@ export class Metric extends BaseMetric { * MathExpression is an abstraction that makes it easy to specify metrics for use in both * alarms and graphs. */ -export class MathExpression extends BaseMetric { +export class MathExpression implements IMetric { /** * The expression defining the metric. */ @@ -373,7 +359,6 @@ export class MathExpression extends BaseMetric { public readonly color?: string; constructor(props: MathExpressionProps) { - super(); this.expression = props.expression; this.expressionMetrics = props.expressionMetrics; this.label = props.label; @@ -404,11 +389,11 @@ export class MathExpression extends BaseMetric { } public toAlarmConfig(): MetricAlarmConfig { - throw new Error("A `Metric` object must be used here."); + throw new Error(`Using a math expression is not supported here. Pass a 'Metric' object instead`); } public toGraphConfig(): MetricGraphConfig { - throw new Error("A `Metric` object must be used here."); + throw new Error(`Using a math expression is not supported here. Pass a 'Metric' object instead`); } public toMetricConfig(): MetricConfig { @@ -424,6 +409,30 @@ export class MathExpression extends BaseMetric { }; } + /** + * Make a new Alarm for this metric + * + * Combines both properties that may adjust the metric (aggregation) as well + * as alarm properties. + */ + public createAlarm(scope: cdk.Construct, id: string, props: CreateAlarmOptions): Alarm { + return new Alarm(scope, id, { + metric: this.with({ + statistic: props.statistic, + period: props.period, + }), + alarmName: props.alarmName, + alarmDescription: props.alarmDescription, + comparisonOperator: props.comparisonOperator, + datapointsToAlarm: props.datapointsToAlarm, + threshold: props.threshold, + evaluationPeriods: props.evaluationPeriods, + evaluateLowSampleCountPercentile: props.evaluateLowSampleCountPercentile, + treatMissingData: props.treatMissingData, + actionsEnabled: props.actionsEnabled, + }); + } + public toString() { return this.label || this.expression; } From de6d63cad90d39817321a5f4bc0a6215ecdc172d Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 31 Dec 2019 14:10:57 +0100 Subject: [PATCH 20/38] Change generated id --- packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts | 4 ++-- packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts b/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts index f0f0817cffcde..dd1ca24ff02ea 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts @@ -245,9 +245,9 @@ function renderAlarmMetric(metric: IMetric) { const mset = new MetricSet(); mset.addPrimary(true, metric); - let mid = 0; + let eid = 0; function uniqueMetricId() { - return `mid${++mid}`; + return `expr_${++eid}`; } return { diff --git a/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts b/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts index dcc5521e07d8a..0a7ea23c81728 100644 --- a/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts +++ b/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts @@ -209,7 +209,7 @@ export = { alarmMetricsAre([ { Expression: "a + b", - Id: "mid1" + Id: "expr_1" }, { Id: "a", @@ -261,7 +261,7 @@ export = { alarmMetricsAre([ { Expression: "a + e", - Id: "mid1" + Id: "expr_1" }, { Id: "a", @@ -342,7 +342,7 @@ export = { alarmMetricsAre([ { Expression: "a + b99", - Id: "mid1" + Id: "expr_1" }, { Id: "a", From 8501005663ae465a510d6e8fe7fd0fdecad4f931 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 31 Dec 2019 14:30:02 +0100 Subject: [PATCH 21/38] Adjust periods on metrics used in expression so they're all the same --- .../aws-cloudwatch/lib/metric-util.ts | 2 +- .../@aws-cdk/aws-cloudwatch/lib/metric.ts | 87 ++++++++++++++++--- 2 files changed, 74 insertions(+), 15 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts index 3ae0cf9d98e08..b16031dd795d3 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts @@ -213,7 +213,7 @@ const METRICKEY_SYMBOL = Symbol('@aws-cdk/aws-cloudwatch.MetricKey'); * Can be used to determine as a hash key to determine if 2 Metric objects * represent the same metric. Excludes rendering properties. */ -function metricKey(metric: IMetric): string { +export function metricKey(metric: IMetric): string { // Cache on the object itself. This is safe because Metric objects are immutable. if ((metric as any)[METRICKEY_SYMBOL] !== undefined) { return (metric as any)[METRICKEY_SYMBOL]; diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts index 02b8cf7fa2be9..422227fd8b534 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts @@ -2,7 +2,7 @@ import * as iam from '@aws-cdk/aws-iam'; import * as cdk from '@aws-cdk/core'; import { Alarm, ComparisonOperator, TreatMissingData } from './alarm'; import { Dimension, IMetric, MetricAlarmConfig, MetricConfig, MetricGraphConfig, Unit } from './metric-types'; -import { validateNoIdConflicts } from './metric-util'; +import { metricKey, validateNoIdConflicts } from './metric-util'; import { normalizeStatistic, parseStatistic } from './util.statistic'; export type DimensionHash = {[dim: string]: any}; @@ -111,15 +111,37 @@ export interface MetricOptions extends CommonMetricOptions { /** * Properties for a MathExpression */ -export interface MathExpressionProps extends CommonMetricOptions { - /** - * The expression defining the metric. - */ - readonly expression: string; - /** - * The metrics used in the expression as KeyValuePair . - */ - readonly expressionMetrics: Record; +export interface MathExpressionProps { + /** + * The expression defining the metric. + */ + readonly expression: string; + + /** + * The metrics used in the expression as KeyValuePair . + */ + readonly expressionMetrics: Record; + + /** + * Label for this metric when added to a Graph in a Dashboard + * + * @default - Expression value is used as label + */ + readonly label?: string; + + /** + * Color for this metric when added to a Graph in a Dashboard + * + * @default - Automatic color + */ + readonly color?: string; + + /** + * The period over which the specified statistic is applied. + * + * @default Duration.minutes(5) + */ + readonly period?: cdk.Duration; } /** @@ -205,7 +227,7 @@ export class Metric implements IMetric { * @param props The set of properties to change. */ public with(props: MetricOptions): Metric { - return new Metric({ + const ret = new Metric({ dimensions: ifUndefined(props.dimensions, this.dimensions), namespace: this.namespace, metricName: this.metricName, @@ -217,6 +239,12 @@ export class Metric implements IMetric { account: ifUndefined(props.account, this.account), region: ifUndefined(props.region, this.region) }); + + // Save on objects: if the returned object is the same as the current + // object, just return ourselves. + if (metricKey(ret) === metricKey(this) && ret.color === this.color && ret.label === this.label) { return this; } + + return ret; } public toMetricConfig(): MetricConfig { @@ -358,9 +386,15 @@ export class MathExpression implements IMetric { */ public readonly color?: string; + /** + * Aggregation period of this metric + */ + public readonly period: cdk.Duration; + constructor(props: MathExpressionProps) { + this.period = props.period || cdk.Duration.minutes(5); this.expression = props.expression; - this.expressionMetrics = props.expressionMetrics; + this.expressionMetrics = changeAllPeriods(props.expressionMetrics, this.period); this.label = props.label; this.color = props.color; @@ -382,9 +416,10 @@ export class MathExpression implements IMetric { public with(props: MetricOptions): MathExpression { return new MathExpression({ expression: this.expression, - expressionMetrics: this.expressionMetrics, + expressionMetrics: props.period ? changeAllPeriods(this.expressionMetrics, props.period) : this.expressionMetrics, label: ifUndefined(props.label, this.label), - color: ifUndefined(props.color, this.color) + color: ifUndefined(props.color, this.color), + period: ifUndefined(props.period, this.period), }); } @@ -543,3 +578,27 @@ function ifUndefined(x: T | undefined, def: T | undefined): T | undefined { } return def; } + +/** + * Change periods of all metrics in the map + */ +function changeAllPeriods(metrics: Record, period: cdk.Duration): Record { + const ret: Record = {}; + for (const [id, metric] of Object.entries(metrics)) { + ret[id] = changePeriod(metric, period); + } + return ret; +} + +/** + * Return a new metric object which is the same type as the input object, but with the period changed + * + * Uses JavaScript prototyping hackery to achieve this. Relies on the fact that + * both implementations of IMetric have a `period` member that contains that particular + * value. + */ +function changePeriod(metric: A, period: cdk.Duration): A { + return Object.create(metric, { + period: { value: period, enumerable: true } + }); +} \ No newline at end of file From 0466f1e54b5ca5a40f773c3053c86c01229520cb Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 31 Dec 2019 16:18:16 +0100 Subject: [PATCH 22/38] Restrict the properties visible to MathExpressions --- .../@aws-cdk/aws-cloudwatch/lib/metric.ts | 72 +++++++++---------- 1 file changed, 33 insertions(+), 39 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts index 422227fd8b534..bc2737aecf895 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts @@ -58,21 +58,6 @@ export interface CommonMetricOptions { * Color for this metric when added to a Graph in a Dashboard */ readonly color?: string; -} - -/** - * Properties for a metric - */ -export interface MetricProps extends CommonMetricOptions { - /** - * Namespace of the metric. - */ - readonly namespace: string; - - /** - * Name of the metric. - */ - readonly metricName: string; /** * Account which this metric comes from. @@ -90,38 +75,30 @@ export interface MetricProps extends CommonMetricOptions { } /** - * Properties of a metric that can be changed + * Properties for a metric */ -export interface MetricOptions extends CommonMetricOptions { +export interface MetricProps extends CommonMetricOptions { /** - * Account which this metric comes from. - * - * @default Deployment account. + * Namespace of the metric. */ - readonly account?: string; + readonly namespace: string; /** - * Region which this metric comes from. - * - * @default Deployment region. + * Name of the metric. */ - readonly region?: string; + readonly metricName: string; } /** - * Properties for a MathExpression + * Properties of a metric that can be changed */ -export interface MathExpressionProps { - /** - * The expression defining the metric. - */ - readonly expression: string; - - /** - * The metrics used in the expression as KeyValuePair . - */ - readonly expressionMetrics: Record; +export interface MetricOptions extends CommonMetricOptions { +} +/** + * Configurable options for MathExpressions + */ +export interface MathExpressionOptions { /** * Label for this metric when added to a Graph in a Dashboard * @@ -137,13 +114,31 @@ export interface MathExpressionProps { readonly color?: string; /** - * The period over which the specified statistic is applied. + * The period over which the expression's statistics are applied. + * + * This period overrides all periods in the metrics used in this + * math expression. * * @default Duration.minutes(5) */ readonly period?: cdk.Duration; } +/** + * Properties for a MathExpression + */ +export interface MathExpressionProps extends MathExpressionOptions { + /** + * The expression defining the metric. + */ + readonly expression: string; + + /** + * The metrics used in the expression as KeyValuePair . + */ + readonly expressionMetrics: Record; +} + /** * A metric emitted by a service * @@ -413,7 +408,7 @@ export class MathExpression implements IMetric { * * @param props The set of properties to change. */ - public with(props: MetricOptions): MathExpression { + public with(props: MathExpressionOptions): MathExpression { return new MathExpression({ expression: this.expression, expressionMetrics: props.period ? changeAllPeriods(this.expressionMetrics, props.period) : this.expressionMetrics, @@ -453,7 +448,6 @@ export class MathExpression implements IMetric { public createAlarm(scope: cdk.Construct, id: string, props: CreateAlarmOptions): Alarm { return new Alarm(scope, id, { metric: this.with({ - statistic: props.statistic, period: props.period, }), alarmName: props.alarmName, From 218d5861809a31e8c5df876ea2fad94eea00506b Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 31 Dec 2019 16:25:58 +0100 Subject: [PATCH 23/38] Rename 'expressionMetrics' => 'usingMetrics' --- .../@aws-cdk/aws-cloudwatch/lib/metric.ts | 17 ++++++---- .../aws-cloudwatch/test/test.metric-math.ts | 34 +++++++++---------- 2 files changed, 27 insertions(+), 24 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts index bc2737aecf895..d2a025b23f624 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts @@ -134,9 +134,12 @@ export interface MathExpressionProps extends MathExpressionOptions { readonly expression: string; /** - * The metrics used in the expression as KeyValuePair . + * The metrics used in the expression, in a map. + * + * The key is the identifier that represents the given metric in the + * expression, and the value is the actual Metric object. */ - readonly expressionMetrics: Record; + readonly usingMetrics: Record; } /** @@ -369,7 +372,7 @@ export class MathExpression implements IMetric { /** * The metrics used in the expression as KeyValuePair . */ - public readonly expressionMetrics: Record; + public readonly usingMetrics: Record; /** * Label for this metric when added to a Graph. @@ -389,11 +392,11 @@ export class MathExpression implements IMetric { constructor(props: MathExpressionProps) { this.period = props.period || cdk.Duration.minutes(5); this.expression = props.expression; - this.expressionMetrics = changeAllPeriods(props.expressionMetrics, this.period); + this.usingMetrics = changeAllPeriods(props.usingMetrics, this.period); this.label = props.label; this.color = props.color; - const invalidVariableNames = Object.keys(props.expressionMetrics).filter(x => !validVariableName(x)); + const invalidVariableNames = Object.keys(props.usingMetrics).filter(x => !validVariableName(x)); if (invalidVariableNames.length > 0) { throw new Error(`Invalid variable names in expression: ${invalidVariableNames}. Must start with lowercase letter and only contain alphanumerics.`); } @@ -411,7 +414,7 @@ export class MathExpression implements IMetric { public with(props: MathExpressionOptions): MathExpression { return new MathExpression({ expression: this.expression, - expressionMetrics: props.period ? changeAllPeriods(this.expressionMetrics, props.period) : this.expressionMetrics, + usingMetrics: props.period ? changeAllPeriods(this.usingMetrics, props.period) : this.usingMetrics, label: ifUndefined(props.label, this.label), color: ifUndefined(props.color, this.color), period: ifUndefined(props.period, this.period), @@ -430,7 +433,7 @@ export class MathExpression implements IMetric { return { mathExpression: { expression: this.expression, - expressionMetrics: this.expressionMetrics + expressionMetrics: this.usingMetrics }, renderingProperties: { label: this.label, diff --git a/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts b/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts index 0a7ea23c81728..23897b510aa4d 100644 --- a/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts +++ b/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts @@ -19,7 +19,7 @@ export = { test.throws(() => { new MathExpression({ expression: 'HAPPY + JOY', - expressionMetrics: { + usingMetrics: { HAPPY: a, JOY: b } @@ -34,11 +34,11 @@ export = { test.throws(() => { new MathExpression({ expression: 'a + e', - expressionMetrics: { + usingMetrics: { a, e: new MathExpression({ expression: 'a + c', - expressionMetrics: { a: b, c } + usingMetrics: { a: b, c } }) } }); @@ -54,7 +54,7 @@ export = { left: [ new MathExpression({ expression: 'a + b', - expressionMetrics: { a, b } + usingMetrics: { a, b } }) ], }); @@ -75,11 +75,11 @@ export = { left: [ new MathExpression({ expression: 'a + e', - expressionMetrics: { + usingMetrics: { a, e: new MathExpression({ expression: 'b + c', - expressionMetrics: { b, c } + usingMetrics: { b, c } }) } }) @@ -103,11 +103,11 @@ export = { left: [ new MathExpression({ expression: 'a + e', - expressionMetrics: { + usingMetrics: { a, e: new MathExpression({ expression: 'b + c', - expressionMetrics: { b: a, c } + usingMetrics: { b: a, c } }) } }) @@ -130,11 +130,11 @@ export = { left: [ new MathExpression({ expression: 'a + e', - expressionMetrics: { + usingMetrics: { a, e: new MathExpression({ expression: 'a + c', - expressionMetrics: { a, c } + usingMetrics: { a, c } }) } }) @@ -158,7 +158,7 @@ export = { a, new MathExpression({ expression: 'a + b', - expressionMetrics: { a, b } + usingMetrics: { a, b } }) ], }); @@ -178,7 +178,7 @@ export = { left: [ new MathExpression({ expression: 'a + b99', - expressionMetrics: { a, b99 } + usingMetrics: { a, b99 } }) ], }); @@ -201,7 +201,7 @@ export = { threshold: 1, evaluationPeriods: 1, metric: new MathExpression({ expression: 'a + b', - expressionMetrics: { a, b } + usingMetrics: { a, b } }) }); @@ -247,11 +247,11 @@ export = { threshold: 1, evaluationPeriods: 1, metric: new MathExpression({ expression: 'a + e', - expressionMetrics: { + usingMetrics: { a, e: new MathExpression({ expression: 'b + c', - expressionMetrics: { b, c } + usingMetrics: { b, c } }) } }) @@ -315,7 +315,7 @@ export = { threshold: 1, evaluationPeriods: 1, metric: new MathExpression({ expression: 'a + b', - expressionMetrics: { a, b: b.with({ period: Duration.minutes(10) }) } + usingMetrics: { a, b: b.with({ period: Duration.minutes(10) }) } }) }); @@ -334,7 +334,7 @@ export = { threshold: 1, evaluationPeriods: 1, metric: new MathExpression({ expression: 'a + b99', - expressionMetrics: { a, b99 } + usingMetrics: { a, b99 } }) }); From 10a597086a073a0859d4d6068407fc5d96966d99 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 31 Dec 2019 16:26:34 +0100 Subject: [PATCH 24/38] Also rename expressionMetrics => usingMetrics in return object --- packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts | 2 +- packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts | 10 +++++----- packages/@aws-cdk/aws-cloudwatch/lib/metric.ts | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts index 6dbf114634073..7c8c127411a11 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts @@ -174,7 +174,7 @@ export interface MetricExpressionConfig { /** * Metrics used in the math expression */ - readonly expressionMetrics: Record; + readonly usingMetrics: Record; } /** diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts index b16031dd795d3..dfdfa050bb07f 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts @@ -175,7 +175,7 @@ export class MetricSet { // Recurse and add children const conf = metric.toMetricConfig(); if (conf.mathExpression) { - for (const [subId, subMetric] of Object.entries(conf.mathExpression.expressionMetrics)) { + for (const [subId, subMetric] of Object.entries(conf.mathExpression.usingMetrics)) { this.addOne(subMetric, undefined, subId); } } @@ -192,7 +192,7 @@ export function validateNoIdConflicts(expression: MathExpression) { // Nothing }, withExpression(expr) { - for (const [id, subMetric] of Object.entries(expr.expressionMetrics)) { + for (const [id, subMetric] of Object.entries(expr.usingMetrics)) { const existing = seen.get(id); if (existing && metricKey(existing) !== metricKey(subMetric)) { throw new Error(`Same id ('${id}') used for two metrics in the expression: '${subMetric}' and '${existing}'. Rename one.`); @@ -223,9 +223,9 @@ export function metricKey(metric: IMetric): string { const conf = metric.toMetricConfig(); if (conf.mathExpression) { parts.push(conf.mathExpression.expression); - for (const id of Object.keys(conf.mathExpression.expressionMetrics).sort()) { + for (const id of Object.keys(conf.mathExpression.usingMetrics).sort()) { parts.push(id); - parts.push(metricKey(conf.mathExpression.expressionMetrics[id])); + parts.push(metricKey(conf.mathExpression.usingMetrics[id])); } } if (conf.metricStat) { @@ -271,7 +271,7 @@ export function metricPeriod(metric: IMetric): Duration { return stat.period; }, withExpression(expr) { - const metrs = Object.values(expr.expressionMetrics); + const metrs = Object.values(expr.usingMetrics); let maxPeriod = Duration.seconds(0); for (const metr of metrs) { const p = metricPeriod(metr); diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts index d2a025b23f624..2ada826994a07 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts @@ -433,7 +433,7 @@ export class MathExpression implements IMetric { return { mathExpression: { expression: this.expression, - expressionMetrics: this.usingMetrics + usingMetrics: this.usingMetrics }, renderingProperties: { label: this.label, From 7b6936f62732aa5b2a21c7b5e7525f16a14cc6ca Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 31 Dec 2019 16:28:30 +0100 Subject: [PATCH 25/38] UPdate docstrings --- packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts | 2 +- packages/@aws-cdk/aws-cloudwatch/lib/metric.ts | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts index 7c8c127411a11..e39340d10c32c 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts @@ -221,7 +221,7 @@ export interface MetricAlarmConfig { /** * Properties used to construct the Metric identifying part of a Graph - * * + * * @deprecated Replaced by MetricConfig */ export interface MetricGraphConfig { diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts index 2ada826994a07..75d3a109abc6e 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts @@ -44,8 +44,7 @@ export interface CommonMetricOptions { /** * Unit for the metric that is associated with the alarm * - * @deprecated It is just used to select the data points with this specific unit and not to scale the threshold w.r.t the data points unit. - * It is most likely to be misused. + * @deprecated Unused, see package documentation. */ readonly unit?: Unit; From c3aad11e07b822a53dab8babb60a248ae7d7d113 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 2 Jan 2020 10:52:53 +0100 Subject: [PATCH 26/38] Make changePeriod recursive, fix cache key bug for prototyped objects --- .../aws-cloudwatch/lib/metric-util.ts | 3 +- .../@aws-cdk/aws-cloudwatch/lib/metric.ts | 38 ++++++++-- .../aws-cloudwatch/test/test.metric-math.ts | 74 ++++++++++++++++--- 3 files changed, 97 insertions(+), 18 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts index dfdfa050bb07f..597e5fc5bcbc3 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts @@ -215,9 +215,10 @@ const METRICKEY_SYMBOL = Symbol('@aws-cdk/aws-cloudwatch.MetricKey'); */ export function metricKey(metric: IMetric): string { // Cache on the object itself. This is safe because Metric objects are immutable. - if ((metric as any)[METRICKEY_SYMBOL] !== undefined) { + if (metric.hasOwnProperty(METRICKEY_SYMBOL)) { return (metric as any)[METRICKEY_SYMBOL]; } + const parts = new Array(); const conf = metric.toMetricConfig(); diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts index 75d3a109abc6e..85deaf933a473 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts @@ -432,7 +432,7 @@ export class MathExpression implements IMetric { return { mathExpression: { expression: this.expression, - usingMetrics: this.usingMetrics + usingMetrics: this.usingMetrics, }, renderingProperties: { label: this.label, @@ -593,8 +593,36 @@ function changeAllPeriods(metrics: Record, period: cdk.Duration * both implementations of IMetric have a `period` member that contains that particular * value. */ -function changePeriod(metric: A, period: cdk.Duration): A { - return Object.create(metric, { - period: { value: period, enumerable: true } - }); +function changePeriod(metric: A, period: cdk.Duration): IMetric { + if (isModifiableMetric(metric)) { + return metric.with({ period }); + } + + throw new Error(`Metric object should also implement 'with': ${metric}`); +} + +/** + * Private protocol for metrics + * + * Metric types used in a MathExpression need to implement at least this: + * a `with` method that takes at least a `period` and returns a modified copy + * of the metric objecdt. + * + * We put it here instead of on `IMetric` because there is no way to type + * it in jsii in a way that concrete implementations `Metric` and `MathExpression` + * can be statically typable about the fields that are changeable: all + * `with` methods would need to take the same argument type, but not all + * classes have the same `with`-able properties. + * + * This class exists to prevent having to use `instanceof` in the `changePeriod` + * function, so that we have a system where in principle new implementations + * of `IMetric` can be added. Because it will be rare, the mechanism doesn't have + * to be exposed very well, just has to be possible. + */ +interface IModifiableMetric { + with(options: { period?: cdk.Duration }): IMetric; +} + +function isModifiableMetric(m: any): m is IModifiableMetric { + return typeof m === 'object' && m !== null && !!m.with; } \ No newline at end of file diff --git a/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts b/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts index 23897b510aa4d..73a3685fb27e7 100644 --- a/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts +++ b/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts @@ -3,7 +3,7 @@ import { Duration, Stack } from '@aws-cdk/core'; import { Test } from 'nodeunit'; import { Alarm, GraphWidget, IWidget, MathExpression, Metric } from '../lib'; -const a = new Metric({ namespace: 'Test', metricName: 'ACount', period: Duration.seconds(10) }); +const a = new Metric({ namespace: 'Test', metricName: 'ACount' }); const b = new Metric({ namespace: 'Test', metricName: 'BCount', statistic: 'Average' }); const c = new Metric({ namespace: 'Test', metricName: 'CCount' }); const b99 = new Metric({ namespace: 'Test', metricName: 'BCount', statistic: 'p99' }); @@ -62,7 +62,7 @@ export = { // THEN graphMetricsAre(test, graph, [ [ { expression: 'a + b', label: 'a + b' } ], - [ 'Test', 'ACount', { period: 10, visible: false, id: 'a' } ], + [ 'Test', 'ACount', { visible: false, id: 'a' } ], [ 'Test', 'BCount', { visible: false, id: 'b' } ], ]); @@ -89,7 +89,7 @@ export = { // THEN graphMetricsAre(test, graph, [ [ { label: 'a + e', expression: 'a + e' } ], - [ 'Test', 'ACount', { period: 10, visible: false, id: 'a' } ], + [ 'Test', 'ACount', { visible: false, id: 'a' } ], [ { expression: 'b + c', visible: false, id: 'e' } ], [ 'Test', 'BCount', { visible: false, id: 'b' } ], [ 'Test', 'CCount', { visible: false, id: 'c' } ] @@ -116,9 +116,9 @@ export = { graphMetricsAre(test, graph, [ [ { label: 'a + e', expression: 'a + e' } ], - [ 'Test', 'ACount', { period: 10, visible: false, id: 'a' } ], + [ 'Test', 'ACount', { visible: false, id: 'a' } ], [ { expression: 'b + c', visible: false, id: 'e' } ], - [ 'Test', 'ACount', { period: 10, visible: false, id: 'b' } ], + [ 'Test', 'ACount', { visible: false, id: 'b' } ], [ 'Test', 'CCount', { visible: false, id: 'c' } ] ]); @@ -144,7 +144,7 @@ export = { // THEN graphMetricsAre(test, graph, [ [ { label: 'a + e', expression: 'a + e' } ], - [ 'Test', 'ACount', { period: 10, visible: false, id: 'a' } ], + [ 'Test', 'ACount', { visible: false, id: 'a' } ], [ { expression: 'a + c', visible: false, id: 'e' } ], [ 'Test', 'CCount', { visible: false, id: 'c' } ] ]); @@ -165,13 +165,65 @@ export = { // THEN graphMetricsAre(test, graph, [ - [ 'Test', 'ACount', { period: 10, id: 'a' } ], + [ 'Test', 'ACount', { id: 'a' } ], [ { label: 'a + b', expression: 'a + b' } ], [ 'Test', 'BCount', { visible: false, id: 'b' } ] ]); test.done(); }, + 'MathExpression controls period of metrics directly used in it'(test: Test) { + // Check that if we add A with { period: 10s } to a mathexpression of period 5m + // then two metric lines are added for A, one at 10s and one at 5m + const graph = new GraphWidget({ + left: [ + a.with({ period: Duration.seconds(10) }), + new MathExpression({ + expression: 'a + b', + usingMetrics: { a: a.with({ period: Duration.seconds(10) }), b } + }) + ], + }); + + // THEN + graphMetricsAre(test, graph, [ + [ 'Test', 'ACount', { period: 10 } ], + [ { label: 'a + b', expression: 'a + b' } ], + [ 'Test', 'ACount', { visible: false, id: 'a' } ], + [ 'Test', 'BCount', { visible: false, id: 'b' } ] + ]); + test.done(); + }, + + 'MathExpression controls period of metrics transitively used in it'(test: Test) { + // Same as the previous test, but recursively + + const graph = new GraphWidget({ + left: [ + new MathExpression({ + expression: 'a + e', + usingMetrics: { + a, + e: new MathExpression({ + expression: 'a + b', + period: Duration.minutes(1), + usingMetrics: { a, b } + }) + } + }) + ], + }); + + // THEN + graphMetricsAre(test, graph, [ + [ { expression: 'a + e', label: 'a + e' } ], + [ 'Test', 'ACount', { visible: false, id: 'a' } ], + [ { expression: 'a + b', visible: false, id: 'e' } ], + [ 'Test', 'BCount', { visible: false, id: 'b' } ] + ]); + test.done(); + }, + 'can use percentiles in expression metrics in graphs'(test: Test) { // GIVEN const graph = new GraphWidget({ @@ -186,7 +238,7 @@ export = { // THEN graphMetricsAre(test, graph, [ [ { expression: 'a + b99', label: 'a + b99' } ], - [ 'Test', 'ACount', { period: 10, visible: false, id: 'a' } ], + [ 'Test', 'ACount', { visible: false, id: 'a' } ], [ 'Test', 'BCount', { visible: false, id: 'b99', stat: 'p99' } ], ]); @@ -218,7 +270,6 @@ export = { MetricName: "ACount", Namespace: "Test" }, - Period: 10, Stat: "Average" }, ReturnData: false @@ -270,7 +321,6 @@ export = { MetricName: "ACount", Namespace: "Test" }, - Period: 10, Stat: "Average" }, ReturnData: false @@ -314,8 +364,9 @@ export = { const alarm = new Alarm(stack, 'Alarm', { threshold: 1, evaluationPeriods: 1, metric: new MathExpression({ + period: Duration.minutes(10), expression: 'a + b', - usingMetrics: { a, b: b.with({ period: Duration.minutes(10) }) } + usingMetrics: { a, b: b.with({ period: Duration.minutes(20) }) } // This is overridden }) }); @@ -351,7 +402,6 @@ export = { MetricName: "ACount", Namespace: "Test" }, - Period: 10, Stat: "Average" }, ReturnData: false From b3702cc5690f270d5892fc7aaa2e2a7c4ae1b5b0 Mon Sep 17 00:00:00 2001 From: Ahmed Sedek Date: Thu, 2 Jan 2020 11:44:57 +0100 Subject: [PATCH 27/38] Added more tests --- .../@aws-cdk/aws-cloudwatch/lib/metric.ts | 2 +- ...teg.math-alarm-and-dashboard.expected.json | 145 ++++++++++++++++++ .../test/integ.math-alarm-and-dashboard.ts | 67 ++++++++ .../aws-cloudwatch/test/test.metric-math.ts | 85 ++++++++++ .../aws-cloudwatch/test/test.metrics.ts | 10 +- 5 files changed, 307 insertions(+), 2 deletions(-) create mode 100644 packages/@aws-cdk/aws-cloudwatch/test/integ.math-alarm-and-dashboard.expected.json create mode 100644 packages/@aws-cdk/aws-cloudwatch/test/integ.math-alarm-and-dashboard.ts diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts index 85deaf933a473..f00d07fbdb002 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts @@ -202,7 +202,7 @@ export class Metric implements IMetric { this.period = props.period || cdk.Duration.minutes(5); const periodSec = this.period.toSeconds(); if (periodSec !== 1 && periodSec !== 5 && periodSec !== 10 && periodSec !== 30 && periodSec % 60 !== 0) { - throw new Error(`'period' must be 1, 5, 10, 30, or a multiple of 60 seconds, received ${props.period}`); + throw new Error(`'period' must be 1, 5, 10, 30, or a multiple of 60 seconds, received ${periodSec}`); } this.dimensions = props.dimensions; diff --git a/packages/@aws-cdk/aws-cloudwatch/test/integ.math-alarm-and-dashboard.expected.json b/packages/@aws-cdk/aws-cloudwatch/test/integ.math-alarm-and-dashboard.expected.json new file mode 100644 index 0000000000000..7de0e4290cd65 --- /dev/null +++ b/packages/@aws-cdk/aws-cloudwatch/test/integ.math-alarm-and-dashboard.expected.json @@ -0,0 +1,145 @@ +{ + "Resources": { + "queue": { + "Type": "AWS::SQS::Queue" + }, + "Alarm7103F465": { + "Type": "AWS::CloudWatch::Alarm", + "Properties": { + "ComparisonOperator": "GreaterThanOrEqualToThreshold", + "EvaluationPeriods": 3, + "Metrics": [ + { + "Expression": "m1+m2", + "Id": "expr_1", + "Label": "Total Messages" + }, + { + "Id": "m1", + "Label": "Visible Messages", + "MetricStat": { + "Metric": { + "Dimensions": [ + { + "Name": "QueueName", + "Value": { + "Fn::GetAtt": [ + "queue", + "QueueName" + ] + } + } + ], + "MetricName": "ApproximateNumberOfMessagesVisible", + "Namespace": "AWS/SQS" + }, + "Period": 60, + "Stat": "Average" + }, + "ReturnData": false + }, + { + "Id": "m2", + "Label": "NotVisible Messages", + "MetricStat": { + "Metric": { + "Dimensions": [ + { + "Name": "QueueName", + "Value": { + "Fn::GetAtt": [ + "queue", + "QueueName" + ] + } + } + ], + "MetricName": "ApproximateNumberOfMessagesNotVisible", + "Namespace": "AWS/SQS" + }, + "Period": 60, + "Stat": "Average" + }, + "ReturnData": false + } + ], + "Threshold": 100 + } + }, + "DashCCD7F836": { + "Type": "AWS::CloudWatch::Dashboard", + "Properties": { + "DashboardBody": { + "Fn::Join": [ + "", + [ + "{\"widgets\":[{\"type\":\"metric\",\"width\":6,\"height\":6,\"x\":0,\"y\":0,\"properties\":{\"view\":\"timeSeries\",\"title\":\"Total messages in queue\",\"region\":\"", + { + "Ref": "AWS::Region" + }, + "\",\"annotations\":{\"alarms\":[\"", + { + "Fn::GetAtt": [ + "Alarm7103F465", + "Arn" + ] + }, + "\"]},\"yAxis\":{}}},{\"type\":\"metric\",\"width\":6,\"height\":6,\"x\":0,\"y\":6,\"properties\":{\"view\":\"timeSeries\",\"title\":\"More total messages in queue with alarm annotation\",\"region\":\"", + { + "Ref": "AWS::Region" + }, + "\",\"metrics\":[[{\"label\":\"Total Messages\",\"expression\":\"m1+m2\"}],[\"AWS/SQS\",\"ApproximateNumberOfMessagesVisible\",\"QueueName\",\"", + { + "Fn::GetAtt": [ + "queue", + "QueueName" + ] + }, + "\",{\"label\":\"Visible Messages\",\"period\":60,\"visible\":false,\"id\":\"m1\"}],[\"AWS/SQS\",\"ApproximateNumberOfMessagesNotVisible\",\"QueueName\",\"", + { + "Fn::GetAtt": [ + "queue", + "QueueName" + ] + }, + "\",{\"label\":\"NotVisible Messages\",\"period\":60,\"visible\":false,\"id\":\"m2\"}],[\"AWS/SQS\",\"ApproximateNumberOfMessagesVisible\",\"QueueName\",\"", + { + "Fn::GetAtt": [ + "queue", + "QueueName" + ] + }, + "\",{\"label\":\"Visible Messages\",\"period\":10,\"yAxis\":\"right\"}],[\"AWS/SQS\",\"ApproximateNumberOfMessagesNotVisible\",\"QueueName\",\"", + { + "Fn::GetAtt": [ + "queue", + "QueueName" + ] + }, + "\",{\"label\":\"NotVisible Messages\",\"period\":30,\"yAxis\":\"right\"}]],\"annotations\":{\"horizontal\":[{\"label\":\"Total Messages >= 100 for 3 datapoints within 3 minutes\",\"value\":100,\"yAxis\":\"left\"}]},\"yAxis\":{}}},{\"type\":\"metric\",\"width\":6,\"height\":3,\"x\":0,\"y\":12,\"properties\":{\"view\":\"singleValue\",\"title\":\"Current total messages in queue\",\"region\":\"", + { + "Ref": "AWS::Region" + }, + "\",\"metrics\":[[{\"label\":\"Total Messages\",\"expression\":\"m1+m2\"}],[\"AWS/SQS\",\"ApproximateNumberOfMessagesVisible\",\"QueueName\",\"", + { + "Fn::GetAtt": [ + "queue", + "QueueName" + ] + }, + "\",{\"label\":\"Visible Messages\",\"period\":60,\"visible\":false,\"id\":\"m1\"}],[\"AWS/SQS\",\"ApproximateNumberOfMessagesNotVisible\",\"QueueName\",\"", + { + "Fn::GetAtt": [ + "queue", + "QueueName" + ] + }, + "\",{\"label\":\"NotVisible Messages\",\"period\":60,\"visible\":false,\"id\":\"m2\"}]]}}]}" + ] + ] + }, + "DashboardName": "MyMathExpressionDashboardName" + } + } + } +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-cloudwatch/test/integ.math-alarm-and-dashboard.ts b/packages/@aws-cdk/aws-cloudwatch/test/integ.math-alarm-and-dashboard.ts new file mode 100644 index 0000000000000..456e0b5871652 --- /dev/null +++ b/packages/@aws-cdk/aws-cloudwatch/test/integ.math-alarm-and-dashboard.ts @@ -0,0 +1,67 @@ +// Integration test to deploy some resources, create an alarm on it and create a dashboard. +// +// Because literally every other library is going to depend on @aws-cdk/aws-cloudwatch, we drop down +// to the very lowest level to create CloudFormation resources by hand, without even generated +// library support. + +import * as cdk from '@aws-cdk/core'; +import * as cloudwatch from '../lib'; + +const app = new cdk.App(); + +const stack = new cdk.Stack(app, `aws-cdk-cloudwatch`); + +const queue = new cdk.CfnResource(stack, 'queue', { type: 'AWS::SQS::Queue' }); + +const metricA = new cloudwatch.Metric({ + namespace: 'AWS/SQS', + metricName: 'ApproximateNumberOfMessagesVisible', + dimensions: { QueueName: queue.getAtt('QueueName') }, + period: cdk.Duration.seconds(10), + label: "Visible Messages" +}); + +const metricB = new cloudwatch.Metric({ + namespace: 'AWS/SQS', + metricName: 'ApproximateNumberOfMessagesNotVisible', + dimensions: { QueueName: queue.getAtt('QueueName') }, + period: cdk.Duration.seconds(30), + label: "NotVisible Messages" +}); + +const sumExpression = new cloudwatch.MathExpression({ + expression: "m1+m2", + usingMetrics: { + m1: metricA, + m2: metricB + }, + label: "Total Messages", + period: cdk.Duration.minutes(1) +}); + +const alarm = sumExpression.createAlarm(stack, 'Alarm', { + threshold: 100, + evaluationPeriods: 3, +}); + +const dashboard = new cloudwatch.Dashboard(stack, 'Dash', { + dashboardName: 'MyMathExpressionDashboardName' +}); +dashboard.addWidgets(new cloudwatch.AlarmWidget({ + title: 'Total messages in queue', + alarm, +})); + +dashboard.addWidgets(new cloudwatch.GraphWidget({ + title: 'More total messages in queue with alarm annotation', + left: [sumExpression], + right: [metricA, metricB], + leftAnnotations: [alarm.toAnnotation()] +})); + +dashboard.addWidgets(new cloudwatch.SingleValueWidget({ + title: 'Current total messages in queue', + metrics: [sumExpression] +})); + +app.synth(); diff --git a/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts b/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts index 73a3685fb27e7..e5a60ead2859e 100644 --- a/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts +++ b/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts @@ -47,6 +47,18 @@ export = { test.done(); }, + 'can not use invalid period in MathExpression'(test: Test) { + test.throws(() => { + new MathExpression({ + expression: 'a+b', + usingMetrics: {a, b}, + period: Duration.seconds(20) + }); + }, /'period' must be 1, 5, 10, 30, or a multiple of 60 seconds, received 20/); + + test.done(); + }, + 'in graphs': { 'MathExpressions can be added to a graph'(test: Test) { // GIVEN @@ -270,6 +282,7 @@ export = { MetricName: "ACount", Namespace: "Test" }, + Period: 300, Stat: "Average" }, ReturnData: false @@ -321,6 +334,7 @@ export = { MetricName: "ACount", Namespace: "Test" }, + Period: 300, Stat: "Average" }, ReturnData: false @@ -359,6 +373,76 @@ export = { test.done(); }, + 'MathExpression controls period of metrics transitively used in it with alarms'(test: Test) { + // GIVEN + new Alarm(stack, 'Alarm', { + threshold: 1, evaluationPeriods: 1, + metric: new MathExpression({ + expression: 'a + e', + usingMetrics: { + a, + e: new MathExpression({ + expression: 'b + c', + usingMetrics: { b, c }, + period: Duration.minutes(1) + }) + }, + period: Duration.seconds(30) + }) + }); + + // THEN + alarmMetricsAre([ + { + Expression: "a + e", + Id: "expr_1" + }, + { + Id: "a", + MetricStat: { + Metric: { + MetricName: "ACount", + Namespace: "Test" + }, + Period: 30, + Stat: "Average" + }, + ReturnData: false + }, + { + Expression: "b + c", + Id: "e", + ReturnData: false + }, + { + Id: "b", + MetricStat: { + Metric: { + MetricName: "BCount", + Namespace: "Test" + }, + Period: 30, + Stat: "Average" + }, + ReturnData: false + }, + { + Id: "c", + MetricStat: { + Metric: { + MetricName: "CCount", + Namespace: "Test" + }, + Period: 30, + Stat: "Average" + }, + ReturnData: false + } + ]); + + test.done(); + }, + 'annotation for a mathexpression alarm is calculated based upon constituent metrics'(test: Test) { // GIVEN const alarm = new Alarm(stack, 'Alarm', { @@ -402,6 +486,7 @@ export = { MetricName: "ACount", Namespace: "Test" }, + Period: 300, Stat: "Average" }, ReturnData: false diff --git a/packages/@aws-cdk/aws-cloudwatch/test/test.metrics.ts b/packages/@aws-cdk/aws-cloudwatch/test/test.metrics.ts index 45e6319831f87..d3ededab9e771 100644 --- a/packages/@aws-cdk/aws-cloudwatch/test/test.metrics.ts +++ b/packages/@aws-cdk/aws-cloudwatch/test/test.metrics.ts @@ -30,5 +30,13 @@ export = { })); test.done(); - } + }, + + 'can not use invalid period in Metric'(test: Test) { + test.throws(() => { + new Metric({ namespace: 'Test', metricName: 'ACount', period: cdk.Duration.seconds(20) }); + }, /'period' must be 1, 5, 10, 30, or a multiple of 60 seconds, received 20/); + + test.done(); + }, }; From 1a2d99674b06fdf83e77fd12bc6eaada5e7230b6 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 2 Jan 2020 14:32:36 +0100 Subject: [PATCH 28/38] Review comments --- packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts | 2 +- packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts | 6 +++--- packages/@aws-cdk/aws-cloudwatch/lib/metric.ts | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts b/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts index dd1ca24ff02ea..a93489ac81ebb 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts @@ -243,7 +243,7 @@ function renderAlarmMetric(metric: IMetric) { withExpression() { // Expand the math expression metric into a set const mset = new MetricSet(); - mset.addPrimary(true, metric); + mset.addTopLevel(true, metric); let eid = 0; function uniqueMetricId() { diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts index 597e5fc5bcbc3..3308c29f43539 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts @@ -19,8 +19,8 @@ export function allMetricsGraphJson(left: IMetric[], right: IMetric[]): any[][] // Add metrics to a set which will automatically expand them recursively, // making sure to retain conflicting the visible one on conflicting metrics objects. const mset = new MetricSet(); - mset.addPrimary('left', ...left); - mset.addPrimary('right', ...right); + mset.addTopLevel('left', ...left); + mset.addTopLevel('right', ...right); // Render all metrics from the set. return mset.entries.map(entry => metricGraphJson(entry.metric, entry.tag, entry.id)); @@ -109,7 +109,7 @@ export class MetricSet { /** * Add the given set of metrics to this set */ - public addPrimary(tag: A, ...metrics: IMetric[]) { + public addTopLevel(tag: A, ...metrics: IMetric[]) { for (const metric of metrics) { this.addOne(metric, tag); } diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts index 85deaf933a473..451ef98b47689 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts @@ -606,7 +606,7 @@ function changePeriod(metric: A, period: cdk.Duration): IMetr * * Metric types used in a MathExpression need to implement at least this: * a `with` method that takes at least a `period` and returns a modified copy - * of the metric objecdt. + * of the metric object. * * We put it here instead of on `IMetric` because there is no way to type * it in jsii in a way that concrete implementations `Metric` and `MathExpression` From 4de3951d4af87103346a44cbfa1dca15f545cf95 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 2 Jan 2020 14:56:43 +0100 Subject: [PATCH 29/38] Small fixes remaining --- .../aws-cloudwatch/lib/metric-util.ts | 21 +++++-------------- .../@aws-cdk/aws-cloudwatch/lib/metric.ts | 14 ++++++++++--- 2 files changed, 16 insertions(+), 19 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts index 3308c29f43539..05c4fa8d93197 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts @@ -258,29 +258,18 @@ export function metricKey(metric: IMetric): string { /** * Return the period of a metric * - * For a stat metric, return the immediate period. For an expression metric, - * return the longest period of its constituent metrics (because the math expression - * will only emit a data point if all of its parts emit a data point). + * For a stat metric, return the immediate period. * - * Formally it should be the LCM of the constituent periods, but the max is simpler, - * periods are likely to be multiples of one another anyway, and this is only used - * for display purposes. + * For an expression metric, all metrics used in it have been made to have the + * same period, so we return the period of the first inner metric. */ export function metricPeriod(metric: IMetric): Duration { return dispatchMetric(metric, { withStat(stat) { return stat.period; }, - withExpression(expr) { - const metrs = Object.values(expr.usingMetrics); - let maxPeriod = Duration.seconds(0); - for (const metr of metrs) { - const p = metricPeriod(metr); - if (p.toSeconds() > maxPeriod.toSeconds()) { - maxPeriod = p; - } - } - return maxPeriod; + withExpression() { + return (metric as MathExpression).period || Duration.minutes(5); }, }); } diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts index 658b68df44955..865cf06d80867 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts @@ -212,6 +212,7 @@ export class Metric implements IMetric { this.statistic = normalizeStatistic(props.statistic || "Average"); this.label = props.label; this.color = props.color; + this.unit = props.unit; this.account = props.account; this.region = props.region; } @@ -266,7 +267,7 @@ export class Metric implements IMetric { public toAlarmConfig(): MetricAlarmConfig { const metricConfig = this.toMetricConfig(); if (metricConfig.metricStat === undefined) { - throw new Error("A `Metric` object must be used here."); + throw new Error(`Using a math expression is not supported here. Pass a 'Metric' object instead`); } const stat = parseStatistic(metricConfig.metricStat.statistic); @@ -284,7 +285,7 @@ export class Metric implements IMetric { public toGraphConfig(): MetricGraphConfig { const metricConfig = this.toMetricConfig(); if (metricConfig.metricStat === undefined) { - throw new Error("MetricStatConfig need to set"); + throw new Error(`Using a math expression is not supported here. Pass a 'Metric' object instead`); } return { @@ -411,9 +412,16 @@ export class MathExpression implements IMetric { * @param props The set of properties to change. */ public with(props: MathExpressionOptions): MathExpression { + // Short-circuit creating a new object if there would be no effective change + if ((props.label === undefined || props.label === this.label) + && (props.color === undefined || props.color === this.color) + && (props.period === undefined)) { + return this; + } + return new MathExpression({ expression: this.expression, - usingMetrics: props.period ? changeAllPeriods(this.usingMetrics, props.period) : this.usingMetrics, + usingMetrics: this.usingMetrics, label: ifUndefined(props.label, this.label), color: ifUndefined(props.color, this.color), period: ifUndefined(props.period, this.period), From 9763e3357d38f768d6f7cda00268099822827505 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 2 Jan 2020 15:20:01 +0100 Subject: [PATCH 30/38] Update README --- packages/@aws-cdk/aws-cloudwatch/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudwatch/README.md b/packages/@aws-cdk/aws-cloudwatch/README.md index aa4dc460ffdc0..9f8b63397b38a 100644 --- a/packages/@aws-cdk/aws-cloudwatch/README.md +++ b/packages/@aws-cdk/aws-cloudwatch/README.md @@ -49,7 +49,7 @@ For example, a math expression that sums two other metrics looks like this: ```ts const allProblems = new MathExpression({ expression: "errors + faults", - expressionMetrics: { + usingMetrics: { errors: myConstruct.metricErrors(), faults: myConstruct.metricFaults(), } @@ -62,7 +62,7 @@ them in other math expressions: ```ts const problemPercentage = new MathExpression({ expression: "(problems / invocations) * 100", - expressionMetrics: { + usingMetrics: { problems: allProblems, invocations: myConstruct.metricInvocations() } From 728f53f9136d41ac8959d840b922e36be85c60ac Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 2 Jan 2020 15:45:26 +0100 Subject: [PATCH 31/38] Revert rash decision to drop support for `unit` completely, now just warn --- .../lib/target-tracking-scaling-policy.ts | 3 +- .../lib/target-tracking-scaling-policy.ts | 3 +- packages/@aws-cdk/aws-cloudwatch/README.md | 31 ++++++++++--------- packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts | 3 +- .../aws-cloudwatch/lib/metric-types.ts | 11 +++++++ .../@aws-cdk/aws-cloudwatch/lib/metric.ts | 16 ++++++++-- 6 files changed, 46 insertions(+), 21 deletions(-) diff --git a/packages/@aws-cdk/aws-applicationautoscaling/lib/target-tracking-scaling-policy.ts b/packages/@aws-cdk/aws-applicationautoscaling/lib/target-tracking-scaling-policy.ts index 5bf93a90985df..eac5808df0bd9 100644 --- a/packages/@aws-cdk/aws-applicationautoscaling/lib/target-tracking-scaling-policy.ts +++ b/packages/@aws-cdk/aws-applicationautoscaling/lib/target-tracking-scaling-policy.ts @@ -154,7 +154,8 @@ function renderCustomMetric(metric?: cloudwatch.IMetric): CfnScalingPolicy.Custo dimensions: c.dimensions, metricName: c.metricName, namespace: c.namespace, - statistic: c.statistic + statistic: c.statistic, + unit: c.unitFilter, }; } diff --git a/packages/@aws-cdk/aws-autoscaling/lib/target-tracking-scaling-policy.ts b/packages/@aws-cdk/aws-autoscaling/lib/target-tracking-scaling-policy.ts index 9dacc293ce862..81f8d807dd81c 100644 --- a/packages/@aws-cdk/aws-autoscaling/lib/target-tracking-scaling-policy.ts +++ b/packages/@aws-cdk/aws-autoscaling/lib/target-tracking-scaling-policy.ts @@ -151,7 +151,8 @@ function renderCustomMetric(metric?: cloudwatch.IMetric): CfnScalingPolicy.Custo dimensions: c.dimensions, metricName: c.metricName, namespace: c.namespace, - statistic: c.statistic + statistic: c.statistic, + unit: c.unitFilter, }; } diff --git a/packages/@aws-cdk/aws-cloudwatch/README.md b/packages/@aws-cdk/aws-cloudwatch/README.md index 9f8b63397b38a..e9e56345a21a0 100644 --- a/packages/@aws-cdk/aws-cloudwatch/README.md +++ b/packages/@aws-cdk/aws-cloudwatch/README.md @@ -144,21 +144,22 @@ The most important properties to set while creating an Alarms are: ### A note on units -Metrics values are emitted with units, such as `seconds` or `bytes`. In -CloudWatch, `Alarm`s do have a `unit` attribute, but that it is not used to -scale the threshold value but it is scaled to filter metric values inside the -same metric, considering only the metric values emitted under the given unit. - -For example, if a metric value is emitted as `1000 bytes`, and an alarm -threshold is set to `1 kilobyte` (including the unit), the alarm will *not* -trigger, as the units of the values and the alarm are different. - -It is therefore recommended to set the alarm (in this case) to `1000` and -leave out the unit. - -It is still possible to specify `unit` as a property when creating `Metric` -objects, but for safety that property will be ignored, because it will rarely -do what you want. +In CloudWatch, Metrics datums are emitted with units, such as `seconds` or +`bytes`. When `Metric` objects are given a `unit` attribute, it will be used to +*filter* the stream of metric datums for datums emitted using the same `unit` +attribute. + +In particular, it will *not* be used to rescale datums or alarm threshold +values. If you leave out the `unit` field, all metric datums will be retrieved, +regardless of unit. Because most of the time all datums for a given metric will +be emitted using only a single unit, it is recommended to not specify `unit` at +all and retrieve all values. + +Note that in any case, CloudWatch only supports filtering by `unit` for Alarms, +not in Dashboard graphs. + +Please upvote the following GitHub issue if you would like to see support for +unit calculations in CDK: https://github.com/aws/aws-cdk/issues/5595 ## Dashboards diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts b/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts index a93489ac81ebb..95ac4716461e7 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts @@ -237,6 +237,7 @@ function renderAlarmMetric(metric: IMetric) { period: st.period?.toSeconds(), statistic: renderIfSimpleStatistic(st.statistic), extendedStatistic: renderIfExtendedStatistic(st.statistic), + unit: st.unitFilter, }); }, @@ -262,7 +263,7 @@ function renderAlarmMetric(metric: IMetric) { }, period: stat.period.toSeconds(), stat: stat.statistic, - // unit: not used, on purpose. Does not do what we need it to do. + unit: stat.unitFilter, }, id: entry.id || uniqueMetricId(), label: conf.renderingProperties?.label, diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts index e39340d10c32c..22b1c47cc9357 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts @@ -147,6 +147,17 @@ export interface MetricStatConfig { */ readonly statistic: string; + /** + * Unit used to filter the metric stream + * + * Only refer to datums emitted to the metric stream with the given unit and + * ignore all others. Only useful when datums are being emitted to the same + * metric stream under different units. + * + * @default Refer to all metric datums + */ + readonly unitFilter?: Unit; + /** * Region which this metric comes from. * diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts index 865cf06d80867..27ba32956b373 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts @@ -42,9 +42,18 @@ export interface CommonMetricOptions { readonly dimensions?: DimensionHash; /** - * Unit for the metric that is associated with the alarm + * Unit used to filter the metric stream * - * @deprecated Unused, see package documentation. + * Only refer to datums emitted to the metric stream with the given unit and + * ignore all others. Only useful when datums are being emitted to the same + * metric stream under different units. + * + * The default is to use all matric datums in the stream, regardless of unit, + * which is recommended in nearly all cases. + * + * CloudWatch does not honor this property for graphs. + * + * @default All metric datums in the given metric stream */ readonly unit?: Unit; @@ -254,8 +263,9 @@ export class Metric implements IMetric { metricName: this.metricName, period: this.period, statistic: this.statistic, + unitFilter: this.unit, account: this.account, - region: this.region + region: this.region, }, renderingProperties: { color: this.color, From 8a2a67da64851c5ea7d629c4a54c83f098c91a30 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 2 Jan 2020 17:42:44 +0100 Subject: [PATCH 32/38] Update packages/@aws-cdk/aws-applicationautoscaling/lib/step-scaling-policy.ts Co-Authored-By: Romain Marcadier-Muller --- .../aws-applicationautoscaling/lib/step-scaling-policy.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-applicationautoscaling/lib/step-scaling-policy.ts b/packages/@aws-cdk/aws-applicationautoscaling/lib/step-scaling-policy.ts index f6e8bd26d9c19..50514841c229e 100644 --- a/packages/@aws-cdk/aws-applicationautoscaling/lib/step-scaling-policy.ts +++ b/packages/@aws-cdk/aws-applicationautoscaling/lib/step-scaling-policy.ts @@ -184,7 +184,7 @@ export interface ScalingInterval { function aggregationTypeFromMetric(metric: cloudwatch.IMetric): MetricAggregationType | undefined { const statistic = metric.toMetricConfig().metricStat?.statistic; - if (statistic === undefined) { return undefined; } // Math expression, don't know aggregation, leave default + if (statistic == null) { return undefined; } // Math expression, don't know aggregation, leave default switch (statistic) { case 'Average': From 58f63b320548aec5cda930e5787511395ce05f0d Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 2 Jan 2020 17:42:59 +0100 Subject: [PATCH 33/38] Update packages/@aws-cdk/aws-applicationautoscaling/lib/target-tracking-scaling-policy.ts Co-Authored-By: Romain Marcadier-Muller --- .../lib/target-tracking-scaling-policy.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-applicationautoscaling/lib/target-tracking-scaling-policy.ts b/packages/@aws-cdk/aws-applicationautoscaling/lib/target-tracking-scaling-policy.ts index eac5808df0bd9..6f6f7acfe6d6b 100644 --- a/packages/@aws-cdk/aws-applicationautoscaling/lib/target-tracking-scaling-policy.ts +++ b/packages/@aws-cdk/aws-applicationautoscaling/lib/target-tracking-scaling-policy.ts @@ -147,7 +147,7 @@ function renderCustomMetric(metric?: cloudwatch.IMetric): CfnScalingPolicy.Custo const c = metric.toMetricConfig().metricStat!; if (c.statistic.startsWith('p')) { - throw new Error(`Can not use statistic '${c.statistic}' for Target Tracking: only Average, Minimum, Maximum, SampleCount, Sum are supported.`); + throw new Error(`Cannot use statistic '${c.statistic}' for Target Tracking: only 'Average', 'Minimum', 'Maximum', 'SampleCount', and 'Sum' are supported.`); } return { From 70bd8bb2a60ab07aa60fd05ae3457f47d93dc4ba Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 2 Jan 2020 17:48:56 +0100 Subject: [PATCH 34/38] Update packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts Co-Authored-By: Romain Marcadier-Muller --- packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts b/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts index 95ac4716461e7..cebc8855f721c 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts @@ -265,7 +265,7 @@ function renderAlarmMetric(metric: IMetric) { stat: stat.statistic, unit: stat.unitFilter, }, - id: entry.id || uniqueMetricId(), + id: entry.id ?? uniqueMetricId(), label: conf.renderingProperties?.label, returnData: entry.tag ? undefined : false, // Tag stores "primary" attribute, default is "true" }; @@ -316,4 +316,4 @@ function renderIfExtendedStatistic(statistic?: string): string | undefined { return statistic.toLowerCase(); } return undefined; -} \ No newline at end of file +} From 6c0ee40ecc6b5d4477d4da22efc6c8499efb2cd4 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 2 Jan 2020 17:53:59 +0100 Subject: [PATCH 35/38] Revert quotes change --- .../aws-cloudwatch/lib/metric-types.ts | 66 +++++++++---------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts index 22b1c47cc9357..a6054f12b58fb 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts @@ -1,4 +1,4 @@ -import { Duration } from "@aws-cdk/core"; +import { Duration } from '@aws-cdk/core'; /** * Interface for metrics @@ -44,44 +44,44 @@ export interface Dimension { * Statistic to use over the aggregation period */ export enum Statistic { - SAMPLE_COUNT = "SampleCount", - AVERAGE = "Average", - SUM = "Sum", - MINIMUM = "Minimum", - MAXIMUM = "Maximum" + SAMPLE_COUNT = 'SampleCount', + AVERAGE = 'Average', + SUM = 'Sum', + MINIMUM = 'Minimum', + MAXIMUM = 'Maximum' } /** * Unit for metric */ export enum Unit { - SECONDS = "Seconds", - MICROSECONDS = "Microseconds", - MILLISECONDS = "Milliseconds", - BYTES = "Bytes", - KILOBYTES = "Kilobytes", - MEGABYTES = "Megabytes", - GIGABYTES = "Gigabytes", - TERABYTES = "Terabytes", - BITS = "Bits", - KILOBITS = "Kilobits", - MEGABITS = "Megabits", - GIGABITS = "Gigabits", - TERABITS = "Terabits", - PERCENT = "Percent", - COUNT = "Count", - BYTES_PER_SECOND = "Bytes/Second", - KILOBYTES_PER_SECOND = "Kilobytes/Second", - MEGABYTES_PER_SECOND = "Megabytes/Second", - GIGABYTES_PER_SECOND = "Gigabytes/Second", - TERABYTES_PER_SECOND = "Terabytes/Second", - BITS_PER_SECOND = "Bits/Second", - KILOBITS_PER_SECOND = "Kilobits/Second", - MEGABITS_PER_SECOND = "Megabits/Second", - GIGABITS_PER_SECOND = "Gigabits/Second", - TERABITS_PER_SECOND = "Terabits/Second", - COUNT_PER_SECOND = "Count/Second", - NONE = "None" + SECONDS = 'Seconds', + MICROSECONDS = 'Microseconds', + MILLISECONDS = 'Milliseconds', + BYTES = 'Bytes', + KILOBYTES = 'Kilobytes', + MEGABYTES = 'Megabytes', + GIGABYTES = 'Gigabytes', + TERABYTES = 'Terabytes', + BITS = 'Bits', + KILOBITS = 'Kilobits', + MEGABITS = 'Megabits', + GIGABITS = 'Gigabits', + TERABITS = 'Terabits', + PERCENT = 'Percent', + COUNT = 'Count', + BYTES_PER_SECOND = 'Bytes/Second', + KILOBYTES_PER_SECOND = 'Kilobytes/Second', + MEGABYTES_PER_SECOND = 'Megabytes/Second', + GIGABYTES_PER_SECOND = 'Gigabytes/Second', + TERABYTES_PER_SECOND = 'Terabytes/Second', + BITS_PER_SECOND = 'Bits/Second', + KILOBITS_PER_SECOND = 'Kilobits/Second', + MEGABITS_PER_SECOND = 'Megabits/Second', + GIGABITS_PER_SECOND = 'Gigabits/Second', + TERABITS_PER_SECOND = 'Terabits/Second', + COUNT_PER_SECOND = 'Count/Second', + NONE = 'None' } /** From bc86893534f3671f509794c8864584003b5d6aca Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 3 Jan 2020 10:47:13 +0100 Subject: [PATCH 36/38] Review comments --- .../lib/target-tracking-scaling-policy.ts | 2 +- .../lib/target-tracking-scaling-policy.ts | 2 +- packages/@aws-cdk/aws-cloudwatch/README.md | 86 ++++++------- packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts | 8 +- .../aws-cloudwatch/lib/metric-types.ts | 13 +- .../aws-cloudwatch/lib/metric-util.ts | 32 +++-- .../@aws-cdk/aws-cloudwatch/lib/metric.ts | 16 ++- .../@aws-cdk/aws-codebuild/lib/project.ts | 2 +- yarn.lock | 113 ++++-------------- 9 files changed, 111 insertions(+), 163 deletions(-) diff --git a/packages/@aws-cdk/aws-applicationautoscaling/lib/target-tracking-scaling-policy.ts b/packages/@aws-cdk/aws-applicationautoscaling/lib/target-tracking-scaling-policy.ts index 6f6f7acfe6d6b..053dca7f4f289 100644 --- a/packages/@aws-cdk/aws-applicationautoscaling/lib/target-tracking-scaling-policy.ts +++ b/packages/@aws-cdk/aws-applicationautoscaling/lib/target-tracking-scaling-policy.ts @@ -116,7 +116,7 @@ export class TargetTrackingScalingPolicy extends cdk.Construct { } if (props.customMetric && !props.customMetric.toMetricConfig().metricStat) { - throw new Error(`Math expressions are not supported for Target Tracking. Use Step Scaling or supply a Metric object.`); + throw new Error(`Only direct metrics are supported for Target Tracking. Use Step Scaling or supply a Metric object.`); } super(scope, id); diff --git a/packages/@aws-cdk/aws-autoscaling/lib/target-tracking-scaling-policy.ts b/packages/@aws-cdk/aws-autoscaling/lib/target-tracking-scaling-policy.ts index 81f8d807dd81c..fde4e3319428d 100644 --- a/packages/@aws-cdk/aws-autoscaling/lib/target-tracking-scaling-policy.ts +++ b/packages/@aws-cdk/aws-autoscaling/lib/target-tracking-scaling-policy.ts @@ -118,7 +118,7 @@ export class TargetTrackingScalingPolicy extends cdk.Construct { } if (props.customMetric && !props.customMetric.toMetricConfig().metricStat) { - throw new Error(`Math expressions are not supported for Target Tracking. Use Step Scaling or supply a Metric object.`); + throw new Error(`Only direct metrics are supported for Target Tracking. Use Step Scaling or supply a Metric object.`); } super(scope, id); diff --git a/packages/@aws-cdk/aws-cloudwatch/README.md b/packages/@aws-cdk/aws-cloudwatch/README.md index e9e56345a21a0..e3484a83236fa 100644 --- a/packages/@aws-cdk/aws-cloudwatch/README.md +++ b/packages/@aws-cdk/aws-cloudwatch/README.md @@ -33,11 +33,11 @@ you can instantiate a `Metric` object to represent it. For example: ```ts const metric = new Metric({ - namespace: 'MyNamespace', - metricName: 'MyMetric', - dimensions: { - ProcessingStep: 'Download' - } + namespace: 'MyNamespace', + metricName: 'MyMetric', + dimensions: { + ProcessingStep: 'Download' + } }); ``` @@ -48,11 +48,11 @@ For example, a math expression that sums two other metrics looks like this: ```ts const allProblems = new MathExpression({ - expression: "errors + faults", - usingMetrics: { - errors: myConstruct.metricErrors(), - faults: myConstruct.metricFaults(), - } + expression: "errors + faults", + usingMetrics: { + errors: myConstruct.metricErrors(), + faults: myConstruct.metricFaults(), + } }) ``` @@ -61,11 +61,11 @@ them in other math expressions: ```ts const problemPercentage = new MathExpression({ - expression: "(problems / invocations) * 100", - usingMetrics: { - problems: allProblems, - invocations: myConstruct.metricInvocations() - } + expression: "(problems / invocations) * 100", + usingMetrics: { + problems: allProblems, + invocations: myConstruct.metricInvocations() + } }) ``` @@ -85,9 +85,9 @@ to the metric function call: ```ts const minuteErrorRate = fn.metricErrors({ - statistic: 'avg', - period: Duration.minutes(1), - label: 'Lambda failure rate' + statistic: 'avg', + period: Duration.minutes(1), + label: 'Lambda failure rate' }); ``` @@ -120,9 +120,9 @@ object, passing the `Metric` object to set the alarm on: ```ts new Alarm(this, 'Alarm', { - metric: fn.metricErrors(), - threshold: 100, - evaluationPeriods: 2, + metric: fn.metricErrors(), + threshold: 100, + evaluationPeriods: 2, }); ``` @@ -130,8 +130,8 @@ Alternatively, you can call `metric.createAlarm()`: ```ts fn.metricErrors().createAlarm(this, 'Alarm', { - threshold: 100, - evaluationPeriods: 2, + threshold: 100, + evaluationPeriods: 2, }); ``` @@ -149,17 +149,17 @@ In CloudWatch, Metrics datums are emitted with units, such as `seconds` or *filter* the stream of metric datums for datums emitted using the same `unit` attribute. -In particular, it will *not* be used to rescale datums or alarm threshold -values. If you leave out the `unit` field, all metric datums will be retrieved, -regardless of unit. Because most of the time all datums for a given metric will -be emitted using only a single unit, it is recommended to not specify `unit` at -all and retrieve all values. +In particular, the `unit` field is *not* used to rescale datums or alarm threshold +values (for example, it cannot be used to specify an alarm threshold in +*Megabytes* if the metric stream is being emitted as *bytes*). -Note that in any case, CloudWatch only supports filtering by `unit` for Alarms, -not in Dashboard graphs. +You almost certainly don't want to specify the `unit` property when creating +`Metric` objects (which will retrieve all datums regardless of their unit), +unless you have very specific requirements. Note that in any case, CloudWatch +only supports filtering by `unit` for Alarms, not in Dashboard graphs. -Please upvote the following GitHub issue if you would like to see support for -unit calculations in CDK: https://github.com/aws/aws-cdk/issues/5595 +Please see the following GitHub issue for a discussion on real unit +calculations in CDK: https://github.com/aws/aws-cdk/issues/5595 ## Dashboards @@ -183,15 +183,15 @@ A graph widget can display any number of metrics on either the `left` or ```ts dashboard.addWidgets(new GraphWidget({ - title: "Executions vs error rate", + title: "Executions vs error rate", - left: [executionCountMetric], + left: [executionCountMetric], - right: [errorCountMetric.with({ - statistic: "average", - label: "Error rate", - color: "00FF00" - })] + right: [errorCountMetric.with({ + statistic: "average", + label: "Error rate", + color: "00FF00" + })] })); ``` @@ -201,8 +201,8 @@ An alarm widget shows the graph and the alarm line of a single alarm: ```ts dashboard.addWidgets(new AlarmWidget({ - title: "Errors", - alarm: errorAlarm, + title: "Errors", + alarm: errorAlarm, })); ``` @@ -213,7 +213,7 @@ to a graph of the value over time): ```ts dashboard.addWidgets(new SingleValueWidget({ - metrics: [visitorCount, purchaseCount], + metrics: [visitorCount, purchaseCount], })); ``` @@ -224,7 +224,7 @@ to your dashboard: ```ts dashboard.addWidgets(new TextWidget({ - markdown: '# Key Performance Indicators' + markdown: '# Key Performance Indicators' })); ``` diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts b/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts index cebc8855f721c..1977335d5d455 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts @@ -4,7 +4,7 @@ import { CfnAlarm } from './cloudwatch.generated'; import { HorizontalAnnotation } from './graph'; import { CreateAlarmOptions } from './metric'; import { IMetric } from './metric-types'; -import { dispatchMetric, dropUndef, metricPeriod, MetricSet } from './metric-util'; +import { dispatchMetric, dropUndefined, metricPeriod, MetricSet } from './metric-util'; import { parseStatistic } from './util.statistic'; export interface IAlarm extends IResource { @@ -143,7 +143,7 @@ export class Alarm extends Resource implements IAlarm { // Metric ...renderAlarmMetric(props.metric), - ...dropUndef({ + ...dropUndefined({ // Alarm overrides period: props.period && props.period.toSeconds(), statistic: renderIfSimpleStatistic(props.statistic), @@ -230,7 +230,7 @@ export class Alarm extends Resource implements IAlarm { function renderAlarmMetric(metric: IMetric) { return dispatchMetric(metric, { withStat(st) { - return dropUndef({ + return dropUndefined({ dimensions: st.dimensions, namespace: st.namespace, metricName: st.metricName, @@ -273,7 +273,7 @@ function renderAlarmMetric(metric: IMetric) { withExpression(expr, conf) { return { expression: expr.expression, - id: entry.id || uniqueMetricId(), + id: entry.id ?? uniqueMetricId(), label: conf.renderingProperties?.label, returnData: entry.tag ? undefined : false, // Tag stores "primary" attribute, default is "true" }; diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts index a6054f12b58fb..424d7160045f4 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric-types.ts @@ -91,14 +91,14 @@ export interface MetricConfig { /** * In case the metric represents a query, the details of the query * - * @default unset + * @default - unset */ readonly metricStat?: MetricStatConfig; /** * In case the metric is a math expression, the details of the math expression * - * @default unset + * @default - unset */ readonly mathExpression?: MetricExpressionConfig; @@ -110,7 +110,7 @@ export interface MetricConfig { * * @default {} */ - readonly renderingProperties?: Record; + readonly renderingProperties?: Record; } /** @@ -154,7 +154,10 @@ export interface MetricStatConfig { * ignore all others. Only useful when datums are being emitted to the same * metric stream under different units. * - * @default Refer to all metric datums + * This field has been renamed from plain `unit` to clearly communicate + * its purpose. + * + * @default - Refer to all metric datums */ readonly unitFilter?: Unit; @@ -295,7 +298,7 @@ export interface MetricGraphConfig { /** * Custom rendering properties that override the default rendering properties specified in the yAxis parameter of the widget object. * - * @deprecated Replaced by `MetricConfig`. + * @deprecated Replaced by MetricConfig. */ export interface MetricRenderingProperties { /** diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts index 05c4fa8d93197..f6c8767fecced 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric-util.ts @@ -1,6 +1,6 @@ -import { Duration } from "@aws-cdk/core"; -import { MathExpression } from "./metric"; -import { IMetric, MetricConfig, MetricExpressionConfig, MetricStatConfig } from "./metric-types"; +import { Duration } from '@aws-cdk/core'; +import { MathExpression } from './metric'; +import { IMetric, MetricConfig, MetricExpressionConfig, MetricStatConfig } from './metric-types'; /** * Return the JSON structure which represents these metrics in a graph. @@ -30,7 +30,7 @@ function metricGraphJson(metric: IMetric, yAxis?: string, id?: string) { const config = metric.toMetricConfig(); const ret: any[] = []; - const options: any = { ...config.renderingProperties || {} }; + const options: any = {...config.renderingProperties}; dispatchMetric(metric, { withStat(stat) { @@ -61,14 +61,14 @@ function metricGraphJson(metric: IMetric, yAxis?: string, id?: string) { if (yAxis !== 'left') { options.yAxis = yAxis; } if (id) { options.id = id; } - // If math expressions don't have a label (or an ID), they'll render in an ugly - // way (like "metric_alias0"). The ID may be autogenerated in a silly way, so if a - // mathexpression doesn't have a label, use its toString() as the label. + // If math expressions don't have a label (or an ID), they'll render with an unelegant + // autogenerated id ("metric_alias0"). Our ids may in the future also be autogenerated, + // so if an ME doesn't have a label, use its toString() as the label (renders the expression). if (options.visible !== false && options.expression && !options.label) { - options.label = `${metric}`; + options.label = metric.toString(); } - const renderedOpts = dropUndef(options); + const renderedOpts = dropUndefined(options); if (Object.keys(renderedOpts).length !== 0) { ret.push(renderedOpts); @@ -115,7 +115,10 @@ export class MetricSet { } } - public get entries(): Array> { + /** + * Access all the accumulated timeseries entries + */ + public get entries(): ReadonlyArray> { return this.metrics; } @@ -195,7 +198,7 @@ export function validateNoIdConflicts(expression: MathExpression) { for (const [id, subMetric] of Object.entries(expr.usingMetrics)) { const existing = seen.get(id); if (existing && metricKey(existing) !== metricKey(subMetric)) { - throw new Error(`Same id ('${id}') used for two metrics in the expression: '${subMetric}' and '${existing}'. Rename one.`); + throw new Error(`The ID '${id}' used for two metrics in the expression: '${subMetric}' and '${existing}'. Rename one.`); } seen.set(id, subMetric); visit(subMetric); @@ -277,7 +280,9 @@ export function metricPeriod(metric: IMetric): Duration { // tslint:disable-next-line:max-line-length export function dispatchMetric(metric: IMetric, fns: { withStat: (x: MetricStatConfig, c: MetricConfig) => A, withExpression: (x: MetricExpressionConfig, c: MetricConfig) => B }): A | B { const conf = metric.toMetricConfig(); - if (conf.metricStat) { + if (conf.metricStat && conf.mathExpression) { + throw new Error(`Metric object must not produce both 'metricStat' and 'mathExpression'`); + } else if (conf.metricStat) { return fns.withStat(conf.metricStat, conf); } else if (conf.mathExpression) { return fns.withExpression(conf.mathExpression, conf); @@ -286,7 +291,8 @@ export function dispatchMetric(metric: IMetric, fns: { withStat: (x: Metri } } -export function dropUndef(x: T): T { +export function dropUndefined(x: T): T { + if (x === null) { return x; } const ret: any = {}; for (const [key, value] of Object.entries(x)) { if (value !== undefined) { diff --git a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts index 27ba32956b373..b6447122dc803 100644 --- a/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts +++ b/packages/@aws-cdk/aws-cloudwatch/lib/metric.ts @@ -305,14 +305,14 @@ export class Metric implements IMetric { renderingProperties: { period: metricConfig.metricStat.period.toSeconds(), stat: metricConfig.metricStat.statistic, - color: metricConfig.renderingProperties?.color, - label: metricConfig.renderingProperties?.label + color: asString(metricConfig.renderingProperties?.color), + label: asString(metricConfig.renderingProperties?.label), }, // deprecated properties for backwards compatibility period: metricConfig.metricStat.period.toSeconds(), statistic: metricConfig.metricStat.statistic, - color: metricConfig.renderingProperties?.color, - label: metricConfig.renderingProperties?.label, + color: asString(metricConfig.renderingProperties?.color), + label: asString(metricConfig.renderingProperties?.label), unit: this.unit }; } @@ -361,6 +361,14 @@ export class Metric implements IMetric { } } +function asString(x?: unknown): string | undefined { + if (x === undefined) { return undefined; } + if (typeof x !== 'string') { + throw new Error(`Expected string, got ${x}`); + } + return x; +} + /** * A math expression built with metric(s) emitted by a service * diff --git a/packages/@aws-cdk/aws-codebuild/lib/project.ts b/packages/@aws-cdk/aws-codebuild/lib/project.ts index 9dd836bba2b67..ebdaabbf1f9f7 100644 --- a/packages/@aws-cdk/aws-codebuild/lib/project.ts +++ b/packages/@aws-cdk/aws-codebuild/lib/project.ts @@ -322,7 +322,7 @@ abstract class ProjectBase extends Resource implements IProject { metricName, dimensions: { ProjectName: this.projectName }, ...props - }); + }).attachTo(this); } /** diff --git a/yarn.lock b/yarn.lock index 2bb7a017bc6dc..806118d8c6a88 100644 --- a/yarn.lock +++ b/yarn.lock @@ -432,6 +432,13 @@ dependencies: jsonschema "^1.2.5" +"@jsii/spec@^0.21.0": + version "0.21.0" + resolved "https://registry.yarnpkg.com/@jsii/spec/-/spec-0.21.0.tgz#beb1403e3afbe055e7b7b31ab282febbbf1b023e" + integrity sha512-6VV3aOeo3YlElqPtpZncPt4jiZgurF1fyi1qjReSvpVYRuO3zC3aDP+fPzFrjROxcAXewte0+1HGxCaC5cB2pw== + dependencies: + jsonschema "^1.2.5" + "@lerna/add@3.20.0": version "3.20.0" resolved "https://registry.yarnpkg.com/@lerna/add/-/add-3.20.0.tgz#bea7edf36fc93fb72ec34cb9ba854c48d4abf309" @@ -1668,11 +1675,6 @@ anymatch@^2.0.0: micromatch "^3.1.4" normalize-path "^2.1.1" -app-root-path@^2.2.1: - version "2.2.1" - resolved "https://registry.yarnpkg.com/app-root-path/-/app-root-path-2.2.1.tgz#d0df4a682ee408273583d43f6f79e9892624bc9a" - integrity sha512-91IFKeKk7FjfmezPKkwtaRvSpnUc4gDwPAjA1YZ9Gn0q0PPeW+vbeUsZuyDwjI7+QTHhcLen2v25fi/AmhvbJA== - append-transform@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/append-transform/-/append-transform-1.0.0.tgz#046a52ae582a228bd72f58acfbe2967c678759ab" @@ -2414,6 +2416,15 @@ codemaker@^0.20.11: decamelize "^1.2.0" fs-extra "^8.1.0" +codemaker@^0.21.0: + version "0.21.0" + resolved "https://registry.yarnpkg.com/codemaker/-/codemaker-0.21.0.tgz#48a2689b2f9dae0a17605cbfdc3f4cb87d241b30" + integrity sha512-DC+ri9UWyAuJriTuAm5SIFhkP82KkwLFNDjKsvmfyl6t4rA2K8C327bAoWQLINUOxsNUZTArcRMhbf5ZDQerQw== + dependencies: + camelcase "^5.3.1" + decamelize "^1.2.0" + fs-extra "^8.1.0" + collection-visit@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/collection-visit/-/collection-visit-1.0.0.tgz#4bc0373c164bc3291b4d368c829cf1a80a59dca0" @@ -3184,16 +3195,6 @@ dot-prop@^4.2.0: dependencies: is-obj "^1.0.0" -dotenv-json@^1.0.0: - version "1.0.0" - resolved "https://registry.yarnpkg.com/dotenv-json/-/dotenv-json-1.0.0.tgz#fc7f672aafea04bed33818733b9f94662332815c" - integrity sha512-jAssr+6r4nKhKRudQ0HOzMskOFFi9+ubXWwmrSGJFgTvpjyPXCXsCsYbjif6mXp7uxA7xY3/LGaiTQukZzSbOQ== - -dotenv@^8.0.0: - version "8.2.0" - resolved "https://registry.yarnpkg.com/dotenv/-/dotenv-8.2.0.tgz#97e619259ada750eea3e4ea3e26bceea5424b16a" - integrity sha512-8sJ78ElpbDJBHNeBzUbUVLsqKdccaa/BXF1uPTw3GrvQTBgrQrtObr2mUrE38vzYd8cEv+m/JBfDLioYcfXoaw== - dreamopt@~0.6.0: version "0.6.0" resolved "https://registry.yarnpkg.com/dreamopt/-/dreamopt-0.6.0.tgz#d813ccdac8d39d8ad526775514a13dda664d6b4b" @@ -3366,11 +3367,6 @@ escodegen@^1.9.1: optionalDependencies: source-map "~0.6.1" -eslint-config-standard@^14.1.0: - version "14.1.0" - resolved "https://registry.yarnpkg.com/eslint-config-standard/-/eslint-config-standard-14.1.0.tgz#b23da2b76fe5a2eba668374f246454e7058f15d4" - integrity sha512-EF6XkrrGVbvv8hL/kYa/m6vnvmUT+K82pJJc4JJVMM6+Qgqh0pnwprSxdduDLB9p/7bIxD+YV5O0wfb8lmcPbA== - eslint-import-resolver-node@^0.3.2: version "0.3.2" resolved "https://registry.yarnpkg.com/eslint-import-resolver-node/-/eslint-import-resolver-node-0.3.2.tgz#58f15fb839b8d0576ca980413476aab2472db66a" @@ -3398,14 +3394,6 @@ eslint-module-utils@^2.4.1: debug "^2.6.9" pkg-dir "^2.0.0" -eslint-plugin-es@^2.0.0: - version "2.0.0" - resolved "https://registry.yarnpkg.com/eslint-plugin-es/-/eslint-plugin-es-2.0.0.tgz#0f5f5da5f18aa21989feebe8a73eadefb3432976" - integrity sha512-f6fceVtg27BR02EYnBhgWLFQfK6bN4Ll0nQFrBHOlCsAyxeZkn0NHns5O0YZOPrV1B3ramd6cgFwaoFLcSkwEQ== - dependencies: - eslint-utils "^1.4.2" - regexpp "^3.0.0" - eslint-plugin-import@^2.19.1: version "2.19.1" resolved "https://registry.yarnpkg.com/eslint-plugin-import/-/eslint-plugin-import-2.19.1.tgz#5654e10b7839d064dd0d46cd1b88ec2133a11448" @@ -3424,28 +3412,6 @@ eslint-plugin-import@^2.19.1: read-pkg-up "^2.0.0" resolve "^1.12.0" -eslint-plugin-node@^10.0.0: - version "10.0.0" - resolved "https://registry.yarnpkg.com/eslint-plugin-node/-/eslint-plugin-node-10.0.0.tgz#fd1adbc7a300cf7eb6ac55cf4b0b6fc6e577f5a6" - integrity sha512-1CSyM/QCjs6PXaT18+zuAXsjXGIGo5Rw630rSKwokSs2jrYURQc4R5JZpoanNCqwNmepg+0eZ9L7YiRUJb8jiQ== - dependencies: - eslint-plugin-es "^2.0.0" - eslint-utils "^1.4.2" - ignore "^5.1.1" - minimatch "^3.0.4" - resolve "^1.10.1" - semver "^6.1.0" - -eslint-plugin-promise@^4.2.1: - version "4.2.1" - resolved "https://registry.yarnpkg.com/eslint-plugin-promise/-/eslint-plugin-promise-4.2.1.tgz#845fd8b2260ad8f82564c1222fce44ad71d9418a" - integrity sha512-VoM09vT7bfA7D+upt+FjeBO5eHIJQBUWki1aPvB+vbNiHS3+oGIJGIeyBtKQTME6UPXXy3vV07OL1tHd3ANuDw== - -eslint-plugin-standard@^4.0.1: - version "4.0.1" - resolved "https://registry.yarnpkg.com/eslint-plugin-standard/-/eslint-plugin-standard-4.0.1.tgz#ff0519f7ffaff114f76d1bd7c3996eef0f6e20b4" - integrity sha512-v/KBnfyaOMPmZc/dmc6ozOdWqekGp7bBGq4jLAecEfPGmfKiWS4sA8sC0LqiV9w5qmXAtXVn4M3p1jSyhY85SQ== - eslint-scope@^5.0.0: version "5.0.0" resolved "https://registry.yarnpkg.com/eslint-scope/-/eslint-scope-5.0.0.tgz#e87c8887c73e8d1ec84f1ca591645c358bfc8fb9" @@ -3454,7 +3420,7 @@ eslint-scope@^5.0.0: esrecurse "^4.1.0" estraverse "^4.1.1" -eslint-utils@^1.4.2, eslint-utils@^1.4.3: +eslint-utils@^1.4.3: version "1.4.3" resolved "https://registry.yarnpkg.com/eslint-utils/-/eslint-utils-1.4.3.tgz#74fec7c54d0776b6f67e0251040b5806564e981f" integrity sha512-fbBN5W2xdY45KulGXmLHZ3c3FHfVYmKg0IrAKGOkT/464PQsx2UeIzfz1RmEci+KLm1bBaAzZAh8+/E+XAeZ8Q== @@ -4390,11 +4356,6 @@ ignore@^4.0.3, ignore@^4.0.6: resolved "https://registry.yarnpkg.com/ignore/-/ignore-4.0.6.tgz#750e3db5862087b4737ebac8207ffd1ef27b25fc" integrity sha512-cyFDKrqc/YdcWFniJhzI42+AzS+gNwmUzOSFcRCQYwySuBBBy/KjuxWLZ/FHEH6Moq1NizMOBWyTcv8O4OZIMg== -ignore@^5.1.1: - version "5.1.4" - resolved "https://registry.yarnpkg.com/ignore/-/ignore-5.1.4.tgz#84b7b3dbe64552b6ef0eca99f6743dbec6d97adf" - integrity sha512-MzbUSahkTW1u7JpKKjY7LCARd1fU5W2rLdxlM4kdkayuCwZImjkpluF9CM1aLewYJguPDqewLam18Y6AU69A8A== - immediate@~3.0.5: version "3.0.6" resolved "https://registry.yarnpkg.com/immediate/-/immediate-3.0.6.tgz#9db1dbd0faf8de6fbe0f5dd5e56bb606280de69b" @@ -5529,24 +5490,6 @@ kleur@^3.0.3: resolved "https://registry.yarnpkg.com/kleur/-/kleur-3.0.3.tgz#a79c9ecc86ee1ce3fa6206d1216c501f147fc07e" integrity sha512-eTIzlVOSUR+JxdDFepEYcBMtZ9Qqdef+rnzWdRZuMbOywu5tO2w2N7rqjoANZ5k9vywhL6Br1VRjUIgTQx4E8w== -lambda-leak@^2.0.0: - version "2.0.0" - resolved "https://registry.yarnpkg.com/lambda-leak/-/lambda-leak-2.0.0.tgz#771985d3628487f6e885afae2b54510dcfb2cd7e" - integrity sha1-dxmF02KEh/boha+uK1RRDc+yzX4= - -lambda-tester@^3.6.0: - version "3.6.0" - resolved "https://registry.yarnpkg.com/lambda-tester/-/lambda-tester-3.6.0.tgz#ceb7d4f4f0da768487a05cff37dcd088508b5247" - integrity sha512-F2ZTGWCLyIR95o/jWK46V/WnOCFAEUG/m/V7/CLhPJ7PCM+pror1rZ6ujP3TkItSGxUfpJi0kqwidw+M/nEqWw== - dependencies: - app-root-path "^2.2.1" - dotenv "^8.0.0" - dotenv-json "^1.0.0" - lambda-leak "^2.0.0" - semver "^6.1.1" - uuid "^3.3.2" - vandium-utils "^1.1.1" - lazystream@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/lazystream/-/lazystream-1.0.0.tgz#f6995fe0f820392f61396be89462407bb77168e4" @@ -7519,13 +7462,6 @@ resolve@1.x, resolve@^1.10.0, resolve@^1.11.1, resolve@^1.3.2: dependencies: path-parse "^1.0.6" -resolve@^1.10.1: - version "1.14.1" - resolved "https://registry.yarnpkg.com/resolve/-/resolve-1.14.1.tgz#9e018c540fcf0c427d678b9931cbf45e984bcaff" - integrity sha512-fn5Wobh4cxbLzuHaE+nphztHy43/b++4M6SsGFC2gB8uYwf0C8LcarfCz1un7UTW8OFQg9iNjZ4xpcFVGebDPg== - dependencies: - path-parse "^1.0.6" - resolve@^1.12.0, resolve@^1.5.0: version "1.13.1" resolved "https://registry.yarnpkg.com/resolve/-/resolve-1.13.1.tgz#be0aa4c06acd53083505abb35f4d66932ab35d16" @@ -7663,7 +7599,7 @@ sax@>=0.6.0, sax@^1.2.4: resolved "https://registry.yarnpkg.com/semver/-/semver-5.7.1.tgz#a954f931aeba508d307bbf069eff0c01c96116f7" integrity sha512-sauaDf/PZdVgrLTNYHRtpXa1iRiKcaebiKQ1BJdpQlWH2lCvexQdX55snPFyK7QzpudqbCI0qXFfOasHdyNDGQ== -semver@^6.0.0, semver@^6.1.0, semver@^6.1.1, semver@^6.1.2, semver@^6.2.0, semver@^6.3.0: +semver@^6.0.0, semver@^6.1.2, semver@^6.2.0, semver@^6.3.0: version "6.3.0" resolved "https://registry.yarnpkg.com/semver/-/semver-6.3.0.tgz#ee0a64c8af5e8ceea67687b133761e1becbd1d3d" integrity sha512-b39TBaTSfV6yBrapU89p5fKekE2m/NwnDocOVruQFS1/veMgdzuPcnOM34M6CwxW8jH/lxEa5rBoDeUwu5HHTw== @@ -8813,11 +8749,6 @@ validate-npm-package-name@^3.0.0: dependencies: builtins "^1.0.3" -vandium-utils@^1.1.1: - version "1.2.0" - resolved "https://registry.yarnpkg.com/vandium-utils/-/vandium-utils-1.2.0.tgz#44735de4b7641a05de59ebe945f174e582db4f59" - integrity sha1-RHNd5LdkGgXeWevpRfF05YLbT1k= - verror@1.10.0: version "1.10.0" resolved "https://registry.yarnpkg.com/verror/-/verror-1.10.0.tgz#3a105ca17053af55d6e270c1f8288682e18da400" @@ -9154,10 +9085,10 @@ yargs@^14.2.2: y18n "^4.0.0" yargs-parser "^15.0.0" -yargs@^15.0.1, yargs@^15.0.2: - version "15.0.2" - resolved "https://registry.yarnpkg.com/yargs/-/yargs-15.0.2.tgz#4248bf218ef050385c4f7e14ebdf425653d13bd3" - integrity sha512-GH/X/hYt+x5hOat4LMnCqMd8r5Cv78heOMIJn1hr7QPPBqfeC6p89Y78+WB9yGDvfpCvgasfmWLzNzEioOUD9Q== +yargs@^15.0.2, yargs@^15.1.0: + version "15.1.0" + resolved "https://registry.yarnpkg.com/yargs/-/yargs-15.1.0.tgz#e111381f5830e863a89550bd4b136bb6a5f37219" + integrity sha512-T39FNN1b6hCW4SOIk1XyTOWxtXdcen0t+XYrysQmChzSipvhBO8Bj0nK1ozAasdk24dNWuMZvr4k24nz+8HHLg== dependencies: cliui "^6.0.0" decamelize "^1.2.0" From 41c00c047fdba7c6db79cee37f541a702dfc8e94 Mon Sep 17 00:00:00 2001 From: Ahmed Sedek Date: Fri, 3 Jan 2020 11:43:11 +0100 Subject: [PATCH 37/38] Fixed test --- packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts b/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts index e5a60ead2859e..93cd65d7d7859 100644 --- a/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts +++ b/packages/@aws-cdk/aws-cloudwatch/test/test.metric-math.ts @@ -42,7 +42,7 @@ export = { }) } }); - }, /Same id \('a'\) used for two metrics/); + }, /The ID 'a' used for two metrics in the expression: 'BCount' and 'ACount'. Rename one/); test.done(); }, From a35662f2e0f7214225fc605d6f9a27074866c26b Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 3 Jan 2020 13:37:51 +0100 Subject: [PATCH 38/38] attachTo() is not in this branch yet --- packages/@aws-cdk/aws-codebuild/lib/project.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-codebuild/lib/project.ts b/packages/@aws-cdk/aws-codebuild/lib/project.ts index ebdaabbf1f9f7..9dd836bba2b67 100644 --- a/packages/@aws-cdk/aws-codebuild/lib/project.ts +++ b/packages/@aws-cdk/aws-codebuild/lib/project.ts @@ -322,7 +322,7 @@ abstract class ProjectBase extends Resource implements IProject { metricName, dimensions: { ProjectName: this.projectName }, ...props - }).attachTo(this); + }); } /**