Cloudwatch metrics #180
Cloudwatch metrics #180
Conversation
This makes it more flexible to match resources that may have complex properties. A typical case in which you would need this is if you have resources that have JSON embedded in a string on which you might want to do structural matching.
Supports both Alarms and Dashboards. Adds a 'Metric' abstraction which can be exposed by other resources.
|
|
||
| Making Alarms | ||
| ------------- | ||
|
|
There was a problem hiding this comment.
Yeah, I just posted what I had so far to give y'all a chance of shooting at it. Still need to write some sections and add tests.
There was a problem hiding this comment.
Also would be nice to document the idiom of construct.metricOfXxx
| vertical axes. | ||
| - `AlarmWidget` -- shows the graph and alarm line for a single alarm. | ||
| - `SingleValueWidget` -- shows the current value of a set of metrics. | ||
| - `TextWidget` -- shows some static Markdown. |
There was a problem hiding this comment.
Should we call this MarkdownWidget (what's the name used in the CloudWatch docs?)
There was a problem hiding this comment.
text, actually. But we're already deviating slightly from their widget structure. I'm okay with calling it MarkdownWidget.
There was a problem hiding this comment.
So let's stay with text. It's not worth the deviation.
| Widgets given in the same call will be laid out horizontally. Widgets given | ||
| in different calls will be laid out vertically. To make more complex layouts, | ||
| you can use the following widgets to pack widgets together in different ways: | ||
|
|
| */ | ||
| export interface AlarmProps { | ||
| /** | ||
| * The metric to add the alarm on |
There was a problem hiding this comment.
A little documentation hint on how to obtain/create metrics
| /** | ||
| * The number of periods over which data is compared to the specified threshold. | ||
| */ | ||
| evaluationPeriods: number; |
There was a problem hiding this comment.
Can we provide a sensible default? 1 maybe?
There was a problem hiding this comment.
I suppose. But I kinda feel this is a decision you must consciously make. And 1 is almost never the right answer, I would default to 2 or 3.
There was a problem hiding this comment.
.... if there is no clear sensible default than I guess we shouldn't provide one... Maybe that's a question for the CW team
| title: this.props.title, | ||
| region: this.props.region || new AwsRegion(), | ||
| annotations: { | ||
| alarms: [this.props.alarm.alarmArn] |
There was a problem hiding this comment.
Is there a way for me to add multiple alarms to a graph?
There was a problem hiding this comment.
Is it a use case? Seems like could be easy to add some API to support. But we can wait for the requirement.
| * | ||
| * @default Average | ||
| */ | ||
| statistic?: string; |
There was a problem hiding this comment.
Maybe an enum-like class (already supported in jsii):
class StatisticType {
static Minimum = new StatisticType('minimum');
static Average = new StatisticType('average');
static percentile(x, y = 0) { return new StatisticType(`${x}.{y}`); };
static P99 = StatisticType.percentile(99);
}There was a problem hiding this comment.
Yes, we can do this, and though in general I hate stringly typing things, I think in this particular case the usability of some well-understood strings is better than those highly modeled objects with long names.
There was a problem hiding this comment.
Fair enough. So just make sure we have good runtime validation...
| } | ||
|
|
||
| export interface MetricAlarmJson { | ||
| dimensions?: Dimension[]; |
| /** | ||
| * Name of the dimension | ||
| */ | ||
| name: string; |
There was a problem hiding this comment.
Wouldn't it make more sense to just model this as a simple hash?
There was a problem hiding this comment.
For what the user sees, it is a hash. This is internals to type the JSON that we generate for CloudWatch' benefit.
| /** | ||
| * A single dashboard widget | ||
| */ | ||
| export interface Widget { |
There was a problem hiding this comment.
IWidget? (I am not sure, but I guess we need a little bit of consistency)
| * | ||
| * @default average over 5 minutes | ||
| */ | ||
| public get publishSizeMetric(): Metric { |
There was a problem hiding this comment.
I think metrics should have a common prefix and not a common suffix for discoverabilility and grouping.
| return new Metric({ | ||
| namespace: 'AWS/SNS', | ||
| metricName: 'NumberOfMessagesFailed', | ||
| dimensions: { TopicName: this.topicName }, |
There was a problem hiding this comment.
Would be nice to create have a private method that returns a metric that has namespace and dimensions already set and the these accessors only apply the metric name.
There was a problem hiding this comment.
I think that all of those should actually be methods. Sometimes parameterization will be needed (for example, ElasticCache clusters emit metrics based on the number of shards).
There was a problem hiding this comment.
And also, this method shouldn’t be private, as an escape hatch in case a new metric is now emitted and the construct doesn’t support it explicitly yes. So I think all constructs should have an “metricOf(name)” and then “metricOfXxxx([params...])” methods
|
We should also add at least one literate integration test and include it in the README. |
- Metric providers on resources are now functions. - Add integration test. - Add unit tests on graphs. - Introduce tokenAwareJsonify() which was necessary to properly create a dashboard with references to alarms in it.
| * Properties can be: | ||
| * | ||
| * - An object, in which case its properties will be compared to those of the actual resource found | ||
| * - A callablage, in which case it will be treated as a predicate that is applied to the Properties of the found resources. |
There was a problem hiding this comment.
What's a callablage? :)
Various changes in different commits. Start work on CloudWatch libraries with support for Alarms and Dashboards (the only two CloudWatch entities with a server-side presence).
Expose object model for other libraries to expose metrics. Use those metrics in Lambda and SNS.
Update assertion library with extra matching feature I needed.
Under construction, the README is not done, there are no unit tests for the various metric widgets, no integ tests yet.
By submitting this pull request, I confirm that my contribution is made under
the terms of the beta license.