Skip to content

Added support to set custom timestamp #110

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

Merged
merged 18 commits into from
Sep 13, 2023

Conversation

rayabagi
Copy link
Contributor

@rayabagi rayabagi commented Sep 9, 2023

*Issue # #59

Adding support to set custom timestamp to the metrics context.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@rayabagi rayabagi changed the title Added support set custom timestamp Added support to set custom timestamp Sep 11, 2023
@markkuhn markkuhn linked an issue Sep 11, 2023 that may be closed by this pull request
@rayabagi rayabagi requested review from gordonpn and Himtanaya and removed request for Himtanaya September 13, 2023 16:56
@rayabagi rayabagi removed the request for review from gordonpn September 13, 2023 21:19
@markkuhn markkuhn merged commit 4c36304 into awslabs:master Sep 13, 2023
@hussam789
Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Impact
Possible issue
Handle invalid datetime.min properly

The function returns 0 for datetime.min, but this will cause validation to fail
later since the validator rejects timestamps that are too old. Instead, the
function should raise an appropriate exception or handle this case differently.

aws_embedded_metrics/utils.py [19-23]

 def convert_to_milliseconds(dt: datetime) -> int:
     if dt == datetime.min:
-        return 0
+        raise InvalidTimestampError("datetime.min is not a valid timestamp")
 
     return int(round(dt.timestamp() * 1000))
  • Apply this suggestion
Suggestion importance[1-10]: 8

__

Why: The suggestion addresses a functional bug where returning 0 for datetime.min could cause later validation to fail; raising an exception aligns behavior with expectations in the validator and tests.

Medium
Fix inconsistent datetime.min handling

The test for datetime.min will fail because convert_to_milliseconds returns 0
for datetime.min instead of raising an exception. This inconsistency between the
utility function and the validator needs to be addressed.

tests/logger/test_metrics_context.py [479-492]

 @pytest.mark.parametrize(
     "timestamp",
     [
         None,
-        datetime.min,
         datetime(1970, 1, 1, 0, 0, 0),
         datetime.max,
         datetime(9999, 12, 31, 23, 59, 59, 999999),
         datetime(1, 1, 1, 0, 0, 0, 0, None),
         datetime(1, 1, 1),
         datetime(1, 1, 1, 0, 0),
         datetime.now() - timedelta(milliseconds=MAX_TIMESTAMP_PAST_AGE + 1),
         datetime.now() + timedelta(milliseconds=MAX_TIMESTAMP_FUTURE_AGE + 5000)
     ]
 )
+def test_set_invalid_timestamp_raises_exception(timestamp: datetime):
+    context = MetricsContext()
 
+    with pytest.raises(InvalidTimestampError):
+        context.set_timestamp(timestamp)
+
+def test_set_datetime_min_raises_exception():
+    context = MetricsContext()
+    
+    with pytest.raises(InvalidTimestampError):
+        context.set_timestamp(datetime.min)
+

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why: By removing datetime.min from the parameterized test and adding a separate test case for it, the suggestion improves clarity and consistency in test behavior, though its impact is more on test structure than core functionality.

Low
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow ability to set custom Timestamp
4 participants