Fix time utilities #6692

ofek · 2020-05-19T17:05:17Z

What does this PR do?

Fixes delta computation error Add utilities for working with time #6663 (comment)
Simplified get_aware_datetime to get_current_datetime
Removed use of pytz as per https://discuss.python.org/t/get-local-time-zone/4169/7
Added full test coverage

Notes

We already ship python-dateutil https://github.com/DataDog/integrations-core/blob/7.19.x/datadog_checks_base/datadog_checks/base/data/agent_requirements.in#L54
I didn't do Add utilities for working with time #6663 (comment) as I'm not sure that's worth it to us, lmk what we think
pytz is only used in one other spot https://github.com/DataDog/integrations-core/pull/5524/files should we remove that now too or after the release?

Additional Notes

@pganssle Anything look amiss to you in general? https://github.com/DataDog/integrations-core/blob/ofek/time/datadog_checks_base/datadog_checks/base/utils/time.py

pganssle

Looks very nice. I've left some comments and nitpicks, but I would not be terribly concerned about merging this.

pganssle · 2020-05-19T17:19:36Z

datadog_checks_base/datadog_checks/base/utils/time.py

-        return datetime.now(tz)
-    else:
-        return normalize_datetime(dt)
+        # NOTE: Although the epoch is platform dependent, it appears to be the same


What do you mean "epoch is platform dependent"? You mean that some platforms are configured to ignore leap seconds or something?

I would expect time.time() to always return the unix epoch time.

I was going by CPython's definition https://docs.python.org/3/library/time.html#epoch

Are the docs inaccurate?

Very interesting, I don't think I knew that. I would be very shocked if a system having a different epoch didn't break things horribly (though I guess some systems are configured to do the sane thing and ignore leap seconds — maybe that shows up differently in time.time()).

I'm curious to know what datetime.timestamp() would do on such a system. I guess it's also unclear what you would want to do on such a system, since I always think of a "timestamp" as defined relative to the Unix epoch, but possibly it could be defined relative to any epoch.

If you want to always use the system epoch, you could presumably change your definition of EPOCH to be EPOCH = datetime(*time.gmtime(0)[0:6], tzinfo=UTC).

That said, that documentation seems weird. From what I can tell, the implementation of time_gettime on Windows uses a function that computes time from a totally different epoch (1601-01-01), and then adjusts it to 1970-01-01. On non-Windows systems, it uses clock_gettime and gettimeofday, both of which are standardized in POSIX.1 as using the Unix epoch.

I'm wondering if we should change the time.time documentation to explicitly define it by the Unix epoch, rather than by the "system epoch".

Thanks for explaining. I do think that the time.time documentation should be updated then, especially since it makes extra efforts to achieve POSIX compliance on Windows. I'll remove this comment.

pganssle · 2020-05-19T17:21:41Z

datadog_checks_base/datadog_checks/base/utils/time.py

+        # - Alpine
+        return epoch_offset()
+
+    return (normalize_datetime(dt) - EPOCH).total_seconds()


I think using .timestamp() is almost certainly better on Python 3, but I suppose I can see why you'd want to unify the implementation if you're still supporting Python 2.

Yes I agree the official method is preferred 🙂 We'll definitely switch when we drop Python 2

We should add a comment to identify this as a possible optimization for when we drop Python 2.

Probably nitpicking, but alternatively, to benefit from the standard behavior on Python 3, use the old strategy on Python 2, and make it clear that we can drop the Py2 branch in due time without a TODO that might go out of date, we could use something like this in a compat.py module:

# datadog_checks_base/datadog_checks/base/utils/compat.py if PY2: def to_timestamp(dt): return (normalize_datetime(dt) - EPOCH).total_seconds() else: def to_timestamp(dt): return normalize_datetime(dt).timestamp()

(We might benefit from introducing such a compat.py like this anyway, eg for imports in contextlib vs contextlib2, etc. But I agree that'd be a precedent to make, so not too fussed about the current impl.)

I had it like that #6663 (comment)

@AlexandreYang ?

In our case, dt.timestamp() in py3 is just a shortcut for (self - _EPOCH).total_seconds().

https://github.com/python/cpython/blob/3.8/Lib/datetime.py#L1794

I guess both are fine for me, up to you @ofek :)

