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

Store current time when TaskResult started #432

Merged
merged 3 commits into from
Jun 15, 2024

Conversation

yktakaha4
Copy link
Contributor

Thank you to all maintainers of this great OSS.

Referring to the issue below, I generate TaskResult when Task is PENDING.
ref: #286 (comment)

This code works fine, but I can't use date_created as the start time of the Task because the TaskResult already exists when STARTED is recorded.
Additionally, in my use case, I want to measure the time from when a task is added to the message broker until the worker starts processing it.

By recording date_started as the time associated with STARTED, we can calculate the waiting time in the queue or the actual processing time of the worker.
Use django.db.models.functions.Now to get the current time to avoid being affected by the server's time settings.
ref: https://docs.djangoproject.com/en/5.0/ref/models/database-functions/#now

This PR is backward compatible because it simply adds a column and sets a value.
For users who do not create TaskResults themselves, date_created and date_started have the same meaning.

Testing

We can check the value of date_started in django admin.

admin

For testing, I implemented a 5 second sleep on @shared_task.
The time increases in the order date_created < date_started < date_done.

select id, status, date_created, date_started, date_done
from django_celery_results_taskresult
where id = 35;
id status date_created date_started date_done
35 SUCCESS 2024-06-08 06:14:37.631030 +00:00 2024-06-08 06:14:37.650764 +00:00 2024-06-08 06:14:43.334453 +00:00

Please let me know if there is anything else that should be tested.

@auvipy auvipy self-requested a review June 9, 2024 05:09
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

I like the intent of the PR. however I need more through review before merge it

@auvipy
Copy link
Member

auvipy commented Jun 9, 2024

in the mean time, can you please review this PR? #407

@yktakaha4
Copy link
Contributor Author

@auvipy Thank you for your quick review.

I have confirmed the fix for #407.
I noticed that no tests were written and no documentation was provided, but I couldn't think of any points that would functionally conflict with my PR.
(Should I point this out in the review?)

In my personal opinion, TaskResult.state is just for storing celery.states, and I thought it might make state customization possible in Celery and then change it, but I'm not confident.

@@ -61,8 +61,8 @@ To use :pypi:`django-celery-results` with your project you need to follow these

CELERY_RESULT_EXTENDED = True

If you want to track the execution duration of your tasks (by comparing `date_created` and `date_done` in TaskResult), enable the :setting:`track_started` setting.

If you want to track the execution duration of your tasks (by comparing `date_started` and `date_done` in TaskResult), enable the :setting:`track_started` setting.
Copy link
Member

Choose a reason for hiding this comment

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

can we think of any example, using the new feature here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@auvipy Added example. However, I'm not good at English, so I would like you to correct the sentence.

By the way, you previously asked whether this package should include the behavior of registering TaskResult at states.PENDING timing.
ref: #286 (comment)

If it is possible for me, I would like to create another PR. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

you can certainly open another PR. but what are the issues with the open PR?

Copy link
Contributor Author

@yktakaha4 yktakaha4 Jun 15, 2024

Choose a reason for hiding this comment

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

Sorry I'm not good at explaining. There is nothing problems with this PR.
I would like to submit a new PR for the PENDING status registration.
If there is no problem with this PR, could you please merge it?

Copy link
Member

Choose a reason for hiding this comment

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

you are most welcome to come with a new PR

@auvipy auvipy merged commit 015830e into celery:main Jun 15, 2024
21 checks passed
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.

2 participants