fix(metrics): addDimensions() documentation and tests #3964
Conversation
|
Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Thanks for catching this! I've reverted the types/index.ts file to its original state. This was an unintended change that happened during the implementation. The PR now only contains documentation updates and test fixes. |
|
Hi @matteofigus, I'll pick up the review for this right after the release of Bedrock Agent Functions on Tue 06/03. |
8f8454b to
6278aa8
Compare
|
There was a problem hiding this comment.
Thanks for the changes, and apologies for the delayed review. Got caught between PTO and other launches.
I've left a few comments, I think we're getting closer but we need to tweak the logic to align with the addDimension (singular) method.
I've also answered to your questions in the linked issue, I hope it'll clarify the doubts - apologies for the confusion there.
| @@ -267,9 +273,19 @@ class Metrics extends Utility implements MetricsInterface { | |||
| * @param dimensions - An object with key-value pairs of dimensions | |||
| */ | |||
| public addDimensions(dimensions: Dimensions): void { | |||
| for (const [name, value] of Object.entries(dimensions)) { | |||
| this.addDimension(name, value); | |||
| const newDimensionKeys = Object.keys(dimensions).filter( | |||
There was a problem hiding this comment.
This implementation doesn't match the addDimension method (nor the docstring above).
We are not checking if the dimension value is valid like we do in the other method, and when overwriting a dimension we don't let customers know.
Other than that I think it could work. Here's a suggestion:
const newDimensionSet: Dimensions = {};
for (const [key, value] of Object.entries(dimensions)) {
if (!value) {
this.#logger.warn(
`The dimension ${key} doesn't meet the requirements and won't be added. Ensure the dimension name and value are non empty strings`
);
continue;
}
if (
Object.hasOwn(this.dimensions, key) ||
Object.hasOwn(this.defaultDimensions, key) ||
Object.hasOwn(newDimensionSet, key)
) {
this.#logger.warn(
`Dimension "${key}" has already been added. The previous value will be overwritten.`
);
}
newDimensionSet[key] = value;
if (MAX_DIMENSION_COUNT <= this.getCurrentDimensionsCount()) {
throw new RangeError(
`The number of metric dimensions must be lower than ${MAX_DIMENSION_COUNT}`
);
}
}
this.dimensionSets.push(dimensions);| ); | ||
|
|
||
| if ( | ||
| newDimensionKeys.length + this.getCurrentDimensionsCount() >= |
There was a problem hiding this comment.
We should also update the this.getCurrentDimensionsCount() method so that it factors in the this.dimensionSets - even if in a set, these dimensions still count against the total 30 allowed.
|
|
||
| return { | ||
| _aws: { | ||
| Timestamp: this.#timestamp ?? new Date().getTime(), | ||
| CloudWatchMetrics: [ | ||
| { | ||
| Namespace: this.namespace || DEFAULT_NAMESPACE, | ||
| Dimensions: [dimensionNames], | ||
| Dimensions: dimensionNames as [string[]], |
There was a problem hiding this comment.
Unsure about this change, was it intentional?
| ...this.dimensions, | ||
| ...metricValues, | ||
| ...this.metadata, | ||
| ...Object.assign( |
There was a problem hiding this comment.
Can we put back the spread operator (...)? With current implementation we're instantiating a new object (with Object.assing()) only to immediately spread it.
Fix:
addDimensions()should create a setSummary
Changes
This PR fixes issue #3777 where
addDimensions()was incorrectly adding dimensions to the existing set instead of creating a new dimension set.Changes made:
addDimensions()method inMetrics.tsto create a new dimension set rather than adding to existing dimensionsdimensionSetsarray to store multiple dimension setsaddDimensions()Documentation updates:
addDimensions()to create multiple dimension setsaddDimensions()creates a new set rather than modifying existing dimensionsIssue number: #3777
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.