Let's merge. When the time comes there will be a massive py(thon)? ?[23] search anyway 😉

When the time comes

Hopefully not too late ;)

pganssle · 2020-05-19T17:22:54Z

datadog_checks_base/tests/test_time.py

+        nyc = tz.gettz('America/New_York')
+        now = datetime.now(nyc)
+
+        assert normalize_datetime(now).tzinfo is nyc


Do you also want to assert normalize_datetime(now) is now? Creating a new datetime can be annoyingly expensive considering these things are immutable 😛.

Done, thanks!

pganssle · 2020-05-19T17:28:00Z

datadog_checks_base/tests/test_time.py

+
+
+class TestCurrentDatetime:
+    def test_default_utc(self):


I'm not crazy about tests that rely on now(). It might be better to mock out datetime.now() and assert that get_current_datetime() called with no arguments calls datetime.now with UTC as the argument?

Alternatively, there's freezegun, which works from a practical perspective, but I've become mildly concerned with keeping it as a dependency.

Agree, not fan either relying on current time. +1 for mocking datetime.now.

The assert get_timestamp(now) - get_timestamp(dt) < 0.01 might be flaky is the system running the test is slowing down for unrelated reason.

pganssle · 2020-05-19T17:29:48Z

datadog_checks_base/tests/test_time.py

+    def test_utc(self):
+        from datetime import timezone
+
+        assert UTC.utcoffset(None) == timezone.utc.utcoffset(None)


What is the purpose of this test?

Is it because you're exposing a public UTC object and it's an implementation detail that you're exposing it directly from dateutil?

I'm trying to assert that the constant we expose is a tzinfo instance and represents UTC. How would you do that?

This is fine I think. I was just surprised because UTC is already tested upstream, so it's already part of the dateutil contract that it's UTC. I think it's fair to write your own test for it, though, considering you see this as part of the public interface (and the fact that it's supplied directly from dateutil.tz is an implementation detail.

That said, if in the future you drop Python 2 and change it from UTC = dateutil.tz.UTC to UTC = datetime.timezone.utc, would you still want this test?

Aha, that's an interesting question. Yes when that happens I will indeed remove this test. Therefore, I will remove it now since the contract is the same (tested upstream). Good call.

mgarabed

LGTM!

AlexandreYang · 2020-05-19T19:47:51Z

datadog_checks_base/tests/test_time.py

+
+
+class TestCurrentDatetime:
+    def test_default_utc(self):


Agree, not fan either relying on current time. +1 for mocking datetime.now.

The assert get_timestamp(now) - get_timestamp(dt) < 0.01 might be flaky is the system running the test is slowing down for unrelated reason.

datadog_checks_base/tests/test_time.py

datadog_checks_base/datadog_checks/base/utils/time.py

AlexandreYang

Looks great.

Just a feedback about the naming of normalize_datetime and possibly adding ensure_utc. See comment below.

datadog_checks_base/tests/test_time.py

AlexandreYang · 2020-05-20T07:58:58Z

datadog_checks_base/datadog_checks/base/utils/time.py

@@ -71,3 +40,6 @@ def normalize_datetime(dt, default_tz=UTC):
        dt = dt.replace(tzinfo=default_tz)


I'm thinking that we might also want to provide now or later a way to ensure that the dt returned is UTC.

