Handle namespaces for metric_bucket rate limits

getsentry · Apr 3, 2024 · e8fef31 · e8fef31
1 parent 3c50e8b
commit e8fef31
Show file tree

Hide file tree

Showing 5 changed files with 103 additions and 29 deletions.
diff --git 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
@@ -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
@@ -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,36 +36,80 @@ 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<string, int> The map of time instants for each event category after
      *                         which an HTTP request can be retried
      */
     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
@@ -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
@@ -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' => [