diff --git a/src/Transport/HttpTransport.php b/src/Transport/HttpTransport.php index a8acf19a5..cf8a6549b 100644 --- a/src/Transport/HttpTransport.php +++ b/src/Transport/HttpTransport.php @@ -114,6 +114,16 @@ public function send(Event $event): Result ); } } + + // Attachments are rate limited independently of the event they belong to, + // so only the attachments are dropped and the event is still sent. + if ($event->getAttachments() !== [] && $this->rateLimiter->isRateLimited(RateLimiter::DATA_CATEGORY_ATTACHMENT)) { + $event->setAttachments([]); + $this->logger->warning( + 'Rate limit exceeded for sending requests of type "attachment". The attachments have been dropped.', + ['event' => $event] + ); + } } $request = new Request(); diff --git a/src/Transport/RateLimiter.php b/src/Transport/RateLimiter.php index fa6ad31e9..5e28098be 100644 --- a/src/Transport/RateLimiter.php +++ b/src/Transport/RateLimiter.php @@ -16,6 +16,16 @@ final class RateLimiter */ public const DATA_CATEGORY_PROFILE = 'profile'; + /** + * @var string + */ + public const DATA_CATEGORY_ATTACHMENT = 'attachment'; + + /** + * @var string + */ + private const DATA_CATEGORY_ATTACHMENT_ITEM = 'attachment_item'; + /** * @var string */ @@ -131,7 +141,14 @@ public function getDisabledUntil($eventType): int $eventType = self::DATA_CATEGORY_CHECK_IN; } - return max($this->rateLimits['all'] ?? 0, $this->rateLimits[$eventType] ?? 0); + $disabledUntil = max($this->rateLimits['all'] ?? 0, $this->rateLimits[$eventType] ?? 0); + + // Attachments can be dropped for size or for count, so we have to check the count here as well + if ($eventType === self::DATA_CATEGORY_ATTACHMENT) { + $disabledUntil = max($disabledUntil, $this->rateLimits[self::DATA_CATEGORY_ATTACHMENT_ITEM] ?? 0); + } + + return $disabledUntil; } private function parseRetryAfterHeader(int $currentTime, string $header): int diff --git a/tests/Transport/HttpTransportTest.php b/tests/Transport/HttpTransportTest.php index 177075748..2baf81ead 100644 --- a/tests/Transport/HttpTransportTest.php +++ b/tests/Transport/HttpTransportTest.php @@ -8,6 +8,7 @@ use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; +use Sentry\Attachment\Attachment; use Sentry\Event; use Sentry\HttpClient\HttpClientInterface; use Sentry\HttpClient\Response; @@ -375,6 +376,116 @@ public function testDropsProfileAndSendsTransactionWhenProfileRateLimited(): voi $this->assertNull($event->getSdkMetadata('profile')); } + /** + * @group time-sensitive + * + * @dataProvider eventsWithAttachmentsDataProvider + */ + public function testDropsAttachmentsAndSendsEventWhenAttachmentsRateLimited(callable $eventFactory, string $rateLimitsHeader): void + { + ClockMock::withClockMock(1644105600); + + $transport = new HttpTransport( + new Options(['dsn' => 'http://public@example.com/1']), + $this->httpClient, + $this->payloadSerializer, + $this->logger + ); + + /** @var Event $event */ + $event = $eventFactory(); + $event->setAttachments([Attachment::fromBytes('test.txt', 'test')]); + + $this->payloadSerializer->expects($this->exactly(2)) + ->method('serialize') + ->willReturn('{"foo":"bar"}'); + + $this->httpClient->expects($this->exactly(2)) + ->method('sendRequest') + ->willReturnOnConsecutiveCalls( + new Response(200, ['X-Sentry-Rate-Limits' => [$rateLimitsHeader]], ''), + new Response(200, [], '') + ); + + // First request informs about the attachment rate limit + $result = $transport->send($event); + + $this->assertEquals(ResultStatus::success(), $result->getStatus()); + + // attachments are still present + $this->assertCount(1, $event->getAttachments()); + + $event = $eventFactory(); + $event->setAttachments([Attachment::fromBytes('test.txt', 'test')]); + + $this->logger->expects($this->once()) + ->method('warning') + ->with( + $this->stringContains('Rate limit exceeded for sending requests of type "attachment".'), + ['event' => $event] + ); + + $result = $transport->send($event); + + // Sending the event is successful because only attachments are rate limited + $this->assertEquals(ResultStatus::success(), $result->getStatus()); + + // attachments are removed because they were rate limited + $this->assertSame([], $event->getAttachments()); + } + + public static function eventsWithAttachmentsDataProvider(): \Generator + { + yield 'Event with attachment rate limit' => [ + [Event::class, 'createEvent'], + '60:attachment:organization', + ]; + + yield 'Event with attachment_item rate limit' => [ + [Event::class, 'createEvent'], + '60:attachment_item:organization', + ]; + + yield 'Transaction with attachment rate limit' => [ + [Event::class, 'createTransaction'], + '60:attachment:organization', + ]; + } + + /** + * @group time-sensitive + */ + public function testKeepsAttachmentsWhenNotRateLimited(): void + { + ClockMock::withClockMock(1644105600); + + $transport = new HttpTransport( + new Options(['dsn' => 'http://public@example.com/1']), + $this->httpClient, + $this->payloadSerializer, + $this->logger + ); + + $this->payloadSerializer->expects($this->exactly(2)) + ->method('serialize') + ->willReturn('{"foo":"bar"}'); + + $this->httpClient->expects($this->exactly(2)) + ->method('sendRequest') + ->willReturn(new Response(200, ['X-Sentry-Rate-Limits' => ['60:profile:organization']], '')); + + // First request informs about a rate limit unrelated to attachments + $transport->send(Event::createEvent()); + + $event = Event::createEvent(); + $event->setAttachments([Attachment::fromBytes('test.txt', 'test')]); + + $result = $transport->send($event); + + $this->assertEquals(ResultStatus::success(), $result->getStatus()); + $this->assertCount(1, $event->getAttachments()); + } + /** * @group time-sensitive */ diff --git a/tests/Transport/RateLimiterTest.php b/tests/Transport/RateLimiterTest.php index 70285a3e8..a90b50998 100644 --- a/tests/Transport/RateLimiterTest.php +++ b/tests/Transport/RateLimiterTest.php @@ -96,6 +96,53 @@ public function testHandleResponseWithMultipleCommaSpaceSeparatedLimits(): void $this->assertSame(1644105600 + 2700, $this->rateLimiter->getDisabledUntil(EventType::event())); } + /** + * @dataProvider attachmentRateLimitsDataProvider + */ + public function testAttachmentsAreRateLimited(string $rateLimitsHeader, int $expectedDisabledUntil): void + { + ClockMock::withClockMock(1644105600); + + $this->rateLimiter->handleResponse(new Response(429, ['X-Sentry-Rate-Limits' => [$rateLimitsHeader]], '')); + + $this->assertTrue($this->rateLimiter->isRateLimited(RateLimiter::DATA_CATEGORY_ATTACHMENT)); + $this->assertSame($expectedDisabledUntil, $this->rateLimiter->getDisabledUntil(RateLimiter::DATA_CATEGORY_ATTACHMENT)); + + // Attachment rate limits must not affect the events the attachments belong to + $this->assertEventTypesAreRateLimited([]); + + ClockMock::withClockMock($expectedDisabledUntil); + + $this->assertFalse($this->rateLimiter->isRateLimited(RateLimiter::DATA_CATEGORY_ATTACHMENT)); + } + + public static function attachmentRateLimitsDataProvider(): \Generator + { + yield 'Back-off using X-Sentry-Rate-Limits header with attachment category' => [ + '60:attachment:organization', + 1644105600 + 60, + ]; + + yield 'Back-off using X-Sentry-Rate-Limits header with attachment_item category' => [ + '60:attachment_item:organization', + 1644105600 + 60, + ]; + + yield 'Back-off using the longest of the attachment and attachment_item categories' => [ + '120:attachment_item:organization, 60:attachment:organization', + 1644105600 + 120, + ]; + } + + public function testAttachmentsAreRateLimitedWhenAllCategoriesAreRateLimited(): void + { + ClockMock::withClockMock(1644105600); + + $this->rateLimiter->handleResponse(new Response(429, ['Retry-After' => ['60']], '')); + + $this->assertTrue($this->rateLimiter->isRateLimited(RateLimiter::DATA_CATEGORY_ATTACHMENT)); + } + public function testIsRateLimited(): void { // Events should not be rate-limited at all