Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle namespaces for metric_bucket rate limits #1728

Merged
merged 2 commits into from
Apr 4, 2024
Merged

Handle namespaces for metric_bucket rate limits #1728

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e8fef31
Handle namespaces for `metric_bucket` rate limits
cleptric Apr 3, 2024
1bd4e3a
Clarify namespace comment
cleptric Apr 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/Serializer/EnvelopItems/MetricsItem.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
cleptric marked this conversation as resolved.
Show resolved Hide resolved
* we have to alter the RateLimiter::class to properly handle these
* namespaces.
*/

// key - my.metric
$line = preg_replace(self::KEY_PATTERN, '_', $metric->getKey());

Expand Down
12 changes: 2 additions & 10 deletions src/Transport/HttpTransport.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -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]
);
}
cleptric marked this conversation as resolved.
Show resolved Hide resolved
$this->rateLimiter->handleResponse($response);

$resultStatus = ResultStatus::createFromHttpStatusCode($response->getStatusCode());

Expand Down
78 changes: 67 additions & 11 deletions src/Transport/RateLimiter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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);
Comment on lines +63 to +70
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use something like list($retry_after, $categories, ...) here to translate the comment into code? Note that the last few components are optional, though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will fail miserably, if not set 😬

Copy link
Collaborator

@stayallive stayallive Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can append str_repeat(':', 5) to the $limit so the explode always returns at least 5 parts 😉 Missing entries will be empty strings then.


$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]);
}
cleptric marked this conversation as resolved.
Show resolved Hide resolved

/**
* 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;
}
Expand All @@ -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);
Expand All @@ -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;
}
}
4 changes: 2 additions & 2 deletions tests/Transport/HttpTransportTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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".',
],
],
];
Expand Down Expand Up @@ -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]]
);

Expand Down
32 changes: 26 additions & 6 deletions tests/Transport/RateLimiterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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' => [
Expand Down
Loading