[WIP] feat(aws-cloudwatch) design and implement metric math #1396
Conversation
There was a problem hiding this comment.
Looks like an awesome direction.
A few general notes:
- I wonder if we can build this in layers to enable both a lower-level, weakly-typed usage and on top of it implement a higher level strongly-typed DSL.
- It's important to think about how this will behave in other languages, especially when we venture in to DSLs. Java is usually a good extreme example to consider.
It seems like you are exporting many free floating functions - bear in mind that we don't have support for free-floating functions in jsii (languages like Java don't have support for those), so when you think about ergonomics, you need to make sure you have an anchor class.
design/metric-math.md
Outdated
| @@ -0,0 +1,202 @@ | |||
| # Overview | |||
| Our L2 `Alarm` construct currently only supports specifying a single metric to monitor. This design discusses options for extending this construct to support metric math, AWS's domain-specific language (DSL) which defines data types, operators and functions for specifying mathematical aggregations of CloudWatch metric data. | |||
There was a problem hiding this comment.
I wonder if this file should go under the cloudwatch module directory and not at the global cdk level...
| @@ -0,0 +1,4 @@ | |||
| brazil-runtime-exec get_mcm_template.py \ | |||
design/metric-math.md
Outdated
| ``` | ||
|
|
||
| We have requested two specific metrics, `errors` and `requests`. This brings those metrics into the scope of the query with their respective IDs. Think of it like assigning local variables for use later on. Also note how the value of `ReturnData` is `false` for these metrics - as previously mentioned, only one entry in an alarm definition is permitted to return data. By specifying `false`, we are indicating that this value is an 'intermediate' value used only as part of the larger computation. The expression, `error_rate`, then computes the error rate as a percentage using a simple expression, `errors / requests * 100`. `ReturnData` is set to true because we want to alarm on this `TS` value. | ||
|
|
There was a problem hiding this comment.
Love the overview section. It gave me enough context to be able to reason about the design without needing to go look up all the material!
design/metric-math.md
Outdated
| * Risk of us introducing bugs is higher - especially for large complicated expressions | ||
| * Expressing paranthesis to control mathematical precedence rules may be ugly or unintuitive | ||
| * Could be problematic for JSII, although it does pass its checks :) | ||
|
|
There was a problem hiding this comment.
Might be also interesting to explore something between option 2 and option 3:
const errors = new cloudwatch.Metric(...);
const requests = new cloudwatch.Metric(...);
const alarm = new cloudwatch.Alarm(this, 'woof', {
expression: `${errors} / ${requests} * 100`
});Pros:
- Utilizes the CloudWatch syntax, which makes it easier for people to look up help, docs, examples, etc.
- Concise
Cons:
- Not type-safe
- Magic all around
There was a problem hiding this comment.
Interesting idea. Are you thinking that toString() of Metric would encode its information as a string so that Alarm can parse the expression and construct the JSON payload? Similar to how Tokens work now?
There was a problem hiding this comment.
Exactly. We can probably even use the existing token stringification mechanism to achieve this. It's designed to be quite general purpose (albeit a bit complex).
|
Closing for now, #1077 will be remained open with a reference to this PR in case someone wants to pick this. |
Fixes #1077
Pull Request Checklist
Please check all boxes, including N/A items:
Testing
Documentation
Title and description
fix(module): <title>bug fix (patch)feat(module): <title>feature/capability (minor)chore(module): <title>won't appear in changelogbuild(module): <title>won't appear in changelogBREAKING CHANGE: <describe exactly what changed and how to achieve similar behavior + link to documentation/gist/issue if more details are required>Fixes #xxxorCloses #xxxBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.