-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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.
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
Conversation
@@ -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'); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
Implementing as per this PR means that in the Electron SDK we can just remove makeShouldSendTransport
and the same behaviour will be retained.
This PR adds a extra callback to the offline transport that allows delaying sending of envelopes. This callback has existed in the Electron offline transport for a couple of major versions.