fix(taskworker) Remove pickle usage from sentryapp hooks #91435
Conversation
Redo of #91372 which was reverted because of rate limiting. This time around, use an option to control task variants, and only generate a payload when a hook is going to be sent. For projects with multiple service hooks, this should result in fewer snuba calls as events are only serialized once.
github-actions
bot
added
the
Scope: Backend
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #91435 +/- ##
==========================================
- Coverage 87.68% 87.64% -0.05%
==========================================
Files 10320 10313 -7
Lines 588042 586255 -1787
Branches 22604 22604
==========================================
- Hits 515639 513815 -1824
- Misses 71975 72012 +37
Partials 428 428 |
src/sentry/tasks/post_process.py
Outdated
| # only generate the payload once to reduce snuba queries | ||
| if payload is None: | ||
| # TODO: when we have multiple service hook versions, this will need to change. | ||
| payload = json.dumps(get_payload_v0(event)) |
There was a problem hiding this comment.
Is there a way to serialize the event without doing this call up front? Then we could move all this to the async task function?
There was a problem hiding this comment.
There should be a way to get the event/project/group id, and then use that to rebuild the event in the webhook task. For projects with multiple hooks, we'll read the event multiple times though.
There was a problem hiding this comment.
Updated to pass project_id, group_id, and event_id. This approach moves serialization of the event until the hook is delivered. We have to re-read the event from nodestore, but that read should be cached most of the time as the event was recently in post_process.
I've also checked that all the events that go to service hooks are error events, so we don't have to worry about issue occurrences. 🤞
There was a problem hiding this comment.
O nice change! I'm surprised the current implementation doesn't currently cause a bunch of rate limited errors (?). Like serializing Group -> snuba call so we may have to live with rate limited errors 🤷♀️ I think sentry apps' event webhooks (issue.created) also has this rate limited issue which we ignore :oopsie:
I guess the question here is, I think it's likely we'll still get rate limited errors, so at what threshold do we tolerate having the error happen vs must address?
I think we're squeaking by because not all orgs/projects use sentryapps. If they did, we would be facing rate limits for sure 😢
I think as long as we stay above SLO targets we're good 😄 With the current approach we can't entirely avoid rate limits. |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Redo of #91372 which was reverted because of rate limiting. This time around, use an option to control task variants, and only generate a payload when a hook is going to be sent. For projects with multiple service hooks, this should result in fewer snuba calls as events are only serialized once.