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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore(sentry apps): Create forward shims for sentry apps tasks #78344

Merged

Christinarlong merged 14 commits into master from forward-sentryapp-shims

Oct 2, 2024

Contributor

Christinarlong commented Sep 30, 2024

Before we start moving task logic for sentry app tasks, we need shims for forward compatibility.

issue ref(#73857 )


          Create forward shims for sentry apps tasks

33016fc

github-actions bot added the Scope: Backend label

Christinarlong added 2 commits

September 30, 2024 13:09


          add barrel file

96e9775


          add to celery imports

c1ba2b1

vercel bot deployed to Preview

September 30, 2024 20:38

View deployment

codecov bot commented Sep 30, 2024 •

edited

Loading

Codecov Report

Attention: Patch coverage is 84.78261% with 14 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	82.22%	8 Missing ⚠️
src/sentry/tasks/sentry_apps.py	86.48%	5 Missing ⚠️
src/sentry/sentry_apps/tasks/service_hooks.py	85.71%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #78344    +/-   ##
========================================
  Coverage   78.08%   78.09%            
========================================
  Files        7066     7079    +13     
  Lines      311763   312112   +349     
  Branches    50956    51025    +69     
========================================
+ Hits       243452   243753   +301     
- Misses      61961    62001    +40     
- Partials     6350     6358     +8

Christinarlong requested a review from markstory

September 30, 2024 21:13


          type this monster monka

6d77d05

vercel bot deployed to Preview

October 1, 2024 10:26

View deployment

markstory reviewed

View reviewed changes

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

+                  "sentry.sentry_apps.tasks.sentry_apps.process_resource_change_bound", bind=True, **TASK_OPTIONS
+              )
+              @retry_decorator
+              def process_resource_change_bound(self, action, sender, instance_id, *args, **kwargs):

Member

markstory Oct 1, 2024

I think we're going to need types on the parameters and return types in this module to get through CI.

Contributor Author

Christinarlong Oct 1, 2024 •

edited

Loading

Oddly enough a lot of untyped functions in this file don't trigger mypy :thonk, spitballing, but maybe because their callers don't use types? The functions are not in classes? idk, but yes we should type them while we're at it

Edit: oop not this file, but in sentry/tasks/sentry_apps.py [original file], so nvm yea we'll need to type this func

src/sentry/sentry_apps/tasks/sentry_apps.py Outdated Show resolved Hide resolved

src/sentry/tasks/sentry_apps.py Outdated

                   # The class is serialized as a string when enqueueing the class.
-                  model = TYPES[sender]
+                  model: Event | Group | Activity = TYPES[sender]

Member

markstory Oct 1, 2024

You might need this to be type[Event] | type[Model]

src/sentry/tasks/sentry_apps.py Outdated

                   # The Event model has different hooks for the different event types. The sender
                   # determines which type eg. Error and therefore the 'name' eg. error
-                  if issubclass(model, Event):
+                  if isinstance(model, Event):

Member

markstory Oct 1, 2024

Wouldn't this change how the function works in a meaningful way? issubclass() works with class objects, whereas isinstance() works with class instances.

Contributor Author

Christinarlong Oct 1, 2024 •

edited

Loading

yes, 100%, it turns out I didn't actually push the corrections... 🫠 [ I would like to blame vscode editor for this one ]


          fix types i thinks

b4e5fce

vercel bot deployed to Preview

October 1, 2024 21:36

View deployment


          fix test

27e385f

vercel bot deployed to Preview

October 1, 2024 23:46

View deployment

Christinarlong added 2 commits

October 1, 2024 16:55


          fix mypy multiple positional arguements issue


          missed a typing

e3f0062

vercel bot deployed to Preview

October 2, 2024 00:03

View deployment

Christinarlong and others added 2 commits

October 1, 2024 17:03


          missed a typing

2951eec


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

52fa02d

Co-authored-by: Mark Story <[email protected]>

vercel bot deployed to Preview

October 2, 2024 00:10

View deployment

Christinarlong commented

View reviewed changes

src/sentry/tasks/sentry_apps.py

Comment on lines +227 to +245

+                      assert org, "organization must exist to get related sentry app installations"
+                      installations: list[RpcSentryAppInstallation] = [
+                          installation
+                          for installation in app_service.get_installed_for_organization(organization_id=org.id)
+                          if event in installation.sentry_app.events
+                      ]
+                      for installation in installations:
+                          data = {}
+                          if isinstance(instance, (Event, GroupEvent)):
+                              assert instance.group_id, "group id is required to create webhook event data"
+                              data[name] = _webhook_event_data(instance, instance.group_id, instance.project_id)
+                          else:
+                              data[name] = serialize(instance)
+                          # Trigger a new task for each webhook
+                          send_resource_change_webhook.delay(
+                              installation_id=installation.id, event=event, data=data
+                          )

