ref(sentry apps): Don't pass in event for send_alert_event

[Christinarlong]

refactor send_alert_event to get event data & IssueOccurance from nodestore
This task occurs like 3/s in US (compared to 500/s for send_process_resource_change) so shouldn't hit any rate limits

[codecov]

Codecov Report

Attention: Patch coverage is 82.00000% with 9 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/sentry/sentry_apps/tasks/sentry_apps.py	81.63%	9 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #80263       +/-   ##
===========================================
+ Coverage   58.97%   80.34%   +21.37%     
===========================================
  Files        7203     7215       +12     
  Lines      318858   319453      +595     
  Branches    20775    20775               
===========================================
+ Hits       188031   256652    +68621     
+ Misses     130433    62407    -68026     
  Partials      394      394               

[Christinarlong]

note: we don't actually use/expose any of the occurrence data currently in the eventual webhook payload. See _webhook_event_data where as_dict only pulls out attributes from the data attribute which is the same for both Event & GroupEvent objects. I don't know if we're not supposed to expose the occurrence info (?) but if we aren't we can get rid of any occurrence fetches & potentially change the typing to be only Event

[markstory]

That is odd. If we're not using any of the data from the occurence I don't see a good reason to fetch it as we're just wasting resource. Perhaps someone from @getsentry/issues knows why we include occurrence data and don't use it. We could remove the occurence data after fixing the pickle issue as well.

[wedamija]

As part of implementing the issue platform we just passed around the group event with occurrence everywhere since we wanted consistency. It's possible we've used this in places where we don't need to, or that the use got lost over time and we didn't remove it.

I'd guess it's probably just related to

sentry/src/sentry/sentry_apps/tasks/sentry_apps.py

Lines 241 to 243 in 15cb8de

	data[name] = _webhook_event_data(instance, instance.group_id, instance.project_id)
	else:
	data[name] = serialize(instance)

.

But if we're not using the occurrence here then feel free to remove it

[Christinarlong]

For context this webhook/task (send_alert_event) fires whenever any corresponding alert (error, metric, issue etc) is triggered. So I'd assume we'd want to send the occurrence data in the webhook (unless it's data we usually don't show). Idk maybe _webhook_event_data was written before GroupEvents/occurrences were a thing 🤷‍♀️ .

[markstory]

You can't remove this parameter entirely. There will be messages in the queues that have this parameter. Instead, I would make the signature:

def send_alert_event(
    rule: str,
    sentry_app_id: int,
    # instance_id, group_id, occurence_id will become required
    instance_id: str | None = None
    group_id: int | None = None,
    occurence_id: str | None = None,
    event: Event | GroupEvent | None = None,   # deprecated
    additional_payload_key: str | None = None,
    additional_payload: Mapping[str, Any] | None = None,
) -> None:

Another approach would be to add **kwargs to the signature and read the deprecated event and new parameters from kwargs.

def send_alert_event(
    rule: str,
    sentry_app_id: int,
    additional_payload_key: str | None = None,
    additional_payload: Mapping[str, Any] | None = None,
    **kwargs: Any
) -> None:

[Christinarlong]

We talked about this a bit but new plan is to do cutover, i.e

1. Make a new task (send_alert_event_v2), put the task behind option flag and wait for the task to be present in all regions
2. Turn on the option flag and observe results/performance in regions
3. If all goes well, move option to 100
4. Lastly, deprecate & remove the old task
5. profit 🥳
   Lmk if I missed any steps 😓

[markstory]

That sounds like a good plan to me.

[markstory]

Left a few nits on aligning names. We can also do that when the old task is cleaned up.