-
Notifications
You must be signed in to change notification settings - Fork 90
feat(metrics): New metrics module implementation with support for Metrics providers and usage without annotations #1863
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
Conversation
…n to be able to support multiple metrics providers.
…metrics annotation. MetricsLoggerBuilder, Environment variables.
…interface with Mockito.
…csLogger to enable user configuration of the MetricsLogger singleton.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
As discussed in the review session, let's address a few minor details around:
- supporting custom timestamps + validation
- removing service name dimension when not set
- aligning behavior on overwriting dimensions & metadata (wherever possible)
- considering changing name of single metric method to use the verb
flush
- adding a section about testing your code (nice to have)
…dd multi-dimensional dimensions also for non default dimensions.
…csBuilder, rename MetricsLoggerFactory to MetricsFactory.
Hey @dreamorosi, thanks again for the in-depth review. I implemented your changes plus some additional things I found.
|
E2E tests pass without any changes in assertions: https://github.com/aws-powertools/powertools-lambda-java/actions/runs/15469183318 |
...cs/src/main/java/software/amazon/lambda/powertools/metrics/internal/LambdaMetricsAspect.java
Show resolved
Hide resolved
I see we accepted a SonarQube finding around complexity, is there any way at all that we can break the logic down in smaller chunks? If not, then I'm ok to keep it. |
Sonar passes now. |
|
I also applied some changes to the GraalVM metadata files after renaming the |
Hey @dreamorosi, thanks again for your feedback. This PR is ready for review again. |
Issue #, if available: #1848
Description of changes:
This PR re-implements the metrics module providing an interface fully owned by Powertools. It lays the foundation to support multiple Metrics providers in the future (see related TypeScript issue aws-powertools/powertools-lambda-typescript#1812).
This PR:
@Metrics
annotationMetricsLoggerBuilder
Documentation preview deployed here: https://dealn7fl31ram.cloudfront.net/core/metrics/
Checklist
Breaking change checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.