From 5693635b24c56b1864d3eef893e40f0d4793735f Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Sun, 19 Feb 2023 14:50:21 +0100 Subject: [PATCH 1/2] fix: Exclude client reports from offline storage --- packages/core/src/transports/offline.ts | 17 ++------- .../core/test/lib/transports/offline.test.ts | 38 +++++++++++++++++++ packages/utils/src/envelope.ts | 27 ++++++++++--- 3 files changed, 62 insertions(+), 20 deletions(-) diff --git a/packages/core/src/transports/offline.ts b/packages/core/src/transports/offline.ts index 9105ad63558a..06ad7fc464a5 100644 --- a/packages/core/src/transports/offline.ts +++ b/packages/core/src/transports/offline.ts @@ -1,22 +1,10 @@ import type { Envelope, InternalBaseTransportOptions, Transport, TransportMakeRequestResponse } from '@sentry/types'; -import { forEachEnvelopeItem, logger, parseRetryAfterHeader } from '@sentry/utils'; +import { envelopeContainsItemType, logger, parseRetryAfterHeader } from '@sentry/utils'; export const MIN_DELAY = 100; // 100 ms export const START_DELAY = 5_000; // 5 seconds const MAX_DELAY = 3.6e6; // 1 hour -function isReplayEnvelope(envelope: Envelope): boolean { - let isReplay = false; - - forEachEnvelopeItem(envelope, (_, type) => { - if (type === 'replay_event') { - isReplay = true; - } - }); - - return isReplay; -} - function log(msg: string, error?: Error): void { __DEBUG_BUILD__ && logger.info(`[Offline]: ${msg}`, error); } @@ -74,7 +62,8 @@ export function makeOfflineTransport( // We don't queue Session Replay envelopes because they are: // - Ordered and Replay relies on the response status to know when they're successfully sent. // - Likely to fill the queue quickly and block other events from being sent. - if (isReplayEnvelope(env)) { + // We also want to drop client reports because they can be generated when we retry sending events while offline. + if (envelopeContainsItemType(env, 'replay_event', 'replay_recording', 'client_report')) { return false; } diff --git a/packages/core/test/lib/transports/offline.test.ts b/packages/core/test/lib/transports/offline.test.ts index ad38f0363279..eebe7c8874ed 100644 --- a/packages/core/test/lib/transports/offline.test.ts +++ b/packages/core/test/lib/transports/offline.test.ts @@ -1,4 +1,5 @@ import type { + ClientReport, Envelope, EventEnvelope, EventItem, @@ -9,6 +10,7 @@ import type { TransportMakeRequestResponse, } from '@sentry/types'; import { + createClientReportEnvelope, createEnvelope, createEventEnvelopeHeaders, dsnFromString, @@ -54,6 +56,25 @@ const RELAY_ENVELOPE = createEnvelope( ], ); +const DEFAULT_DISCARDED_EVENTS: ClientReport['discarded_events'] = [ + { + reason: 'before_send', + category: 'error', + quantity: 30, + }, + { + reason: 'network_error', + category: 'transaction', + quantity: 23, + }, +]; + +const CLIENT_REPORT_ENVELOPE = createClientReportEnvelope( + DEFAULT_DISCARDED_EVENTS, + 'https://public@dsn.ingest.sentry.io/1337', + 123456, +); + const transportOptions = { recordDroppedEvent: () => undefined, // noop textEncoder: new TextEncoder(), @@ -288,6 +309,23 @@ describe('makeOfflineTransport', () => { expect(getCalls()).toEqual([]); }); + it('should not store client report envelopes on send failure', async () => { + const { getCalls, store } = createTestStore(); + const { getSendCount, baseTransport } = createTestTransport(new Error()); + const queuedCount = 0; + const transport = makeOfflineTransport(baseTransport)({ + ...transportOptions, + createStore: store, + shouldStore: () => true, + }); + const result = transport.send(CLIENT_REPORT_ENVELOPE); + + await expect(result).rejects.toBeInstanceOf(Error); + expect(queuedCount).toEqual(0); + expect(getSendCount()).toEqual(0); + expect(getCalls()).toEqual([]); + }); + it( 'Follows the Retry-After header', async () => { diff --git a/packages/utils/src/envelope.ts b/packages/utils/src/envelope.ts index b72f905fcbd4..b73a1f0c9b30 100644 --- a/packages/utils/src/envelope.ts +++ b/packages/utils/src/envelope.ts @@ -6,7 +6,6 @@ import type { DataCategory, DsnComponents, Envelope, - EnvelopeItem, EnvelopeItemType, Event, EventEnvelopeHeaders, @@ -41,16 +40,32 @@ export function addItemToEnvelope(envelope: E, newItem: E[1] /** * Convenience function to loop through the items and item types of an envelope. * (This function was mostly created because working with envelope types is painful at the moment) + * + * If the callback returns true, the rest of the items will be skipped. */ export function forEachEnvelopeItem( envelope: Envelope, - callback: (envelopeItem: E[1][number], envelopeItemType: E[1][number][0]['type']) => void, -): void { + callback: (envelopeItem: E[1][number], envelopeItemType: E[1][number][0]['type']) => boolean | void, +): boolean { const envelopeItems = envelope[1]; - envelopeItems.forEach((envelopeItem: EnvelopeItem) => { + + for (const envelopeItem of envelopeItems) { const envelopeItemType = envelopeItem[0].type; - callback(envelopeItem, envelopeItemType); - }); + const result = callback(envelopeItem, envelopeItemType); + + if (result) { + return true; + } + } + + return false; +} + +/** + * Returns true if the envelope contains any of the given envelope item types + */ +export function envelopeContainsItemType(envelope: Envelope, ...types: EnvelopeItemType[]): boolean { + return forEachEnvelopeItem(envelope, (_, type) => types.includes(type)); } /** From 9cdcdc27504980aa93d39b5c29ba020ee7d8dd1f Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Mon, 20 Feb 2023 10:27:59 +0100 Subject: [PATCH 2/2] Dont use spread args --- packages/core/src/transports/offline.ts | 2 +- packages/utils/src/envelope.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/src/transports/offline.ts b/packages/core/src/transports/offline.ts index 06ad7fc464a5..e318793624ce 100644 --- a/packages/core/src/transports/offline.ts +++ b/packages/core/src/transports/offline.ts @@ -63,7 +63,7 @@ export function makeOfflineTransport( // - Ordered and Replay relies on the response status to know when they're successfully sent. // - Likely to fill the queue quickly and block other events from being sent. // We also want to drop client reports because they can be generated when we retry sending events while offline. - if (envelopeContainsItemType(env, 'replay_event', 'replay_recording', 'client_report')) { + if (envelopeContainsItemType(env, ['replay_event', 'replay_recording', 'client_report'])) { return false; } diff --git a/packages/utils/src/envelope.ts b/packages/utils/src/envelope.ts index b73a1f0c9b30..705326b6c9ba 100644 --- a/packages/utils/src/envelope.ts +++ b/packages/utils/src/envelope.ts @@ -64,7 +64,7 @@ export function forEachEnvelopeItem( /** * Returns true if the envelope contains any of the given envelope item types */ -export function envelopeContainsItemType(envelope: Envelope, ...types: EnvelopeItemType[]): boolean { +export function envelopeContainsItemType(envelope: Envelope, types: EnvelopeItemType[]): boolean { return forEachEnvelopeItem(envelope, (_, type) => types.includes(type)); }