Skip to content

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 4 commits into from
Jan 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
73 changes: 66 additions & 7 deletions packages/nextjs/src/edge/transport.ts
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 */
Expand All @@ -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>)[] = [];
Copy link
Member

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.

Copy link
Contributor Author

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 of Promises.

Copy link
Member

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.

Copy link
Contributor Author

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


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.
*/
Expand All @@ -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));
Copy link
Member

Choose a reason for hiding this comment

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

m: If we export IsolatedPromiseBuffer, we can add unit tests for drain, I think we should have that to prevent regressions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add some!

}
20 changes: 8 additions & 12 deletions packages/nextjs/src/edge/utils/edgeWrapperUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,19 +44,15 @@ export function withEdgeWrapping<H extends EdgeRouteHandler>(

const dynamicSamplingContext = baggageHeaderToDynamicSamplingContext(req.headers.get('baggage'));

span = startTransaction(
{
name: options.spanDescription,
op: options.spanOp,
...traceparentData,
metadata: {
dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext,
source: 'route',
},
span = startTransaction({
name: options.spanDescription,
op: options.spanOp,
...traceparentData,
metadata: {
dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext,
source: 'route',
},
// extra context passed to the `tracesSampler`
{ request: req },
);
});
}

currentScope?.setSpan(span);
Expand Down
1 change: 0 additions & 1 deletion packages/nextjs/test/edge/edgeWrapperUtils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ describe('withEdgeWrapping', () => {
expect(startTransactionSpy).toHaveBeenCalledTimes(1);
expect(startTransactionSpy).toHaveBeenCalledWith(
expect.objectContaining({ metadata: { source: 'route' }, name: 'some label', op: 'some op' }),
{ request },
);
});

Expand Down
55 changes: 54 additions & 1 deletion packages/nextjs/test/edge/transport.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { createEnvelope, serializeEnvelope } from '@sentry/utils';
import { TextEncoder } from 'util';

import type { EdgeTransportOptions } from '../../src/edge/transport';
import { makeEdgeTransport } from '../../src/edge/transport';
import { IsolatedPromiseBuffer, makeEdgeTransport } from '../../src/edge/transport';

const DEFAULT_EDGE_TRANSPORT_OPTIONS: EdgeTransportOptions = {
url: 'https://sentry.io/api/42/store/?sentry_key=123&sentry_version=7',
Expand Down Expand Up @@ -51,6 +51,7 @@ describe('Edge Transport', () => {

expect(mockFetch).toHaveBeenCalledTimes(0);
await transport.send(ERROR_ENVELOPE);
await transport.flush();
expect(mockFetch).toHaveBeenCalledTimes(1);

expect(mockFetch).toHaveBeenLastCalledWith(DEFAULT_EDGE_TRANSPORT_OPTIONS.url, {
Expand All @@ -77,6 +78,7 @@ describe('Edge Transport', () => {

expect(headers.get).toHaveBeenCalledTimes(0);
await transport.send(ERROR_ENVELOPE);
await transport.flush();

expect(headers.get).toHaveBeenCalledTimes(2);
expect(headers.get).toHaveBeenCalledWith('X-Sentry-Rate-Limits');
Expand All @@ -101,10 +103,61 @@ describe('Edge Transport', () => {
const transport = makeEdgeTransport({ ...DEFAULT_EDGE_TRANSPORT_OPTIONS, fetchOptions: REQUEST_OPTIONS });

await transport.send(ERROR_ENVELOPE);
await transport.flush();
expect(mockFetch).toHaveBeenLastCalledWith(DEFAULT_EDGE_TRANSPORT_OPTIONS.url, {
body: serializeEnvelope(ERROR_ENVELOPE, new TextEncoder()),
method: 'POST',
...REQUEST_OPTIONS,
});
});
});

describe('IsolatedPromiseBuffer', () => {
it('should not call tasks until drained', async () => {
const ipb = new IsolatedPromiseBuffer();

const task1 = jest.fn(() => Promise.resolve({}));
const task2 = jest.fn(() => Promise.resolve({}));

await ipb.add(task1);
await ipb.add(task2);

expect(task1).not.toHaveBeenCalled();
expect(task2).not.toHaveBeenCalled();

await ipb.drain();

expect(task1).toHaveBeenCalled();
expect(task2).toHaveBeenCalled();
});

it('should not allow adding more items than the specified limit', async () => {
const ipb = new IsolatedPromiseBuffer(3);

const task1 = jest.fn(() => Promise.resolve({}));
const task2 = jest.fn(() => Promise.resolve({}));
const task3 = jest.fn(() => Promise.resolve({}));
const task4 = jest.fn(() => Promise.resolve({}));

await ipb.add(task1);
await ipb.add(task2);
await ipb.add(task3);

await expect(ipb.add(task4)).rejects.toThrowError('Not adding Promise because buffer limit was reached.');
});

it('should not throw when one of the tasks throws when drained', async () => {
const ipb = new IsolatedPromiseBuffer();

const task1 = jest.fn(() => Promise.resolve({}));
const task2 = jest.fn(() => Promise.reject(new Error()));

await ipb.add(task1);
await ipb.add(task2);

await expect(ipb.drain()).resolves.toEqual(true);

expect(task1).toHaveBeenCalled();
expect(task2).toHaveBeenCalled();
});
});
1 change: 0 additions & 1 deletion packages/nextjs/test/edge/withSentryAPI.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ describe('wrapApiHandlerWithSentry', () => {
name: 'POST /user/[userId]/post/[postId]',
op: 'http.server',
}),
{ request },
);
});

Expand Down