Contributor Author

Christinarlong Oct 2, 2024

There is a slight logical change here that this code will only be ran if the instance is a Group | Event | GroupEvent (i.e if the instance is an Activity we do nothing) but I believe this was the case with the previous iteration too (?) since we don't get an organization and therefore installation to work with.

Sidenote: Do we process Activity/Comments here ? I couldn't find any related code calling this func for those event types.

Member

markstory Oct 2, 2024

There is a slight logical change here that this code will only be ran if the instance is a Group | Event | GroupEvent (i.e if the instance is an Activity we do nothing) but I believe this was the case with the previous iteration too

I don't think that difference in behavior will matter. Currently if Activity is used there will be an AttributeError for org.id which doesn't exist.

Sidenote: Do we process Activity/Comments here ? I couldn't find any related code calling this func for those event types.

There are no tests covering Comments/Activity, and no call sites that I could find. Once we've finished moving the tasks we could add a metric/log message to validate that there are no indirect paths that use Activity and then remove this code if it is truly unused.


          update process to be keyword only and update type to Model

9d9a44a

vercel bot deployed to Preview

October 2, 2024 01:09

View deployment

markstory approved these changes

View reviewed changes

src/sentry/tasks/sentry_apps.py

Comment on lines +227 to +245

+                      assert org, "organization must exist to get related sentry app installations"
+                      installations: list[RpcSentryAppInstallation] = [
+                          installation
+                          for installation in app_service.get_installed_for_organization(organization_id=org.id)
+                          if event in installation.sentry_app.events
+                      ]
+                      for installation in installations:
+                          data = {}
+                          if isinstance(instance, (Event, GroupEvent)):
+                              assert instance.group_id, "group id is required to create webhook event data"
+                              data[name] = _webhook_event_data(instance, instance.group_id, instance.project_id)
+                          else:
+                              data[name] = serialize(instance)
+                          # Trigger a new task for each webhook
+                          send_resource_change_webhook.delay(
+                              installation_id=installation.id, event=event, data=data
+                          )

Member

markstory Oct 2, 2024

There is a slight logical change here that this code will only be ran if the instance is a Group | Event | GroupEvent (i.e if the instance is an Activity we do nothing) but I believe this was the case with the previous iteration too

I don't think that difference in behavior will matter. Currently if Activity is used there will be an AttributeError for org.id which doesn't exist.

Sidenote: Do we process Activity/Comments here ? I couldn't find any related code calling this func for those event types.

There are no tests covering Comments/Activity, and no call sites that I could find. Once we've finished moving the tasks we could add a metric/log message to validate that there are no indirect paths that use Activity and then remove this code if it is truly unused.

src/sentry/tasks/sentry_apps.py Outdated

@@ @@ -449,6 +473,7 @@ def send_webhooks(installation, event, **kwargs): @@
                       kwargs["install"] = installation
                       request_data = AppPlatformEvent(**kwargs)
+                      assert servicehook.sentry_app, "sentry app must exist to get webhook url"

Member

markstory Oct 2, 2024

Interesting that we use both installation.sentry_app and servicehook.sentry_app here 🤔

Contributor Author

Christinarlong Oct 2, 2024

🌵

Christinarlong marked this pull request as ready for review

October 2, 2024 16:53

Christinarlong requested review from a team as code owners

October 2, 2024 16:53


          dentge, we can use the installation's sentry_app

fa311a6

Christinarlong mentioned this pull request

chore(sentry apps): Move logic to new Sentry App tasks #78347

Merged


          type all funcs for completeness

6b4ac23

Christinarlong enabled auto-merge (squash)

October 2, 2024 17:43

vercel bot deployed to Preview

October 2, 2024 17:45

View deployment


          fix typing for notify

2a88776

vercel bot deployed to Preview

October 2, 2024 20:42

View deployment

Christinarlong merged commit c4fe51a into master

49 checks passed

Christinarlong deleted the forward-sentryapp-shims branch

October 2, 2024 21:15

sentry-io bot commented Oct 3, 2024 •

edited

Loading

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ RateLimitExceeded: Query on could not be run due to allocation policies, info: {'details': {'ConcurrentRateLimitAllo... sentry.tasks.process_resource_change_bound View Issue
‼️ SnubaError: HTTPConnectionPool(host='127.0.0.1', port=10006): Read timed out. (read timeout=30) sentry.tasks.process_resource_change_bound View Issue

_{Did you find this useful? React with a 👍 or 👎}

github-actions bot locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels