-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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)); | ||
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 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! |
||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
_taskProducers
instead 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
() => Promise
whereas$
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
drain
is 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