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.

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(metrics): store service name in defaultDimensions to avoid clearing it #1146

Merged

Conversation

dreamorosi
Copy link
Contributor

Description of your changes

As described in #1145, v1.4.0 highlighted a bug that causes the service name to be cleared from metrics after flushing.

The issue contains examples of the behavior and proposes a fix which has been implemented in this PR.

How to verify this change

See checks in PR.

Related issues, RFCs

Issue number: #1145

PR status

Is this ready for review?: YES
Is it a breaking change?: NO

Checklist

  • My changes meet the tenets criteria
  • I have performed a self-review of my own code
  • I have commented my code where necessary, particularly in areas that should be flagged with a TODO, or hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding changes to the examples
  • My changes generate no new warnings
  • The code coverage hasn't decreased
  • I have added tests that prove my change is effective and works
  • New and existing unit tests pass locally and in Github Actions
  • Any dependent changes have been merged and published in downstream module
  • The PR title follows the conventional commit semantics

Breaking change checklist

N/A


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@dreamorosi dreamorosi linked an issue Nov 8, 2022 that may be closed by this pull request
@github-actions github-actions bot added the bug Something isn't working label Nov 8, 2022
@dreamorosi dreamorosi marked this pull request as draft November 8, 2022 14:36
@dreamorosi
Copy link
Contributor Author

@dreamorosi dreamorosi added the metrics This item relates to the Metrics Utility label Nov 8, 2022
@dreamorosi dreamorosi self-assigned this Nov 8, 2022
@dreamorosi dreamorosi requested review from flochaz and ijemmy November 8, 2022 14:47
@dreamorosi dreamorosi marked this pull request as ready for review November 8, 2022 14:55
@@ -247,14 +247,14 @@ describe('Middy middleware', () => {
CloudWatchMetrics: [
{
Namespace: 'serverlessAirline',
Dimensions: [[ 'environment', 'aws_region', 'service', 'function_name' ]],
Dimensions: [[ 'service', 'environment', 'aws_region', 'function_name' ]],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change (& the one a few lines below) is needed because we are comparing the JSON.stringif-ied version of these strings, so order counts.

I could have converted to object comparison but wanted to keep the diff small.

@dreamorosi dreamorosi merged commit a979202 into main Nov 9, 2022
@dreamorosi dreamorosi deleted the 1145-bug-metrics-removing-service-name-after-flushing branch November 9, 2022 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working metrics This item relates to the Metrics Utility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Metrics removing service name after flushing
2 participants