-
Notifications
You must be signed in to change notification settings - Fork 403
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(metrics): support to bring your own metrics provider #2194
feat(metrics): support to bring your own metrics provider #2194
Conversation
Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need. |
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## develop #2194 +/- ##
===========================================
- Coverage 97.57% 96.43% -1.14%
===========================================
Files 162 169 +7
Lines 7475 7666 +191
Branches 1416 1447 +31
===========================================
+ Hits 7294 7393 +99
- Misses 133 217 +84
- Partials 48 56 +8
☔ View full report in Codecov by Sentry. |
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.
Hi @roger-zhangg, excellent work this PR! This is a feature that we really want to have because it is a need for some customers. I made some initial considerations that can help us to make the code simpler and optimize some things.
I still have more to suggest, but I need to look at the code in more detail.
I'll leave some notes to remember what we still have to do to get this ready:
- Create documentation
- Check the change that can generate breaking change
- Use the simplified version of typing from Python 3.10
Thank you very much for this PR, let's work together to merge as soon as possible.
aws_lambda_powertools/metrics/provider/cloudwatchemf_provider.py
Outdated
Show resolved
Hide resolved
|
4 similar comments
|
|
|
|
Hello @heitorlessa! About the documentation, do you think it makes sense to add it now OR when we add the Datadog provider? In my opinion, we should add Datadog and create the documentation. Can you make a final review, please? |
@roger-zhangg I would like your review here also. |
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.
LGTM
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.
Metrics Provider PR notes
- Remove docs until we cleanup implementations and have our first provider (Datadog)
- Check whether we have a gap in tests for default dimensions not being updated locally but within a provider
Separate PR for cleanup
- Export CloudWatch EMF stuff within cloudwatch_emf
- MetricsBase and MetricsProviderBase confusion
- Refactor
_extract_*
into standalone functions to reduce call graph - Split up functional and unit tests with new providers
peer review last comments so we can merge this today. We can cleanup the slight confusion in OOO / Protocol, docs, etc. in a separate PR (easier to review later). CAN'T WAIT FOR THIS! |
I removed the docs for now and I'll create an additional issue to pay this tech debt. ALMOST THERE! @roger-zhangg! |
Kudos, SonarCloud Quality Gate passed! |
pending CI checks, feel free to merge ;) Gotta go cook dinner. Thank you both @roger-zhangg and @leandrodamascena - this marks the initial steps of supporting any observability provider 🍺 🎉 |
Issue number: #2015
Summary
Changes
This PR refactors Metrics class to use Metrics Provider. Enabling customer to bring their own provider for outputting custom metrics format.The default CloudWatch EMF logic is refactored into a EMF provider.
User experience
Metric provider UX
The default use case for metrics before is
metrics=Metrics()
After we have this provider feature, Customers can still use original CloudWatch Metrics by
metrics=Metrics()
ormetrics=CloudWatchEMF()
. They can also use provider for third party provider by e.g.:Metrics=DataDogMetrics()
self-defined metrics provider UX
If the customer would like to use another observability provider, or define their own metrics functions, the provider acts as an interface that the customer can implement and pass to the Metrics class using the
provider
parameterTasks
Not covered in this PR (Added - 12/07/2023)
Checklist
If your change doesn't seem to apply, please leave them unchecked.
Is this a breaking change?
RFC issue number:
Checklist:
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.