Use correct signatures for Celery Task Hooks #791
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #791 +/- ##
=======================================
Coverage 98.01% 98.01%
=======================================
Files 442 443 +1
Lines 36597 36533 -64
=======================================
- Hits 35869 35808 -61
+ Misses 728 725 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #791 +/- ##
=======================================
Coverage 98.01% 98.01%
=======================================
Files 442 443 +1
Lines 36597 36533 -64
=======================================
- Hits 35869 35808 -61
+ Misses 728 725 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #791 +/- ##
=======================================
Coverage 98.01% 98.01%
=======================================
Files 442 443 +1
Lines 36597 36533 -64
=======================================
- Hits 35869 35808 -61
+ Misses 728 725 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #791 +/- ##
=======================================
Coverage 98.01% 98.01%
=======================================
Files 442 443 +1
Lines 36597 36533 -64
=======================================
- Hits 35869 35808 -61
+ Misses 728 725 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Swatinem
force-pushed
the
swatinem/task-hooks
branch
from
7844c7a
to
4d077e4
Compare
matt-codecov
approved these changes
Oct 16, 2024
Swatinem
force-pushed
the
swatinem/task-hooks
branch
from
4d077e4
to
18698bb
Compare
This explicitly declares and forwards all the arguments to the `on_success/retry/failure` task hooks, as they are documented on https://docs.celeryq.dev/en/main/_modules/celery/app/task.html#Task.on_success The reason being that the Sentry tags set via the `MetricContext` constructor are not making their way to Sentry, possibly because the `kwargs` were misused in the previous hooks, and were overwriting those tag values with `None`. So this should ideally solve that mystery. As a driveby change, I also took the liberty of removing all the deprecated statsd `metrics` calls. All of the relevant metrics also have prometheus equivalents.
Swatinem
force-pushed
the
swatinem/task-hooks
branch
from
18698bb
to
8c66f91
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This explicitly declares and forwards all the arguments to the
on_success/retry/failuretask hooks, as they are documented on https://docs.celeryq.dev/en/main/_modules/celery/app/task.html#Task.on_successThe reason being that the Sentry tags set via the
MetricContextconstructor are not making their way to Sentry, possibly because thekwargswere misused in the previous hooks, and were overwriting those tag values withNone.So this should ideally solve that mystery.
As a driveby change, I also took the liberty of removing all the deprecated statsd
metricscalls. All of the relevant metrics also have prometheus equivalents.