Skip to content

Commit 2053a20

Browse files
Lms24Lms24
committed
also handle ratelimit_backoff
1 parent 3388237 commit 2053a20

2 files changed

Lines changed: 56 additions & 30 deletions

File tree

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

Lines changed: 16 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { DEBUG_BUILD } from '../debug-build';
22
import type { EventDropReason } from '../types/clientreport';
3-
import type { Envelope, EnvelopeItem } from '../types/envelope';
3+
import type { Envelope, EnvelopeItem, EnvelopeItemType } from '../types/envelope';
44
import type { Event } from '../types/event';
55
import type {
66
InternalBaseTransportOptions,
@@ -37,14 +37,24 @@ export function createTransport(
3737
let rateLimits: RateLimits = {};
3838
const flush = (timeout?: number): PromiseLike<boolean> => buffer.drain(timeout);
3939

40+
function recordItemLoss(reason: EventDropReason, [headers, payload]: EnvelopeItem, type: EnvelopeItemType): void {
41+
const dataCategory = envelopeItemTypeToDataCategory(type);
42+
if (type === 'transaction') {
43+
options.recordDroppedEvent(reason, dataCategory);
44+
// The transaction itself counts as a span, too
45+
options.recordDroppedEvent(reason, 'span', ((payload as Event).spans?.length ?? 0) + 1);
46+
} else {
47+
options.recordDroppedEvent(reason, dataCategory, typeof headers.item_count === 'number' ? headers.item_count : 1);
48+
}
49+
}
50+
4051
function send(envelope: Envelope): PromiseLike<TransportMakeRequestResponse> {
4152
const filteredEnvelopeItems: EnvelopeItem[] = [];
4253

4354
// Drop rate limited items from envelope
4455
forEachEnvelopeItem(envelope, (item, type) => {
45-
const dataCategory = envelopeItemTypeToDataCategory(type);
46-
if (isRateLimited(rateLimits, dataCategory)) {
47-
options.recordDroppedEvent('ratelimit_backoff', dataCategory);
56+
if (isRateLimited(rateLimits, envelopeItemTypeToDataCategory(type))) {
57+
recordItemLoss('ratelimit_backoff', item, type);
4858
} else {
4959
filteredEnvelopeItems.push(item);
5060
}
@@ -64,19 +74,8 @@ export function createTransport(
6474
DEBUG_BUILD && debug.warn(`Dropping client report. Will not send outcomes (reason: ${reason}).`);
6575
return;
6676
}
67-
forEachEnvelopeItem(filteredEnvelope, ([headers, payload], type) => {
68-
const dataCategory = envelopeItemTypeToDataCategory(type);
69-
if (type === 'transaction') {
70-
options.recordDroppedEvent(reason, dataCategory);
71-
// The transaction itself counts as a span, too
72-
options.recordDroppedEvent(reason, 'span', ((payload as Event).spans?.length ?? 0) + 1);
73-
} else {
74-
options.recordDroppedEvent(
75-
reason,
76-
dataCategory,
77-
typeof headers.item_count === 'number' ? headers.item_count : 1,
78-
);
79-
}
77+
forEachEnvelopeItem(filteredEnvelope, (item, type) => {
78+
recordItemLoss(reason, item, type);
8079
});
8180
};
8281

‎packages/core/test/lib/transports/base.test.ts‎

Lines changed: 40 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,7 @@ describe('createTransport', () => {
157157

158158
await transport.send(ERROR_ENVELOPE);
159159
expect(requestExecutor).not.toHaveBeenCalled();
160-
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'error');
160+
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'error', 1);
161161
requestExecutor.mockClear();
162162
recordDroppedEventCallback.mockClear();
163163

@@ -199,7 +199,7 @@ describe('createTransport', () => {
199199

200200
await transport.send(ERROR_ENVELOPE); // Error envelope should not be sent because of pending rate limit
201201
expect(requestExecutor).not.toHaveBeenCalled();
202-
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'error');
202+
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'error', 1);
203203
requestExecutor.mockClear();
204204
recordDroppedEventCallback.mockClear();
205205

@@ -242,18 +242,19 @@ describe('createTransport', () => {
242242
await transport.send(TRANSACTION_ENVELOPE); // Transaction envelope should not be sent because of pending rate limit
243243
expect(requestExecutor).not.toHaveBeenCalled();
244244
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'transaction');
245+
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'span', 1);
245246
requestExecutor.mockClear();
246247
recordDroppedEventCallback.mockClear();
247248

248249
await transport.send(ERROR_ENVELOPE); // Error envelope should not be sent because of pending rate limit
249250
expect(requestExecutor).not.toHaveBeenCalled();
250-
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'error');
251+
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'error', 1);
251252
requestExecutor.mockClear();
252253
recordDroppedEventCallback.mockClear();
253254

254255
await transport.send(ATTACHMENT_ENVELOPE); // Attachment envelope should not be sent because of pending rate limit
255256
expect(requestExecutor).not.toHaveBeenCalled();
256-
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'attachment');
257+
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'attachment', 1);
257258
requestExecutor.mockClear();
258259
recordDroppedEventCallback.mockClear();
259260

@@ -302,12 +303,13 @@ describe('createTransport', () => {
302303
await transport.send(TRANSACTION_ENVELOPE); // Transaction envelope should not be sent because of pending rate limit
303304
expect(requestExecutor).not.toHaveBeenCalled();
304305
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'transaction');
306+
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'span', 1);
305307
requestExecutor.mockClear();
306308
recordDroppedEventCallback.mockClear();
307309

308310
await transport.send(ERROR_ENVELOPE); // Error envelope should not be sent because of pending rate limit
309311
expect(requestExecutor).not.toHaveBeenCalled();
310-
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'error');
312+
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'error', 1);
311313
requestExecutor.mockClear();
312314
recordDroppedEventCallback.mockClear();
313315

@@ -324,10 +326,35 @@ describe('createTransport', () => {
324326
expect(requestExecutor).toHaveBeenCalledTimes(1);
325327
expect(recordDroppedEventCallback).not.toHaveBeenCalled();
326328
});
329+
330+
it('records the item count of rate limited container items', async () => {
331+
const { retryAfterSeconds, beforeLimit, withinLimit } = setRateLimitTimes();
332+
const [transport, , requestExecutor, recordDroppedEventCallback] = createTestTransport({
333+
headers: {
334+
'x-sentry-rate-limits': `${retryAfterSeconds}:log_item:scope`,
335+
'retry-after': null,
336+
},
337+
});
338+
339+
const dateNowSpy = vi.spyOn(Date, 'now').mockImplementation(() => beforeLimit);
340+
341+
await transport.send(ERROR_ENVELOPE);
342+
requestExecutor.mockClear();
343+
344+
dateNowSpy.mockImplementation(() => withinLimit);
345+
346+
await transport.send(
347+
createEnvelope<Envelope>({ sent_at: '123' }, [
348+
[{ type: 'log', item_count: 3, content_type: 'application/vnd.sentry.items.log+json' }, { items: [] }],
349+
] as Envelope[1]),
350+
);
351+
expect(requestExecutor).not.toHaveBeenCalled();
352+
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'log_item', 3);
353+
});
327354
});
328355