Hence, normalize_datetime might not be the best name (it's also not clear since "normalize" might mean many things).

WDYT about renaming normalize_datetime to ensure_aware. And possibly adding ensure_utc ? Example:

def ensure_aware(dt, default_tz=UTC): """ Ensures that the returned datetime object is an aware datetime. """ if dt.tzinfo is None: dt = dt.replace(tzinfo=default_tz) return dt.astimezone(UTC) def ensure_utc(dt): """ Ensures that the datetime is aware and UTC. """ return normalize_datetime(dt, UTC).astimezone(UTC)

My comment is mainly about making normalize_datetime naming more clear.
ensure_utc might be implemented later.

FWIW, this is called default_tzinfo in dateutil: https://dateutil.readthedocs.io/en/stable/utils.html#dateutil.utils.default_tzinfo (though also I guess you could just use that function).

Indeed, quite similar to https://github.com/dateutil/dateutil/blob/master/dateutil/utils.py#L58-L61

The only difference is that the second argument defaults to UTC (which is convenient I think).

florimondmanca

Nice one about using dateutil.tz, learnt something today from Paul's article. 👍

florimondmanca · 2020-05-20T10:39:09Z

datadog_checks_base/datadog_checks/base/utils/time.py

+        # - Alpine
+        return epoch_offset()
+
+    return (normalize_datetime(dt) - EPOCH).total_seconds()


Probably nitpicking, but alternatively, to benefit from the standard behavior on Python 3, use the old strategy on Python 2, and make it clear that we can drop the Py2 branch in due time without a TODO that might go out of date, we could use something like this in a compat.py module:

# datadog_checks_base/datadog_checks/base/utils/compat.py if PY2: def to_timestamp(dt): return (normalize_datetime(dt) - EPOCH).total_seconds() else: def to_timestamp(dt): return normalize_datetime(dt).timestamp()

(We might benefit from introducing such a compat.py like this anyway, eg for imports in contextlib vs contextlib2, etc. But I agree that'd be a precedent to make, so not too fussed about the current impl.)

ofek · 2020-05-20T17:26:39Z

Thanks again @pganssle!

* Fix time utilities * address feedback - assert returned dt is same object * address feedback - remove upstream test * add comment * remove comment * address feedback * address

Fix time utilities

918ca95

ofek added the changelog/Fixed label May 19, 2020

ofek requested review from a team as code owners May 19, 2020 17:05

github-actions bot added the integration/datadog_checks_base label May 19, 2020

pganssle previously approved these changes May 19, 2020

View reviewed changes

address feedback - assert returned dt is same object

8f433ed

ofek dismissed pganssle’s stale review via 8f433ed May 19, 2020 17:45

ofek added 2 commits May 19, 2020 14:03

address feedback - remove upstream test

0026775

add comment

ca6aa51

mgarabed previously approved these changes May 19, 2020

View reviewed changes

remove comment

dbfb4db

ofek dismissed mgarabed’s stale review via dbfb4db May 19, 2020 19:50

AlexandreYang reviewed May 19, 2020

View reviewed changes

datadog_checks_base/datadog_checks/base/utils/time.py Show resolved Hide resolved

address feedback

2013baf

AlexandreYang requested changes May 20, 2020

View reviewed changes

florimondmanca reviewed May 20, 2020

View reviewed changes

address

7001eb7

AlexandreYang approved these changes May 20, 2020

View reviewed changes

ofek merged commit 4c83920 into master May 20, 2020

ofek deleted the ofek/time branch May 20, 2020 17:26

ChristineTChen pushed a commit that referenced this pull request May 21, 2020

Fix time utilities (#6692)

d453d1d

* Fix time utilities * address feedback - assert returned dt is same object * address feedback - remove upstream test * add comment * remove comment * address feedback * address

ofek mentioned this pull request Oct 14, 2020

Implements token reader for DC/OS Auth JWT token retrieval with login #7785

Merged

4 tasks

ofek mentioned this pull request Jan 13, 2021

Remove unneccessary pytz dependency #8354

Merged

yzhan289 mentioned this pull request Feb 24, 2023

Fix duplicate events bug #14020

Merged

5 tasks

mmalecki mentioned this pull request Jun 20, 2024

Send out container uptime metrics for ECS Fargate #17880

Closed

4 tasks

		@@ -71,3 +40,6 @@ def normalize_datetime(dt, default_tz=UTC):
		dt = dt.replace(tzinfo=default_tz)

Fix time utilities #6692

Fix time utilities #6692

Uh oh!

Conversation

ofek commented May 19, 2020

What does this PR do?

Notes

Additional Notes

Uh oh!

pganssle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ofek May 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgarabed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AlexandreYang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlexandreYang May 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

florimondmanca left a comment

ofek May 20, 2020 •

edited

Loading

AlexandreYang May 20, 2020 •

edited

Loading