- Notifications
You must be signed in to change notification settings - Fork4.8k
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Add runtime in success signal#9516
base:main
Are you sure you want to change the base?
Conversation
| @@ -528,7 +528,7 @@ def trace_task(uuid, args, kwargs, request=None): | |||
| if task_on_success: | |||
| task_on_success(retval, uuid, args, kwargs) | |||
| if success_receivers: | |||
| send_success(sender=task, result=retval) | |||
| send_success(sender=task, result=retval, runtime=T) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
can you add a test case to verify this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry if the test isn't perfect, I did my best to fit it in with everything else :D.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hello, may I resolve this change request, or are further changes required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it seems good. but since we are on the middle of a new stable release, so thinking if we should add or hold this for a while
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah, completely understand, no problem!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@## main #9516 +/- ##======================================= Coverage 78.24% 78.24% ======================================= Files 153 153 Lines 19041 19041 Branches 2520 2520 ======================================= Hits 14899 14899 Misses 3856 3856 Partials 286 286
Flags with carried forward coverage won't be shown.Click here to find out more. ☔ View full report in Codecov by Sentry. |
31a0b5b to50a34a7CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we can consider it for v5.5 or defer to another release,@Nusnus what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Note: Before submitting this pull request, please review ourcontributing
guidelines.
Description
Send the task runtime to the success signal so users can access this information.
Linked to thisissue