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

feat: upgraded embedded metrics library for high resolution metrics #1550

Merged
merged 16 commits into from
Jun 24, 2024

Conversation

jdoherty
Copy link
Contributor

@jdoherty jdoherty commented Jan 5, 2024

**Issue 1041 **

Description of changes:

Updated emf library allowing users to publish high resolution metrics

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.

Notes:

Copy link

github-actions bot commented Jan 5, 2024

💾 Artifacts Size Report

Module Version Size (KB)
powertools-common 2.0.0-SNAPSHOT 9.59
powertools-serialization 2.0.0-SNAPSHOT 18.22
powertools-logging 2.0.0-SNAPSHOT 33.09
powertools-logging-log4j 2.0.0-SNAPSHOT 20.63
powertools-logging-logback 2.0.0-SNAPSHOT 17.04
powertools-tracing 2.0.0-SNAPSHOT 14.02
powertools-metrics 2.0.0-SNAPSHOT 14.07
powertools-parameters 2.0.0-SNAPSHOT 17.49
powertools-validation 2.0.0-SNAPSHOT 20.83
powertools-cloudformation 2.0.0-SNAPSHOT 17.01
powertools-idempotency-core 2.0.0-SNAPSHOT 35.57
powertools-idempotency-dynamodb 2.0.0-SNAPSHOT 12.92
powertools-large-messages 2.0.0-SNAPSHOT 17.48
powertools-batch 2.0.0-SNAPSHOT 16.89
powertools-parameters-ssm 2.0.0-SNAPSHOT 10.73
powertools-parameters-secrets 2.0.0-SNAPSHOT 9.93
powertools-parameters-dynamodb 2.0.0-SNAPSHOT 11.97
powertools-parameters-appconfig 2.0.0-SNAPSHOT 12.00

@codecov-commenter
Copy link

codecov-commenter commented Jan 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.91%. Comparing base (82d4b30) to head (cda20c7).
Report is 66 commits behind head on v2.

Current head cda20c7 differs from pull request most recent head 204dfe4

Please upload reports for the commit 204dfe4 to get more accurate results.

Additional details and impacted files
@@              Coverage Diff              @@
##                 v2    #1550       +/-   ##
=============================================
- Coverage     89.79%   76.91%   -12.89%     
- Complexity      406      466       +60     
=============================================
  Files            44       49        +5     
  Lines          1274     1685      +411     
  Branches        165      254       +89     
=============================================
+ Hits           1144     1296      +152     
- Misses           88      300      +212     
- Partials         42       89       +47     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Jan 5, 2024
@pull-request-size pull-request-size bot added size/M and removed size/L labels Jan 5, 2024
Copy link

sonarqubecloud bot commented Jan 5, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@scottgerring scottgerring self-requested a review February 15, 2024 12:21
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@scottgerring
Copy link
Contributor

scottgerring commented Feb 15, 2024

Hey @jdoherty sorry for the high latency on this one! LGTM (and tests 🎸 )

Couple small things -->

I think the spotbugs warning needs an ignore in the overrides file:

[ERROR] Medium: Public static software.amazon.lambda.powertools.metrics.MetricsUtils.metricsLogger() may expose internal representation by returning MetricsUtils.metricsLogger [software.amazon.lambda.powertools.metrics.MetricsUtils] At MetricsUtils.java:[line 53] MS_EXPOSE_REP

TBH i'm not sure why this is being picked up now. You've not touched the class itself, but the implementation of metricsLogger has changed 🤷 Regardless, should be a quick fix.

Can you also chuck some words in the docs? The python guys have a section for high-resolution metrics that might be inspirational/reusable.

E2E tests are broken in v2 by my recent merge of the idempotency split. We can ignore that here.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Jun 24, 2024
@jeromevdl
Copy link
Contributor

I think the spotbugs warning needs an ignore in the overrides file:

[ERROR] Medium: Public static software.amazon.lambda.powertools.metrics.MetricsUtils.metricsLogger() may expose internal representation by returning MetricsUtils.metricsLogger [software.amazon.lambda.powertools.metrics.MetricsUtils] At MetricsUtils.java:[line 53] MS_EXPOSE_REP

TBH i'm not sure why this is being picked up now. You've not touched the class itself, but the implementation of metricsLogger has changed 🤷 Regardless, should be a quick fix.

Fixed

Can you also chuck some words in the docs? The python guys have a section for high-resolution metrics that might be inspirational/reusable.

Done

@scottgerring, you can have a quick look but we should be good with that one.

Copy link

Copy link
Contributor

@scottgerring scottgerring left a comment

Choose a reason for hiding this comment

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

LGTM! Merci @jdoherty and @jeromevdl

@scottgerring scottgerring merged commit 42b8325 into v2 Jun 24, 2024
16 checks passed
@scottgerring scottgerring deleted the feature/high-res-metrics branch June 24, 2024 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants