Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cloudwatch): automatic metric math label cannot be suppressed #17639

Merged
merged 16 commits into from
Apr 3, 2022
+100 −7
Merged

fix(cloudwatch): automatic metric math label cannot be suppressed #17639

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
5e14fbf
fix(cloudwatch): allow empty label im a math expression
NetaNir Nov 23, 2021
885eecb
english
NetaNir Nov 23, 2021
461487e
update tests
NetaNir Nov 23, 2021
79d0982
i never liked this linter anyway
NetaNir Nov 23, 2021
c2bf6c7
emoty string
NetaNir Nov 27, 2021
cc97964
Merge branch 'neta/allow-empty-label' of github.com:aws/aws-cdk into …
NetaNir Nov 27, 2021
147c399
revert test
NetaNir Nov 27, 2021
b6f0856
scope test
NetaNir Nov 27, 2021
d1d6760
Merge branch 'master' into neta/allow-empty-label
NetaNir Nov 29, 2021
21455ac
Merge remote-tracking branch 'origin/master' into neta/allow-empty-label
rix0rrr Apr 1, 2022
be3c016
Update docs
rix0rrr Apr 1, 2022
5d7c26c
Change implementation a bit
rix0rrr Apr 1, 2022
65269f8
Also explain dynamic labels in the README
rix0rrr Apr 1, 2022
598b6a8
Fix compilation of example
rix0rrr Apr 2, 2022
5d104f7
Fix another example compilation failure
rix0rrr Apr 3, 2022
d378bcc
Merge branch 'master' into neta/allow-empty-label
mergify[bot] Apr 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-cloudwatch/lib/metric.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ export interface MathExpressionOptions {
/**
* Label for this metric when added to a Graph in a Dashboard
*
* @default - Expression value is used as label
* @default - The legend will only show the original metric labels
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we change the default here, won't customers be surprised that their stacks have changed upon update? That seems problematic for a stable module.

To have the legend show only the original metric labels, set Expression-Label to be empty.

Instead of omitting the label entirely, can you set label = ''? To me, that seems to follow the cloudformation docs of explicitly setting the label to be empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we change the default here, won't customers be surprised that their stacks have changed upon update? That seems problematic for a stable module.

I would argue that the current default behavior is unexpected, but I see your point about the stability of the API and limiting the change to a specific input

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like Neta's behavior better, actually, so I would recommend doing that. My apologies for implementing the original thing (shittily).

If we are concerned about introducing unexpected changes, I would recommend going with a feature flag instead.

*/
readonly label?: string;

Expand Down
7 changes: 0 additions & 7 deletions packages/@aws-cdk/aws-cloudwatch/lib/private/rendering.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,6 @@ 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 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.toString();
}

const renderedOpts = dropUndefined(options);

if (Object.keys(renderedOpts).length !== 0) {
Expand Down
21 changes: 10 additions & 11 deletions packages/@aws-cdk/aws-cloudwatch/test/metric-math.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ describe('Metric Math', () => {

// THEN
graphMetricsAre(graph, [
[{ expression: 'a + b', label: 'a + b' }],
[{ expression: 'a + b' }],
['Test', 'ACount', { visible: false, id: 'a' }],
['Test', 'BCount', { visible: false, id: 'b' }],
]);
Expand All @@ -110,7 +110,7 @@ describe('Metric Math', () => {

// THEN
graphMetricsAre(graph, [
[{ label: 'a + e', expression: 'a + e' }],
[{ expression: 'a + e' }],
['Test', 'ACount', { visible: false, id: 'a' }],
[{ expression: 'b + c', visible: false, id: 'e' }],
['Test', 'BCount', { visible: false, id: 'b' }],
Expand All @@ -137,7 +137,7 @@ describe('Metric Math', () => {
});

graphMetricsAre(graph, [
[{ label: 'a + e', expression: 'a + e' }],
[{ expression: 'a + e' }],
['Test', 'ACount', { visible: false, id: 'a' }],
[{ expression: 'b + c', visible: false, id: 'e' }],
['Test', 'ACount', { visible: false, id: 'b' }],
Expand Down Expand Up @@ -165,7 +165,7 @@ describe('Metric Math', () => {

// THEN
graphMetricsAre(graph, [
[{ label: 'a + e', expression: 'a + e' }],
[{ expression: 'a + e' }],
['Test', 'ACount', { visible: false, id: 'a' }],
[{ expression: 'a + c', visible: false, id: 'e' }],
['Test', 'CCount', { visible: false, id: 'c' }],
Expand All @@ -188,7 +188,7 @@ describe('Metric Math', () => {
// THEN
graphMetricsAre(graph, [
['Test', 'ACount', { id: 'a' }],
[{ label: 'a + b', expression: 'a + b' }],
[{ expression: 'a + b' }],
['Test', 'BCount', { visible: false, id: 'b' }],
]);

Expand All @@ -210,7 +210,7 @@ describe('Metric Math', () => {
// THEN
graphMetricsAre(graph, [
['Test', 'ACount', { period: 10 }],
[{ label: 'a + b', expression: 'a + b' }],
[{ expression: 'a + b' }],
['Test', 'ACount', { visible: false, id: 'a' }],
['Test', 'BCount', { visible: false, id: 'b' }],
]);
Expand All @@ -232,7 +232,7 @@ describe('Metric Math', () => {
// THEN
graphMetricsAre(graph, [
['Test', 'ACount'],
[{ label: 'a + b', expression: 'a + b', period: 60 }],
[{ expression: 'a + b', period: 60 }],
['Test', 'ACount', { visible: false, id: 'a', period: 60 }],
['Test', 'BCount', { visible: false, id: 'b', period: 60 }],
]);
Expand Down Expand Up @@ -260,7 +260,7 @@ describe('Metric Math', () => {

// THEN
graphMetricsAre(graph, [
[{ expression: 'a + e', label: 'a + e' }],
[{ expression: 'a + e' }],
['Test', 'ACount', { visible: false, id: 'a' }],
[{ expression: 'a + b', visible: false, id: 'e' }],
['Test', 'BCount', { visible: false, id: 'b' }],
Expand All @@ -281,7 +281,7 @@ describe('Metric Math', () => {

// THEN
graphMetricsAre(graph, [
[{ expression: 'a + b99', label: 'a + b99' }],
[{ expression: 'a + b99' }],
['Test', 'ACount', { visible: false, id: 'a' }],
['Test', 'BCount', { visible: false, id: 'b99', stat: 'p99' }],
]);
Expand All @@ -308,9 +308,8 @@ describe('Metric Math', () => {

// THEN
graphMetricsAre(graph, [
[{ label: 'a + 1', expression: 'a + 1' }],
['Test', 'ACount', { visible: false, id: 'a' }],
[{ label: 'a + 2', expression: 'a + 2', yAxis: 'right' }],
[{ expression: 'a + 2', yAxis: 'right' }],
]);


Expand Down