Skip to content

Commit

Permalink
feat(cloudwatch): parse all metrics statistics and support long format (
Browse files Browse the repository at this point in the history
#23095)

### Description

Previous PR added support for missing statistics #23074
This PR implements a proper parsing of all these statistics.

- Support "short" format `ts99`
- Support "long" format 
	- `TS(10%:90%)` | `TS(10:90)`
	- `TS(:90)` | `TS(:90%)`
	- `TS(10:)` | `TS(10%:)`
- Formats are case insensitive (no breaking changes)
- If "long" format and only upper boundary% `TS(:90%)`, can be translated to "short" format `ts90` (`stat.asSingleStatStr`)


### Note

I noticed that the following code expected the parsing to throw if it failed, but it actually will **not** fail in any case (it just return `GenericStatistic` if no format matched).
I will **not** change this behavior here as I'm not willing to spend more effort testing if this breaks stuff elsewhere.

https://github.com/aws/aws-cdk/blob/47943d206c8ff28923e19028acd5991d8e387ac9/packages/%40aws-cdk/aws-cloudwatch/lib/metric.ts#L295-L296

### Followup work

As is, this PR does not change any customer-facing logic. To make use of it, make the parsing throw if  no format is recognized.

At the end of the parser function, just replace

```ts
return {
  type: 'generic',
  statistic: stat,
} as GenericStatistic;
```

with 

```ts
throw new UnrecognizedStatisticFormatError()
```

---

You can see all tested inputs here: https://regexr.com/7351s

![2022-11-24_15-52-57](https://user-images.githubusercontent.com/26366184/204067982-05b7bd4f-1c56-4466-8c97-c626e8ac31b2.png)


----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
rigwild committed Feb 20, 2023
1 parent 5f33a26 commit 853e3d6
Show file tree
Hide file tree
Showing 12 changed files with 795 additions and 98 deletions.
54 changes: 54 additions & 0 deletions packages/@aws-cdk/aws-cloudwatch/README.md
Expand Up @@ -166,6 +166,60 @@ below).
> happen to know the Metric you want to alarm on makes sense as a rate
> (`Average`) you can always choose to change the statistic.
### Available Aggregation Statistics

For your metrics aggregation, you can use the following statistics:

| Statistic | Short format | Long format | Factory name |
| ------------------------ | :-----------------: | :------------------------------------------: | -------------------- |
| SampleCount (n) ||| `Stats.SAMPLE_COUNT` |
| Average (avg) ||| `Stats.AVERAGE` |
| Sum ||| `Stats.SUM` |
| Minimum (min) ||| `Stats.MINIMUM` |
| Maximum (max) ||| `Stats.MAXIMUM` |
| Interquartile mean (IQM) ||| `Stats.IQM` |
| Percentile (p) | `p99` || `Stats.p(99)` |
| Winsorized mean (WM) | `wm99` = `WM(:99%)` | `WM(x:y) \| WM(x%:y%) \| WM(x%:) \| WM(:y%)` | `Stats.wm(10, 90)` |
| Trimmed count (TC) | `tc99` = `TC(:99%)` | `TC(x:y) \| TC(x%:y%) \| TC(x%:) \| TC(:y%)` | `Stats.tc(10, 90)` |
| Trimmed sum (TS) | `ts99` = `TS(:99%)` | `TS(x:y) \| TS(x%:y%) \| TS(x%:) \| TS(:y%)` | `Stats.ts(10, 90)` |
| Percentile rank (PR) || `PR(x:y) \| PR(x:) \| PR(:y)` | `Stats.pr(10, 5000)` |

The most common values are provided in the `cloudwatch.Stats` class. You can provide any string if your statistic is not in the class.

Read more at [CloudWatch statistics definitions](https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/Statistics-definitions.html).

```ts
new cloudwatch.Metric({
namespace: 'AWS/Route53',
metricName: 'DNSQueries',
dimensionsMap: {
HostedZoneId: hostedZone.hostedZoneId
},
statistic: cloudwatch.Stats.SAMPLE_COUNT,
period: cloudwatch.Duration.minutes(5)
});

new cloudwatch.Metric({
namespace: 'AWS/Route53',
metricName: 'DNSQueries',
dimensionsMap: {
HostedZoneId: hostedZone.hostedZoneId
},
statistic: cloudwatch.Stats.p(99),
period: cloudwatch.Duration.minutes(5)
});

new cloudwatch.Metric({
namespace: 'AWS/Route53',
metricName: 'DNSQueries',
dimensionsMap: {
HostedZoneId: hostedZone.hostedZoneId
},
statistic: 'TS(7.5%:90%)',
period: cloudwatch.Duration.minutes(5)
});
```

### Labels

Metric labels are displayed in the legend of graphs that include the metrics.
Expand Down
13 changes: 4 additions & 9 deletions packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts
Expand Up @@ -9,7 +9,7 @@ import { IMetric, MetricExpressionConfig, MetricStatConfig } from './metric-type
import { dispatchMetric, metricPeriod } from './private/metric-util';
import { dropUndefined } from './private/object';
import { MetricSet } from './private/rendering';
import { parseStatistic } from './private/statistic';
import { normalizeStatistic, parseStatistic } from './private/statistic';

/**
* Properties for Alarms
Expand Down Expand Up @@ -413,7 +413,7 @@ function renderIfSimpleStatistic(statistic?: string): string | undefined {

const parsed = parseStatistic(statistic);
if (parsed.type === 'simple') {
return parsed.statistic;
return normalizeStatistic(parsed);
}
return undefined;
}
Expand All @@ -422,14 +422,9 @@ function renderIfExtendedStatistic(statistic?: string): string | undefined {
if (statistic === undefined) { return undefined; }

const parsed = parseStatistic(statistic);
if (parsed.type === 'percentile') {
// Already percentile. Avoid parsing because we might get into
// floating point rounding issues, return as-is but lowercase the p.
return statistic.toLowerCase();
} else if (parsed.type === 'generic') {
return statistic;
if (parsed.type === 'single' || parsed.type === 'pair') {
return normalizeStatistic(parsed);
}

return undefined;
}

Expand Down
58 changes: 40 additions & 18 deletions packages/@aws-cdk/aws-cloudwatch/lib/metric.ts
Expand Up @@ -2,11 +2,12 @@ import * as iam from '@aws-cdk/aws-iam';
import * as cdk from '@aws-cdk/core';
import { Construct, IConstruct } from 'constructs';
import { Alarm, ComparisonOperator, TreatMissingData } from './alarm';
import { Dimension, IMetric, MetricAlarmConfig, MetricConfig, MetricGraphConfig, Unit } from './metric-types';
import { Dimension, IMetric, MetricAlarmConfig, MetricConfig, MetricGraphConfig, Statistic, Unit } from './metric-types';
import { dispatchMetric, metricKey } from './private/metric-util';
import { normalizeStatistic, parseStatistic } from './private/statistic';
import { normalizeStatistic, pairStatisticToString, parseStatistic, singleStatisticToString } from './private/statistic';
import { Stats } from './stats';

export type DimensionHash = {[dim: string]: any};
export type DimensionHash = { [dim: string]: any };

export type DimensionsMap = { [dim: string]: string };

Expand All @@ -24,6 +25,8 @@ export interface CommonMetricOptions {
/**
* What function to use for aggregating.
*
* Use the `aws_cloudwatch.Stats` helper class to construct valid input strings.
*
* Can be one of the following:
*
* - "Minimum" | "min"
Expand All @@ -38,8 +41,6 @@ export interface CommonMetricOptions {
* - "tcNN.NN" | "tc(NN.NN%:NN.NN%)"
* - "tsNN.NN" | "ts(NN.NN%:NN.NN%)"
*
* Use the factory functions on the `Stats` object to construct valid input strings.
*
* @default Average
*/
readonly statistic?: string;
Expand Down Expand Up @@ -197,13 +198,13 @@ export interface MathExpressionOptions {
readonly searchAccount?: string;

/**
* Region to evaluate search expressions within.
*
* Specifying a searchRegion has no effect to the region used
* for metrics within the expression (passed via usingMetrics).
*
* @default - Deployment region.
*/
* Region to evaluate search expressions within.
*
* Specifying a searchRegion has no effect to the region used
* for metrics within the expression (passed via usingMetrics).
*
* @default - Deployment region.
*/
readonly searchRegion?: string;
}

Expand Down Expand Up @@ -291,17 +292,30 @@ export class Metric implements IMetric {
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 ${periodSec}`);
}

this.warnings = undefined;
this.dimensions = this.validateDimensions(props.dimensionsMap ?? props.dimensions);
this.namespace = props.namespace;
this.metricName = props.metricName;
// Try parsing, this will throw if it's not a valid stat
this.statistic = normalizeStatistic(props.statistic || 'Average');

const parsedStat = parseStatistic(props.statistic || Stats.AVERAGE);
if (parsedStat.type === 'generic') {
// Unrecognized statistic, do not throw, just warn
// There may be a new statistic that this lib does not support yet
const label = props.label ? `, label "${props.label}"`: '';
this.warnings = [
`Unrecognized statistic "${props.statistic}" for metric with namespace "${props.namespace}"${label} and metric name "${props.metricName}".` +
' Preferably use the `aws_cloudwatch.Stats` helper class to specify a statistic.' +
' You can ignore this warning if your statistic is valid but not yet supported by the `aws_cloudwatch.Stats` helper class.',
];
}
this.statistic = normalizeStatistic(parsedStat);

this.label = props.label;
this.color = props.color;
this.unit = props.unit;
this.account = props.account;
this.region = props.region;
this.warnings = undefined;
}

/**
Expand Down Expand Up @@ -389,14 +403,22 @@ export class Metric implements IMetric {
throw new Error('Using a math expression is not supported here. Pass a \'Metric\' object instead');
}

const stat = parseStatistic(metricConfig.metricStat.statistic);
const parsed = parseStatistic(metricConfig.metricStat.statistic);

let extendedStatistic: string | undefined = undefined;
if (parsed.type === 'single') {
extendedStatistic = singleStatisticToString(parsed);
} else if (parsed.type === 'pair') {
extendedStatistic = pairStatisticToString(parsed);
}

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,
statistic: parsed.type === 'simple' ? parsed.statistic as Statistic : undefined,
extendedStatistic,
unit: this.unit,
};
}
Expand Down

0 comments on commit 853e3d6

Please sign in to comment.