Skip to content

feat(core): Allow delayed sending with offline transport #15937

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 11, 2025
Merged

feat(core): Allow delayed sending with offline transport #15937

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
facda2d
feat(core): Allow delayed sending with offline transport
timfish Apr 1, 2025
dad169b
Lint
timfish Apr 1, 2025
c4dcdaa
Merge branch 'develop' into feat/offline-delayed-sending
timfish Apr 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions packages/core/src/transports/offline.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,19 @@ export interface OfflineTransportOptions extends InternalBaseTransportOptions {
* @param envelope The envelope that failed to send.
* @param error The error that occurred.
* @param retryDelay The current retry delay in milliseconds.
* @returns Whether the envelope should be stored.
*/
shouldStore?: (envelope: Envelope, error: Error, retryDelay: number) => boolean | Promise<boolean>;

/**
* Should an attempt be made to send the envelope to Sentry.
*
* If this function is supplied and returns false, `shouldStore` will be called to determine if the envelope should be stored.
*
* @param envelope The envelope that will be sent.
* @returns Whether we should attempt to send the envelope
*/
shouldSend?: (envelope: Envelope) => boolean | Promise<boolean>;
}

type Timer = number | { unref?: () => void };
Expand Down Expand Up @@ -128,6 +139,10 @@ export function makeOfflineTransport<TO>(
}

try {
if (options.shouldSend && (await options.shouldSend(envelope)) === false) {
throw new Error('Envelope not sent because `shouldSend` callback returned false');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this mean that shouldQueue still can trigger from this? Wouldn't that lead to an infinite loop?

throwing an error seems strange, shouldn't we just do an early return?

Copy link
Collaborator Author

@timfish timfish Apr 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this mean that shouldQueue still can trigger from this? Wouldn't that lead to an infinite loop?

No because shouldQueue doesn't result in immediate sending, it backs off starting from a 5 second delay.

throwing an error seems strange, shouldn't we just do an early return?

Sorry, I should have explained that further!

We can't use an early return here without making breaking changes or duplicating a lot of code. If shouldSend returns false, we want to hit shouldQueue and that is currently in the catch block and the easiest way to end up in the catch block is to throw:

sentry-javascript/packages/core/src/transports/offline.ts

Lines 141 to 166 in c4dcdaa

try {
if (options.shouldSend && (await options.shouldSend(envelope)) === false) {
throw new Error('Envelope not sent because `shouldSend` callback returned false');
}
const result = await transport.send(envelope);
let delay = MIN_DELAY;
if (result) {
// If there's a retry-after header, use that as the next delay.
if (result.headers?.['retry-after']) {
delay = parseRetryAfterHeader(result.headers['retry-after']);
} else if (result.headers?.['x-sentry-rate-limits']) {
delay = 60_000; // 60 seconds
} // If we have a server error, return now so we don't flush the queue.
else if ((result.statusCode || 0) >= 400) {
return result;
}
}
flushIn(delay);
retryDelay = START_DELAY;
return result;
} catch (e) {
if (await shouldQueue(envelope, e as Error, retryDelay)) {

Throwing errors for control flow isn't ideal but this PR also needs to replace this feature like-for-like in the Electron SDK so we don't need to make a breaking change there. This callback was implemented in the Electron SDK as a simple transport wrapper that throws when shouldSend returns true:

https://github.com/getsentry/sentry-electron/blob/6169e0ddea8d1fc7e9a75c8dfcc40e57d48b98d9/src/main/transports/electron-offline-net.ts#L22-L41

Implementing as per this PR means that in the Electron SDK we can just remove makeShouldSendTransport and the same behaviour will be retained.

}

const result = await transport.send(envelope);

let delay = MIN_DELAY;
Expand Down
21 changes: 21 additions & 0 deletions packages/core/test/lib/transports/offline.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,27 @@ describe('makeOfflineTransport', () => {
expect(getCalls()).toEqual([]);
});

it('shouldSend can stop envelopes from being sent', async () => {
const { getCalls, store } = createTestStore();
const { getSendCount, baseTransport } = createTestTransport(new Error());
let queuedCount = 0;
const transport = makeOfflineTransport(baseTransport)({
...transportOptions,
createStore: store,
shouldSend: () => false,
shouldStore: () => {
queuedCount += 1;
return true;
},
});
const result = transport.send(ERROR_ENVELOPE);

await expect(result).resolves.toEqual({});
expect(queuedCount).toEqual(1);
expect(getSendCount()).toEqual(0);
expect(getCalls()).toEqual(['push']);
});

it('should not store client report envelopes on send failure', async () => {
const { getCalls, store } = createTestStore();
const { getSendCount, baseTransport } = createTestTransport(new Error());
Expand Down
Loading