329356
describe('Client Reports', () => {
330-
it('should not record outcomes when client reports fail to send', async () => {
357+
it('does not record outcomes when client reports fail to send', async () => {
331358
expect.assertions(2);
332359

333360
const mockRecordDroppedEventCallback = vi.fn();
@@ -347,7 +374,7 @@ describe('createTransport', () => {
347374
expect(mockRecordDroppedEventCallback).not.toHaveBeenCalled();
348375
});
349376

350-
it('should not record outcomes when client reports fail due to buffer overflow', async () => {
377+
it('does not record outcomes when client reports fail due to buffer overflow', async () => {
351378
expect.assertions(2);
352379

353380
const mockRecordDroppedEventCallback = vi.fn();
@@ -371,7 +398,7 @@ describe('createTransport', () => {
371398
expect(mockRecordDroppedEventCallback).not.toHaveBeenCalled();
372399
});
373400

374-
it('should record outcomes when regular events fail to send', async () => {
401+
it('records outcomes when regular events fail to send', async () => {
375402
expect.assertions(2);
376403

377404
const mockRecordDroppedEventCallback = vi.fn();
@@ -393,7 +420,7 @@ describe('createTransport', () => {
393420
});
394421

395422
describe('HTTP 413 Content Too Large', () => {
396-
it('should record send_error outcome when receiving 413 response', async () => {
423+
it('records send_error outcome when receiving 413 response', async () => {
397424
const mockRecordDroppedEventCallback = vi.fn();
398425

399426
const transport = createTransport({ recordDroppedEvent: mockRecordDroppedEventCallback }, () =>
@@ -408,7 +435,7 @@ describe('createTransport', () => {
408435
expect(mockRecordDroppedEventCallback).toHaveBeenCalledWith('send_error', 'error', 1);
409436
});
410437

411-
it('should record send_error for each item in envelope when receiving 413', async () => {
438+
it('records send_error for each item in envelope when receiving 413', async () => {
412439
const mockRecordDroppedEventCallback = vi.fn();
413440

414441
const multiItemEnvelope = createEnvelope<EventEnvelope>(
@@ -431,7 +458,7 @@ describe('createTransport', () => {
431458
expect(mockRecordDroppedEventCallback).toHaveBeenCalledWith('send_error', 'span', 3);
432459
});
433460

434-
it('should record send_error with the item count of container items when receiving 413', async () => {
461+
it('records send_error with the item count of container items when receiving 413', async () => {
435462
const mockRecordDroppedEventCallback = vi.fn();
436463

437464
const containerEnvelope = createEnvelope<Envelope>({ sent_at: '123' }, [
@@ -455,7 +482,7 @@ describe('createTransport', () => {
455482
expect(mockRecordDroppedEventCallback).toHaveBeenCalledWith('send_error', 'metric', 2);
456483
});
457484

458-
it('should not record outcomes for client reports when receiving 413', async () => {
485+
it('does not record outcomes for client reports when receiving 413', async () => {
459486
const mockRecordDroppedEventCallback = vi.fn();
460487

461488
const transport = createTransport({ recordDroppedEvent: mockRecordDroppedEventCallback }, () =>
@@ -470,7 +497,7 @@ describe('createTransport', () => {
470497
expect(mockRecordDroppedEventCallback).not.toHaveBeenCalled();
471498
});
472499

473-
it('should not apply rate limits after receiving 413', async () => {
500+
it('does not apply rate limits after receiving 413', async () => {
474501
const mockRecordDroppedEventCallback = vi.fn();
475502
const mockRequestExecutor = vi.fn(() => resolvedSyncPromise({ statusCode: 413 }));
476503

0 commit comments

Comments
 (0)