From e8fef31c3e2c1dd2d740c26b243d804fa081541c Mon Sep 17 00:00:00 2001 From: Michael Hoffmann Date: Wed, 3 Apr 2024 17:11:21 +0200 Subject: [PATCH] Handle namespaces for `metric_bucket` rate limits --- src/Serializer/EnvelopItems/MetricsItem.php | 6 ++ src/Transport/HttpTransport.php | 12 +--- src/Transport/RateLimiter.php | 78 ++++++++++++++++++--- tests/Transport/HttpTransportTest.php | 4 +- tests/Transport/RateLimiterTest.php | 32 +++++++-- 5 files changed, 103 insertions(+), 29 deletions(-) diff --git a/src/Serializer/EnvelopItems/MetricsItem.php b/src/Serializer/EnvelopItems/MetricsItem.php index 77ae05757..e03140ee3 100644 --- a/src/Serializer/EnvelopItems/MetricsItem.php +++ b/src/Serializer/EnvelopItems/MetricsItem.php @@ -37,6 +37,12 @@ public static function toEnvelopeItem(Event $event): string $metricMetaPayload = []; foreach ($metrics as $metric) { + /** + * In case of us adding support for setting a namespace for metrics, + * we have to alter the RateLimiter::class to properly handle these + * namespaces. + */ + // key - my.metric $line = preg_replace(self::KEY_PATTERN, '_', $metric->getKey()); diff --git a/src/Transport/HttpTransport.php b/src/Transport/HttpTransport.php index 74982a369..6fab31833 100644 --- a/src/Transport/HttpTransport.php +++ b/src/Transport/HttpTransport.php @@ -59,7 +59,7 @@ public function __construct( $this->httpClient = $httpClient; $this->payloadSerializer = $payloadSerializer; $this->logger = $logger ?? new NullLogger(); - $this->rateLimiter = new RateLimiter(); + $this->rateLimiter = new RateLimiter($this->logger); } /** @@ -123,15 +123,7 @@ public function send(Event $event): Result return new Result(ResultStatus::unknown()); } - if ($this->rateLimiter->handleResponse($response)) { - $eventType = $event->getType(); - $disabledUntil = $this->rateLimiter->getDisabledUntil($eventType); - - $this->logger->warning( - sprintf('Rate limited exceeded for requests of type "%s", backing off until "%s".', (string) $eventType, gmdate(\DATE_ATOM, $disabledUntil)), - ['event' => $event] - ); - } + $this->rateLimiter->handleResponse($response); $resultStatus = ResultStatus::createFromHttpStatusCode($response->getStatusCode()); diff --git a/src/Transport/RateLimiter.php b/src/Transport/RateLimiter.php index 330ba0dcd..dbc8bcf22 100644 --- a/src/Transport/RateLimiter.php +++ b/src/Transport/RateLimiter.php @@ -4,11 +4,23 @@ namespace Sentry\Transport; +use Psr\Log\LoggerInterface; +use Psr\Log\NullLogger; use Sentry\EventType; use Sentry\HttpClient\Response; final class RateLimiter { + /** + * @var string + */ + private const DATA_CATEGORY_ERROR = 'error'; + + /** + * @var string + */ + private const DATA_CATEGORY_METRIC_BUCKET = 'metric_bucket'; + /** * The name of the header to look at to know the rate limits for the events * categories supported by the server. @@ -24,7 +36,7 @@ final class RateLimiter /** * The number of seconds after which an HTTP request can be retried. */ - private const DEFAULT_RETRY_AFTER_DELAY_SECONDS = 60; + private const DEFAULT_RETRY_AFTER_SECONDS = 60; /** * @var array The map of time instants for each event category after @@ -32,28 +44,72 @@ final class RateLimiter */ private $rateLimits = []; + /** + * @var LoggerInterface A PSR-3 logger + */ + private $logger; + + public function __construct(?LoggerInterface $logger = null) + { + $this->logger = $logger ?? new NullLogger(); + } + public function handleResponse(Response $response): bool { $now = time(); if ($response->hasHeader(self::RATE_LIMITS_HEADER)) { foreach (explode(',', $response->getHeaderLine(self::RATE_LIMITS_HEADER)) as $limit) { - $parameters = explode(':', $limit, 3); - $parameters = array_splice($parameters, 0, 2); - $delay = ctype_digit($parameters[0]) ? (int) $parameters[0] : self::DEFAULT_RETRY_AFTER_DELAY_SECONDS; + /** + * $parameters[0] - retry_after + * $parameters[1] - categories + * $parameters[2] - scope (not used) + * $parameters[3] - reason_code (not used) + * $parameters[4] - namespaces (only returned if categories contains "metric_bucket"). + */ + $parameters = explode(':', $limit, 5); + + $retryAfter = $now + (ctype_digit($parameters[0]) ? (int) $parameters[0] : self::DEFAULT_RETRY_AFTER_SECONDS); foreach (explode(';', $parameters[1]) as $category) { - $this->rateLimits[$category ?: 'all'] = $now + $delay; + switch ($category) { + case self::DATA_CATEGORY_METRIC_BUCKET: + $namespaces = []; + if (isset($parameters[4])) { + $namespaces = explode(';', $parameters[4]); + } + + /** + * As we do not support setting any metric namespaces in the SDK, + * checking for the custom namespace suffices. + * In case the namespace was ommited in the response header, + * we'll also back off. + */ + if ($namespaces === [] || \in_array('custom', $namespaces)) { + $this->rateLimits[self::DATA_CATEGORY_METRIC_BUCKET] = $retryAfter; + } + break; + default: + $this->rateLimits[$category ?: 'all'] = $retryAfter; + } + + $this->logger->warning( + sprintf('Rate limited exceeded for category "%s", backing off until "%s".', $category, gmdate(\DATE_ATOM, $retryAfter)) + ); } } - return true; + return $this->rateLimits !== []; } if ($response->hasHeader(self::RETRY_AFTER_HEADER)) { - $delay = $this->parseRetryAfterHeader($now, $response->getHeaderLine(self::RETRY_AFTER_HEADER)); + $retryAfter = $now + $this->parseRetryAfterHeader($now, $response->getHeaderLine(self::RETRY_AFTER_HEADER)); + + $this->rateLimits['all'] = $retryAfter; - $this->rateLimits['all'] = $now + $delay; + $this->logger->warning( + sprintf('Rate limited exceeded for all categories, backing off until "%s".', gmdate(\DATE_ATOM, $retryAfter)) + ); return true; } @@ -73,11 +129,11 @@ public function getDisabledUntil(EventType $eventType): int $category = (string) $eventType; if ($eventType === EventType::event()) { - $category = 'error'; + $category = self::DATA_CATEGORY_ERROR; } if ($eventType === EventType::metrics()) { - $category = 'metric_bucket'; + $category = self::DATA_CATEGORY_METRIC_BUCKET; } return max($this->rateLimits['all'] ?? 0, $this->rateLimits[$category] ?? 0); @@ -95,6 +151,6 @@ private function parseRetryAfterHeader(int $currentTime, string $header): int return $headerDate->getTimestamp() - $currentTime; } - return self::DEFAULT_RETRY_AFTER_DELAY_SECONDS; + return self::DEFAULT_RETRY_AFTER_SECONDS; } } diff --git a/tests/Transport/HttpTransportTest.php b/tests/Transport/HttpTransportTest.php index 93790e632..148c3bbb6 100644 --- a/tests/Transport/HttpTransportTest.php +++ b/tests/Transport/HttpTransportTest.php @@ -125,7 +125,7 @@ public static function sendDataProvider(): iterable 'Sent event [%s] to %s [project:%s]. Result: "rate_limit" (status: 429).', ], 'warning' => [ - 'Rate limited exceeded for requests of type "event", backing off until "2022-02-06T00:01:00+00:00".', + 'Rate limited exceeded for all categories, backing off until "2022-02-06T00:01:00+00:00".', ], ], ]; @@ -230,7 +230,7 @@ public function testSendFailsDueToExceedingRateLimits(): void $logger->expects($this->exactly(2)) ->method('warning') ->withConsecutive( - ['Rate limited exceeded for requests of type "event", backing off until "2022-02-06T00:01:00+00:00".', ['event' => $event]], + ['Rate limited exceeded for all categories, backing off until "2022-02-06T00:01:00+00:00".'], ['Rate limit exceeded for sending requests of type "event".', ['event' => $event]] ); diff --git a/tests/Transport/RateLimiterTest.php b/tests/Transport/RateLimiterTest.php index f813e9e16..be6c4e24c 100644 --- a/tests/Transport/RateLimiterTest.php +++ b/tests/Transport/RateLimiterTest.php @@ -32,9 +32,7 @@ public function testHandleResponse(Response $response, bool $shouldBeHandled, ar { ClockMock::withClockMock(1644105600); - $rateLimiterResponse = $this->rateLimiter->handleResponse($response); - - $this->assertSame($shouldBeHandled, $rateLimiterResponse); + $this->rateLimiter->handleResponse($response); $this->assertEventTypesAreRateLimited($eventTypesLimited); } @@ -63,6 +61,12 @@ public static function handleResponseDataProvider(): \Generator ], ]; + yield 'Back-off using X-Sentry-Rate-Limits header with missing categories should lock them all' => [ + new Response(429, ['X-Sentry-Rate-Limits' => ['60::org']], ''), + true, + EventType::cases(), + ]; + yield 'Back-off using X-Sentry-Rate-Limits header with metric_bucket category' => [ new Response(429, ['X-Sentry-Rate-Limits' => ['60:metric_bucket:organization:quota_exceeded:custom']], ''), true, @@ -71,6 +75,14 @@ public static function handleResponseDataProvider(): \Generator ], ]; + yield 'Back-off using X-Sentry-Rate-Limits header with metric_bucket category, namespace custom and foo' => [ + new Response(429, ['X-Sentry-Rate-Limits' => ['60:metric_bucket:organization:quota_exceeded:custom;foo']], ''), + true, + [ + EventType::metrics(), + ], + ]; + yield 'Back-off using X-Sentry-Rate-Limits header with metric_bucket category without reason code' => [ new Response(429, ['X-Sentry-Rate-Limits' => ['60:metric_bucket:organization::custom']], ''), true, @@ -79,10 +91,18 @@ public static function handleResponseDataProvider(): \Generator ], ]; - yield 'Back-off using X-Sentry-Rate-Limits header with missing categories should lock them all' => [ - new Response(429, ['X-Sentry-Rate-Limits' => ['60::org']], ''), + yield 'Back-off using X-Sentry-Rate-Limits header with metric_bucket category without metric namespaces' => [ + new Response(429, ['X-Sentry-Rate-Limits' => ['60:metric_bucket:organization:quota_exceeded']], ''), true, - EventType::cases(), + [ + EventType::metrics(), + ], + ]; + + yield 'Do not back-off using X-Sentry-Rate-Limits header with metric_bucket category, namespace foo' => [ + new Response(429, ['X-Sentry-Rate-Limits' => ['60:metric_bucket:organization:quota_exceeded:foo']], ''), + false, + [], ]; yield 'Back-off using Retry-After header with number-based value' => [