Adding basic events to celery Batches #82
Conversation
* Add events task-received, task-succeded and task-started * Format to follow black formating
I think this is based on #34? |
| @@ -150,6 +150,7 @@ def test_signals(celery_app: Celery, celery_worker: TestWorkController) -> None: | |||
| # Other task signals are not implemented. | |||
| (signals.task_retry, 0), | |||
| (signals.task_success, 1), | |||
| (signals.task_received, 3), | |||
There was a problem hiding this comment.
I think we'll also want to assert the other signals.
It might make sense to keep the PRs small and implement them one at a time?
There was a problem hiding this comment.
I can split the PR into more small ones (one for received, and one for success).
There was a problem hiding this comment.
I'd say that if you know task_received works already, let's get that one done and then move onto the others?
There was a problem hiding this comment.
Can we add the missing asserts for task_succeeded and/or task_started.
| for request in requests: | ||
| request.send_event("task-started") |
There was a problem hiding this comment.
The previous PR (#34) sets the req.start_time for each first. Any idea if this is necessary?
There was a problem hiding this comment.
I believe, for what we are trying to implement now, this time_start is not necessary for now. It will surely come handy later if we implement all events for monitoring.
There was a problem hiding this comment.
OK, we can probably start w/o it. I'll need to double check the celery code around here so we stay similar-ish.
There was a problem hiding this comment.
Fine :) I'll wait for the push then and continue working on other events such as failed/revoked/retry (where failed and retry would be the most important for application monitoring)
There was a problem hiding this comment.
I'll wait for the push then and continue working on other events
Sorry -- I'm unsure what you mean by this. Are you waiting on me for something?
There was a problem hiding this comment.
I'll wait for the push then and continue working on other events
Sorry -- I'm unsure what you mean by this. Are you waiting on me for something?
No do not worry, that would be an other topic/PR (related to our latest messages in the issue 83)
There was a problem hiding this comment.
OK! Do you see this ready for review then?
Following preceding works (not updated since), I do this PR to add the following modifications
I intend (with your help if possible) to implement at least
task-failedandtask-retriedevents as well.