Skip to content

Commit 6d2e788

Browse files
Luca ForstnerAbhiPrasad
Luca Forstner
authored and
AbhiPrasad
committed
fix(utils): Consider 429 responses in transports (#5062)
1 parent b8d41c8 commit 6d2e788

File tree

9 files changed

+70
-40
lines changed

9 files changed

+70
-40
lines changed

‎packages/browser/src/transports/fetch.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ export function makeFetchTransport(
2121
};
2222

2323
return nativeFetch(options.url, requestOptions).then(response => ({
24+
statusCode: response.status,
2425
headers: {
2526
'x-sentry-rate-limits': response.headers.get('X-Sentry-Rate-Limits'),
2627
'retry-after': response.headers.get('Retry-After'),

‎packages/browser/src/transports/xhr.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,13 @@ export function makeXHRTransport(options: BrowserTransportOptions): Transport {
2626

2727
xhr.onreadystatechange = (): void => {
2828
if (xhr.readyState === XHR_READYSTATE_DONE) {
29-
const response = {
29+
resolve({
30+
statusCode: xhr.status,
3031
headers: {
3132
'x-sentry-rate-limits': xhr.getResponseHeader('X-Sentry-Rate-Limits'),
3233
'retry-after': xhr.getResponseHeader('Retry-After'),
3334
},
34-
};
35-
resolve(response);
35+
});
3636
}
3737
};
3838

‎packages/core/src/transports/base.ts

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -70,13 +70,11 @@ export function createTransport(
7070

7171
const requestTask = (): PromiseLike<void> =>
7272
makeRequest({ body: serializeEnvelope(filteredEnvelope) }).then(
73-
({ headers }): void => {
74-
if (headers) {
75-
rateLimits = updateRateLimits(rateLimits, headers);
76-
}
73+
response => {
74+
rateLimits = updateRateLimits(rateLimits, response);
7775
},
7876
error => {
79-
IS_DEBUG_BUILD && logger.error('Failed while recording event:', error);
77+
IS_DEBUG_BUILD && logger.error('Failed while sending event:', error);
8078
recordEnvelopeLoss('network_error');
8179
},
8280
);

‎packages/node/src/transports/http.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,7 @@ function createRequestExecutor(
113113
const rateLimitsHeader = res.headers['x-sentry-rate-limits'] ?? null;
114114

115115
resolve({
116+
statusCode: res.statusCode,
116117
headers: {
117118
'retry-after': retryAfterHeader,
118119
'x-sentry-rate-limits': Array.isArray(rateLimitsHeader) ? rateLimitsHeader[0] : rateLimitsHeader,

‎packages/node/test/transports/http.test.ts

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -237,10 +237,7 @@ describe('makeNewHttpTransport()', () => {
237237
it('should register TransportRequestExecutor that returns the correct object from server response (rate limit)', async () => {
238238
await setupTestServer({
239239
statusCode: RATE_LIMIT,
240-
responseHeaders: {
241-
'Retry-After': '2700',
242-
'X-Sentry-Rate-Limits': '60::organization, 2700::organization',
243-
},
240+
responseHeaders: {},
244241
});
245242

246243
makeNodeTransport(defaultOptions);
@@ -253,10 +250,7 @@ describe('makeNewHttpTransport()', () => {
253250

254251
await expect(executorResult).resolves.toEqual(
255252
expect.objectContaining({
256-
headers: {
257-
'retry-after': '2700',
258-
'x-sentry-rate-limits': '60::organization, 2700::organization',
259-
},
253+
statusCode: RATE_LIMIT,
260254
}),
261255
);
262256
});
@@ -276,6 +270,7 @@ describe('makeNewHttpTransport()', () => {
276270

277271
await expect(executorResult).resolves.toEqual(
278272
expect.objectContaining({
273+
statusCode: SUCCESS,
279274
headers: {
280275
'retry-after': null,
281276
'x-sentry-rate-limits': null,
@@ -303,6 +298,7 @@ describe('makeNewHttpTransport()', () => {
303298

304299
await expect(executorResult).resolves.toEqual(
305300
expect.objectContaining({
301+
statusCode: SUCCESS,
306302
headers: {
307303
'retry-after': '2700',
308304
'x-sentry-rate-limits': '60::organization, 2700::organization',
@@ -330,6 +326,7 @@ describe('makeNewHttpTransport()', () => {
330326

331327
await expect(executorResult).resolves.toEqual(
332328
expect.objectContaining({
329+
statusCode: RATE_LIMIT,
333330
headers: {
334331
'retry-after': '2700',
335332
'x-sentry-rate-limits': '60::organization, 2700::organization',

‎packages/node/test/transports/https.test.ts

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -290,10 +290,7 @@ describe('makeNewHttpsTransport()', () => {
290290
it('should register TransportRequestExecutor that returns the correct object from server response (rate limit)', async () => {
291291
await setupTestServer({
292292
statusCode: RATE_LIMIT,
293-
responseHeaders: {
294-
'Retry-After': '2700',
295-
'X-Sentry-Rate-Limits': '60::organization, 2700::organization',
296-
},
293+
responseHeaders: {},
297294
});
298295

299296
makeNodeTransport(defaultOptions);
@@ -306,10 +303,7 @@ describe('makeNewHttpsTransport()', () => {
306303

307304
await expect(executorResult).resolves.toEqual(
308305
expect.objectContaining({
309-
headers: {
310-
'retry-after': '2700',
311-
'x-sentry-rate-limits': '60::organization, 2700::organization',
312-
},
306+
statusCode: RATE_LIMIT,
313307
}),
314308
);
315309
});
@@ -329,6 +323,7 @@ describe('makeNewHttpsTransport()', () => {
329323

330324
await expect(executorResult).resolves.toEqual(
331325
expect.objectContaining({
326+
statusCode: SUCCESS,
332327
headers: {
333328
'retry-after': null,
334329
'x-sentry-rate-limits': null,
@@ -356,6 +351,7 @@ describe('makeNewHttpsTransport()', () => {
356351

357352
await expect(executorResult).resolves.toEqual(
358353
expect.objectContaining({
354+
statusCode: SUCCESS,
359355
headers: {
360356
'retry-after': '2700',
361357
'x-sentry-rate-limits': '60::organization, 2700::organization',
@@ -383,6 +379,7 @@ describe('makeNewHttpsTransport()', () => {
383379

384380
await expect(executorResult).resolves.toEqual(
385381
expect.objectContaining({
382+
statusCode: RATE_LIMIT,
386383
headers: {
387384
'retry-after': '2700',
388385
'x-sentry-rate-limits': '60::organization, 2700::organization',

‎packages/types/src/transport.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ export type TransportRequest = {
77
};
88

99
export type TransportMakeRequestResponse = {
10+
statusCode?: number;
1011
headers?: {
1112
[key: string]: string | null;
1213
'x-sentry-rate-limits': string | null;

‎packages/utils/src/ratelimit.ts

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import { TransportMakeRequestResponse } from '@sentry/types';
2+
13
// Intentionally keeping the key broad, as we don't know for sure what rate limit headers get returned from backend
24
export type RateLimits = Record<string, number>;
35

@@ -43,7 +45,7 @@ export function isRateLimited(limits: RateLimits, category: string, now: number
4345
*/
4446
export function updateRateLimits(
4547
limits: RateLimits,
46-
headers: Record<string, string | null | undefined>,
48+
{ statusCode, headers }: TransportMakeRequestResponse,
4749
now: number = Date.now(),
4850
): RateLimits {
4951
const updatedRateLimits: RateLimits = {
@@ -52,8 +54,8 @@ export function updateRateLimits(
5254

5355
// "The name is case-insensitive."
5456
// https://developer.mozilla.org/en-US/docs/Web/API/Headers/get
55-
const rateLimitHeader = headers['x-sentry-rate-limits'];
56-
const retryAfterHeader = headers['retry-after'];
57+
const rateLimitHeader = headers && headers['x-sentry-rate-limits'];
58+
const retryAfterHeader = headers && headers['retry-after'];
5759

5860
if (rateLimitHeader) {
5961
/**
@@ -69,19 +71,21 @@ export function updateRateLimits(
6971
* <reason_code> is an arbitrary string like "org_quota" - ignored by SDK
7072
*/
7173
for (const limit of rateLimitHeader.trim().split(',')) {
72-
const parameters = limit.split(':', 2);
73-
const headerDelay = parseInt(parameters[0], 10);
74+
const [retryAfter, categories] = limit.split(':', 2);
75+
const headerDelay = parseInt(retryAfter, 10);
7476
const delay = (!isNaN(headerDelay) ? headerDelay : 60) * 1000; // 60sec default
75-
if (!parameters[1]) {
77+
if (!categories) {
7678
updatedRateLimits.all = now + delay;
7779
} else {
78-
for (const category of parameters[1].split(';')) {
80+
for (const category of categories.split(';')) {
7981
updatedRateLimits[category] = now + delay;
8082
}
8183
}
8284
}
8385
} else if (retryAfterHeader) {
8486
updatedRateLimits.all = now + parseRetryAfterHeader(retryAfterHeader, now);
87+
} else if (statusCode === 429) {
88+
updatedRateLimits.all = now + 60 * 1000;
8589
}
8690

8791
return updatedRateLimits;

‎packages/utils/test/ratelimit.test.ts

Lines changed: 40 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -70,66 +70,73 @@ describe('updateRateLimits()', () => {
7070
test('should update the `all` category based on `retry-after` header ', () => {
7171
const rateLimits: RateLimits = {};
7272
const headers = {
73+
'x-sentry-rate-limits': null,
7374
'retry-after': '42',
7475
};
75-
const updatedRateLimits = updateRateLimits(rateLimits, headers, 0);
76+
const updatedRateLimits = updateRateLimits(rateLimits, { headers }, 0);
7677
expect(updatedRateLimits.all).toEqual(42 * 1000);
7778
});
7879

7980
test('should update a single category based on `x-sentry-rate-limits` header', () => {
8081
const rateLimits: RateLimits = {};
8182
const headers = {
83+
'retry-after': null,
8284
'x-sentry-rate-limits': '13:error',
8385
};
84-
const updatedRateLimits = updateRateLimits(rateLimits, headers, 0);
86+
const updatedRateLimits = updateRateLimits(rateLimits, { headers }, 0);
8587
expect(updatedRateLimits.error).toEqual(13 * 1000);
8688
});
8789

8890
test('should update multiple categories based on `x-sentry-rate-limits` header', () => {
8991
const rateLimits: RateLimits = {};
9092
const headers = {
93+
'retry-after': null,
9194
'x-sentry-rate-limits': '13:error;transaction',
9295
};
93-
const updatedRateLimits = updateRateLimits(rateLimits, headers, 0);
96+
const updatedRateLimits = updateRateLimits(rateLimits, { headers }, 0);
9497
expect(updatedRateLimits.error).toEqual(13 * 1000);
9598
expect(updatedRateLimits.transaction).toEqual(13 * 1000);
9699
});
97100

98101
test('should update multiple categories with different values based on multi `x-sentry-rate-limits` header', () => {
99102
const rateLimits: RateLimits = {};
100103
const headers = {
104+
'retry-after': null,
101105
'x-sentry-rate-limits': '13:error,15:transaction',
102106
};
103-
const updatedRateLimits = updateRateLimits(rateLimits, headers, 0);
107+
const updatedRateLimits = updateRateLimits(rateLimits, { headers }, 0);
104108
expect(updatedRateLimits.error).toEqual(13 * 1000);
105109
expect(updatedRateLimits.transaction).toEqual(15 * 1000);
106110
});
107111

108112
test('should use last entry from multi `x-sentry-rate-limits` header for a given category', () => {
109113
const rateLimits: RateLimits = {};
110114
const headers = {
115+
'retry-after': null,
111116
'x-sentry-rate-limits': '13:error,15:transaction;error',
112117
};
113-
const updatedRateLimits = updateRateLimits(rateLimits, headers, 0);
118+
const updatedRateLimits = updateRateLimits(rateLimits, { headers }, 0);
114119
expect(updatedRateLimits.error).toEqual(15 * 1000);
115120
expect(updatedRateLimits.transaction).toEqual(15 * 1000);
116121
});
117122

118123
test('should fallback to `all` if `x-sentry-rate-limits` header is missing a category', () => {
119124
const rateLimits: RateLimits = {};
120125
const headers = {
126+
'retry-after': null,
121127
'x-sentry-rate-limits': '13',
122128
};
123-
const updatedRateLimits = updateRateLimits(rateLimits, headers, 0);
129+
const updatedRateLimits = updateRateLimits(rateLimits, { headers }, 0);
124130
expect(updatedRateLimits.all).toEqual(13 * 1000);
125131
});
126132

127133
test('should use 60s default if delay in `x-sentry-rate-limits` header is malformed', () => {
128134
const rateLimits: RateLimits = {};
129135
const headers = {
136+
'retry-after': null,
130137
'x-sentry-rate-limits': 'x',
131138
};
132-
const updatedRateLimits = updateRateLimits(rateLimits, headers, 0);
139+
const updatedRateLimits = updateRateLimits(rateLimits, { headers }, 0);
133140
expect(updatedRateLimits.all).toEqual(60 * 1000);
134141
});
135142

@@ -138,9 +145,10 @@ describe('updateRateLimits()', () => {
138145
error: 1337,
139146
};
140147
const headers = {
148+
'retry-after': null,
141149
'x-sentry-rate-limits': '13:transaction',
142150
};
143-
const updatedRateLimits = updateRateLimits(rateLimits, headers, 0);
151+
const updatedRateLimits = updateRateLimits(rateLimits, { headers }, 0);
144152
expect(updatedRateLimits.error).toEqual(1337);
145153
expect(updatedRateLimits.transaction).toEqual(13 * 1000);
146154
});
@@ -151,8 +159,31 @@ describe('updateRateLimits()', () => {
151159
'retry-after': '42',
152160
'x-sentry-rate-limits': '13:error',
153161
};
154-
const updatedRateLimits = updateRateLimits(rateLimits, headers, 0);
162+
const updatedRateLimits = updateRateLimits(rateLimits, { headers }, 0);
155163
expect(updatedRateLimits.error).toEqual(13 * 1000);
156164
expect(updatedRateLimits.all).toBeUndefined();
157165
});
166+
167+
test('should apply a global rate limit of 60s when no headers are provided on a 429 status code', () => {
168+
const rateLimits: RateLimits = {};
169+
const updatedRateLimits = updateRateLimits(rateLimits, { statusCode: 429 }, 0);
170+
expect(updatedRateLimits.all).toBe(60_000);
171+
});
172+
173+
test('should not apply a global rate limit specific headers are provided on a 429 status code', () => {
174+
const rateLimits: RateLimits = {};
175+
const headers = {
176+
'retry-after': null,
177+
'x-sentry-rate-limits': '13:error',
178+
};
179+
const updatedRateLimits = updateRateLimits(rateLimits, { statusCode: 429, headers }, 0);
180+
expect(updatedRateLimits.error).toEqual(13 * 1000);
181+
expect(updatedRateLimits.all).toBeUndefined();
182+
});
183+
184+
test('should not apply a default rate limit on a non-429 status code', () => {
185+
const rateLimits: RateLimits = {};
186+
const updatedRateLimits = updateRateLimits(rateLimits, { statusCode: 200 }, 0);
187+
expect(updatedRateLimits).toEqual(rateLimits);
188+
});
158189
});

0 commit comments

Comments
 (0)