-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
feat(cloudwatch): validate parameters for a metric dimensions (closes #3116) #14365
Changes from 4 commits
6e253f1
e988e93
8589b7c
4ba576c
4c0b71c
91bcd04
38290cd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -216,7 +216,9 @@ 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}`); | ||||||
} | ||||||
|
||||||
if (props.dimensions) { | ||||||
this.validateDimensions(props.dimensions); | ||||||
} | ||||||
this.dimensions = props.dimensions; | ||||||
this.namespace = props.namespace; | ||||||
this.metricName = props.metricName; | ||||||
|
@@ -395,6 +397,26 @@ export class Metric implements IMetric { | |||||
|
||||||
return list; | ||||||
} | ||||||
|
||||||
//rules from here: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cw-dimension.html | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add this link to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It can be removed from here if it's on the prop itslef. If you want to keep it, add as a method documentation. |
||||||
private validateDimensions(dims: DimensionHash): void { | ||||||
if (Object.keys(dims)?.length > 10) { | ||||||
throw new Error('The maximum number of dimensions is 10'); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it says that |
||||||
} | ||||||
|
||||||
Object.keys(dims).sort().map(key => { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why the |
||||||
if (dims[key] === undefined || dims[key] === null) { | ||||||
throw new Error(`Dimension value of '${dims[key]}' is invalid`); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't see in the docs that undefined is not allowed, assuming you verified it in deployment? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I ran the code sample in the original issue (#3116), and it failed. I think it is because of the |
||||||
}; | ||||||
if (key.length < 1 || key.length > 255) { | ||||||
throw new Error(`Dimension name must be at least 1 and no more than 255 characters; received ${key}`); | ||||||
}; | ||||||
|
||||||
if (dims[key].length < 1 || dims[key].length > 255) { | ||||||
throw new Error(`Dimension value must be at least 1 and no more than 255 characters; received ${dims[key]}`); | ||||||
}; | ||||||
}); | ||||||
} | ||||||
} | ||||||
|
||||||
function asString(x?: unknown): string | undefined { | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,4 +51,81 @@ export = { | |
|
||
test.done(); | ||
}, | ||
|
||
'cannot use null dimension value'(test: Test) { | ||
test.throws(() => { | ||
new Metric({ | ||
namespace: 'Test', | ||
metricName: 'ACount', | ||
period: cdk.Duration.minutes(10), | ||
dimensions: { | ||
DimensionWithNull: null, | ||
}, | ||
}); | ||
}, /Dimension value of 'null' is invalid/); | ||
|
||
test.done(); | ||
}, | ||
|
||
'cannot use undefined dimension value'(test: Test) { | ||
test.throws(() => { | ||
new Metric({ | ||
namespace: 'Test', | ||
metricName: 'ACount', | ||
period: cdk.Duration.minutes(10), | ||
dimensions: { | ||
DimensionWithUndefined: undefined, | ||
}, | ||
}); | ||
}, /Dimension value of 'undefined' is invalid/); | ||
|
||
test.done(); | ||
}, | ||
|
||
'cannot use long dimension values'(test: Test) { | ||
const invalidDimensionValue = 'SomeLongValueSomeLongValueSomeLongValueSomeLongValueSomeLongValueSomeLongValue\ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets try something like this to make it more readable:
|
||
SomeLongValueSomeLongValueSomeLongValueSomeLongValueSomeLongValueSomeLongValueSomeLongValueSomeLongValue\ | ||
SomeLongValueSomeLongValueSomeLongValueSomeLongValueSomeLongValueSomeLongValueSomeLongValueSomeLongValue\ | ||
SomeLongValueSomeLongValueSomeLongValueSomeLongValueSomeLongValueSomeLongValueSomeLongValueSomeLongValue\ | ||
SomeLongValueSomeLongValueSomeLongValueSomeLongValueSomeLongValueSomeLongValueSomeLongValueSomeLongValue\ | ||
SomeLongValueSomeLongValueSomeLongValueSomeLongValueSomeLongValueSomeLongValueSomeLongValueSomeLongValue'; | ||
|
||
test.throws(() => { | ||
new Metric({ | ||
namespace: 'Test', | ||
metricName: 'ACount', | ||
period: cdk.Duration.minutes(10), | ||
dimensions: { | ||
DimensionWithLongValue: invalidDimensionValue, | ||
}, | ||
}); | ||
}, `Dimension value must be at least 1 and no more than 255 characters; received ${invalidDimensionValue}`); | ||
|
||
test.done(); | ||
}, | ||
|
||
'throws error when there are more than 10 dimensions'(test: Test) { | ||
test.throws(() => { | ||
new Metric({ | ||
namespace: 'Test', | ||
metricName: 'ACount', | ||
period: cdk.Duration.minutes(10), | ||
dimensions: { | ||
dimensionA: 'value1', | ||
dimensionB: 'value2', | ||
dimensionC: 'value3', | ||
dimensionD: 'value4', | ||
dimensionE: 'value5', | ||
dimensionF: 'value6', | ||
dimensionG: 'value7', | ||
dimensionH: 'value8', | ||
dimensionI: 'value9', | ||
dimensionJ: 'value10', | ||
dimensionK: 'value11', | ||
}, | ||
} ); | ||
}, /The maximum number of dimensions is 10/); | ||
|
||
test.done(); | ||
}, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to add it in the README, documentation on the property is sufficient. The README is more suited for features/code samples.