Skip to content
This repository has been archived by the owner on Nov 8, 2024. It is now read-only.

fix: add timezone to timestamps #64

Merged
merged 2 commits into from
Aug 22, 2024
Merged

Conversation

rasendubi
Copy link
Collaborator


labels: mergeable

Fixes: #issue

Motivation and Context

This fixes timestamps, so they get properly serialized with timezone information.

Description

>>> datetime.datetime.now(datetime.timezone.utc).isoformat()
'2024-08-20T17:58:45.374349+00:00'
>>> datetime.datetime.utcnow().isoformat()
'2024-08-20T18:01:31.482134'

How has this been tested?

Not tested.

@schmit schmit self-requested a review August 20, 2024 18:04
@@ -222,7 +222,7 @@ def get_assignment_detail(
"featureFlag": flag_key,
"variation": result.variation.key if result and result.variation else None,
"subject": subject_key,
"timestamp": datetime.datetime.utcnow().isoformat(),
"timestamp": datetime.datetime.now(datetime.timezone.utc).isoformat(),
Copy link
Member

Choose a reason for hiding this comment

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

A desirable test would be to refactor this into a function and the unit test would have consistent time:

import unittest
import datetime
from freezegun import freeze_time

def get_utc_timestamp():
    return datetime.datetime.now(datetime.timezone.utc).isoformat()

class TestUTCTimestamp(unittest.TestCase):
    @freeze_time("2023-08-20 14:30:45.123456+00:00")
    def test_get_utc_timestamp(self):
        expected = "2023-08-20T14:30:45.123456+00:00"
        result = get_utc_timestamp()
        self.assertEqual(result, expected)

    def test_timezone_present(self):
        result = get_utc_timestamp()
        self.assertIn('+00:00', result)

    def test_iso_format(self):
        result = get_utc_timestamp()
        try:
            datetime.datetime.fromisoformat(result)
        except ValueError:
            self.fail("get_utc_timestamp() did not return a valid ISO format")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added two higher-level tests asserting that event timestamps carry the timezone information (ultimately, we don't care whether string uses +00:00 or Z suffix):

event = mock_logger.log_assignment.call_args.args[0]
timestamp = datetime.datetime.fromisoformat(event["timestamp"])
assert timestamp.tzinfo == datetime.timezone.utc

Copy link
Contributor

@schmit schmit left a comment

Choose a reason for hiding this comment

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

Great catch -- perhaps we should refactor into a get_time() function (probably with a better name) and call that so we don't risk calling the wrong function in the future (although of course we could forget that the get_time function exists...

Also don't forget a version bump!

@rasendubi
Copy link
Collaborator Author

@schmit we actually have a good utcnow() in eval.py.

I'm signing off for today so didn't have much time for a nice fix with tests. I can improve it tomorrow or feel free to pick it up

@rasendubi
Copy link
Collaborator Author

@schmit @leoromanovsky refactored this into a function + added tests

Copy link
Member

@leoromanovsky leoromanovsky left a comment

Choose a reason for hiding this comment

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

👍 👍 👍

@rasendubi rasendubi merged commit 79e5259 into Eppo-exp:main Aug 22, 2024
4 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants