Skip to content

fix(metrics): addDimensions() documentation and tests #3964

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

Jump to bottom
Merged
merged 9 commits into from
Jul 4, 2025
Merged

fix(metrics): addDimensions() documentation and tests #3964

merged 9 commits into from
Jul 4, 2025
+338 −12

Conversation

matteofigus
Copy link
Member

@matteofigus matteofigus commented May 22, 2025 •
edited
Loading

Fix: addDimensions() should create a set

Summary

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:

  1. Fixed the addDimensions() method in Metrics.ts to create a new dimension set rather than adding to existing dimensions
  2. Added a dimensionSets array to store multiple dimension sets
  3. Updated the metrics serialization logic to handle multiple dimension sets
  4. Added unit tests to verify the correct behavior of dimension sets
  5. Updated documentation to explain the correct behavior of addDimensions()

Documentation updates:

  • Added a new section in the metrics documentation explaining how to use dimension sets
  • Created a new code example showing how to use addDimensions() to create multiple dimension sets
  • Clarified that addDimensions() creates a new set rather than modifying existing dimensions

Issue 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.

@boring-cyborg boring-cyborg bot added documentation Improvements or additions to documentation metrics This item relates to the Metrics Utility tests PRs that add or change tests labels May 22, 2025
@pull-request-size pull-request-size bot added the size/L PRs between 100-499 LOC label May 22, 2025
@boring-cyborg boring-cyborg
Copy link

boring-cyborg bot commented May 22, 2025

Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need.
In the meantime, check out the #typescript channel on our Powertools for AWS Lambda Discord: Invite link

@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot added do-not-merge This item should not be merged need-issue This PR needs an issue before it can be reviewed/worked on further labels May 22, 2025
@github-actions GitHub Actions

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot added the bug Something isn't working label May 22, 2025
dreamorosi
dreamorosi reviewed May 22, 2025
View reviewed changes
@dreamorosi dreamorosi removed do-not-merge This item should not be merged need-issue This PR needs an issue before it can be reviewed/worked on further labels May 22, 2025
@matteofigus
Copy link
Member Author

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.

@dreamorosi dreamorosi linked an issue May 22, 2025 that may be closed by this pull request
Bug: addDimensions() should create a set #3777
Closed
@dreamorosi dreamorosi changed the title fix: addDimensions() documentation and tests fix(metrics): addDimensions() documentation and tests May 22, 2025
@matteofigus matteofigus marked this pull request as draft May 23, 2025 14:01
@dreamorosi
Copy link
Contributor

Hi @matteofigus, I'll pick up the review for this right after the release of Bedrock Agent Functions on Tue 06/03.

@matteofigus matteofigus force-pushed the fix/issue-3777-metrics-dimension-sets branch from 8f8454b to 6278aa8 Compare May 30, 2025 01:22
@matteofigus matteofigus marked this pull request as ready for review May 30, 2025 01:33
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

dreamorosi
dreamorosi requested changes Jun 6, 2025
View reviewed changes
Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

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.

@matteofigus matteofigus force-pushed the fix/issue-3777-metrics-dimension-sets branch 2 times, most recently from 6fcd6db to cc82cf8 Compare July 1, 2025 00:01
@matteofigus matteofigus requested a review from dreamorosi July 1, 2025 00:05
@matteofigus
perf(metrics): optimize dimension handling and reduce object creation
2f38802
@matteofigus
fix: address PR review comments
fa51dd4 
- Align addDimensions() validation with addDimension() method
- Add proper value validation and duplicate dimension warnings
- Update getCurrentDimensionsCount() to include dimensionSets
- Fix dimension count validation logic
@matteofigus
test: add missing test cases for addDimensions validation
1e8f479 
- Add tests for invalid dimension values (empty, null, undefined)
- Add tests for dimension overwrite warnings
- Achieve 100% code coverage for addDimensions method
@matteofigus
chore: clean up and optimize code structure
4fb32b7
@matteofigus matteofigus force-pushed the fix/issue-3777-metrics-dimension-sets branch from cc82cf8 to 4fb32b7 Compare July 2, 2025 12:33
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Jul 4, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

dreamorosi
dreamorosi approved these changes Jul 4, 2025
View reviewed changes
Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this bug!

@dreamorosi dreamorosi merged commit a801636 into aws-powertools:main Jul 4, 2025
46 checks passed
@boring-cyborg boring-cyborg
Copy link

boring-cyborg bot commented Jul 4, 2025

Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dreamorosi dreamorosi dreamorosi approved these changes

Assignees

@matteofigus matteofigus

Labels
bug Something isn't working documentation Improvements or additions to documentation metrics This item relates to the Metrics Utility size/L PRs between 100-499 LOC tests PRs that add or change tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: addDimensions() should create a set
2 participants
@matteofigus @dreamorosi