Browse Source

chore(hybridcloud) Remove deprecated logic in webhook handling (#68701)

- Remove deprecated methods/parameters
- Improve logging as gitlab integrations are still not sharding as I
would expect.
- lower threshold for parallel delivery. Mailboxes with 150+ messages
would benefit from more throughput.
Mark Story 11 months ago
parent
commit
dbfb711a32

+ 2 - 2
src/sentry/hybridcloud/tasks/deliver_webhooks.py

@@ -106,8 +106,8 @@ def schedule_webhook_delivery(**kwargs) -> None:
         updated_count = WebhookPayload.objects.filter(id__in=Subquery(mailbox_batch)).update(
             schedule_for=timezone.now() + BATCH_SCHEDULE_OFFSET
         )
-        # If we have a full batch we should process in parallel as we're likely behind.
-        if use_parallel and updated_count >= MAX_MAILBOX_DRAIN:
+        # If we have a half-full batch we should process in parallel as we're likely behind.
+        if use_parallel and updated_count >= int(MAX_MAILBOX_DRAIN / 2):
             drain_mailbox_parallel.delay(record["id"])
         else:
             drain_mailbox.delay(record["id"])

+ 3 - 7
src/sentry/middleware/integrations/parsers/base.py

@@ -149,7 +149,6 @@ class BaseRequestParser(abc.ABC):
         self,
         regions: Sequence[Region],
         identifier: int | str | None = None,
-        shard_identifier_override: int | None = None,  # deprecated used in getsentry
         integration_id: int | None = None,
     ):
         """
@@ -159,7 +158,7 @@ class BaseRequestParser(abc.ABC):
         if len(regions) < 1:
             return HttpResponse(status=status.HTTP_202_ACCEPTED)
 
-        shard_identifier = identifier or shard_identifier_override or self.webhook_identifier.value
+        shard_identifier = identifier or self.webhook_identifier.value
         for region in regions:
             WebhookPayload.create_from_request(
                 region=region.name,
@@ -171,9 +170,6 @@ class BaseRequestParser(abc.ABC):
 
         return HttpResponse(status=status.HTTP_202_ACCEPTED)
 
-    # Alias to prop up getsentry
-    get_response_from_outbox_creation = get_response_from_webhookpayload
-
     def get_response_from_webhookpayload_for_integration(
         self, regions: Sequence[Region], integration: Integration | RpcIntegration
     ):
@@ -203,7 +199,7 @@ class BaseRequestParser(abc.ABC):
             # buckets for the next day.
             cache.set(use_buckets_key, 1, timeout=ONE_DAY)
             use_buckets = True
-            logging.info(
+            logger.info(
                 "integrations.parser.activate_buckets",
                 extra={"provider": self.provider, "integration_id": integration.id},
             )
@@ -213,7 +209,7 @@ class BaseRequestParser(abc.ABC):
 
         mailbox_bucket_id = self.mailbox_bucket_id(data)
         if mailbox_bucket_id is None:
-            logging.info(
+            logger.info(
                 "integrations.parser.no_bucket_id",
                 extra={"provider": self.provider, "integration_id": integration.id},
             )

+ 1 - 1
src/sentry/middleware/integrations/parsers/bitbucket.py

@@ -46,7 +46,7 @@ class BitbucketRequestParser(BaseRequestParser):
             logger.info("%s.no_region", self.provider, extra=logging_extra)
             return self.get_response_from_control_silo()
         return self.get_response_from_webhookpayload(
-            regions=[region], shard_identifier_override=mapping.organization_id
+            regions=[region], identifier=mapping.organization_id
         )
 
     def get_response(self):

+ 2 - 1
src/sentry/middleware/integrations/parsers/gitlab.py

@@ -81,7 +81,8 @@ class GitlabRequestParser(BaseRequestParser, GitlabWebhookMixin):
 
         try:
             data = json.loads(self.request.body)
-        except ValueError:
+        except ValueError as e:
+            logger.info("gitlab.body.parse_error", extra={"error": str(e)})
             data = {}
 
         return self.get_response_from_webhookpayload(

+ 1 - 1
src/sentry/middleware/integrations/parsers/plugin.py

@@ -55,5 +55,5 @@ class PluginRequestParser(BaseRequestParser):
         # Because outboxes are now sharded by integration and plugins don't have one,
         # we use the org ID as the shard ID to batch these changes.
         return self.get_response_from_webhookpayload(
-            regions=[region], shard_identifier_override=mapping.organization_id
+            regions=[region], identifier=mapping.organization_id
         )

+ 1 - 1
tests/sentry/hybridcloud/tasks/test_deliver_webhooks.py

@@ -138,7 +138,7 @@ class ScheduleWebhooksTest(TestCase):
     @override_options({"hybridcloud.webhookpayload.use_parallel": True})
     @patch("sentry.hybridcloud.tasks.deliver_webhooks.drain_mailbox_parallel")
     def test_schedule_mailbox_parallel_task(self, mock_deliver):
-        for _ in range(0, MAX_MAILBOX_DRAIN + 1):
+        for _ in range(0, int(MAX_MAILBOX_DRAIN / 2 + 1)):
             self.create_webhook_payload(
                 mailbox_name="github:123",
                 region_name="us",

+ 2 - 2
tests/sentry/middleware/integrations/parsers/test_base.py

@@ -89,7 +89,7 @@ class BaseRequestParserTest(TestCase):
     @override_settings(SILO_MODE=SiloMode.CONTROL)
     def test_get_response_from_webhookpayload_creation(self):
         with pytest.raises(NotImplementedError):
-            self.parser.get_response_from_outbox_creation(regions=self.region_config)
+            self.parser.get_response_from_webhookpayload(regions=self.region_config)
 
         class MockParser(BaseRequestParser):
             webhook_identifier = WebhookProviderIdentifier.SLACK
@@ -97,7 +97,7 @@ class BaseRequestParserTest(TestCase):
 
         parser = MockParser(self.request, self.response_handler)
 
-        response = parser.get_response_from_outbox_creation(regions=self.region_config)
+        response = parser.get_response_from_webhookpayload(regions=self.region_config)
         assert response.status_code == status.HTTP_202_ACCEPTED
         payloads = WebhookPayload.objects.all()
         assert len(payloads) == 2