fix(cloudwatch): MathExpression id contract is not clear
#19825
Conversation
It is intended that all metric identifiers referenced in a MathExpression are included in the `usingMetrics` map. This allows passing around complex metrics as a single object, because the math expression object carries around its dependencies with it. This is slightly different than what people might be used to from raw CloudWatch, where there is no hierarchy and all metrics are supposed to be listed in the graph widget or alarm with a unique ID, and then referenced by ID. We can't make this contract obvious anymore by adding a hard validation, but we can add warnings to hint people at the right way to reference metrics in math expressions. Fixes #13942, closes #17126.
|
|
github-actions
bot
added
bug
| function recurse(w: IWidget) { | ||
| ret.push(w); | ||
| if (hasSubWidgets(w)) { | ||
| w.widgets.forEach(recurse); | ||
| } | ||
| } |
There was a problem hiding this comment.
Is there a chance that we have a circular dependency in any widgets? That is, is it possible for a widget to be a subwidget of itself? If so we should add a depth check and throw a useful error message if we catch one.
There was a problem hiding this comment.
You can write code to do that, but it already won't work today with a stack blowing error. I'm not too concerned that someone would do it in practice and then not know what's going on.
Good catch though, I like the way you're thinking about correctness 😎
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
It is intended that all metric identifiers referenced in a MathExpression are included in the `usingMetrics` map. This allows passing around complex metrics as a single object, because the math expression object carries around its dependencies with it. This is slightly different than what people might be used to from raw CloudWatch, where there is no hierarchy and all metrics are supposed to be listed in the graph widget or alarm with a unique ID, and then referenced by ID. We can't make this contract obvious anymore by adding a hard validation, but we can add warnings to hint people at the right way to reference metrics in math expressions. Fixes aws#13942, closes aws#17126. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
It is intended that all metric identifiers referenced in a
MathExpression are included in the
usingMetricsmap. Thisallows passing around complex metrics as a single object,
because the math expression object carries around its dependencies
with it.
This is slightly different than what people might be used
to from raw CloudWatch, where there is no hierarchy and all
metrics are supposed to be listed in the graph widget or alarm
with a unique ID, and then referenced by ID.
We can't make this contract obvious anymore by adding a hard
validation, but we can add warnings to hint people at the right
way to reference metrics in math expressions.
Fixes #13942, closes #17126.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license