Skip to content

ref(nextjs): Switch edge wrapping to OTEL #13937

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 26 commits into from
Oct 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
12a90d1
ref(nextjs): Switch edge wrapping to OTEL
lforst Oct 10, 2024
75f9a6e
Set `http.server.middleware` op on middleware spans
lforst Oct 10, 2024
793a4b5
Delete tests that have become redundant
lforst Oct 10, 2024
a393108
Refactor middleware wrapper to not use edge wrapper utils
lforst Oct 14, 2024
f433c84
Refactor edge api handler wrapper to not use edge wrapper utils
lforst Oct 14, 2024
3e0e83f
rm rf edge wrapper utils
lforst Oct 14, 2024
f5640f4
Merge remote-tracking branch 'origin/lforst-nextjs-otel' into lforst-…
lforst Oct 14, 2024
015cdfa
Always flush
lforst Oct 14, 2024
e5f78bc
Flush on span end instead
lforst Oct 14, 2024
4c93037
Update edge route test
lforst Oct 14, 2024
d98d2a3
Unskip tests
lforst Oct 14, 2024
bc67d68
skip
lforst Oct 14, 2024
d3257b8
rm debugging statement
lforst Oct 14, 2024
2c31dbf
Put back withIsolationScope
lforst Oct 15, 2024
f1aea3f
Fix tests
lforst Oct 15, 2024
9df7f7c
Add comments
lforst Oct 15, 2024
9d9168e
Merge branch 'lforst-nextjs-otel' into lforst-nextjs-edge-wrapping-otel
lforst Oct 15, 2024
967696e
Fix test
lforst Oct 15, 2024
c6d7b24
Merge branch 'lforst-nextjs-otel' into lforst-nextjs-edge-wrapping-otel
lforst Oct 15, 2024
82a00a1
Restore isolation scope on root span
lforst Oct 16, 2024
156ba88
Fix test?
lforst Oct 16, 2024
b571e69
Skip test
lforst Oct 16, 2024
29e9cdd
Merge branch 'lforst-nextjs-otel' into lforst-nextjs-edge-wrapping-otel
lforst Oct 16, 2024
52dc131
Merge branch 'lforst-nextjs-otel' into lforst-nextjs-edge-wrapping-otel
lforst Oct 16, 2024
52dd39f
wat
lforst Oct 16, 2024
3f71195
braincells--
lforst Oct 16, 2024
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
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,5 @@ export async function middleware(request: NextRequest) {

// See "Matching Paths" below to learn more
export const config = {
matcher: ['/api/endpoint-behind-middleware'],
matcher: ['/api/endpoint-behind-middleware', '/api/endpoint-behind-faulty-middleware'],
};
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@
/// <reference types="next/navigation-types/compat/navigation" />

// NOTE: This file should not be edited
// see https://nextjs.org/docs/basic-features/typescript for more information.
// see https://nextjs.org/docs/app/building-your-application/configuring/typescript for more information.
Original file line number Diff line number Diff line change
Expand Up @@ -7,21 +7,22 @@ export const config = {
export default async function handler() {
// Without a working async context strategy the two spans created by `Sentry.startSpan()` would be nested.

const outerSpanPromise = Sentry.withIsolationScope(() => {
return Sentry.startSpan({ name: 'outer-span' }, () => {
return new Promise<void>(resolve => setTimeout(resolve, 300));
});
const outerSpanPromise = Sentry.startSpan({ name: 'outer-span' }, () => {
return new Promise<void>(resolve => setTimeout(resolve, 300));
});

setTimeout(() => {
Sentry.withIsolationScope(() => {
return Sentry.startSpan({ name: 'inner-span' }, () => {
const innerSpanPromise = new Promise<void>(resolve => {
setTimeout(() => {
Sentry.startSpan({ name: 'inner-span' }, () => {
return new Promise<void>(resolve => setTimeout(resolve, 100));
}).then(() => {
resolve();
});
});
}, 100);
}, 100);
});

await outerSpanPromise;
await innerSpanPromise;

return new Response('ok', { status: 200 });
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import type { NextApiRequest, NextApiResponse } from 'next';

type Data = {
name: string;
};

export default function handler(req: NextApiRequest, res: NextApiResponse<Data>) {
res.status(200).json({ name: 'John Doe' });
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@ import { waitForTransaction } from '@sentry-internal/test-utils';

test('Should allow for async context isolation in the edge SDK', async ({ request }) => {
const edgerouteTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => {
return transactionEvent?.transaction === 'GET /api/async-context-edge-endpoint';
return (
transactionEvent?.transaction === 'GET /api/async-context-edge-endpoint' &&
transactionEvent.contexts?.runtime?.name === 'vercel-edge'
);
});

await request.get('/api/async-context-edge-endpoint');
Expand All @@ -13,8 +16,5 @@ test('Should allow for async context isolation in the edge SDK', async ({ reques
const outerSpan = asyncContextEdgerouteTransaction.spans?.find(span => span.description === 'outer-span');
const innerSpan = asyncContextEdgerouteTransaction.spans?.find(span => span.description === 'inner-span');

// @ts-expect-error parent_span_id exists
expect(outerSpan?.parent_span_id).toStrictEqual(asyncContextEdgerouteTransaction.contexts?.trace?.span_id);
// @ts-expect-error parent_span_id exists
expect(innerSpan?.parent_span_id).toStrictEqual(asyncContextEdgerouteTransaction.contexts?.trace?.span_id);
expect(outerSpan?.parent_span_id).toStrictEqual(innerSpan?.parent_span_id);
});
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ test('Should create a transaction for edge routes', async ({ request }) => {
const edgerouteTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => {
return (
transactionEvent?.transaction === 'GET /api/edge-endpoint' &&
transactionEvent?.contexts?.trace?.status === 'ok' &&
transactionEvent.contexts?.runtime?.name === 'vercel-edge'
);
});
Expand All @@ -24,31 +23,11 @@ test('Should create a transaction for edge routes', async ({ request }) => {
expect(edgerouteTransaction.request?.headers?.['x-yeet']).toBe('test-value');
});

test('Should create a transaction with error status for faulty edge routes', async ({ request }) => {
test('Faulty edge routes', async ({ request }) => {
const edgerouteTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => {
return (
transactionEvent?.transaction === 'GET /api/error-edge-endpoint' &&
transactionEvent?.contexts?.trace?.status === 'unknown_error'
);
return transactionEvent?.transaction === 'GET /api/error-edge-endpoint';
});

request.get('/api/error-edge-endpoint').catch(() => {
// Noop
});

const edgerouteTransaction = await edgerouteTransactionPromise;

expect(edgerouteTransaction.contexts?.trace?.status).toBe('unknown_error');
expect(edgerouteTransaction.contexts?.trace?.op).toBe('http.server');
expect(edgerouteTransaction.contexts?.runtime?.name).toBe('vercel-edge');

// Assert that isolation scope works properly
expect(edgerouteTransaction.tags?.['my-isolated-tag']).toBe(true);
expect(edgerouteTransaction.tags?.['my-global-scope-isolated-tag']).not.toBeDefined();
});

// TODO(lforst): This cannot make it into production - Make sure to fix this test
test.skip('Should record exceptions for faulty edge routes', async ({ request }) => {
const errorEventPromise = waitForError('nextjs-app-dir', errorEvent => {
return errorEvent?.exception?.values?.[0]?.value === 'Edge Route Error';
});
Expand All @@ -57,11 +36,21 @@ test.skip('Should record exceptions for faulty edge routes', async ({ request })
// Noop
});

const errorEvent = await errorEventPromise;
const [edgerouteTransaction, errorEvent] = await Promise.all([
test.step('should create a transaction', () => edgerouteTransactionPromise),
test.step('should create an error event', () => errorEventPromise),
]);

// Assert that isolation scope works properly
expect(errorEvent.tags?.['my-isolated-tag']).toBe(true);
expect(errorEvent.tags?.['my-global-scope-isolated-tag']).not.toBeDefined();
test.step('should create transactions with the right fields', () => {
expect(edgerouteTransaction.contexts?.trace?.status).toBe('unknown_error');
expect(edgerouteTransaction.contexts?.trace?.op).toBe('http.server');
expect(edgerouteTransaction.contexts?.runtime?.name).toBe('vercel-edge');
});

expect(errorEvent.transaction).toBe('GET /api/error-edge-endpoint');
test.step('should have scope isolation', () => {
expect(edgerouteTransaction.tags?.['my-isolated-tag']).toBe(true);
expect(edgerouteTransaction.tags?.['my-global-scope-isolated-tag']).not.toBeDefined();
expect(errorEvent.tags?.['my-isolated-tag']).toBe(true);
expect(errorEvent.tags?.['my-global-scope-isolated-tag']).not.toBeDefined();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,19 @@ test('Should record exceptions for faulty edge server components', async ({ page
expect(errorEvent.transaction).toBe(`Page Server Component (/edge-server-components/error)`);
});

// TODO(lforst): This test skip cannot make it into production - make sure to fix this test before merging into develop branch
// TODO(lforst): Fix this test
test.skip('Should record transaction for edge server components', async ({ page }) => {
const serverComponentTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => {
return transactionEvent?.transaction === 'GET /edge-server-components';
return (
transactionEvent?.transaction === 'GET /edge-server-components' &&
transactionEvent.contexts?.runtime?.name === 'vercel-edge'
);
});

await page.goto('/edge-server-components');

const serverComponentTransaction = await serverComponentTransactionPromise;

expect(serverComponentTransaction).toBe(1);
expect(serverComponentTransaction).toBeDefined();
expect(serverComponentTransaction.request?.headers).toBeDefined();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { waitForError, waitForTransaction } from '@sentry-internal/test-utils';

test('Should create a transaction for middleware', async ({ request }) => {
const middlewareTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => {
return transactionEvent?.transaction === 'middleware' && transactionEvent?.contexts?.trace?.status === 'ok';
return transactionEvent?.transaction === 'middleware GET /api/endpoint-behind-middleware';
});

const response = await request.get('/api/endpoint-behind-middleware');
Expand All @@ -12,54 +12,48 @@ test('Should create a transaction for middleware', async ({ request }) => {
const middlewareTransaction = await middlewareTransactionPromise;

expect(middlewareTransaction.contexts?.trace?.status).toBe('ok');
expect(middlewareTransaction.contexts?.trace?.op).toBe('middleware.nextjs');
expect(middlewareTransaction.contexts?.trace?.op).toBe('http.server.middleware');
expect(middlewareTransaction.contexts?.runtime?.name).toBe('vercel-edge');

// Assert that isolation scope works properly
expect(middlewareTransaction.tags?.['my-isolated-tag']).toBe(true);
expect(middlewareTransaction.tags?.['my-global-scope-isolated-tag']).not.toBeDefined();
});

test('Should create a transaction with error status for faulty middleware', async ({ request }) => {
test('Faulty middlewares', async ({ request }) => {
const middlewareTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => {
return (
transactionEvent?.transaction === 'middleware' && transactionEvent?.contexts?.trace?.status === 'unknown_error'
);
return transactionEvent?.transaction === 'middleware GET /api/endpoint-behind-faulty-middleware';
});

request.get('/api/endpoint-behind-middleware', { headers: { 'x-should-throw': '1' } }).catch(() => {
// Noop
});

const middlewareTransaction = await middlewareTransactionPromise;

expect(middlewareTransaction.contexts?.trace?.status).toBe('unknown_error');
expect(middlewareTransaction.contexts?.trace?.op).toBe('middleware.nextjs');
expect(middlewareTransaction.contexts?.runtime?.name).toBe('vercel-edge');
});

// TODO(lforst): This cannot make it into production - Make sure to fix this test
test.skip('Records exceptions happening in middleware', async ({ request }) => {
const errorEventPromise = waitForError('nextjs-app-dir', errorEvent => {
return errorEvent?.exception?.values?.[0]?.value === 'Middleware Error';
});

request.get('/api/endpoint-behind-middleware', { headers: { 'x-should-throw': '1' } }).catch(() => {
request.get('/api/endpoint-behind-faulty-middleware', { headers: { 'x-should-throw': '1' } }).catch(() => {
// Noop
});

const errorEvent = await errorEventPromise;
await test.step('should record transactions', async () => {
const middlewareTransaction = await middlewareTransactionPromise;
expect(middlewareTransaction.contexts?.trace?.status).toBe('unknown_error');
expect(middlewareTransaction.contexts?.trace?.op).toBe('http.server.middleware');
expect(middlewareTransaction.contexts?.runtime?.name).toBe('vercel-edge');
});

// Assert that isolation scope works properly
expect(errorEvent.tags?.['my-isolated-tag']).toBe(true);
expect(errorEvent.tags?.['my-global-scope-isolated-tag']).not.toBeDefined();
expect(errorEvent.transaction).toBe('middleware');
await test.step('should record exceptions', async () => {
const errorEvent = await errorEventPromise;

// Assert that isolation scope works properly
expect(errorEvent.tags?.['my-isolated-tag']).toBe(true);
expect(errorEvent.tags?.['my-global-scope-isolated-tag']).not.toBeDefined();
expect(errorEvent.transaction).toBe('middleware GET /api/endpoint-behind-faulty-middleware');
});
});

test('Should trace outgoing fetch requests inside middleware and create breadcrumbs for it', async ({ request }) => {
const middlewareTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => {
return (
transactionEvent?.transaction === 'middleware' &&
transactionEvent?.transaction === 'middleware GET /api/endpoint-behind-middleware' &&
!!transactionEvent.spans?.find(span => span.op === 'http.client')
);
});
Expand Down
91 changes: 0 additions & 91 deletions packages/nextjs/src/common/utils/edgeWrapperUtils.ts

This file was deleted.

Loading
Loading