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: split cold start metric from application metrics to prevent duplicate app metrics #126

Merged

Conversation

heitorlessa
Copy link
Contributor

@heitorlessa heitorlessa commented Aug 22, 2020

Issue #, if available: #125

Description of changes:

Checklist

Breaking change checklist

RFC issue #:

  • Migration process documented
  • Implement 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.

* develop:
  chore: version bump to 1.3.1
  fix(capture_method): should yield inside with (#124)
  chore: bump version to 1.3.0 (#122)
  chore(deps): bump prismjs from 1.20.0 to 1.21.0 in /docs
  chore(deps): bump elliptic from 6.5.2 to 6.5.3 in /docs
  feat: add parameter utility (#96)
  chore: bump version to 1.2.0 (#119)
  feat: add support for tracing of generators using capture_method decorator (#113)
@codecov-commenter
Copy link

codecov-commenter commented Aug 22, 2020

Codecov Report

Merging #126 into develop will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##           develop      #126   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        24           
  Lines          698       697    -1     
  Branches        64        64           
=========================================
- Hits           698       697    -1     
Impacted Files Coverage Δ
aws_lambda_powertools/metrics/metric.py 100.00% <ø> (ø)
aws_lambda_powertools/metrics/base.py 100.00% <100.00%> (ø)
aws_lambda_powertools/metrics/metrics.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 330c76a...dde963d. Read the comment docs.

Copy link
Contributor

@to-mc to-mc left a comment

Choose a reason for hiding this comment

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

LGTM with 2 tiny nitpicks!

CHANGELOG.md Outdated Show resolved Hide resolved
tests/functional/test_metrics.py Outdated Show resolved Hide resolved
@michaelbrewer
Copy link
Contributor

@heitorlessa good idea, the only thing i would note in the change log is that this is a behavior change (kind of a breaking change if people where relying on this before).

@heitorlessa
Copy link
Contributor Author

I'm on a fence to whether call this a bugfix - I can't see a scenario where a customer would like to have the same business data split into two different metrics, because dimensions were added.

That said, I do want to call this out in the Release notes too

@heitorlessa heitorlessa added bug Something isn't working and removed enhancement labels Aug 22, 2020
@heitorlessa heitorlessa changed the title fix: split cold start metric from application metrics fix: split cold start metric from application metrics to prevent duplicate app metrics Aug 22, 2020
@heitorlessa heitorlessa merged commit ad00a92 into aws-powertools:develop Aug 22, 2020
@heitorlessa heitorlessa deleted the fix/metrics-cold-start-split branch August 22, 2020 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants