From 6e5dbaa65714399e845fa35dd6958525b9a82277 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Og=C3=B3rek?= Date: Thu, 8 Oct 2020 16:25:47 +0200 Subject: [PATCH 1/4] Implement X-Sentry-Rate-Limits handling for browser SDK transports --- packages/browser/src/transports/base.ts | 46 +- packages/browser/src/transports/fetch.ts | 35 +- packages/browser/src/transports/xhr.ts | 35 +- .../test/unit/transports/fetch.test.ts | 401 ++++++++++++++---- .../browser/test/unit/transports/xhr.test.ts | 378 ++++++++++++++--- 5 files changed, 727 insertions(+), 168 deletions(-) diff --git a/packages/browser/src/transports/base.ts b/packages/browser/src/transports/base.ts index 00c3ef817cbc..7eb89fe6897f 100644 --- a/packages/browser/src/transports/base.ts +++ b/packages/browser/src/transports/base.ts @@ -1,6 +1,6 @@ import { API } from '@sentry/core'; import { Event, Response, Transport, TransportOptions } from '@sentry/types'; -import { PromiseBuffer, SentryError } from '@sentry/utils'; +import { parseRetryAfterHeader, PromiseBuffer, SentryError } from '@sentry/utils'; /** Base Transport class implementation */ export abstract class BaseTransport implements Transport { @@ -15,6 +15,9 @@ export abstract class BaseTransport implements Transport { /** A simple buffer holding all requests. */ protected readonly _buffer: PromiseBuffer = new PromiseBuffer(30); + /** Locks transport after receiving 429 response */ + protected readonly _rateLimits: Record = {}; + public constructor(public options: TransportOptions) { this._api = new API(this.options.dsn); // eslint-disable-next-line deprecation/deprecation @@ -34,4 +37,45 @@ export abstract class BaseTransport implements Transport { public close(timeout?: number): PromiseLike { return this._buffer.drain(timeout); } + + /** + * Gets the time that given category is disabled until for rate limiting + */ + protected _disabledUntil(category: string): Date { + return this._rateLimits[category] || this._rateLimits.all; + } + + /** + * Checks if a category is ratelimited + */ + protected _isRateLimited(category: string): boolean { + // We use `new Date(Date.now())` instead of just `new Date()` despite them being the same thing, + // as it's easier to mock `now` method on `Date` instance instead of whole `Date` object in tests. + return this._disabledUntil(category) > new Date(Date.now()); + } + + /** + * Sets internal _rateLimits from incoming headers + */ + protected _handleRateLimit(headers: Record): boolean { + const now = Date.now(); + const rlHeader = headers['x-sentry-rate-limits']; + const raHeader = headers['retry-after']; + + if (rlHeader) { + 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(now, raHeader)); + return true; + } + return false; + } } diff --git a/packages/browser/src/transports/fetch.ts b/packages/browser/src/transports/fetch.ts index c08541af8b60..dfe2e31788a8 100644 --- a/packages/browser/src/transports/fetch.ts +++ b/packages/browser/src/transports/fetch.ts @@ -1,6 +1,6 @@ import { eventToSentryRequest } from '@sentry/core'; import { Event, Response, Status } from '@sentry/types'; -import { getGlobalObject, logger, parseRetryAfterHeader, supportsReferrerPolicy, SyncPromise } from '@sentry/utils'; +import { getGlobalObject, logger, supportsReferrerPolicy, SyncPromise } from '@sentry/utils'; import { BaseTransport } from './base'; @@ -8,17 +8,16 @@ const global = getGlobalObject(); /** `fetch` based transport */ export class FetchTransport extends BaseTransport { - /** Locks transport after receiving 429 response */ - private _disabledUntil: Date = new Date(Date.now()); - /** * @inheritDoc */ public sendEvent(event: Event): PromiseLike { - if (new Date(Date.now()) < this._disabledUntil) { + const eventType = event.type || 'event'; + + if (this._isRateLimited(eventType)) { return Promise.reject({ event, - reason: `Transport locked till ${this._disabledUntil} due to too many requests.`, + reason: `Transport locked till ${this._disabledUntil(eventType)} due to too many requests.`, status: 429, }); } @@ -50,20 +49,24 @@ export class FetchTransport extends BaseTransport { .then(response => { const status = Status.fromHttpCode(response.status); - if (status === Status.Success) { - resolve({ status }); - return; - } - - if (status === Status.RateLimit) { - const now = Date.now(); + // Request with 200 that contain `x-sentry-retry-limits` should still handle that header. + if (status === Status.Success || status === Status.RateLimit) { /** * "The name is case-insensitive." * https://developer.mozilla.org/en-US/docs/Web/API/Headers/get */ - const retryAfterHeader = response.headers.get('Retry-After'); - this._disabledUntil = new Date(now + parseRetryAfterHeader(now, retryAfterHeader)); - logger.warn(`Too many requests, backing off till: ${this._disabledUntil}`); + const limited = this._handleRateLimit({ + 'x-sentry-rate-limits': response.headers.get('X-Sentry-Rate-Limits'), + 'retry-after': response.headers.get('Retry-After'), + }); + if (limited) { + logger.warn(`Too many requests, backing off till: ${this._disabledUntil(eventType)}`); + } + } + + if (status === Status.Success) { + resolve({ status }); + return; } reject(response); diff --git a/packages/browser/src/transports/xhr.ts b/packages/browser/src/transports/xhr.ts index a977a53fe54f..6b6e634e05df 100644 --- a/packages/browser/src/transports/xhr.ts +++ b/packages/browser/src/transports/xhr.ts @@ -1,22 +1,21 @@ import { eventToSentryRequest } from '@sentry/core'; import { Event, Response, Status } from '@sentry/types'; -import { logger, parseRetryAfterHeader, SyncPromise } from '@sentry/utils'; +import { logger, SyncPromise } from '@sentry/utils'; import { BaseTransport } from './base'; /** `XHR` based transport */ export class XHRTransport extends BaseTransport { - /** Locks transport after receiving 429 response */ - private _disabledUntil: Date = new Date(Date.now()); - /** * @inheritDoc */ public sendEvent(event: Event): PromiseLike { - if (new Date(Date.now()) < this._disabledUntil) { + const eventType = event.type || 'event'; + + if (this._isRateLimited(eventType)) { return Promise.reject({ event, - reason: `Transport locked till ${this._disabledUntil} due to too many requests.`, + reason: `Transport locked till ${this._disabledUntil(eventType)} due to too many requests.`, status: 429, }); } @@ -34,20 +33,24 @@ export class XHRTransport extends BaseTransport { const status = Status.fromHttpCode(request.status); - if (status === Status.Success) { - resolve({ status }); - return; - } - - if (status === Status.RateLimit) { - const now = Date.now(); + // Request with 200 that contain `x-sentry-retry-limits` should still handle that header. + if (status === Status.Success || status === Status.RateLimit) { /** * "The search for the header name is case-insensitive." * https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/getResponseHeader */ - const retryAfterHeader = request.getResponseHeader('Retry-After'); - this._disabledUntil = new Date(now + parseRetryAfterHeader(now, retryAfterHeader)); - logger.warn(`Too many requests, backing off till: ${this._disabledUntil}`); + const limited = this._handleRateLimit({ + 'x-sentry-rate-limits': request.getResponseHeader('X-Sentry-Rate-Limits'), + 'retry-after': request.getResponseHeader('Retry-After'), + }); + if (limited) { + logger.warn(`Too many requests, backing off till: ${this._disabledUntil(eventType)}`); + } + } + + if (status === Status.Success) { + resolve({ status }); + return; } reject(request); diff --git a/packages/browser/test/unit/transports/fetch.test.ts b/packages/browser/test/unit/transports/fetch.test.ts index 012d98166dd6..8193ba4d372b 100644 --- a/packages/browser/test/unit/transports/fetch.test.ts +++ b/packages/browser/test/unit/transports/fetch.test.ts @@ -1,16 +1,16 @@ import { expect } from 'chai'; import { SinonStub, stub } from 'sinon'; -import { Status, Transports } from '../../../src'; +import { Event, Status, Transports } from '../../../src'; const testDsn = 'https://123@sentry.io/42'; -const transportUrl = 'https://sentry.io/api/42/store/?sentry_key=123&sentry_version=7'; -const payload = { +const storeUrl = 'https://sentry.io/api/42/store/?sentry_key=123&sentry_version=7'; +const eventPayload: Event = { event_id: '1337', - message: 'Pickle Rick', - user: { - username: 'Morty', - }, +}; +const transactionPayload: Event = { + event_id: '42', + type: 'transaction', }; let fetch: SinonStub; @@ -28,22 +28,22 @@ describe('FetchTransport', () => { it('inherits composeEndpointUrl() implementation', () => { // eslint-disable-next-line deprecation/deprecation - expect(transport.url).equal(transportUrl); + expect(transport.url).equal(storeUrl); }); describe('sendEvent()', async () => { it('sends a request to Sentry servers', async () => { - const response = { status: 200 }; + const response = { status: 200, headers: new Headers() }; fetch.returns(Promise.resolve(response)); - const res = await transport.sendEvent(payload); + const res = await transport.sendEvent(eventPayload); expect(res.status).equal(Status.Success); expect(fetch.calledOnce).equal(true); expect( - fetch.calledWith(transportUrl, { - body: JSON.stringify(payload), + fetch.calledWith(storeUrl, { + body: JSON.stringify(eventPayload), method: 'POST', referrerPolicy: 'origin', }), @@ -51,18 +51,18 @@ describe('FetchTransport', () => { }); it('rejects with non-200 status code', async () => { - const response = { status: 403 }; + const response = { status: 403, headers: new Headers() }; fetch.returns(Promise.resolve(response)); try { - await transport.sendEvent(payload); + await transport.sendEvent(eventPayload); } catch (res) { expect(res.status).equal(403); expect(fetch.calledOnce).equal(true); expect( - fetch.calledWith(transportUrl, { - body: JSON.stringify(payload), + fetch.calledWith(storeUrl, { + body: JSON.stringify(eventPayload), method: 'POST', referrerPolicy: 'origin', }), @@ -71,65 +71,17 @@ describe('FetchTransport', () => { }); it('pass the error to rejection when fetch fails', async () => { - const response = { status: 403 }; + const response = { status: 403, headers: new Headers() }; fetch.returns(Promise.reject(response)); try { - await transport.sendEvent(payload); + await transport.sendEvent(eventPayload); } catch (res) { expect(res).equal(response); } }); - it('back-off using Retry-After header', async () => { - const retryAfterSeconds = 10; - const headers = new Map(); - headers.set('Retry-After', retryAfterSeconds); - const response = { status: 429, headers }; - fetch.returns(Promise.resolve(response)); - - const now = Date.now(); - const dateStub = stub(Date, 'now') - // Check for first event - .onCall(0) - .returns(now) - // Setting disableUntil - .onCall(1) - .returns(now) - // Check for second event - .onCall(2) - .returns(now + (retryAfterSeconds / 2) * 1000) - // Check for third event - .onCall(3) - .returns(now + retryAfterSeconds * 1000); - - try { - await transport.sendEvent(payload); - } catch (res) { - expect(res.status).equal(429); - expect(res.reason).equal(undefined); - } - - try { - await transport.sendEvent(payload); - } catch (res) { - expect(res.status).equal(429); - expect(res.reason).equal( - `Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`, - ); - } - - try { - await transport.sendEvent(payload); - } catch (res) { - expect(res.status).equal(429); - expect(res.reason).equal(undefined); - } - - dateStub.restore(); - }); - it('passes in headers', async () => { transport = new Transports.FetchTransport({ dsn: testDsn, @@ -137,16 +89,16 @@ describe('FetchTransport', () => { Authorization: 'Basic GVzdDp0ZXN0Cg==', }, }); - const response = { status: 200 }; + const response = { status: 200, headers: new Headers() }; fetch.returns(Promise.resolve(response)); - const res = await transport.sendEvent(payload); + const res = await transport.sendEvent(eventPayload); expect(res.status).equal(Status.Success); expect( - fetch.calledWith(transportUrl, { - body: JSON.stringify(payload), + fetch.calledWith(storeUrl, { + body: JSON.stringify(eventPayload), headers: { Authorization: 'Basic GVzdDp0ZXN0Cg==', }, @@ -163,21 +115,322 @@ describe('FetchTransport', () => { credentials: 'include', }, }); - const response = { status: 200 }; + const response = { status: 200, headers: new Headers() }; fetch.returns(Promise.resolve(response)); - const res = await transport.sendEvent(payload); + const res = await transport.sendEvent(eventPayload); expect(res.status).equal(Status.Success); expect( - fetch.calledWith(transportUrl, { - body: JSON.stringify(payload), + fetch.calledWith(storeUrl, { + body: JSON.stringify(eventPayload), credentials: 'include', method: 'POST', referrerPolicy: 'origin', }), ).equal(true); }); + + describe('Rate-limiting', () => { + it('back-off using Retry-After header', async () => { + const retryAfterSeconds = 10; + const beforeLimit = Date.now(); + const withinLimit = beforeLimit + (retryAfterSeconds / 2) * 1000; + const afterLimit = beforeLimit + retryAfterSeconds * 1000; + + const dateStub = stub(Date, 'now') + // 1st event - _isRateLimited - false + .onCall(0) + .returns(beforeLimit) + // 1st event - _handleRateLimit + .onCall(1) + .returns(beforeLimit) + // 2nd event - _isRateLimited - true + .onCall(2) + .returns(withinLimit) + // 3rd event - _isRateLimited - false + .onCall(3) + .returns(afterLimit) + // 3rd event - _handleRateLimit + .onCall(4) + .returns(afterLimit); + + const headers = new Headers(); + headers.set('Retry-After', `${retryAfterSeconds}`); + fetch.returns(Promise.resolve({ status: 429, headers })); + + try { + await transport.sendEvent(eventPayload); + throw new Error('unreachable!'); + } catch (res) { + expect(res.status).equal(429); + expect(res.reason).equal(undefined); + } + + try { + await transport.sendEvent(eventPayload); + throw new Error('unreachable!'); + } catch (res) { + expect(res.status).equal(429); + expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + } + + fetch.returns(Promise.resolve({ status: 200, headers: new Headers() })); + + const eventRes = await transport.sendEvent(eventPayload); + expect(eventRes.status).equal(Status.Success); + + dateStub.restore(); + }); + + it('back-off using X-Sentry-Rate-Limits with single category', async () => { + const retryAfterSeconds = 10; + const beforeLimit = Date.now(); + const withinLimit = beforeLimit + (retryAfterSeconds / 2) * 1000; + const afterLimit = beforeLimit + retryAfterSeconds * 1000; + + const dateStub = stub(Date, 'now') + // 1st event - _isRateLimited - false + .onCall(0) + .returns(beforeLimit) + // 1st event - _handleRateLimit + .onCall(1) + .returns(beforeLimit) + // 2nd event - _isRateLimited - false (different category) + .onCall(2) + .returns(withinLimit) + // 2nd event - _handleRateLimit + .onCall(3) + .returns(withinLimit) + // 3rd event - _isRateLimited - true + .onCall(4) + .returns(withinLimit) + // 4th event - _isRateLimited - false + .onCall(5) + .returns(afterLimit) + // 4th event - _handleRateLimit + .onCall(6) + .returns(afterLimit); + + const headers = new Headers(); + headers.set('X-Sentry-Rate-Limits', `${retryAfterSeconds}:event:scope`); + fetch.returns(Promise.resolve({ status: 429, headers })); + + try { + await transport.sendEvent(eventPayload); + throw new Error('unreachable!'); + } catch (res) { + expect(res.status).equal(429); + expect(res.reason).equal(undefined); + } + + fetch.returns(Promise.resolve({ status: 200, headers: new Headers() })); + + const transactionRes = await transport.sendEvent(transactionPayload); + expect(transactionRes.status).equal(Status.Success); + + try { + await transport.sendEvent(eventPayload); + throw new Error('unreachable!'); + } catch (res) { + expect(res.status).equal(429); + expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + } + + const eventRes = await transport.sendEvent(eventPayload); + expect(eventRes.status).equal(Status.Success); + + dateStub.restore(); + }); + + it('back-off using X-Sentry-Rate-Limits with multiple categories', async () => { + const retryAfterSeconds = 10; + const beforeLimit = Date.now(); + const withinLimit = beforeLimit + (retryAfterSeconds / 2) * 1000; + const afterLimit = beforeLimit + retryAfterSeconds * 1000; + + const dateStub = stub(Date, 'now') + // 1st event - _isRateLimited - false + .onCall(0) + .returns(beforeLimit) + // 1st event - _handleRateLimit + .onCall(1) + .returns(beforeLimit) + // 2nd event - _isRateLimited - true (event category) + .onCall(2) + .returns(withinLimit) + // 3rd event - _isRateLimited - true (transaction category) + .onCall(3) + .returns(withinLimit) + // 4th event - _isRateLimited - false (event category) + .onCall(4) + .returns(afterLimit) + // 4th event - _handleRateLimit + .onCall(5) + .returns(afterLimit) + // 5th event - _isRateLimited - false (transaction category) + .onCall(6) + .returns(afterLimit) + // 5th event - _handleRateLimit + .onCall(7) + .returns(afterLimit); + + const headers = new Headers(); + headers.set('X-Sentry-Rate-Limits', `${retryAfterSeconds}:event;transaction:scope`); + fetch.returns(Promise.resolve({ status: 429, headers })); + + try { + await transport.sendEvent(eventPayload); + throw new Error('unreachable!'); + } catch (res) { + expect(res.status).equal(429); + expect(res.reason).equal(undefined); + } + + try { + await transport.sendEvent(eventPayload); + throw new Error('unreachable!'); + } catch (res) { + expect(res.status).equal(429); + expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + } + + try { + await transport.sendEvent(transactionPayload); + throw new Error('unreachable!'); + } catch (res) { + expect(res.status).equal(429); + expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + } + + fetch.returns(Promise.resolve({ status: 200, headers: new Headers() })); + + const eventRes = await transport.sendEvent(eventPayload); + expect(eventRes.status).equal(Status.Success); + + const transactionRes = await transport.sendEvent(transactionPayload); + expect(transactionRes.status).equal(Status.Success); + + dateStub.restore(); + }); + + it('back-off using X-Sentry-Rate-Limits with missing categories should lock them all', async () => { + const retryAfterSeconds = 10; + const beforeLimit = Date.now(); + const withinLimit = beforeLimit + (retryAfterSeconds / 2) * 1000; + const afterLimit = beforeLimit + retryAfterSeconds * 1000; + + const dateStub = stub(Date, 'now') + // 1st event - _isRateLimited - false + .onCall(0) + .returns(beforeLimit) + // 1st event - _handleRateLimit + .onCall(1) + .returns(beforeLimit) + // 2nd event - _isRateLimited - true (event category) + .onCall(2) + .returns(withinLimit) + // 3rd event - _isRateLimited - true (transaction category) + .onCall(3) + .returns(withinLimit) + // 4th event - _isRateLimited - false (event category) + .onCall(4) + .returns(afterLimit) + // 4th event - _handleRateLimit + .onCall(5) + .returns(afterLimit) + // 5th event - _isRateLimited - false (transaction category) + .onCall(6) + .returns(afterLimit) + // 5th event - _handleRateLimit + .onCall(7) + .returns(afterLimit); + + const headers = new Headers(); + headers.set('X-Sentry-Rate-Limits', `${retryAfterSeconds}:event;transaction:scope`); + fetch.returns(Promise.resolve({ status: 429, headers })); + + try { + await transport.sendEvent(eventPayload); + throw new Error('unreachable!'); + } catch (res) { + expect(res.status).equal(429); + expect(res.reason).equal(undefined); + } + + try { + await transport.sendEvent(eventPayload); + throw new Error('unreachable!'); + } catch (res) { + expect(res.status).equal(429); + expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + } + + try { + await transport.sendEvent(transactionPayload); + throw new Error('unreachable!'); + } catch (res) { + expect(res.status).equal(429); + expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + } + + fetch.returns(Promise.resolve({ status: 200, headers: new Headers() })); + + const eventRes = await transport.sendEvent(eventPayload); + expect(eventRes.status).equal(Status.Success); + + const transactionRes = await transport.sendEvent(transactionPayload); + expect(transactionRes.status).equal(Status.Success); + + dateStub.restore(); + }); + + it('back-off using X-Sentry-Rate-Limits should also trigger for 200 responses', async () => { + const retryAfterSeconds = 10; + const beforeLimit = Date.now(); + const withinLimit = beforeLimit + (retryAfterSeconds / 2) * 1000; + const afterLimit = beforeLimit + retryAfterSeconds * 1000; + + const dateStub = stub(Date, 'now') + // 1st event - _isRateLimited - false + .onCall(0) + .returns(beforeLimit) + // 1st event - _handleRateLimit + .onCall(1) + .returns(beforeLimit) + // 2nd event - _isRateLimited - true + .onCall(2) + .returns(withinLimit) + // 3rd event - _isRateLimited - false + .onCall(3) + .returns(afterLimit) + // 3rd event - _handleRateLimit + .onCall(4) + .returns(afterLimit); + + const headers = new Headers(); + headers.set('X-Sentry-Rate-Limits', `${retryAfterSeconds}:event;transaction:scope`); + fetch.returns(Promise.resolve({ status: 200, headers })); + + let eventRes = await transport.sendEvent(eventPayload); + expect(eventRes.status).equal(Status.Success); + + try { + await transport.sendEvent(eventPayload); + throw new Error('unreachable!'); + } catch (res) { + expect(res.status).equal(429); + expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + } + + fetch.returns(Promise.resolve({ status: 200, headers: new Headers() })); + + eventRes = await transport.sendEvent(eventPayload); + expect(eventRes.status).equal(Status.Success); + + dateStub.restore(); + }); + }); }); }); diff --git a/packages/browser/test/unit/transports/xhr.test.ts b/packages/browser/test/unit/transports/xhr.test.ts index 51bb1f6351ee..5bf57f76321f 100644 --- a/packages/browser/test/unit/transports/xhr.test.ts +++ b/packages/browser/test/unit/transports/xhr.test.ts @@ -1,16 +1,17 @@ import { expect } from 'chai'; import { fakeServer, SinonFakeServer, stub } from 'sinon'; -import { Status, Transports } from '../../../src'; +import { Event, Status, Transports } from '../../../src'; const testDsn = 'https://123@sentry.io/42'; -const transportUrl = 'https://sentry.io/api/42/store/?sentry_key=123&sentry_version=7'; -const payload = { +const storeUrl = 'https://sentry.io/api/42/store/?sentry_key=123&sentry_version=7'; +const envelopeUrl = 'https://sentry.io/api/42/envelope/?sentry_key=123&sentry_version=7'; +const eventPayload: Event = { event_id: '1337', - message: 'Pickle Rick', - user: { - username: 'Morty', - }, +}; +const transactionPayload: Event = { + event_id: '42', + type: 'transaction', }; let server: SinonFakeServer; @@ -29,81 +30,36 @@ describe('XHRTransport', () => { it('inherits composeEndpointUrl() implementation', () => { // eslint-disable-next-line deprecation/deprecation - expect(transport.url).equal(transportUrl); + expect(transport.url).equal(storeUrl); }); describe('sendEvent()', async () => { it('sends a request to Sentry servers', async () => { - server.respondWith('POST', transportUrl, [200, {}, '']); + server.respondWith('POST', storeUrl, [200, {}, '']); - const res = await transport.sendEvent(payload); + const res = await transport.sendEvent(eventPayload); expect(res.status).equal(Status.Success); const request = server.requests[0]; expect(server.requests.length).equal(1); expect(request.method).equal('POST'); - expect(JSON.parse(request.requestBody)).deep.equal(payload); + expect(JSON.parse(request.requestBody)).deep.equal(eventPayload); }); it('rejects with non-200 status code', async () => { - server.respondWith('POST', transportUrl, [403, {}, '']); + server.respondWith('POST', storeUrl, [403, {}, '']); try { - await transport.sendEvent(payload); + await transport.sendEvent(eventPayload); } catch (res) { expect(res.status).equal(403); const request = server.requests[0]; expect(server.requests.length).equal(1); expect(request.method).equal('POST'); - expect(JSON.parse(request.requestBody)).deep.equal(payload); + expect(JSON.parse(request.requestBody)).deep.equal(eventPayload); } }); - it('back-off using Retry-After header', async () => { - const retryAfterSeconds = 10; - server.respondWith('POST', transportUrl, [429, { 'Retry-After': retryAfterSeconds }, '']); - - const now = Date.now(); - const dateStub = stub(Date, 'now') - // Check for first event - .onCall(0) - .returns(now) - // Setting disableUntil - .onCall(1) - .returns(now) - // Check for second event - .onCall(2) - .returns(now + (retryAfterSeconds / 2) * 1000) - // Check for third event - .onCall(3) - .returns(now + retryAfterSeconds * 1000); - - try { - await transport.sendEvent(payload); - } catch (res) { - expect(res.status).equal(429); - expect(res.reason).equal(undefined); - } - - try { - await transport.sendEvent(payload); - } catch (res) { - expect(res.status).equal(429); - expect(res.reason).equal( - `Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`, - ); - } - - try { - await transport.sendEvent(payload); - } catch (res) { - expect(res.status).equal(429); - expect(res.reason).equal(undefined); - } - - dateStub.restore(); - }); - it('passes in headers', async () => { transport = new Transports.XHRTransport({ dsn: testDsn, @@ -112,8 +68,8 @@ describe('XHRTransport', () => { }, }); - server.respondWith('POST', transportUrl, [200, {}, '']); - const res = await transport.sendEvent(payload); + server.respondWith('POST', storeUrl, [200, {}, '']); + const res = await transport.sendEvent(eventPayload); const request = server.requests[0]; expect(res.status).equal(Status.Success); @@ -121,5 +77,305 @@ describe('XHRTransport', () => { const authHeaderLabel = 'Authorization'; expect(requestHeaders[authHeaderLabel]).equal('Basic GVzdDp0ZXN0Cg=='); }); + + describe('Rate-limiting', () => { + it('back-off using Retry-After header', async () => { + const retryAfterSeconds = 10; + const beforeLimit = Date.now(); + const withinLimit = beforeLimit + (retryAfterSeconds / 2) * 1000; + const afterLimit = beforeLimit + retryAfterSeconds * 1000; + + server.respondWith('POST', storeUrl, [429, { 'Retry-After': `${retryAfterSeconds}` }, '']); + + const dateStub = stub(Date, 'now') + // 1st event - _isRateLimited - false + .onCall(0) + .returns(beforeLimit) + // 1st event - _handleRateLimit + .onCall(1) + .returns(beforeLimit) + // 2nd event - _isRateLimited - true + .onCall(2) + .returns(withinLimit) + // 3rd event - _isRateLimited - false + .onCall(3) + .returns(afterLimit) + // 3rd event - _handleRateLimit + .onCall(4) + .returns(afterLimit); + + try { + await transport.sendEvent(eventPayload); + throw new Error('unreachable!'); + } catch (res) { + expect(res.status).equal(429); + expect(res.reason).equal(undefined); + } + + try { + await transport.sendEvent(eventPayload); + throw new Error('unreachable!'); + } catch (res) { + expect(res.status).equal(429); + expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + } + + server.respondWith('POST', storeUrl, [200, {}, '']); + + const eventRes = await transport.sendEvent(eventPayload); + expect(eventRes.status).equal(Status.Success); + + dateStub.restore(); + }); + + it('back-off using X-Sentry-Rate-Limits with single category', async () => { + const retryAfterSeconds = 10; + const beforeLimit = Date.now(); + const withinLimit = beforeLimit + (retryAfterSeconds / 2) * 1000; + const afterLimit = beforeLimit + retryAfterSeconds * 1000; + + server.respondWith('POST', storeUrl, [429, { 'X-Sentry-Rate-Limits': `${retryAfterSeconds}:event:scope` }, '']); + server.respondWith('POST', envelopeUrl, [200, {}, '']); + + const dateStub = stub(Date, 'now') + // 1st event - _isRateLimited - false + .onCall(0) + .returns(beforeLimit) + // 1st event - _handleRateLimit + .onCall(1) + .returns(beforeLimit) + // 2nd event - _isRateLimited - false (different category) + .onCall(2) + .returns(withinLimit) + // 2nd event - _handleRateLimit + .onCall(3) + .returns(withinLimit) + // 3rd event - _isRateLimited - true + .onCall(4) + .returns(withinLimit) + // 4th event - _isRateLimited - false + .onCall(5) + .returns(afterLimit) + // 4th event - _handleRateLimit + .onCall(6) + .returns(afterLimit); + + try { + await transport.sendEvent(eventPayload); + throw new Error('unreachable!'); + } catch (res) { + expect(res.status).equal(429); + expect(res.reason).equal(undefined); + } + + const transactionRes = await transport.sendEvent(transactionPayload); + expect(transactionRes.status).equal(Status.Success); + + try { + await transport.sendEvent(eventPayload); + throw new Error('unreachable!'); + } catch (res) { + expect(res.status).equal(429); + expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + } + + server.respondWith('POST', storeUrl, [200, {}, '']); + + const eventRes = await transport.sendEvent(eventPayload); + expect(eventRes.status).equal(Status.Success); + + dateStub.restore(); + }); + + it('back-off using X-Sentry-Rate-Limits with multiple categories', async () => { + const retryAfterSeconds = 10; + const beforeLimit = Date.now(); + const withinLimit = beforeLimit + (retryAfterSeconds / 2) * 1000; + const afterLimit = beforeLimit + retryAfterSeconds * 1000; + + server.respondWith('POST', storeUrl, [ + 429, + { 'X-Sentry-Rate-Limits': `${retryAfterSeconds}:event;transaction:scope` }, + '', + ]); + server.respondWith('POST', envelopeUrl, [200, {}, '']); + + const dateStub = stub(Date, 'now') + // 1st event - _isRateLimited - false + .onCall(0) + .returns(beforeLimit) + // 1st event - _handleRateLimit + .onCall(1) + .returns(beforeLimit) + // 2nd event - _isRateLimited - true (event category) + .onCall(2) + .returns(withinLimit) + // 3rd event - _isRateLimited - true (transaction category) + .onCall(3) + .returns(withinLimit) + // 4th event - _isRateLimited - false (event category) + .onCall(4) + .returns(afterLimit) + // 4th event - _handleRateLimit + .onCall(5) + .returns(afterLimit) + // 5th event - _isRateLimited - false (transaction category) + .onCall(6) + .returns(afterLimit) + // 5th event - _handleRateLimit + .onCall(7) + .returns(afterLimit); + + try { + await transport.sendEvent(eventPayload); + throw new Error('unreachable!'); + } catch (res) { + expect(res.status).equal(429); + expect(res.reason).equal(undefined); + } + + try { + await transport.sendEvent(eventPayload); + throw new Error('unreachable!'); + } catch (res) { + expect(res.status).equal(429); + expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + } + + try { + await transport.sendEvent(transactionPayload); + throw new Error('unreachable!'); + } catch (res) { + expect(res.status).equal(429); + expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + } + + server.respondWith('POST', storeUrl, [200, {}, '']); + server.respondWith('POST', envelopeUrl, [200, {}, '']); + + const eventRes = await transport.sendEvent(eventPayload); + expect(eventRes.status).equal(Status.Success); + + const transactionRes = await transport.sendEvent(transactionPayload); + expect(transactionRes.status).equal(Status.Success); + + dateStub.restore(); + }); + + it('back-off using X-Sentry-Rate-Limits with missing categories should lock them all', async () => { + const retryAfterSeconds = 10; + const beforeLimit = Date.now(); + const withinLimit = beforeLimit + (retryAfterSeconds / 2) * 1000; + const afterLimit = beforeLimit + retryAfterSeconds * 1000; + + server.respondWith('POST', storeUrl, [429, { 'X-Sentry-Rate-Limits': `${retryAfterSeconds}::scope` }, '']); + server.respondWith('POST', envelopeUrl, [200, {}, '']); + + const dateStub = stub(Date, 'now') + // 1st event - _isRateLimited - false + .onCall(0) + .returns(beforeLimit) + // 1st event - _handleRateLimit + .onCall(1) + .returns(beforeLimit) + // 2nd event - _isRateLimited - true (event category) + .onCall(2) + .returns(withinLimit) + // 3rd event - _isRateLimited - true (transaction category) + .onCall(3) + .returns(withinLimit) + // 4th event - _isRateLimited - false (event category) + .onCall(4) + .returns(afterLimit) + // 4th event - _handleRateLimit + .onCall(5) + .returns(afterLimit) + // 5th event - _isRateLimited - false (transaction category) + .onCall(6) + .returns(afterLimit) + // 5th event - _handleRateLimit + .onCall(7) + .returns(afterLimit); + + try { + await transport.sendEvent(eventPayload); + throw new Error('unreachable!'); + } catch (res) { + expect(res.status).equal(429); + expect(res.reason).equal(undefined); + } + + try { + await transport.sendEvent(eventPayload); + throw new Error('unreachable!'); + } catch (res) { + expect(res.status).equal(429); + expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + } + + try { + await transport.sendEvent(transactionPayload); + throw new Error('unreachable!'); + } catch (res) { + expect(res.status).equal(429); + expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + } + + server.respondWith('POST', storeUrl, [200, {}, '']); + server.respondWith('POST', envelopeUrl, [200, {}, '']); + + const eventRes = await transport.sendEvent(eventPayload); + expect(eventRes.status).equal(Status.Success); + + const transactionRes = await transport.sendEvent(transactionPayload); + expect(transactionRes.status).equal(Status.Success); + + dateStub.restore(); + }); + + it('back-off using X-Sentry-Rate-Limits should also trigger for 200 responses', async () => { + const retryAfterSeconds = 10; + const beforeLimit = Date.now(); + const withinLimit = beforeLimit + (retryAfterSeconds / 2) * 1000; + const afterLimit = beforeLimit + retryAfterSeconds * 1000; + + server.respondWith('POST', storeUrl, [200, { 'X-Sentry-Rate-Limits': `${retryAfterSeconds}:event:scope` }, '']); + + const dateStub = stub(Date, 'now') + // 1st event - _isRateLimited - false + .onCall(0) + .returns(beforeLimit) + // 1st event - _handleRateLimit + .onCall(1) + .returns(beforeLimit) + // 2nd event - _isRateLimited - true + .onCall(2) + .returns(withinLimit) + // 3rd event - _isRateLimited - false + .onCall(3) + .returns(afterLimit) + // 3rd event - _handleRateLimit + .onCall(4) + .returns(afterLimit); + + let eventRes = await transport.sendEvent(eventPayload); + expect(eventRes.status).equal(Status.Success); + + try { + await transport.sendEvent(eventPayload); + throw new Error('unreachable!'); + } catch (res) { + expect(res.status).equal(429); + expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + } + + server.respondWith('POST', storeUrl, [200, {}, '']); + + eventRes = await transport.sendEvent(eventPayload); + expect(eventRes.status).equal(Status.Success); + + dateStub.restore(); + }); + }); }); }); From f6f19f6f135ea6a9af2d8ed43057e054f1447feb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Og=C3=B3rek?= Date: Mon, 12 Oct 2020 11:14:22 +0200 Subject: [PATCH 2/4] Apply suggestions from code review Co-authored-by: Rodolfo Carvalho --- packages/browser/src/transports/base.ts | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/packages/browser/src/transports/base.ts b/packages/browser/src/transports/base.ts index 7eb89fe6897f..a3c430df650c 100644 --- a/packages/browser/src/transports/base.ts +++ b/packages/browser/src/transports/base.ts @@ -15,7 +15,7 @@ export abstract class BaseTransport implements Transport { /** A simple buffer holding all requests. */ protected readonly _buffer: PromiseBuffer = new PromiseBuffer(30); - /** Locks transport after receiving 429 response */ + /** Locks transport after receiving rate limits in a response */ protected readonly _rateLimits: Record = {}; public constructor(public options: TransportOptions) { @@ -46,16 +46,14 @@ export abstract class BaseTransport implements Transport { } /** - * Checks if a category is ratelimited + * Checks if a category is rate limited */ - protected _isRateLimited(category: string): boolean { - // We use `new Date(Date.now())` instead of just `new Date()` despite them being the same thing, - // as it's easier to mock `now` method on `Date` instance instead of whole `Date` object in tests. - return this._disabledUntil(category) > new Date(Date.now()); + protected _isRateLimited(category: string, now = new Date()): boolean { + return this._disabledUntil(category) > now; } /** - * Sets internal _rateLimits from incoming headers + * Sets internal _rateLimits from incoming headers. Returns true if headers contains a non-empty rate limiting header. */ protected _handleRateLimit(headers: Record): boolean { const now = Date.now(); From ade23d78ed03dfb0ac5b26917cd4df4a31f2398d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Og=C3=B3rek?= Date: Mon, 12 Oct 2020 12:05:58 +0200 Subject: [PATCH 3/4] Refactor response handling and add server requests tests --- packages/browser/src/transports/base.ts | 55 +++++++++++++++++-- packages/browser/src/transports/fetch.ts | 33 +---------- packages/browser/src/transports/xhr.ts | 32 ++--------- .../test/unit/transports/fetch.test.ts | 20 +++++++ .../browser/test/unit/transports/xhr.test.ts | 20 +++++++ 5 files changed, 96 insertions(+), 64 deletions(-) diff --git a/packages/browser/src/transports/base.ts b/packages/browser/src/transports/base.ts index a3c430df650c..513353db5498 100644 --- a/packages/browser/src/transports/base.ts +++ b/packages/browser/src/transports/base.ts @@ -1,6 +1,9 @@ import { API } from '@sentry/core'; -import { Event, Response, Transport, TransportOptions } from '@sentry/types'; -import { parseRetryAfterHeader, PromiseBuffer, SentryError } from '@sentry/utils'; +import { Event, Response, Status, Transport, TransportOptions } from '@sentry/types'; +import { logger, parseRetryAfterHeader, PromiseBuffer, SentryError } from '@sentry/utils'; + +const rlHeaderKey = 'x-sentry-rate-limits'; +const raHeaderKey = 'retry-after'; /** Base Transport class implementation */ export abstract class BaseTransport implements Transport { @@ -38,6 +41,46 @@ export abstract class BaseTransport implements Transport { return this._buffer.drain(timeout); } + /** + * Handle Sentry repsonse for promise-based transports. + */ + protected _handleResponse({ + eventType, + response, + resolve, + reject, + }: { + eventType: string; + response: globalThis.Response | XMLHttpRequest; + resolve: (value?: Response | PromiseLike | null | undefined) => void; + reject: (reason?: unknown) => void; + }): void { + const status = Status.fromHttpCode(response.status); + const headers = + response instanceof XMLHttpRequest + ? { + [rlHeaderKey]: response.getResponseHeader(rlHeaderKey), + [raHeaderKey]: response.getResponseHeader(raHeaderKey), + } + : { + [rlHeaderKey]: response.headers.get(rlHeaderKey), + [raHeaderKey]: response.headers.get(raHeaderKey), + }; + /** + * "The name is case-insensitive." + * https://developer.mozilla.org/en-US/docs/Web/API/Headers/get + */ + const limited = this._handleRateLimit(headers); + if (limited) logger.warn(`Too many requests, backing off till: ${this._disabledUntil(eventType)}`); + + if (status === Status.Success) { + resolve({ status }); + return; + } + + reject(response); + } + /** * Gets the time that given category is disabled until for rate limiting */ @@ -48,8 +91,8 @@ export abstract class BaseTransport implements Transport { /** * Checks if a category is rate limited */ - protected _isRateLimited(category: string, now = new Date()): boolean { - return this._disabledUntil(category) > now; + protected _isRateLimited(category: string): boolean { + return this._disabledUntil(category) > new Date(Date.now()); } /** @@ -57,8 +100,8 @@ export abstract class BaseTransport implements Transport { */ protected _handleRateLimit(headers: Record): boolean { const now = Date.now(); - const rlHeader = headers['x-sentry-rate-limits']; - const raHeader = headers['retry-after']; + const rlHeader = headers[rlHeaderKey]; + const raHeader = headers[raHeaderKey]; if (rlHeader) { for (const limit of rlHeader.trim().split(',')) { diff --git a/packages/browser/src/transports/fetch.ts b/packages/browser/src/transports/fetch.ts index dfe2e31788a8..2a7a4e160c2b 100644 --- a/packages/browser/src/transports/fetch.ts +++ b/packages/browser/src/transports/fetch.ts @@ -1,6 +1,6 @@ import { eventToSentryRequest } from '@sentry/core'; -import { Event, Response, Status } from '@sentry/types'; -import { getGlobalObject, logger, supportsReferrerPolicy, SyncPromise } from '@sentry/utils'; +import { Event, Response } from '@sentry/types'; +import { getGlobalObject, supportsReferrerPolicy, SyncPromise } from '@sentry/utils'; import { BaseTransport } from './base'; @@ -23,7 +23,6 @@ export class FetchTransport extends BaseTransport { } const sentryReq = eventToSentryRequest(event, this._api); - const options: RequestInit = { body: sentryReq.body, method: 'POST', @@ -33,11 +32,9 @@ export class FetchTransport extends BaseTransport { // REF: https://github.com/getsentry/raven-js/issues/1233 referrerPolicy: (supportsReferrerPolicy() ? 'origin' : '') as ReferrerPolicy, }; - if (this.options.fetchParameters !== undefined) { Object.assign(options, this.options.fetchParameters); } - if (this.options.headers !== undefined) { options.headers = this.options.headers; } @@ -46,31 +43,7 @@ export class FetchTransport extends BaseTransport { new SyncPromise((resolve, reject) => { global .fetch(sentryReq.url, options) - .then(response => { - const status = Status.fromHttpCode(response.status); - - // Request with 200 that contain `x-sentry-retry-limits` should still handle that header. - if (status === Status.Success || status === Status.RateLimit) { - /** - * "The name is case-insensitive." - * https://developer.mozilla.org/en-US/docs/Web/API/Headers/get - */ - const limited = this._handleRateLimit({ - 'x-sentry-rate-limits': response.headers.get('X-Sentry-Rate-Limits'), - 'retry-after': response.headers.get('Retry-After'), - }); - if (limited) { - logger.warn(`Too many requests, backing off till: ${this._disabledUntil(eventType)}`); - } - } - - if (status === Status.Success) { - resolve({ status }); - return; - } - - reject(response); - }) + .then(response => this._handleResponse({ response, eventType, resolve, reject })) .catch(reject); }), ); diff --git a/packages/browser/src/transports/xhr.ts b/packages/browser/src/transports/xhr.ts index 6b6e634e05df..3570a015809c 100644 --- a/packages/browser/src/transports/xhr.ts +++ b/packages/browser/src/transports/xhr.ts @@ -1,6 +1,6 @@ import { eventToSentryRequest } from '@sentry/core'; -import { Event, Response, Status } from '@sentry/types'; -import { logger, SyncPromise } from '@sentry/utils'; +import { Event, Response } from '@sentry/types'; +import { SyncPromise } from '@sentry/utils'; import { BaseTransport } from './base'; @@ -27,33 +27,9 @@ export class XHRTransport extends BaseTransport { const request = new XMLHttpRequest(); request.onreadystatechange = (): void => { - if (request.readyState !== 4) { - return; + if (request.readyState === 4) { + this._handleResponse({ response: request, eventType, resolve, reject }); } - - const status = Status.fromHttpCode(request.status); - - // Request with 200 that contain `x-sentry-retry-limits` should still handle that header. - if (status === Status.Success || status === Status.RateLimit) { - /** - * "The search for the header name is case-insensitive." - * https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/getResponseHeader - */ - const limited = this._handleRateLimit({ - 'x-sentry-rate-limits': request.getResponseHeader('X-Sentry-Rate-Limits'), - 'retry-after': request.getResponseHeader('Retry-After'), - }); - if (limited) { - logger.warn(`Too many requests, backing off till: ${this._disabledUntil(eventType)}`); - } - } - - if (status === Status.Success) { - resolve({ status }); - return; - } - - reject(request); }; request.open('POST', sentryReq.url); diff --git a/packages/browser/test/unit/transports/fetch.test.ts b/packages/browser/test/unit/transports/fetch.test.ts index 8193ba4d372b..2442e3f13618 100644 --- a/packages/browser/test/unit/transports/fetch.test.ts +++ b/packages/browser/test/unit/transports/fetch.test.ts @@ -166,6 +166,7 @@ describe('FetchTransport', () => { } catch (res) { expect(res.status).equal(429); expect(res.reason).equal(undefined); + expect(fetch.calledOnce).equal(true); } try { @@ -174,12 +175,14 @@ describe('FetchTransport', () => { } catch (res) { expect(res.status).equal(429); expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + expect(fetch.calledOnce).equal(true); } fetch.returns(Promise.resolve({ status: 200, headers: new Headers() })); const eventRes = await transport.sendEvent(eventPayload); expect(eventRes.status).equal(Status.Success); + expect(fetch.calledTwice).equal(true); dateStub.restore(); }); @@ -223,12 +226,14 @@ describe('FetchTransport', () => { } catch (res) { expect(res.status).equal(429); expect(res.reason).equal(undefined); + expect(fetch.calledOnce).equal(true); } fetch.returns(Promise.resolve({ status: 200, headers: new Headers() })); const transactionRes = await transport.sendEvent(transactionPayload); expect(transactionRes.status).equal(Status.Success); + expect(fetch.calledTwice).equal(true); try { await transport.sendEvent(eventPayload); @@ -236,10 +241,12 @@ describe('FetchTransport', () => { } catch (res) { expect(res.status).equal(429); expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + expect(fetch.calledTwice).equal(true); } const eventRes = await transport.sendEvent(eventPayload); expect(eventRes.status).equal(Status.Success); + expect(fetch.calledThrice).equal(true); dateStub.restore(); }); @@ -286,6 +293,7 @@ describe('FetchTransport', () => { } catch (res) { expect(res.status).equal(429); expect(res.reason).equal(undefined); + expect(fetch.calledOnce).equal(true); } try { @@ -294,6 +302,7 @@ describe('FetchTransport', () => { } catch (res) { expect(res.status).equal(429); expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + expect(fetch.calledOnce).equal(true); } try { @@ -302,15 +311,18 @@ describe('FetchTransport', () => { } catch (res) { expect(res.status).equal(429); expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + expect(fetch.calledOnce).equal(true); } fetch.returns(Promise.resolve({ status: 200, headers: new Headers() })); const eventRes = await transport.sendEvent(eventPayload); expect(eventRes.status).equal(Status.Success); + expect(fetch.calledTwice).equal(true); const transactionRes = await transport.sendEvent(transactionPayload); expect(transactionRes.status).equal(Status.Success); + expect(fetch.calledThrice).equal(true); dateStub.restore(); }); @@ -357,6 +369,7 @@ describe('FetchTransport', () => { } catch (res) { expect(res.status).equal(429); expect(res.reason).equal(undefined); + expect(fetch.calledOnce).equal(true); } try { @@ -365,6 +378,7 @@ describe('FetchTransport', () => { } catch (res) { expect(res.status).equal(429); expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + expect(fetch.calledOnce).equal(true); } try { @@ -373,15 +387,18 @@ describe('FetchTransport', () => { } catch (res) { expect(res.status).equal(429); expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + expect(fetch.calledOnce).equal(true); } fetch.returns(Promise.resolve({ status: 200, headers: new Headers() })); const eventRes = await transport.sendEvent(eventPayload); expect(eventRes.status).equal(Status.Success); + expect(fetch.calledTwice).equal(true); const transactionRes = await transport.sendEvent(transactionPayload); expect(transactionRes.status).equal(Status.Success); + expect(fetch.calledThrice).equal(true); dateStub.restore(); }); @@ -415,6 +432,7 @@ describe('FetchTransport', () => { let eventRes = await transport.sendEvent(eventPayload); expect(eventRes.status).equal(Status.Success); + expect(fetch.calledOnce).equal(true); try { await transport.sendEvent(eventPayload); @@ -422,12 +440,14 @@ describe('FetchTransport', () => { } catch (res) { expect(res.status).equal(429); expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + expect(fetch.calledOnce).equal(true); } fetch.returns(Promise.resolve({ status: 200, headers: new Headers() })); eventRes = await transport.sendEvent(eventPayload); expect(eventRes.status).equal(Status.Success); + expect(fetch.calledTwice).equal(true); dateStub.restore(); }); diff --git a/packages/browser/test/unit/transports/xhr.test.ts b/packages/browser/test/unit/transports/xhr.test.ts index 5bf57f76321f..ef52176b9347 100644 --- a/packages/browser/test/unit/transports/xhr.test.ts +++ b/packages/browser/test/unit/transports/xhr.test.ts @@ -110,6 +110,7 @@ describe('XHRTransport', () => { } catch (res) { expect(res.status).equal(429); expect(res.reason).equal(undefined); + expect(server.requests.length).equal(1); } try { @@ -118,12 +119,14 @@ describe('XHRTransport', () => { } catch (res) { expect(res.status).equal(429); expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + expect(server.requests.length).equal(1); } server.respondWith('POST', storeUrl, [200, {}, '']); const eventRes = await transport.sendEvent(eventPayload); expect(eventRes.status).equal(Status.Success); + expect(server.requests.length).equal(2); dateStub.restore(); }); @@ -166,10 +169,12 @@ describe('XHRTransport', () => { } catch (res) { expect(res.status).equal(429); expect(res.reason).equal(undefined); + expect(server.requests.length).equal(1); } const transactionRes = await transport.sendEvent(transactionPayload); expect(transactionRes.status).equal(Status.Success); + expect(server.requests.length).equal(2); try { await transport.sendEvent(eventPayload); @@ -177,12 +182,14 @@ describe('XHRTransport', () => { } catch (res) { expect(res.status).equal(429); expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + expect(server.requests.length).equal(2); } server.respondWith('POST', storeUrl, [200, {}, '']); const eventRes = await transport.sendEvent(eventPayload); expect(eventRes.status).equal(Status.Success); + expect(server.requests.length).equal(3); dateStub.restore(); }); @@ -232,6 +239,7 @@ describe('XHRTransport', () => { } catch (res) { expect(res.status).equal(429); expect(res.reason).equal(undefined); + expect(server.requests.length).equal(1); } try { @@ -240,6 +248,7 @@ describe('XHRTransport', () => { } catch (res) { expect(res.status).equal(429); expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + expect(server.requests.length).equal(1); } try { @@ -248,6 +257,7 @@ describe('XHRTransport', () => { } catch (res) { expect(res.status).equal(429); expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + expect(server.requests.length).equal(1); } server.respondWith('POST', storeUrl, [200, {}, '']); @@ -255,9 +265,11 @@ describe('XHRTransport', () => { const eventRes = await transport.sendEvent(eventPayload); expect(eventRes.status).equal(Status.Success); + expect(server.requests.length).equal(2); const transactionRes = await transport.sendEvent(transactionPayload); expect(transactionRes.status).equal(Status.Success); + expect(server.requests.length).equal(3); dateStub.restore(); }); @@ -303,6 +315,7 @@ describe('XHRTransport', () => { } catch (res) { expect(res.status).equal(429); expect(res.reason).equal(undefined); + expect(server.requests.length).equal(1); } try { @@ -311,6 +324,7 @@ describe('XHRTransport', () => { } catch (res) { expect(res.status).equal(429); expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + expect(server.requests.length).equal(1); } try { @@ -319,6 +333,7 @@ describe('XHRTransport', () => { } catch (res) { expect(res.status).equal(429); expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + expect(server.requests.length).equal(1); } server.respondWith('POST', storeUrl, [200, {}, '']); @@ -326,9 +341,11 @@ describe('XHRTransport', () => { const eventRes = await transport.sendEvent(eventPayload); expect(eventRes.status).equal(Status.Success); + expect(server.requests.length).equal(2); const transactionRes = await transport.sendEvent(transactionPayload); expect(transactionRes.status).equal(Status.Success); + expect(server.requests.length).equal(3); dateStub.restore(); }); @@ -360,6 +377,7 @@ describe('XHRTransport', () => { let eventRes = await transport.sendEvent(eventPayload); expect(eventRes.status).equal(Status.Success); + expect(server.requests.length).equal(1); try { await transport.sendEvent(eventPayload); @@ -367,12 +385,14 @@ describe('XHRTransport', () => { } catch (res) { expect(res.status).equal(429); expect(res.reason).equal(`Transport locked till ${new Date(afterLimit)} due to too many requests.`); + expect(server.requests.length).equal(1); } server.respondWith('POST', storeUrl, [200, {}, '']); eventRes = await transport.sendEvent(eventPayload); expect(eventRes.status).equal(Status.Success); + expect(server.requests.length).equal(2); dateStub.restore(); }); From e2b197e1a0713b903ee61ea2b29d06f754e20674 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Og=C3=B3rek?= Date: Mon, 12 Oct 2020 12:29:36 +0200 Subject: [PATCH 4/4] Dont extract headers in base --- packages/browser/src/transports/base.ts | 19 ++++--------------- packages/browser/src/transports/fetch.ts | 8 +++++++- packages/browser/src/transports/xhr.ts | 6 +++++- 3 files changed, 16 insertions(+), 17 deletions(-) diff --git a/packages/browser/src/transports/base.ts b/packages/browser/src/transports/base.ts index 513353db5498..2fb865a5e53e 100644 --- a/packages/browser/src/transports/base.ts +++ b/packages/browser/src/transports/base.ts @@ -2,9 +2,6 @@ import { API } from '@sentry/core'; import { Event, Response, Status, Transport, TransportOptions } from '@sentry/types'; import { logger, parseRetryAfterHeader, PromiseBuffer, SentryError } from '@sentry/utils'; -const rlHeaderKey = 'x-sentry-rate-limits'; -const raHeaderKey = 'retry-after'; - /** Base Transport class implementation */ export abstract class BaseTransport implements Transport { /** @@ -47,25 +44,17 @@ export abstract class BaseTransport implements Transport { protected _handleResponse({ eventType, response, + headers, resolve, reject, }: { eventType: string; response: globalThis.Response | XMLHttpRequest; + headers: Record; resolve: (value?: Response | PromiseLike | null | undefined) => void; reject: (reason?: unknown) => void; }): void { const status = Status.fromHttpCode(response.status); - const headers = - response instanceof XMLHttpRequest - ? { - [rlHeaderKey]: response.getResponseHeader(rlHeaderKey), - [raHeaderKey]: response.getResponseHeader(raHeaderKey), - } - : { - [rlHeaderKey]: response.headers.get(rlHeaderKey), - [raHeaderKey]: response.headers.get(raHeaderKey), - }; /** * "The name is case-insensitive." * https://developer.mozilla.org/en-US/docs/Web/API/Headers/get @@ -100,8 +89,8 @@ export abstract class BaseTransport implements Transport { */ protected _handleRateLimit(headers: Record): boolean { const now = Date.now(); - const rlHeader = headers[rlHeaderKey]; - const raHeader = headers[raHeaderKey]; + const rlHeader = headers['x-sentry-rate-limits']; + const raHeader = headers['retry-after']; if (rlHeader) { for (const limit of rlHeader.trim().split(',')) { diff --git a/packages/browser/src/transports/fetch.ts b/packages/browser/src/transports/fetch.ts index 2a7a4e160c2b..1ac9ac0b86d2 100644 --- a/packages/browser/src/transports/fetch.ts +++ b/packages/browser/src/transports/fetch.ts @@ -43,7 +43,13 @@ export class FetchTransport extends BaseTransport { new SyncPromise((resolve, reject) => { global .fetch(sentryReq.url, options) - .then(response => this._handleResponse({ response, eventType, resolve, reject })) + .then(response => { + const headers = { + 'x-sentry-rate-limits': response.headers.get('X-Sentry-Rate-Limits'), + 'retry-after': response.headers.get('Retry-After'), + }; + this._handleResponse({ eventType, response, headers, resolve, reject }); + }) .catch(reject); }), ); diff --git a/packages/browser/src/transports/xhr.ts b/packages/browser/src/transports/xhr.ts index 3570a015809c..e770918e4678 100644 --- a/packages/browser/src/transports/xhr.ts +++ b/packages/browser/src/transports/xhr.ts @@ -28,7 +28,11 @@ export class XHRTransport extends BaseTransport { request.onreadystatechange = (): void => { if (request.readyState === 4) { - this._handleResponse({ response: request, eventType, resolve, reject }); + const headers = { + 'x-sentry-rate-limits': request.getResponseHeader('X-Sentry-Rate-Limits'), + 'retry-after': request.getResponseHeader('Retry-After'), + }; + this._handleResponse({ eventType, response: request, headers, resolve, reject }); } };