Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Removing utcnow method deprecated at python3.12 #2415

Merged
merged 7 commits into from
Oct 9, 2023
Merged

Removing utcnow method deprecated at python3.12 #2415

merged 7 commits into from
Oct 9, 2023

Conversation

rmad17
Copy link
Contributor

@rmad17 rmad17 commented Oct 4, 2023

Replacing datetime.utcnow() due to deprecation as discussed in #2407

@sentrivana sentrivana linked an issue Oct 5, 2023 that may be closed by this pull request
datetime.datetime.utcnow() is deprecated #2407
Closed
@sentrivana
Copy link
Contributor

sentrivana commented Oct 5, 2023 •
edited

Thanks for the PR @rmad17! This works for Python 3.12 as well as older Python 3 versions, but we need to support 2.7 too, where datetime.timezone doesn't exist -- there we still need to keep doing it the old way.

My suggestion would be to add a new utcnow() function to the _compat.py file that does datetime.utcnow() for py2.7 and datetime.now(timezone.utc) for py3+, and use that function everywhere.

Please also run black on the codebase afterwards.

sentrivana
sentrivana requested changes Oct 5, 2023
View reviewed changes
Copy link
Contributor

@sentrivana sentrivana left a comment

Choose a reason for hiding this comment

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

See comment above 🙂

@rmad17
Copy link
Contributor Author

rmad17 commented Oct 5, 2023

Thanks for the suggestion @sentrivana. I have implemented what you suggested.

@hynek
Copy link
Contributor

hynek commented Oct 6, 2023

Should this also fix the same warnings around datetime.datetime.utcfromtimestamp or do y'all prefer a separate PR?

It seems to be called only once AFAICT:

sentry-python/sentry_sdk/integrations/logging.py

Line 285 in 59a67d3

"timestamp": datetime.datetime.utcfromtimestamp(record.created),

sentrivana
sentrivana requested changes Oct 6, 2023
View reviewed changes
Copy link
Contributor

@sentrivana sentrivana left a comment

Choose a reason for hiding this comment

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

Looks good, just one more thing to make it work in py2!

sentry_sdk/_compat.py Outdated Show resolved Hide resolved
@sentrivana
Copy link
Contributor

Should this also fix the same warnings around datetime.datetime.utcfromtimestamp or do y'all prefer a separate PR?

It seems to be called only once AFAICT:

sentry-python/sentry_sdk/integrations/logging.py

Line 285 in 59a67d3

"timestamp": datetime.datetime.utcfromtimestamp(record.created),

Ah, good catch. Don't really care much if it gets its own PR.

@rmad17 if you also feel like fixing what @hynek pointed out, feel free to add to this PR (not necessary though!). Otherwise I'll open a new issue for it.

@rmad17
Copy link
Contributor Author

rmad17 commented Oct 6, 2023

Thanks @sentrivana for the suggestions. I have made the changes. I have also addressed the issue @hynek pointed out.

@sentrivana sentrivana self-assigned this Oct 9, 2023
sentrivana
sentrivana approved these changes Oct 9, 2023
View reviewed changes
Copy link
Contributor

@sentrivana sentrivana left a comment

Choose a reason for hiding this comment

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

Thank you for squeezing in the utcfromtimestamp change as well @rmad17!

I added some typing-related stuff to make the linters happy. We should be good to go!

@sentrivana sentrivana merged commit 99aea33 into getsentry:master Oct 9, 2023
278 of 279 checks passed
@rmad17
Copy link
Contributor Author

rmad17 commented Oct 9, 2023

Thanks @sentrivana for merging the changes:)

@rmad17 rmad17 deleted the 2407-support-new-utc-method branch October 9, 2023 16:59
@potiuk potiuk mentioned this pull request Oct 14, 2023
Merged
potiuk added a commit to potiuk/airflow that referenced this pull request Oct 14, 2023
@potiuk
Fix tests to respond to Python 3.12 handling of utcnow in sentry-sdk
451f78b 
The sentry-sdk 1.32.0 released on 11th of October fixed handling of
utcnow to make it future-compatible with Python 3.12. The breadcrumb
timestamp returned was naive and now it is timezone aware with
utc specified explicitly as timezone. This broke our tests.

The change in sentry that impacted it is
getsentry/sentry-python#2415

We use the opportunity also to bump sentry sdk minimum version
to be 1.32.0 from very old 0.8.0 (from 2019). Sentry is a service, so
they generally always want you to use the latest version, and sentry has
very little requirements on its own to cause conflicts (for Python
3.8+ it only requires "certifi" without any specific limitations)
potiuk added a commit to apache/airflow that referenced this pull request Oct 14, 2023
@potiuk
Fix tests to respond to Python 3.12 handling of utcnow in sentry-sdk (#…
91581c4 
…34946)

The sentry-sdk 1.32.0 released on 11th of October fixed handling of
utcnow to make it future-compatible with Python 3.12. The breadcrumb
timestamp returned was naive and now it is timezone aware with
utc specified explicitly as timezone. This broke our tests.

The change in sentry that impacted it is
getsentry/sentry-python#2415

We use the opportunity also to bump sentry sdk minimum version
to be 1.32.0 from very old 0.8.0 (from 2019). Sentry is a service, so
they generally always want you to use the latest version, and sentry has
very little requirements on its own to cause conflicts (for Python
3.8+ it only requires "certifi" without any specific limitations)
@sentrivana sentrivana mentioned this pull request Oct 25, 2023
Closed
potiuk added a commit to apache/airflow that referenced this pull request Oct 29, 2023
@potiuk
Fix tests to respond to Python 3.12 handling of utcnow in sentry-sdk (#…
a2b0a6a 
…34946)

The sentry-sdk 1.32.0 released on 11th of October fixed handling of
utcnow to make it future-compatible with Python 3.12. The breadcrumb
timestamp returned was naive and now it is timezone aware with
utc specified explicitly as timezone. This broke our tests.

The change in sentry that impacted it is
getsentry/sentry-python#2415

We use the opportunity also to bump sentry sdk minimum version
to be 1.32.0 from very old 0.8.0 (from 2019). Sentry is a service, so
they generally always want you to use the latest version, and sentry has
very little requirements on its own to cause conflicts (for Python
3.8+ it only requires "certifi" without any specific limitations)

(cherry picked from commit 91581c4)
@sentrivana sentrivana mentioned this pull request Nov 2, 2023
Closed
4 tasks
ahidalgob pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request May 15, 2024
@potiuk
Fix tests to respond to Python 3.12 handling of utcnow in sentry-sdk …
4d893a8 
…(#34946)

The sentry-sdk 1.32.0 released on 11th of October fixed handling of
utcnow to make it future-compatible with Python 3.12. The breadcrumb
timestamp returned was naive and now it is timezone aware with
utc specified explicitly as timezone. This broke our tests.

The change in sentry that impacted it is
getsentry/sentry-python#2415

We use the opportunity also to bump sentry sdk minimum version
to be 1.32.0 from very old 0.8.0 (from 2019). Sentry is a service, so
they generally always want you to use the latest version, and sentry has
very little requirements on its own to cause conflicts (for Python
3.8+ it only requires "certifi" without any specific limitations)

(cherry picked from commit 91581c4991e0f81ac60c64bbaaf31eb51d65922a)

GitOrigin-RevId: a2b0a6aab814a773a7ba0a14f648a77d1dfdbb82
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sentrivana sentrivana sentrivana approved these changes

Assignees

@sentrivana sentrivana

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

datetime.datetime.utcnow() is deprecated
3 participants
@rmad17 @sentrivana @hynek