Skip to content

Commit ad3dd01

Browse files
authored
fix(core): Don't record outcomes for failed client reports (#18808)
[Sentry changed IPs](https://status.sentry.io/incidents/qmh29yyv8bbv) and this caused problems with client report sending. The current problem is that client report sending is stuck in a loop if the client report itself cannot be sent: `Event fails → Client Report #1 fails → Client Report #2 fails → Client Report #3 fails → ∞` With this fix, failed client reports are not sent anymore to prevent this infinite feedback loop. The offline transport already drops client reports when they cannot be sent: https://github.com/getsentry/sentry-javascript/blob/1b41126666e27a311884cf6f7c1ef915f95477de/packages/core/src/transports/offline.ts#L83-L86 I tested this locally by adding an entry to `/etc/hosts`: `0.0.0.0 o1.ingest.sentry.io` (the example in the issue below) Closes #18802
1 parent 7a975c9 commit ad3dd01

File tree

2 files changed

+94
-1
lines changed

2 files changed

+94
-1
lines changed

packages/core/src/transports/base.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import type {
1010
import { debug } from '../utils/debug-logger';
1111
import {
1212
createEnvelope,
13+
envelopeContainsItemType,
1314
envelopeItemTypeToDataCategory,
1415
forEachEnvelopeItem,
1516
serializeEnvelope,
@@ -57,6 +58,11 @@ export function createTransport(
5758

5859
// Creates client report for each item in an envelope
5960
const recordEnvelopeLoss = (reason: EventDropReason): void => {
61+
// Don't record outcomes for client reports - we don't want to create a feedback loop if client reports themselves fail to send
62+
if (envelopeContainsItemType(filteredEnvelope, ['client_report'])) {
63+
DEBUG_BUILD && debug.warn(`Dropping client report. Will not send outcomes (reason: ${reason}).`);
64+
return;
65+
}
6066
forEachEnvelopeItem(filteredEnvelope, (item, type) => {
6167
options.recordDroppedEvent(reason, envelopeItemTypeToDataCategory(type));
6268
});

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

Lines changed: 88 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
import { describe, expect, it, vi } from 'vitest';
22
import { createTransport } from '../../../src/transports/base';
3+
import type { ClientReport } from '../../../src/types-hoist/clientreport';
34
import type { AttachmentItem, EventEnvelope, EventItem } from '../../../src/types-hoist/envelope';
45
import type { TransportMakeRequestResponse } from '../../../src/types-hoist/transport';
6+
import { createClientReportEnvelope } from '../../../src/utils/clientreport';
57
import { createEnvelope, serializeEnvelope } from '../../../src/utils/envelope';
6-
import type { PromiseBuffer } from '../../../src/utils/promisebuffer';
8+
import { type PromiseBuffer, SENTRY_BUFFER_FULL_ERROR } from '../../../src/utils/promisebuffer';
79
import { resolvedSyncPromise } from '../../../src/utils/syncpromise';
810

911
const ERROR_ENVELOPE = createEnvelope<EventEnvelope>({ event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', sent_at: '123' }, [
@@ -31,6 +33,25 @@ const ATTACHMENT_ENVELOPE = createEnvelope<EventEnvelope>(
3133
],
3234
);
3335

36+
const defaultDiscardedEvents: ClientReport['discarded_events'] = [
37+
{
38+
reason: 'before_send',
39+
category: 'error',
40+
quantity: 30,
41+
},
42+
{
43+
reason: 'network_error',
44+
category: 'transaction',
45+
quantity: 23,
46+
},
47+
];
48+
49+
const CLIENT_REPORT_ENVELOPE = createClientReportEnvelope(
50+
defaultDiscardedEvents,
51+
'https://[email protected]/1337',
52+
123456,
53+
);
54+
3455
const transportOptions = {
3556
recordDroppedEvent: () => undefined, // noop
3657
};
@@ -304,5 +325,71 @@ describe('createTransport', () => {
304325
expect(recordDroppedEventCallback).not.toHaveBeenCalled();
305326
});
306327
});
328+
329+
describe('Client Reports', () => {
330+
it('should not record outcomes when client reports fail to send', async () => {
331+
expect.assertions(2);
332+
333+
const mockRecordDroppedEventCallback = vi.fn();
334+
335+
const transport = createTransport({ recordDroppedEvent: mockRecordDroppedEventCallback }, req => {
336+
expect(req.body).toEqual(serializeEnvelope(CLIENT_REPORT_ENVELOPE));
337+
return Promise.reject(new Error('Network error'));
338+
});
339+
340+
try {
341+
await transport.send(CLIENT_REPORT_ENVELOPE);
342+
} catch (e) {
343+
// Expected to throw
344+
}
345+
346+
// recordDroppedEvent should NOT be called when a client report fails
347+
expect(mockRecordDroppedEventCallback).not.toHaveBeenCalled();
348+
});
349+
350+
it('should not record outcomes when client reports fail due to buffer overflow', async () => {
351+
expect.assertions(2);
352+
353+
const mockRecordDroppedEventCallback = vi.fn();
354+
const mockBuffer: PromiseBuffer<TransportMakeRequestResponse> = {
355+
$: [],
356+
add: vi.fn(() => Promise.reject(SENTRY_BUFFER_FULL_ERROR)),
357+
drain: vi.fn(),
358+
};
359+
360+
const transport = createTransport(
361+
{ recordDroppedEvent: mockRecordDroppedEventCallback },
362+
_ => resolvedSyncPromise({}),
363+
mockBuffer,
364+
);
365+
366+
const result = await transport.send(CLIENT_REPORT_ENVELOPE);
367+
368+
// Should resolve without throwing
369+
expect(result).toEqual({});
370+
// recordDroppedEvent should NOT be called when a client report fails
371+
expect(mockRecordDroppedEventCallback).not.toHaveBeenCalled();
372+
});
373+
374+
it('should record outcomes when regular events fail to send', async () => {
375+
expect.assertions(2);
376+
377+
const mockRecordDroppedEventCallback = vi.fn();
378+
379+
const transport = createTransport({ recordDroppedEvent: mockRecordDroppedEventCallback }, req => {
380+
expect(req.body).toEqual(serializeEnvelope(ERROR_ENVELOPE));
381+
return Promise.reject(new Error('Network error'));
382+
});
383+
384+
try {
385+
await transport.send(ERROR_ENVELOPE);
386+
} catch (e) {
387+
// Expected to throw
388+
}
389+
390+
// recordDroppedEvent SHOULD be called for regular events
391+
expect(mockRecordDroppedEventCallback).toHaveBeenCalledWith('network_error', 'error');
392+
});
393+
});
307394
});
308395
});

0 commit comments

Comments
 (0)