From c9978cd83223917f3446d5883f1aee952c4b53d4 Mon Sep 17 00:00:00 2001 From: Ahmed Etefy Date: Tue, 20 Apr 2021 14:40:14 +0200 Subject: [PATCH 1/2] feat(node): Implement category based rate limiting Add Category rate limiting based on SentryRequestType --- packages/node/src/transports/base.ts | 135 +++++-- packages/node/src/transports/http.ts | 4 +- packages/node/src/transports/https.ts | 4 +- packages/node/test/transports/http.test.ts | 412 +++++++++++++++++++- packages/node/test/transports/https.test.ts | 7 +- 5 files changed, 528 insertions(+), 34 deletions(-) diff --git a/packages/node/src/transports/base.ts b/packages/node/src/transports/base.ts index e8522ef0304d..f684816c587b 100644 --- a/packages/node/src/transports/base.ts +++ b/packages/node/src/transports/base.ts @@ -1,5 +1,15 @@ import { API, SDK_VERSION } from '@sentry/core'; -import { DsnProtocol, Event, Response, SentryRequest, Status, Transport, TransportOptions } from '@sentry/types'; +import { + DsnProtocol, + Event, + Response, + SentryRequest, + SentryRequestType, + Session, + Status, + Transport, + TransportOptions, +} from '@sentry/types'; import { logger, parseRetryAfterHeader, PromiseBuffer, SentryError } from '@sentry/utils'; import * as fs from 'fs'; import * as http from 'http'; @@ -34,6 +44,14 @@ export interface HTTPModule { // ): http.ClientRequest; } +const CATEGORY_MAPPING: { + [key in SentryRequestType]: string; +} = { + event: 'error', + transaction: 'transaction', + session: 'session', +}; + /** Base Transport class implementation */ export abstract class BaseTransport implements Transport { /** The Agent used for corresponding transport */ @@ -48,8 +66,8 @@ 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 */ - private _disabledUntil: Date = new Date(Date.now()); + /** Locks transport after receiving rate limits in a response */ + protected readonly _rateLimits: Record = {}; /** Create instance and set this.dsn */ public constructor(public options: TransportOptions) { @@ -123,13 +141,74 @@ export abstract class BaseTransport implements Transport { }; } + /** + * Gets the time that given category is disabled until for rate limiting + */ + protected _disabledUntil(requestType: SentryRequestType): Date { + const category = CATEGORY_MAPPING[requestType]; + return this._rateLimits[category] || this._rateLimits.all; + } + + /** + * Checks if a category is rate limited + */ + 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): 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 + //
,
,.. + // where each
is of the form + // : : : + // where + // is a delay in ms + // is the event type(s) (error, transaction, etc) being rate limited and is of the form + // ;;... + // is what's being limited (org, project, or key) - ignored by SDK + // 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] && parameters[1].split(';')) || ['all']) { + // categoriesAllowed is added here to ensure we are only storing rate limits for categories we support in this + // sdk and any categories that are not supported will not be added redundantly to the rateLimits object + const categoriesAllowed = [ + ...(Object.keys(CATEGORY_MAPPING) as [SentryRequestType]).map(k => CATEGORY_MAPPING[k]), + 'all', + ]; + if (categoriesAllowed.includes(category)) this._rateLimits[category] = new Date(now + delay); + } + } + return true; + } else if (raHeader) { + this._rateLimits.all = new Date(now + parseRetryAfterHeader(now, raHeader)); + return true; + } + return false; + } + /** JSDoc */ - protected async _send(sentryReq: SentryRequest): Promise { + protected async _send(sentryReq: SentryRequest, originalPayload?: Event | Session): Promise { if (!this.module) { throw new SentryError('No module available'); } - if (new Date(Date.now()) < this._disabledUntil) { - return Promise.reject(new SentryError(`Transport locked till ${this._disabledUntil} due to too many requests.`)); + if (originalPayload && this._isRateLimited(sentryReq.type)) { + return Promise.reject({ + event: originalPayload, + type: sentryReq.type, + reason: `Transport locked till ${this._disabledUntil(sentryReq.type)} due to too many requests.`, + status: 429, + }); } if (!this._buffer.isReady()) { @@ -147,29 +226,35 @@ export abstract class BaseTransport implements Transport { res.setEncoding('utf8'); + /** + * "Key-value pairs of header names and values. Header names are lower-cased." + * https://nodejs.org/api/http.html#http_message_headers + */ + let retryAfterHeader = res.headers ? res.headers['retry-after'] : ''; + retryAfterHeader = (Array.isArray(retryAfterHeader) ? retryAfterHeader[0] : retryAfterHeader) as string; + + let rlHeader = res.headers ? res.headers['x-sentry-rate-limits'] : ''; + rlHeader = (Array.isArray(rlHeader) ? rlHeader[0] : rlHeader) as string; + + const headers = { + 'x-sentry-rate-limits': rlHeader, + 'retry-after': retryAfterHeader, + }; + + const limited = this._handleRateLimit(headers); + if (limited) logger.warn(`Too many requests, backing off until: ${this._disabledUntil(sentryReq.type)}`); + + let rejectionMessage = `HTTP Error (${statusCode})`; + if (res.headers && res.headers['x-sentry-error']) { + rejectionMessage += `: ${res.headers['x-sentry-error']}`; + } + if (status === Status.Success) { resolve({ status }); - } else { - if (status === Status.RateLimit) { - const now = Date.now(); - /** - * "Key-value pairs of header names and values. Header names are lower-cased." - * https://nodejs.org/api/http.html#http_message_headers - */ - let retryAfterHeader = res.headers ? res.headers['retry-after'] : ''; - retryAfterHeader = (Array.isArray(retryAfterHeader) ? retryAfterHeader[0] : retryAfterHeader) as string; - this._disabledUntil = new Date(now + parseRetryAfterHeader(now, retryAfterHeader)); - logger.warn(`Too many requests, backing off till: ${this._disabledUntil}`); - } - - let rejectionMessage = `HTTP Error (${statusCode})`; - if (res.headers && res.headers['x-sentry-error']) { - rejectionMessage += `: ${res.headers['x-sentry-error']}`; - } - - reject(new SentryError(rejectionMessage)); } + reject(new SentryError(rejectionMessage)); + // Force the socket to drain res.on('data', () => { // Drain diff --git a/packages/node/src/transports/http.ts b/packages/node/src/transports/http.ts index f7760cdeb827..c4de15073f7a 100644 --- a/packages/node/src/transports/http.ts +++ b/packages/node/src/transports/http.ts @@ -20,13 +20,13 @@ export class HTTPTransport extends BaseTransport { * @inheritDoc */ public sendEvent(event: Event): Promise { - return this._send(eventToSentryRequest(event, this._api)); + return this._send(eventToSentryRequest(event, this._api), event); } /** * @inheritDoc */ public sendSession(session: Session): PromiseLike { - return this._send(sessionToSentryRequest(session, this._api)); + return this._send(sessionToSentryRequest(session, this._api), session); } } diff --git a/packages/node/src/transports/https.ts b/packages/node/src/transports/https.ts index 982bf73d3ae6..96499504a2fa 100644 --- a/packages/node/src/transports/https.ts +++ b/packages/node/src/transports/https.ts @@ -20,13 +20,13 @@ export class HTTPSTransport extends BaseTransport { * @inheritDoc */ public sendEvent(event: Event): Promise { - return this._send(eventToSentryRequest(event, this._api)); + return this._send(eventToSentryRequest(event, this._api), event); } /** * @inheritDoc */ public sendSession(session: Session): PromiseLike { - return this._send(sessionToSentryRequest(session, this._api)); + return this._send(sessionToSentryRequest(session, this._api), session); } } diff --git a/packages/node/test/transports/http.test.ts b/packages/node/test/transports/http.test.ts index 5721d9498048..fd1f20a30cbb 100644 --- a/packages/node/test/transports/http.test.ts +++ b/packages/node/test/transports/http.test.ts @@ -1,5 +1,5 @@ import { Session } from '@sentry/hub'; -import { TransportOptions } from '@sentry/types'; +import { Event, SessionStatus, Status, TransportOptions } from '@sentry/types'; import { SentryError } from '@sentry/utils'; import * as http from 'http'; import * as HttpsProxyAgent from 'https-proxy-agent'; @@ -10,6 +10,27 @@ const mockSetEncoding = jest.fn(); const dsn = 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622'; const storePath = '/mysubpath/api/50622/store/'; const envelopePath = '/mysubpath/api/50622/envelope/'; +const eventPayload: Event = { + event_id: '1337', +}; +const transactionPayload: Event = { + event_id: '42', + type: 'transaction', +}; +const sessionPayload: Session = { + environment: 'test', + release: '1.0', + sid: '353463243253453254', + errors: 0, + started: Date.now(), + timestamp: Date.now(), + init: true, + duration: 0, + status: SessionStatus.Exited, + update: jest.fn(), + close: jest.fn(), + toJSON: jest.fn(), +}; let mockReturnCode = 200; let mockHeaders = {}; @@ -141,9 +162,12 @@ describe('HTTPTransport', () => { try { await transport.sendEvent({ message: 'test' }); } catch (e) { - expect(e).toEqual( - new SentryError(`Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`), + expect(e.status).toEqual(429); + expect(e.reason).toEqual( + `Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`, ); + expect(e.event.message).toEqual('test'); + expect(e.type).toEqual('event'); } try { @@ -155,6 +179,388 @@ describe('HTTPTransport', () => { mock.mockRestore(); }); + test('back-off using x-sentry-rate-limits with bogus headers and missing categories should just lock them all', async () => { + const retryAfterSeconds = 60; + mockReturnCode = 429; + mockHeaders = { + 'x-sentry-rate-limits': `sgthrthewhertht`, + }; + const transport = createTransport({ dsn }); + const now = Date.now(); + const mock = jest + .spyOn(Date, 'now') + // 1st event - _isRateLimited - false + .mockReturnValueOnce(now) + // 1st event - _handleRateLimit + .mockReturnValueOnce(now) + // 2nd event - _isRateLimited - true (event category) + .mockReturnValueOnce(now + (retryAfterSeconds / 2) * 1000) + // 3rd event - _isRateLimited - true (transaction category) + .mockReturnValueOnce(now + (retryAfterSeconds / 2) * 1000) + // 4th event - _isRateLimited - false (event category) + .mockReturnValueOnce(now + retryAfterSeconds * 1000) + // 4th event - _handleRateLimit + .mockReturnValueOnce(now + retryAfterSeconds * 1000) + // 5th event - _isRateLimited - false (transaction category) + .mockReturnValueOnce(now + retryAfterSeconds * 1000) + // 5th event - _handleRateLimit + .mockReturnValueOnce(now + retryAfterSeconds * 1000); + + try { + await transport.sendEvent(eventPayload); + } catch (e) { + expect(e).toEqual(new SentryError(`HTTP Error (${mockReturnCode})`)); + } + + try { + await transport.sendEvent(eventPayload); + } catch (e) { + expect(e.status).toEqual(429); + expect(e.reason).toEqual( + `Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`, + ); + expect(e.event).toEqual(eventPayload); + expect(e.type).toEqual('event'); + } + + try { + await transport.sendEvent(transactionPayload); + } catch (e) { + expect(e.status).toEqual(429); + expect(e.reason).toEqual( + `Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`, + ); + expect(e.event).toEqual(transactionPayload); + expect(e.type).toEqual('transaction'); + } + + mockHeaders = {}; + mockReturnCode = 200; + + const eventRes = await transport.sendEvent(eventPayload); + expect(eventRes.status).toEqual(Status.Success); + + const transactionRes = await transport.sendEvent(transactionPayload); + expect(transactionRes.status).toEqual(Status.Success); + + mock.mockRestore(); + }); + + test('back-off using x-sentry-rate-limits with single category', async () => { + const retryAfterSeconds = 10; + mockReturnCode = 429; + mockHeaders = { + 'x-sentry-rate-limits': `${retryAfterSeconds}:error:scope`, + }; + const transport = createTransport({ dsn }); + const now = Date.now(); + const mock = jest + .spyOn(Date, 'now') + // 1st event - _isRateLimited - false + .mockReturnValueOnce(now) + // 1st event - _handleRateLimit + .mockReturnValueOnce(now) + // 2nd event - _isRateLimited - false (different category) + .mockReturnValueOnce(now + (retryAfterSeconds / 2) * 1000) + // 2nd event - _handleRateLimit + .mockReturnValueOnce(now + (retryAfterSeconds / 2) * 1000) + // 3rd event - _isRateLimited - false (different category - sessions) + .mockReturnValueOnce(now + (retryAfterSeconds / 2) * 1000) + // 3rd event - _handleRateLimit + .mockReturnValueOnce(now + (retryAfterSeconds / 2) * 1000) + // 4th event - _isRateLimited - true + .mockReturnValueOnce(now + (retryAfterSeconds / 2) * 1000) + // 5th event - _isRateLimited - false + .mockReturnValueOnce(now + retryAfterSeconds * 1000) + // 5th event - _handleRateLimit + .mockReturnValueOnce(now + retryAfterSeconds * 1000); + + try { + await transport.sendEvent(eventPayload); + } catch (e) { + expect(e).toEqual(new SentryError(`HTTP Error (${mockReturnCode})`)); + } + + mockHeaders = {}; + mockReturnCode = 200; + + const transactionRes = await transport.sendEvent(transactionPayload); + expect(transactionRes.status).toEqual(Status.Success); + + const sessionsRes = await transport.sendSession(sessionPayload); + expect(sessionsRes.status).toEqual(Status.Success); + + try { + await transport.sendEvent(eventPayload); + } catch (e) { + expect(e.status).toEqual(429); + expect(e.reason).toEqual( + `Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`, + ); + expect(e.event).toEqual(eventPayload); + expect(e.type).toEqual('event'); + } + + const eventRes = await transport.sendEvent(eventPayload); + expect(eventRes.status).toEqual(Status.Success); + + mock.mockRestore(); + }); + + test('back-off using x-sentry-rate-limits with multiple category', async () => { + const retryAfterSeconds = 10; + mockReturnCode = 429; + mockHeaders = { + 'x-sentry-rate-limits': `${retryAfterSeconds}:error;transaction;session:scope`, + }; + const transport = createTransport({ dsn }); + const now = Date.now(); + const mock = jest + .spyOn(Date, 'now') + // 1st event - _isRateLimited - false + .mockReturnValueOnce(now) + // 1st event - _handleRateLimit + .mockReturnValueOnce(now) + // 2nd event - _isRateLimited - true (event category) + .mockReturnValueOnce(now + (retryAfterSeconds / 2) * 1000) + // 3rd event - _isRateLimited - true (sessions category) + .mockReturnValueOnce(now + (retryAfterSeconds / 2) * 1000) + // 4th event - _isRateLimited - true (transactions category) + .mockReturnValueOnce(now + (retryAfterSeconds / 2) * 1000) + // 5th event - _isRateLimited - false (event category) + .mockReturnValueOnce(now + retryAfterSeconds * 1000) + // 5th event - _handleRateLimit + .mockReturnValueOnce(now + retryAfterSeconds * 1000) + // 6th event - _isRateLimited - false (sessions category) + .mockReturnValueOnce(now + retryAfterSeconds * 1000) + // 6th event - handleRateLimit + .mockReturnValueOnce(now + retryAfterSeconds * 1000) + // 7th event - _isRateLimited - false (transaction category) + .mockReturnValueOnce(now + retryAfterSeconds * 1000) + // 7th event - handleRateLimit + .mockReturnValueOnce(now + retryAfterSeconds * 1000); + + try { + await transport.sendEvent(eventPayload); + } catch (e) { + expect(e).toEqual(new SentryError(`HTTP Error (${mockReturnCode})`)); + } + + try { + await transport.sendEvent(eventPayload); + } catch (e) { + expect(e.status).toEqual(429); + expect(e.reason).toEqual( + `Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`, + ); + expect(e.event).toEqual(eventPayload); + expect(e.type).toEqual('event'); + } + + try { + await transport.sendSession(sessionPayload); + } catch (e) { + expect(e.status).toEqual(429); + expect(e.reason).toEqual( + `Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`, + ); + expect(e.event.environment).toEqual(sessionPayload.environment); + expect(e.event.release).toEqual(sessionPayload.release); + expect(e.event.sid).toEqual(sessionPayload.sid); + expect(e.type).toEqual('session'); + } + + try { + await transport.sendEvent(transactionPayload); + } catch (e) { + expect(e.status).toEqual(429); + expect(e.reason).toEqual( + `Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`, + ); + expect(e.event).toEqual(transactionPayload); + expect(e.type).toEqual('transaction'); + } + + mockHeaders = {}; + mockReturnCode = 200; + + const eventRes = await transport.sendEvent(eventPayload); + expect(eventRes.status).toEqual(Status.Success); + + const sessionsRes = await transport.sendSession(sessionPayload); + expect(sessionsRes.status).toEqual(Status.Success); + + const transactionRes = await transport.sendEvent(transactionPayload); + expect(transactionRes.status).toEqual(Status.Success); + + mock.mockRestore(); + }); + + test('back-off using x-sentry-rate-limits with missing categories should lock them all', async () => { + const retryAfterSeconds = 10; + mockReturnCode = 429; + mockHeaders = { + 'x-sentry-rate-limits': `${retryAfterSeconds}::scope`, + }; + const transport = createTransport({ dsn }); + const now = Date.now(); + const mock = jest + .spyOn(Date, 'now') + // 1st event - _isRateLimited - false + .mockReturnValueOnce(now) + // 1st event - _handleRateLimit + .mockReturnValueOnce(now) + // 2nd event - _isRateLimited - true (event category) + .mockReturnValueOnce(now + (retryAfterSeconds / 2) * 1000) + // 3rd event - _isRateLimited - true (transaction category) + .mockReturnValueOnce(now + (retryAfterSeconds / 2) * 1000) + // 4th event - _isRateLimited - false (event category) + .mockReturnValueOnce(now + retryAfterSeconds * 1000) + // 4th event - _handleRateLimit + .mockReturnValueOnce(now + retryAfterSeconds * 1000) + // 5th event - _isRateLimited - false (transaction category) + .mockReturnValueOnce(now + retryAfterSeconds * 1000) + // 5th event - _handleRateLimit + .mockReturnValueOnce(now + retryAfterSeconds * 1000); + + try { + await transport.sendEvent(eventPayload); + } catch (e) { + expect(e).toEqual(new SentryError(`HTTP Error (${mockReturnCode})`)); + } + + try { + await transport.sendEvent(eventPayload); + } catch (e) { + expect(e.status).toEqual(429); + expect(e.reason).toEqual( + `Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`, + ); + expect(e.event).toEqual(eventPayload); + expect(e.type).toEqual('event'); + } + + try { + await transport.sendEvent(transactionPayload); + } catch (e) { + expect(e.status).toEqual(429); + expect(e.reason).toEqual( + `Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`, + ); + expect(e.event).toEqual(transactionPayload); + expect(e.type).toEqual('transaction'); + } + + mockHeaders = {}; + mockReturnCode = 200; + + const eventRes = await transport.sendEvent(eventPayload); + expect(eventRes.status).toEqual(Status.Success); + + const transactionRes = await transport.sendEvent(transactionPayload); + expect(transactionRes.status).toEqual(Status.Success); + + mock.mockRestore(); + }); + + test('back-off using x-sentry-rate-limits with bogus categories should be dropped', async () => { + const retryAfterSeconds = 10; + mockReturnCode = 429; + mockHeaders = { + 'x-sentry-rate-limits': `${retryAfterSeconds}:error;safegreg;eqwerw:scope`, + }; + const transport = createTransport({ dsn }); + const now = Date.now(); + const mock = jest + .spyOn(Date, 'now') + // 1st event - _isRateLimited - false + .mockReturnValueOnce(now) + // 1st event - _handleRateLimit + .mockReturnValueOnce(now) + // 2nd event - _isRateLimited - true (event category) + .mockReturnValueOnce(now + (retryAfterSeconds / 2) * 1000) + // 3rd event - _isRateLimited - false (transaction category) + .mockReturnValueOnce(now + (retryAfterSeconds / 2) * 1000) + // 3rd Event - _handleRateLimit + .mockReturnValueOnce(now + (retryAfterSeconds / 2) * 1000) + // 4th event - _isRateLimited - false (event category) + .mockReturnValueOnce(now + retryAfterSeconds * 1000) + // 4th event - _handleRateLimit + .mockReturnValueOnce(now + retryAfterSeconds * 1000); + + try { + await transport.sendEvent(eventPayload); + } catch (e) { + expect(e).toEqual(new SentryError(`HTTP Error (${mockReturnCode})`)); + } + + try { + await transport.sendEvent(eventPayload); + } catch (e) { + expect(e.status).toEqual(429); + expect(e.reason).toEqual( + `Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`, + ); + expect(e.event).toEqual(eventPayload); + expect(e.type).toEqual('event'); + } + + mockHeaders = {}; + mockReturnCode = 200; + + const transactionRes = await transport.sendEvent(transactionPayload); + expect(transactionRes.status).toEqual(Status.Success); + + const eventRes = await transport.sendEvent(eventPayload); + expect(eventRes.status).toEqual(Status.Success); + + mock.mockRestore(); + }); + + test('back-off using x-sentry-rate-limits should also trigger for 200 responses', async () => { + const retryAfterSeconds = 10; + mockReturnCode = 200; + mockHeaders = { + 'x-sentry-rate-limits': `${retryAfterSeconds}:error;transaction:scope`, + }; + const transport = createTransport({ dsn }); + const now = Date.now(); + const mock = jest + .spyOn(Date, 'now') + // 1st event - _isRateLimited - false + .mockReturnValueOnce(now) + // 1st event - _handleRateLimit + .mockReturnValueOnce(now) + // 2nd event - _isRateLimited - true + .mockReturnValueOnce(now + (retryAfterSeconds / 2) * 1000) + // 3rd event - _isRateLimited - false + .mockReturnValueOnce(now + retryAfterSeconds * 1000) + // 3rd event - _handleRateLimit + .mockReturnValueOnce(now + retryAfterSeconds * 1000); + + let eventRes = await transport.sendEvent(eventPayload); + expect(eventRes.status).toEqual(Status.Success); + + try { + await transport.sendEvent(eventPayload); + } catch (e) { + expect(e.status).toEqual(429); + expect(e.reason).toEqual( + `Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`, + ); + expect(e.event).toEqual(eventPayload); + expect(e.type).toEqual('event'); + } + + mockReturnCode = 200; + mockHeaders = {}; + + eventRes = await transport.sendEvent(eventPayload); + expect(eventRes.status).toEqual(Status.Success); + + mock.mockRestore(); + }); + test('transport options', async () => { mockReturnCode = 200; const transport = createTransport({ diff --git a/packages/node/test/transports/https.test.ts b/packages/node/test/transports/https.test.ts index bd413ff99536..5e8bdec975e2 100644 --- a/packages/node/test/transports/https.test.ts +++ b/packages/node/test/transports/https.test.ts @@ -147,9 +147,12 @@ describe('HTTPSTransport', () => { try { await transport.sendEvent({ message: 'test' }); } catch (e) { - expect(e).toEqual( - new SentryError(`Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`), + expect(e.status).toEqual(429); + expect(e.reason).toEqual( + `Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`, ); + expect(e.event.message).toEqual('test'); + expect(e.type).toEqual('event'); } try { From b05c8ae5b2537d98369018d6567820aaebb7d954 Mon Sep 17 00:00:00 2001 From: Daniel Griesser Date: Thu, 6 May 2021 16:20:00 +0200 Subject: [PATCH 2/2] ref: CodeReview --- packages/node/src/transports/base.ts | 15 ++++++------ packages/node/test/transports/http.test.ts | 26 ++++++++++----------- packages/node/test/transports/https.test.ts | 2 +- 3 files changed, 21 insertions(+), 22 deletions(-) diff --git a/packages/node/src/transports/base.ts b/packages/node/src/transports/base.ts index f684816c587b..688d0511e6cb 100644 --- a/packages/node/src/transports/base.ts +++ b/packages/node/src/transports/base.ts @@ -204,7 +204,7 @@ export abstract class BaseTransport implements Transport { } if (originalPayload && this._isRateLimited(sentryReq.type)) { return Promise.reject({ - event: originalPayload, + payload: originalPayload, type: sentryReq.type, reason: `Transport locked till ${this._disabledUntil(sentryReq.type)} due to too many requests.`, status: 429, @@ -244,17 +244,16 @@ export abstract class BaseTransport implements Transport { const limited = this._handleRateLimit(headers); if (limited) logger.warn(`Too many requests, backing off until: ${this._disabledUntil(sentryReq.type)}`); - let rejectionMessage = `HTTP Error (${statusCode})`; - if (res.headers && res.headers['x-sentry-error']) { - rejectionMessage += `: ${res.headers['x-sentry-error']}`; - } - if (status === Status.Success) { resolve({ status }); + } else { + let rejectionMessage = `HTTP Error (${statusCode})`; + if (res.headers && res.headers['x-sentry-error']) { + rejectionMessage += `: ${res.headers['x-sentry-error']}`; + } + reject(new SentryError(rejectionMessage)); } - reject(new SentryError(rejectionMessage)); - // Force the socket to drain res.on('data', () => { // Drain diff --git a/packages/node/test/transports/http.test.ts b/packages/node/test/transports/http.test.ts index fd1f20a30cbb..5ffd077c9790 100644 --- a/packages/node/test/transports/http.test.ts +++ b/packages/node/test/transports/http.test.ts @@ -166,7 +166,7 @@ describe('HTTPTransport', () => { expect(e.reason).toEqual( `Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`, ); - expect(e.event.message).toEqual('test'); + expect(e.payload.message).toEqual('test'); expect(e.type).toEqual('event'); } @@ -219,7 +219,7 @@ describe('HTTPTransport', () => { expect(e.reason).toEqual( `Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`, ); - expect(e.event).toEqual(eventPayload); + expect(e.payload).toEqual(eventPayload); expect(e.type).toEqual('event'); } @@ -230,7 +230,7 @@ describe('HTTPTransport', () => { expect(e.reason).toEqual( `Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`, ); - expect(e.event).toEqual(transactionPayload); + expect(e.payload).toEqual(transactionPayload); expect(e.type).toEqual('transaction'); } @@ -297,7 +297,7 @@ describe('HTTPTransport', () => { expect(e.reason).toEqual( `Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`, ); - expect(e.event).toEqual(eventPayload); + expect(e.payload).toEqual(eventPayload); expect(e.type).toEqual('event'); } @@ -353,7 +353,7 @@ describe('HTTPTransport', () => { expect(e.reason).toEqual( `Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`, ); - expect(e.event).toEqual(eventPayload); + expect(e.payload).toEqual(eventPayload); expect(e.type).toEqual('event'); } @@ -364,9 +364,9 @@ describe('HTTPTransport', () => { expect(e.reason).toEqual( `Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`, ); - expect(e.event.environment).toEqual(sessionPayload.environment); - expect(e.event.release).toEqual(sessionPayload.release); - expect(e.event.sid).toEqual(sessionPayload.sid); + expect(e.payload.environment).toEqual(sessionPayload.environment); + expect(e.payload.release).toEqual(sessionPayload.release); + expect(e.payload.sid).toEqual(sessionPayload.sid); expect(e.type).toEqual('session'); } @@ -377,7 +377,7 @@ describe('HTTPTransport', () => { expect(e.reason).toEqual( `Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`, ); - expect(e.event).toEqual(transactionPayload); + expect(e.payload).toEqual(transactionPayload); expect(e.type).toEqual('transaction'); } @@ -436,7 +436,7 @@ describe('HTTPTransport', () => { expect(e.reason).toEqual( `Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`, ); - expect(e.event).toEqual(eventPayload); + expect(e.payload).toEqual(eventPayload); expect(e.type).toEqual('event'); } @@ -447,7 +447,7 @@ describe('HTTPTransport', () => { expect(e.reason).toEqual( `Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`, ); - expect(e.event).toEqual(transactionPayload); + expect(e.payload).toEqual(transactionPayload); expect(e.type).toEqual('transaction'); } @@ -501,7 +501,7 @@ describe('HTTPTransport', () => { expect(e.reason).toEqual( `Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`, ); - expect(e.event).toEqual(eventPayload); + expect(e.payload).toEqual(eventPayload); expect(e.type).toEqual('event'); } @@ -548,7 +548,7 @@ describe('HTTPTransport', () => { expect(e.reason).toEqual( `Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`, ); - expect(e.event).toEqual(eventPayload); + expect(e.payload).toEqual(eventPayload); expect(e.type).toEqual('event'); } diff --git a/packages/node/test/transports/https.test.ts b/packages/node/test/transports/https.test.ts index 5e8bdec975e2..eb3bd8679ab2 100644 --- a/packages/node/test/transports/https.test.ts +++ b/packages/node/test/transports/https.test.ts @@ -151,7 +151,7 @@ describe('HTTPSTransport', () => { expect(e.reason).toEqual( `Transport locked till ${new Date(now + retryAfterSeconds * 1000)} due to too many requests.`, ); - expect(e.event.message).toEqual('test'); + expect(e.payload.message).toEqual('test'); expect(e.type).toEqual('event'); }