Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(nextjs): Don't share I/O resources in between requests#6980
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| import { createTransport } from '@sentry/core'; | ||
| import type { BaseTransportOptions, Transport, TransportMakeRequestResponse, TransportRequest } from '@sentry/types'; | ||
| import { SentryError } from '@sentry/utils'; | ||
| export interface EdgeTransportOptions extends BaseTransportOptions { | ||
| /** Fetch API init parameters. Used by the FetchTransport */ | ||
| @@ -8,6 +9,66 @@ export interface EdgeTransportOptions extends BaseTransportOptions { | ||
| headers?: { [key: string]: string }; | ||
| } | ||
| const DEFAULT_TRANSPORT_BUFFER_SIZE = 30; | ||
| /** | ||
| * This is a modified promise buffer that collects tasks until drain is called. | ||
| * We need this in the edge runtime because edge function invocations may not share I/O objects, like fetch requests | ||
| * and responses, and the normal PromiseBuffer inherently buffers stuff inbetween incoming requests. | ||
| * | ||
| * A limitation we need to be aware of is that DEFAULT_TRANSPORT_BUFFER_SIZE is the maximum amount of payloads the | ||
| * SDK can send for a given edge function invocation. | ||
| */ | ||
| export class IsolatedPromiseBuffer { | ||
| // We just have this field because the promise buffer interface requires it. | ||
| // If we ever remove it from the interface we should also remove it here. | ||
| public $: Array<PromiseLike<TransportMakeRequestResponse>> = []; | ||
| private _taskProducers: (() => PromiseLike<TransportMakeRequestResponse>)[] = []; | ||
| public constructor(private readonly _bufferSize: number = DEFAULT_TRANSPORT_BUFFER_SIZE) {} | ||
| /** | ||
| * @inheritdoc | ||
| */ | ||
| public add(taskProducer: () => PromiseLike<TransportMakeRequestResponse>): PromiseLike<void> { | ||
| if (this._taskProducers.length >= this._bufferSize) { | ||
| return Promise.reject(new SentryError('Not adding Promise because buffer limit was reached.')); | ||
| } | ||
| this._taskProducers.push(taskProducer); | ||
| return Promise.resolve(); | ||
| } | ||
| /** | ||
| * @inheritdoc | ||
| */ | ||
| public drain(timeout?: number): PromiseLike<boolean> { | ||
| const oldTaskProducers = [...this._taskProducers]; | ||
| this._taskProducers = []; | ||
| return new Promise(resolve => { | ||
| const timer = setTimeout(() => { | ||
| if (timeout && timeout > 0) { | ||
| resolve(false); | ||
| } | ||
| }, timeout); | ||
| void Promise.all( | ||
| oldTaskProducers.map(taskProducer => | ||
| taskProducer().then(null, () => { | ||
| // catch all failed requests | ||
| }), | ||
| ), | ||
| ).then(() => { | ||
| // resolve to true if all fetch requests settled | ||
| clearTimeout(timer); | ||
| resolve(true); | ||
| }); | ||
| }); | ||
| } | ||
| } | ||
| /** | ||
| * Creates a Transport that uses the Edge Runtimes native fetch API to send events to Sentry. | ||
| */ | ||
| @@ -21,18 +82,16 @@ export function makeEdgeTransport(options: EdgeTransportOptions): Transport { | ||
| ...options.fetchOptions, | ||
| }; | ||
| try { | ||
| return fetch(options.url, requestOptions).then(response => ({ | ||
| return fetch(options.url, requestOptions).then(response => { | ||
| return { | ||
| statusCode: response.status, | ||
| headers: { | ||
| 'x-sentry-rate-limits': response.headers.get('X-Sentry-Rate-Limits'), | ||
| 'retry-after': response.headers.get('Retry-After'), | ||
| }, | ||
| })); | ||
| } catch (e) { | ||
| return Promise.reject(e); | ||
| } | ||
| }; | ||
| }); | ||
| } | ||
| return createTransport(options, makeRequest); | ||
| return createTransport(options, makeRequest, new IsolatedPromiseBuffer(options.bufferSize)); | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. m: If we export ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll add some! | ||
| } | ||
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.
l: Why do we use
_taskProducersinstead of$?$is only used by tests iirc.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.
They're of a different type. We need to store producers in this case because we do not want to actually create the promises right away. Producers are
() => Promisewhereas$is just an array ofPromises.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.
I see - so we are essentially making all outgoing fetch requests lazy, because we want to only send them when
drainis called.I guess this works for now, but there is always the risk that data does not get sent to Sentry because of this right? This is an important caveat that should be put in as a code comment, that we knew about this tradeoff but we still proceeded with this.
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.
Generally, we flush on every edge function invocation so we should be sending everything. This implementation has the drawback though that there is a hard cap on the amount of events sent for any given edge function invocation: https://github.com/getsentry/sentry-javascript/pull/6980/files#diff-750cbb0cd6b1f70c9ca7a215d0e9ef1e0268b822bf194ccf49feddb7a6341d64R19-R20