sentry icon indicating copy to clipboard operation
sentry copied to clipboard

feat: enable updating webhook events for published integration

Open scefali opened this issue 1 year ago • 5 comments

This PR enables updating events for published apps. There is a bug where we would prevent updating the webhook events because the scopes were out of order which actually turned out to be a blessing in disguise because the current implementation for updating the service hooks was not correct for published integrations which are installed by multiple integrations. As it happens, for that to work we have to update every ServiceHook row that is attached to that integration. Because there could be thousands of them, I put that logic in a task because it would almost certainly time out if there are many installations.

Note I still have to update the UI next so we allow updating the installation; right now the UI will prevent the update.

scefali avatar Aug 09 '24 12:08 scefali

🚨 Warning: This pull request contains Frontend and Backend changes!

It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently.

Have questions? Please ask in the #discuss-dev-infra channel.

github-actions[bot] avatar Aug 13 '24 11:08 github-actions[bot]

@markstory I think there is some weird hybrid cloud shenanigans going on in this test which is causing it to fail. I added some print statements for the failure:

Service hook rpc: added hook for organization 4554523754102784 and application 76
Service hook rpc: added hook for organization 4554523754168325 and application 73
Service hook rpc: added hook for organization 4554523754233856 and application 73
start test with organization ids:  4554523754168325 4554523754233856 and application id:  73
running create_or_update_service_hooks_for_installation with application id 73
running create_or_update_service_hooks_for_installation with application id 73
task and outbox runner should be done by now with organization ids: 4554523754168325, 4554523754233856, application id: 73

Which has this test failure:

AssertionError: No service hooks found for Org1 (ID: 4554523754168325), App ID: 73

There are a number of things going on that don't make sense:

  1. The print statements for the service hooks being updated are happening before the API call starts
  2. There is an extraneous application and organization that's getting a service hook update
  3. There are print statements that indeed the service hooks were created before we did the assertion

Here is the link to the test: https://github.com/getsentry/sentry/pull/75903/files#diff-253b2368a9e5befb9458c117ee39b0dda4a30afbd997dd320d0aff1d01ebb051R276

Any idea what's going on? I'm really out of clues and I can't replicate this locally. But I'm pretty sure this has to do with Hybrid Cloud because the project service hooks exist in the region silo while this is a control silo test.

scefali avatar Aug 14 '24 13:08 scefali

@corps Maybe you have some idea about my last comment?

scefali avatar Aug 16 '24 14:08 scefali

I was not able to reproduce the failure when running tests in isolation. However, I was able to reproduce the problem by running the same slice of CI with

TEST_GROUP=2 TOTAL_TEST_GROUPS=11 MATRIX_INSTANCE_TOTAL=11 pytest tests --ignore tests/acceptance --ignore tests/apidocs --ignore tests/js --ignore tests/tools -x

markstory avatar Aug 16 '24 14:08 markstory

1. The print statements for the service hooks being updated are happening before the API call starts

2. There is an extraneous application and organization that's getting a service hook update

3. There are print statements that indeed the service hooks were created before we did the assertion

I don't know, but I suspect given all three things, it has to be test pollution somehow. Given the log outputs, it makes me think, is it possible that a celery job is getting kicked off, not caught in tasks context manager, and is run during another test?

corps avatar Aug 16 '24 15:08 corps