chore(sentry apps): Move logic to new Sentry App tasks #78347
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #78347 +/- ##
==========================================
+ Coverage 78.13% 78.15% +0.01%
==========================================
Files 7089 7088 -1
Lines 312501 312441 -60
Branches 51052 51083 +31
==========================================
+ Hits 244172 244185 +13
+ Misses 61958 61873 -85
- Partials 6371 6383 +12 |
| @@ -14,8 +14,8 @@ | |||
| [ | |||
| # basic tasks that must be passed models still | |||
| "sentry.tasks.process_buffer.process_incr", | |||
| "sentry.tasks.process_resource_change_bound", | |||
| "sentry.tasks.sentry_apps.send_alert_event", | |||
| "sentry.sentry_apps.tasks.sentry_apps.process_resource_change_bound", | |||
There was a problem hiding this comment.
Q: what is pickling/pickle tasks -> Like my google research has told me pickle is a serializer for celery tasks. So is pickling when you pass in a too complex object to get serialized? Is that bad because serializing an object is really expensive?
There was a problem hiding this comment.
pickle is stdlib library to serialize and deserialize python objects. We currently use it with celery so that any parameter that could end up in a task works. Pickle isn't great because it creates a few problems:
- ORM entities can become stale while in the queue message
- Deserializing python objects off of the network can lead to arbitrary code execution.
We've been trying to not introduce new tasks that rely on pickle so that in the future we can remove pickle support and only json encode task parameters.
Normally we shouldn't add to this list, but we're just moving things around so 🤷
| @@ -14,8 +14,8 @@ | |||
| [ | |||
| # basic tasks that must be passed models still | |||
| "sentry.tasks.process_buffer.process_incr", | |||
| "sentry.tasks.process_resource_change_bound", | |||
| "sentry.tasks.sentry_apps.send_alert_event", | |||
| "sentry.sentry_apps.tasks.sentry_apps.process_resource_change_bound", | |||
There was a problem hiding this comment.
pickle is stdlib library to serialize and deserialize python objects. We currently use it with celery so that any parameter that could end up in a task works. Pickle isn't great because it creates a few problems:
- ORM entities can become stale while in the queue message
- Deserializing python objects off of the network can lead to arbitrary code execution.
We've been trying to not introduce new tasks that rely on pickle so that in the future we can remove pickle support and only json encode task parameters.
Normally we shouldn't add to this list, but we're just moving things around so 🤷
| @instrumented_task( | ||
| "sentry.sentry_apps.tasks.sentry_apps.send_resource_change_webhook", **TASK_OPTIONS | ||
| "sentry.sentry_apps.tasks.sentry_apps.send_process_resource_change_webhook", **TASK_OPTIONS |
There was a problem hiding this comment.
Renaming this task could result in dropped tasks as a worker with the new code could create a task that is received by a worker with the old code where this task name doesn't exist.
There was a problem hiding this comment.
F, thank you for catching this.
| ) | ||
|
|
||
|
|
||
| def send_webhooks(installation, event, **kwargs): |
There was a problem hiding this comment.
I think this will need typehints on the parameters to get through CI.
Co-authored-by: Mark Story <mark@mark-story.com>
|
PR reverted: 20d7d33 |
This reverts commit 3016618. Co-authored-by: markstory <24086+markstory@users.noreply.github.com>
Have the new tasks contain the logic and point other refs torward the new tasks.
issue ref (#73857 )