Skip to content

fix(metrics): use same naming for serviceName #401

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 4 commits into from
Jan 5, 2022
Merged

fix(metrics): use same naming for serviceName #401

merged 4 commits into from
Jan 5, 2022

Conversation

flochaz
Copy link
Contributor

@flochaz flochaz commented Jan 4, 2022 •
edited
Loading

Description of your changes

Other modules use serviceName as key while metrics was using service. this PR homogenise that.

Before:

const metrics = new Metrics({service: 'payment', namespace: 'ecommerceApp'});

After:

const metrics = new Metrics({serviceName: 'payment', namespace: 'ecommerceApp'});

We are not changing the resulting service dimension to stay in sync with python PT to make sure people can mix python and typescript functions and get same dimensions.

Regarding doc waiting for #387 to be merge or that one to be merge to update #387

How to verify this change

npm run test
npx jest --group=e2e/metrics/standardFunctions

Related issues, RFCs

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
  • 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

  • I have documented the migration process
  • I have added, implemented necessary warnings (if it can live side by side)

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 added enhancement metrics This item relates to the Metrics Utility labels Jan 4, 2022
@dreamorosi dreamorosi added this to the beta-release milestone Jan 4, 2022
@dreamorosi dreamorosi self-requested a review January 4, 2022 20:42
dreamorosi added a commit that referenced this pull request Jan 4, 2022
@dreamorosi
docs: switched to serviceName according to #401
2539fbb
dreamorosi added a commit that referenced this pull request Jan 4, 2022
@dreamorosi
docs: switched to serviceName according to #401
6e06130
dreamorosi
dreamorosi previously approved these changes Jan 4, 2022
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.

Also updated the docs with serviceName in 2539fbb & 6e06130 for PR #387.

@flochaz flochaz force-pushed the fix/metrics/serviceName branch from 899207a to 7ab2cf4 Compare January 5, 2022 06:26
@flochaz flochaz changed the title fix(metrics) use same naming for serviceName fix(metrics): use same naming for serviceName Jan 5, 2022
@flochaz flochaz force-pushed the fix/metrics/serviceName branch from 7ab2cf4 to 60ca3a4 Compare January 5, 2022 06:33
@flochaz flochaz mentioned this pull request Jan 5, 2022
Closed
23 tasks
dreamorosi
dreamorosi previously approved these changes Jan 5, 2022
View reviewed changes
ijemmy
ijemmy previously approved these changes Jan 5, 2022
View reviewed changes
Copy link
Contributor

@ijemmy ijemmy left a comment

Choose a reason for hiding this comment

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

Can we also update the doc? (in a separate PR so we can start testing)

packages/metrics/package.json
@@ -59,7 +59,7 @@
"@typescript-eslint/parser": "^5.4.0",
"aws-cdk": "^1.137.0",
"aws-sdk": "^2.1048.0",
"esbuild": "^0.14.5",
"esbuild": "0.14.5",
Copy link
Contributor

Choose a reason for hiding this comment

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

(minor) Any particular reason to not auto update this one?

Copy link
Contributor Author

@flochaz flochaz Jan 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, upper version are breaking import of enum . I am creating a simple project to reproduce the issue and create one on esbuild repo , probably v0.14.7 https://github.com/evanw/esbuild/releases

@flochaz flochaz dismissed stale reviews from ijemmy and dreamorosi via 9a8e082 January 5, 2022 09:15
@flochaz flochaz force-pushed the fix/metrics/serviceName branch from 9a8e082 to 8220f34 Compare January 5, 2022 09:18
@flochaz flochaz merged commit 43c7945 into main Jan 5, 2022
@flochaz flochaz deleted the fix/metrics/serviceName branch January 5, 2022 09:21
dreamorosi added a commit that referenced this pull request Jan 5, 2022
@dreamorosi @flochaz
@saragerion @ijemmy
docs(all): make docs more coherent (#387)
a08bf76 
* chore(test): test

* docs(metrics): remove serializeMetrics and fix cloudwatch excerpt

* fix: tracer construct initializer

* Conformed tracer & logger

* Added changes from #376

* Added lambdaInterface to logger examples

* docs: update docs/core/logger.md

Co-authored-by: Sara Gerion <[email protected]>

* docs: updated return type for all async handlers

* docs: update docs/core/metrics.md

Co-authored-by: Sara Gerion <[email protected]>

* docs: update docs/core/metrics.md

Co-authored-by: Sara Gerion <[email protected]>

* docs: update docs/core/metrics.md

Co-authored-by: Sara Gerion <[email protected]>

* docs: updated all references to publishStoredMetrics

* docs: replaced extends w/ implements keyword in classes

* docs: switched to serviceName according to #401

* docs: switched to serviceName according to #401

* docs: update docs/core/tracer.md

Co-authored-by: Florian Chazal <[email protected]>

* docs: update docs/core/metrics.md

Co-authored-by: Florian Chazal <[email protected]>

* docs: update docs/core/tracer.md

Co-authored-by: Florian Chazal <[email protected]>

* docs: update docs/core/logger.md

Co-authored-by: Florian Chazal <[email protected]>

* docs: update note on patching all modules

* docs: updated text in captureMethod section

* docs: update docs/core/metrics.md

Co-authored-by: ijemmy <[email protected]>

* docs: update docs/core/metrics.md

Co-authored-by: ijemmy <[email protected]>

* docs: updated other references to serverlessAirline

Co-authored-by: Florian CHAZAL <[email protected]>
Co-authored-by: Sara Gerion <[email protected]>
Co-authored-by: ijemmy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ijemmy ijemmy ijemmy left review comments

@dreamorosi dreamorosi dreamorosi left review comments

Assignees

@flochaz flochaz

Labels
metrics This item relates to the Metrics Utility
Projects
None yet
Milestone
beta-release
Development

Successfully merging this pull request may close these issues.
3 participants
@flochaz @ijemmy @dreamorosi