Skip to content

ref(browser): Use ratelimit utils in base transport #4686

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.

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

Merged
merged 14 commits into from
Mar 14, 2022
62 changes: 17 additions & 45 deletions packages/browser/src/transports/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,18 @@ import {
} from '@sentry/types';
import {
createClientReportEnvelope,
disabledUntil,
dsnToString,
eventStatusFromHttpCode,
getGlobalObject,
isDebugBuild,
isRateLimited,
logger,
makePromiseBuffer,
parseRetryAfterHeader,
PromiseBuffer,
RateLimits,
serializeEnvelope,
updateRateLimits,
} from '@sentry/utils';

import { sendReport } from './utils';
Expand All @@ -53,7 +56,7 @@ export abstract class BaseTransport implements Transport {
protected readonly _buffer: PromiseBuffer<SentryResponse> = makePromiseBuffer(30);

/** Locks transport after receiving rate limits in a response */
protected readonly _rateLimits: Record<string, Date> = {};
protected _rateLimits: RateLimits = {};

protected _outcomes: { [key: string]: number } = {};

Expand Down Expand Up @@ -165,13 +168,12 @@ export abstract class BaseTransport implements Transport {
reject: (reason?: unknown) => void;
}): void {
const status = eventStatusFromHttpCode(response.status);
/**
* "The name is case-insensitive."
* https://developer.mozilla.org/en-US/docs/Web/API/Headers/get
*/
const limited = this._handleRateLimit(headers);
if (limited) {

this._rateLimits = updateRateLimits(this._rateLimits, headers);
// eslint-disable-next-line deprecation/deprecation
if (this._isRateLimited(requestType) && isDebugBuild()) {
isDebugBuild() &&
// eslint-disable-next-line deprecation/deprecation
logger.warn(`Too many ${requestType} requests, backing off until: ${this._disabledUntil(requestType)}`);
}

Expand All @@ -185,52 +187,22 @@ export abstract class BaseTransport implements Transport {

/**
* Gets the time that given category is disabled until for rate limiting
*
* @deprecated Please use `disabledUntil` from @sentry/utils
*/
protected _disabledUntil(requestType: SentryRequestType): Date {
const category = requestTypeToCategory(requestType);
return this._rateLimits[category] || this._rateLimits.all;
return new Date(disabledUntil(this._rateLimits, category));
}

/**
* Checks if a category is rate limited
*
* @deprecated Please use `isRateLimited` from @sentry/utils
*/
protected _isRateLimited(requestType: SentryRequestType): boolean {
return this._disabledUntil(requestType) > new Date(Date.now());
}

/**
* Sets internal _rateLimits from incoming headers. Returns true if headers contains a non-empty rate limiting header.
*/
protected _handleRateLimit(headers: Record<string, string | null>): boolean {
const now = Date.now();
const rlHeader = headers['x-sentry-rate-limits'];
const raHeader = headers['retry-after'];

if (rlHeader) {
// rate limit headers are of the form
// <header>,<header>,..
// where each <header> is of the form
// <retry_after>: <categories>: <scope>: <reason_code>
// where
// <retry_after> is a delay in ms
// <categories> is the event type(s) (error, transaction, etc) being rate limited and is of the form
// <category>;<category>;...
// <scope> is what's being limited (org, project, or key) - ignored by SDK
// <reason_code> is an arbitrary string like "org_quota" - ignored by SDK
for (const limit of rlHeader.trim().split(',')) {
const parameters = limit.split(':', 2);
const headerDelay = parseInt(parameters[0], 10);
const delay = (!isNaN(headerDelay) ? headerDelay : 60) * 1000; // 60sec default
for (const category of parameters[1].split(';')) {
this._rateLimits[category || 'all'] = new Date(now + delay);
}
}
return true;
} else if (raHeader) {
this._rateLimits.all = new Date(now + parseRetryAfterHeader(raHeader, now));
return true;
}
return false;
const category = requestTypeToCategory(requestType);
return isRateLimited(this._rateLimits, category);
}

protected abstract _sendRequest(
Expand Down
2 changes: 2 additions & 0 deletions packages/browser/src/transports/fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,14 @@ export class FetchTransport extends BaseTransport {
* @param originalPayload Original payload used to create SentryRequest
*/
protected _sendRequest(sentryRequest: SentryRequest, originalPayload: Event | Session): PromiseLike<Response> {
// eslint-disable-next-line deprecation/deprecation
if (this._isRateLimited(sentryRequest.type)) {
this.recordLostEvent('ratelimit_backoff', sentryRequest.type);

return Promise.reject({
event: originalPayload,
type: sentryRequest.type,
// eslint-disable-next-line deprecation/deprecation
reason: `Transport for ${sentryRequest.type} requests locked till ${this._disabledUntil(
sentryRequest.type,
)} due to too many requests.`,
Expand Down
2 changes: 2 additions & 0 deletions packages/browser/src/transports/xhr.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,14 @@ export class XHRTransport extends BaseTransport {
* @param originalPayload Original payload used to create SentryRequest
*/
protected _sendRequest(sentryRequest: SentryRequest, originalPayload: Event | Session): PromiseLike<Response> {
// eslint-disable-next-line deprecation/deprecation
if (this._isRateLimited(sentryRequest.type)) {
this.recordLostEvent('ratelimit_backoff', sentryRequest.type);

return Promise.reject({
event: originalPayload,
type: sentryRequest.type,
// eslint-disable-next-line deprecation/deprecation
reason: `Transport for ${sentryRequest.type} requests locked till ${this._disabledUntil(
sentryRequest.type,
)} due to too many requests.`,
Expand Down
16 changes: 14 additions & 2 deletions packages/browser/test/unit/transports/fetch.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,9 @@ describe('FetchTransport', () => {

jest
.spyOn(Date, 'now')
// 1st event - _isRateLimited - false
// 1st event - updateRateLimits - false
.mockImplementationOnce(() => beforeLimit)
// 1st event - _handleRateLimit
.mockImplementationOnce(() => beforeLimit)
// 1st event - _handleRateLimit
.mockImplementationOnce(() => beforeLimit)
Expand Down Expand Up @@ -247,6 +249,10 @@ describe('FetchTransport', () => {
.mockImplementationOnce(() => beforeLimit)
// 1st event - _handleRateLimit
.mockImplementationOnce(() => beforeLimit)
// 1st event - _isRateLimited
.mockImplementationOnce(() => beforeLimit)
// 1st event - _handleRateLimit
.mockImplementationOnce(() => beforeLimit)
// 2nd event - _isRateLimited - false (different category)
.mockImplementationOnce(() => withinLimit)
// 2nd event - _handleRateLimit
Expand Down Expand Up @@ -303,7 +309,9 @@ describe('FetchTransport', () => {
.spyOn(Date, 'now')
// 1st event - _isRateLimited - false
.mockImplementationOnce(() => beforeLimit)
// 1st event - _handleRateLimit
// 1st event - updateRateLimits
.mockImplementationOnce(() => beforeLimit)
// 1st event - _isRateLimited
.mockImplementationOnce(() => beforeLimit)
// 2nd event - _isRateLimited - true (event category)
.mockImplementationOnce(() => withinLimit)
Expand Down Expand Up @@ -376,6 +384,8 @@ describe('FetchTransport', () => {
.mockImplementationOnce(() => beforeLimit)
// 1st event - _handleRateLimit
.mockImplementationOnce(() => beforeLimit)
// 1st event - _isRateLimited
.mockImplementationOnce(() => beforeLimit)
// 2nd event - _isRateLimited - true (event category)
.mockImplementationOnce(() => withinLimit)
// 3rd event - _isRateLimited - true (transaction category)
Expand Down Expand Up @@ -447,6 +457,8 @@ describe('FetchTransport', () => {
.mockImplementationOnce(() => beforeLimit)
// 1st event - _handleRateLimit
.mockImplementationOnce(() => beforeLimit)
// 1st event - _isRateLimited
.mockImplementationOnce(() => beforeLimit)
// 2nd event - _isRateLimited - true
.mockImplementationOnce(() => withinLimit)
// 3rd event - _isRateLimited - false
Expand Down
12 changes: 12 additions & 0 deletions packages/browser/test/unit/transports/xhr.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,8 @@ describe('XHRTransport', () => {
.mockImplementationOnce(() => beforeLimit)
// 1st event - _handleRateLimit
.mockImplementationOnce(() => beforeLimit)
// 1st event - _handleRateLimit
.mockImplementationOnce(() => beforeLimit)
// 2nd event - _isRateLimited - true
.mockImplementationOnce(() => withinLimit)
// 3rd event - _isRateLimited - false
Expand Down Expand Up @@ -175,6 +177,10 @@ describe('XHRTransport', () => {
.mockImplementationOnce(() => beforeLimit)
// 1st event - _handleRateLimit
.mockImplementationOnce(() => beforeLimit)
// 1st event - _isRateLimited
.mockImplementationOnce(() => beforeLimit)
// 1st event - _handleRateLimit
.mockImplementationOnce(() => beforeLimit)
// 2nd event - _isRateLimited - false (different category)
.mockImplementationOnce(() => withinLimit)
// 2nd event - _handleRateLimit
Expand Down Expand Up @@ -236,6 +242,8 @@ describe('XHRTransport', () => {
.mockImplementationOnce(() => beforeLimit)
// 1st event - _handleRateLimit
.mockImplementationOnce(() => beforeLimit)
// 1st event - _isRateLimited
.mockImplementationOnce(() => beforeLimit)
// 2nd event - _isRateLimited - true (event category)
.mockImplementationOnce(() => withinLimit)
// 3rd event - _isRateLimited - true (transaction category)
Expand Down Expand Up @@ -307,6 +315,8 @@ describe('XHRTransport', () => {
.mockImplementationOnce(() => beforeLimit)
// 1st event - _handleRateLimit
.mockImplementationOnce(() => beforeLimit)
// 1st event - _isRateLimited
.mockImplementationOnce(() => beforeLimit)
// 2nd event - _isRateLimited - true (event category)
.mockImplementationOnce(() => withinLimit)
// 3rd event - _isRateLimited - true (transaction category)
Expand Down Expand Up @@ -377,6 +387,8 @@ describe('XHRTransport', () => {
.mockImplementationOnce(() => beforeLimit)
// 1st event - _handleRateLimit
.mockImplementationOnce(() => beforeLimit)
// 1st event - _handleRateLimit
.mockImplementationOnce(() => beforeLimit)
// 2nd event - _isRateLimited - true
.mockImplementationOnce(() => withinLimit)
// 3rd event - _isRateLimited - false
Expand Down