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.

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

Crons django-celery-beat timezone glitch #2472

Closed
avilaton opened this issue Oct 31, 2023 · 5 comments · Fixed by #2497
Closed

Crons django-celery-beat timezone glitch #2472

avilaton opened this issue Oct 31, 2023 · 5 comments · Fixed by #2497
Assignees

Comments

@avilaton
Copy link

How do you use Sentry?

Sentry Saas (sentry.io)

Version

1.29.2

Steps to Reproduce

  1. Setup django-celery-beat
  2. Create a Periodic task via the admin and set a wacked timezone for it
  3. setup Sentry with CeleryIntegration(monitor_beat_tasks=True),
  4. See the resulting cron monitor in sentry

The current implementation of the _get_monitor_config helper does not consider for the posibility that the schedule might be coming from django_celery_beat (this is understandable since this is all new).
Here is the implementation for it
https://django-celery-beat.readthedocs.io/en/latest/_modules/django_celery_beat/tzcrontab.html#TzAwareCrontab
which has a timezone property. The resulting schedule could have a different timezone from app.conf.timezone and could be per-schedule.

We traced it to here https://github.com/getsentry/sentry-python/blob/master/sentry_sdk/integrations/celery.py#L447 and think that considering that schedule could be an instance of TzAwareCrontab would be the way to go.

Not sure how you would like to code the possible import failure of django_celery_beat and so on, so stopped there.

Expected Result

Expectation is that the cron monitor in sentry has the Americas/Chicago timezone we have setup in django admin for our task

Actual Result

The task cron monitor in sentry has Americas/Los_Angeles as timezone as that is the app.conf.timezone

@avilaton
Copy link
Author

Adding a screenshot for clarity just in case
image

@antonpirker
Copy link
Member

Hey @avilaton !
Thanks for reporting this. I put it on our backlog.

@evanpurkhiser
Copy link
Member

@antonpirker I've seen a number of reports about this actually. Let's see if we can't get this more heavily prioritized?

@sentrivana
Copy link
Contributor

@evanpurkhiser I can take a look at this next week.

@sentrivana sentrivana self-assigned this Nov 3, 2023
@antonpirker antonpirker added the Type: Bug Something isn't working label Nov 7, 2023
@sentrivana sentrivana removed their assignment Nov 8, 2023
@antonpirker antonpirker self-assigned this Nov 8, 2023
@antonpirker
Copy link
Member

@evanpurkhiser the list of timezones in django-celery-beat has values in it, that are not in Sentry's list of timezones:
timezones-django-celery-beat.txt

When I select a timezone that is not available in Sentry's list, then the timezone -- shows up in Sentry's UI.
Is there a way we can map those non existing timezones?

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

Successfully merging a pull request may close this issue.

4 participants