-
Notifications
You must be signed in to change notification settings - Fork 412
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): allow custom timestamps for metrics #4006
feat(metrics): allow custom timestamps for metrics #4006
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #4006 +/- ##
===========================================
- Coverage 96.38% 96.25% -0.13%
===========================================
Files 214 215 +1
Lines 10030 10374 +344
Branches 1846 1925 +79
===========================================
+ Hits 9667 9986 +319
- Misses 259 275 +16
- Partials 104 113 +9 ☔ View full report in Codecov by Sentry. |
Hello everyone! This PR is ready for review. Here are some notes to facilitate the review process: 1 - Timestamp is either a @rubenfonseca I need your review here. @dreamorosi considering your additional experience with metrics and EMF, I'd appreciate it if you could take a look at this PR and provide any thoughts or feedback you have. |
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.
Left a comment about time zones, just want to make sure we are covering this aspect since we support it in other utilities.
Other than that, it looks great!
Reviewing now |
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.
I thing you're treating "ints" as seconds and converting "datetimes" to milliseconds. I believe everything needs to be milliseconds. Can you please check?
|
Thanks for the feedback @rubenfonseca! I've updated the documentation, docstring, and comments to make it clear that we expect epoch time in milliseconds. Ready to review again. |
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.
Two small notes, feel free to ping me offline
Issue number: #3429
Summary
Changes
This pull request adds a functionality to allow custom dates instead of the current timestamp for Metrics EMF, while still retaining epoch time as expected value. Previously, only the current timestamp was used by default.
Questions
1 - Timestamp type
That said, should we only allow
int
inputs with epoch value, or should customers be able to pass datetime objects and have them converted internally? In our Datadog Metrtics provider we just allowint
values.2 - Invalid values
If a customer passes an invalid number, we will emit a warning and fallback to the current timestamp instead of raising an exception.
Update 26/03/2024
1 - Timestamp is either a
Datetime
object or an integer representing an epoch time.2 - Any
Datetime
object will be converted into epoch time.3 - The
timestamp
should not exceed 14 days in the past or be more than 2 hours in the future. Reference: Amazon CloudWatch Documentation4 - If any metric fails to meet the requirements, a warning will be raised, and the metric will be flushed accordingly.
5 - We will not revert the timestamp to the current timestamp under any circumstances to prevent potential side effects.
User experience
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.