Skip to content
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

feat(nextjs): Use spans generated by Next.js for App Router #12729

Merged
merged 29 commits into from
Jul 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
7034d96
this will break in a million ways
lforst Jul 2, 2024
c4fc30b
Apply isolation scope to transaction
lforst Jul 2, 2024
e4b3f1d
Fix generation function tests
lforst Jul 2, 2024
5bd6e0b
Fix request instrumentation tests
lforst Jul 2, 2024
d35762a
Fix next-15 e2e tests
lforst Jul 2, 2024
79fabbf
Fix some nextjs-app-dir tests
lforst Jul 3, 2024
1cdfdc9
test: Fix e2e test race condition by buffering events
lforst Jul 3, 2024
f98a72f
Merge branch 'lforst-event-proxy-no-race-condition' into lforst-otel-…
lforst Jul 3, 2024
8737b7e
Merge branch 'develop' into lforst-otel-for-nextjs
lforst Jul 3, 2024
9ca8137
stuff
lforst Jul 3, 2024
0ba02be
interesting
lforst Jul 3, 2024
cda4fb3
Merge branch 'develop' into lforst-otel-for-nextjs
lforst Jul 4, 2024
8e75acf
wut
lforst Jul 4, 2024
18fe54d
test
lforst Jul 4, 2024
a3146c9
Merge branch 'develop' into lforst-otel-for-nextjs
lforst Jul 4, 2024
d4aa8f1
.
lforst Jul 4, 2024
84d9bd7
logic
lforst Jul 4, 2024
403b28e
Merge branch 'develop' into lforst-otel-for-nextjs
lforst Jul 4, 2024
a949cce
fix some stuff
lforst Jul 4, 2024
1dd7c77
dum dum
lforst Jul 4, 2024
1a8a2db
meep moop
lforst Jul 4, 2024
542bcda
drop ingest spans
lforst Jul 5, 2024
19260d6
span quality
lforst Jul 5, 2024
c5bd534
Merge branch 'develop' into lforst-otel-for-nextjs
lforst Jul 5, 2024
88f9a54
Add test
lforst Jul 5, 2024
2ecb806
Merge remote-tracking branch 'origin/develop' into lforst-otel-for-ne…
lforst Jul 8, 2024
df0c49e
Review feedback
lforst Jul 8, 2024
ceb9d2c
Add changelog entry
lforst Jul 8, 2024
690a07d
Merge branch 'develop' into lforst-otel-for-nextjs
lforst Jul 8, 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
21 changes: 21 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,27 @@

- "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott

### Important Changes

- **feat(nextjs): Use spans generated by Next.js for App Router (#12729)**

Previously, the `@sentry/nextjs` SDK automatically recorded spans in the form of transactions for each of your top-level
server components (pages, layouts, ...). This approach had a few drawbacks, the main ones being that traces didn't have
a root span, and more importantly, if you had data stream to the client, its duration was not captured because the
server component spans had finished before the data could finish streaming.

With this release, we will capture the duration of App Router requests in their entirety as a single transaction with
server component spans being descendants of that transaction. This means you will get more data that is also more
accurate. Note that this does not apply to the Edge runtime. For the Edge runtime, the SDK will emit transactions as it
has before.

Generally speaking, this change means that you will see less _transactions_ and more _spans_ in Sentry. Your will no
longer receive server component transactions like `Page Server Component (/path/to/route)` (unless using the Edge
runtime), and you will instead receive transactions for your App Router SSR requests that look like
`GET /path/to/route`.

If you are on Sentry SaaS, this may have an effect on your quota consumption: Less transactions, more spans.

## 8.15.0

- feat(core): allow unregistering callback through `on` (#11710)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
"test:assert": "pnpm test:prod && pnpm test:dev"
},
"dependencies": {
"@next/font": "13.0.7",
"@sentry/nextjs": "latest || *",
"@types/node": "18.11.17",
"@types/react": "18.0.26",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ import { NextResponse } from 'next/server';
export const dynamic = 'force-dynamic';

export async function GET() {
const data = await fetch(`http://localhost:3030/propagation/test-outgoing-fetch-external-disallowed/check`).then(
res => res.json(),
);
const data = await fetch(`http://localhost:3030/propagation/test-outgoing-fetch-external-disallowed/check`, {
cache: 'no-store',
}).then(res => res.json());
return NextResponse.json(data);
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import { NextResponse } from 'next/server';
export const dynamic = 'force-dynamic';

export async function GET() {
const data = await fetch(`http://localhost:3030/propagation/test-outgoing-fetch/check`).then(res => res.json());
const data = await fetch(`http://localhost:3030/propagation/test-outgoing-fetch/check`, { cache: 'no-store' }).then(
res => res.json(),
);
return NextResponse.json(data);
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,15 @@ if (!testEnv) {
throw new Error('No test env defined');
}

const config = getPlaywrightConfig({
startCommand: testEnv === 'development' ? 'pnpm next dev -p 3030' : 'pnpm next start -p 3030',
port: 3030,
});
const config = getPlaywrightConfig(
{
startCommand: testEnv === 'development' ? 'pnpm next dev -p 3030' : 'pnpm next start -p 3030',
port: 3030,
},
{
// This comes with the risk of tests leaking into each other but the tests run quite slow so we should parallelize
workers: '100%',
Copy link
Member

Choose a reason for hiding this comment

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

🤞

Copy link
Member Author

Choose a reason for hiding this comment

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

my thought was: If the tests leak, the tests are bad anyways.

Copy link
Member

Choose a reason for hiding this comment

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

👍

},
);

export default config;
Original file line number Diff line number Diff line change
@@ -1,19 +1,29 @@
import { expect, test } from '@playwright/test';
import { waitForError, waitForTransaction } from '@sentry-internal/test-utils';

test('Should send a transaction event for a generateMetadata() function invokation', async ({ page }) => {
const testTitle = 'foobarasdf';
test('Should emit a span for a generateMetadata() function invokation', async ({ page }) => {
const testTitle = 'should-emit-span';

const transactionPromise = waitForTransaction('nextjs-14', async transactionEvent => {
return (
transactionEvent?.transaction === 'Page.generateMetadata (/generation-functions)' &&
(transactionEvent.extra?.route_data as any)?.searchParams?.metadataTitle === testTitle
transactionEvent.contexts?.trace?.data?.['http.target'] === `/generation-functions?metadataTitle=${testTitle}`
);
});

await page.goto(`/generation-functions?metadataTitle=${testTitle}`);

expect(await transactionPromise).toBeDefined();
const transaction = await transactionPromise;

expect(transaction.spans).toContainEqual(
expect.objectContaining({
description: 'generateMetadata /generation-functions/page',
origin: 'manual',
parent_span_id: expect.any(String),
span_id: expect.any(String),
status: 'ok',
trace_id: expect.any(String),
}),
);

const pageTitle = await page.title();
expect(pageTitle).toBe(testTitle);
Expand All @@ -22,12 +32,12 @@ test('Should send a transaction event for a generateMetadata() function invokati
test('Should send a transaction and an error event for a faulty generateMetadata() function invokation', async ({
page,
}) => {
const testTitle = 'foobarbaz';
const testTitle = 'should-emit-error';

const transactionPromise = waitForTransaction('nextjs-14', async transactionEvent => {
return (
transactionEvent.transaction === 'Page.generateMetadata (/generation-functions)' &&
(transactionEvent.extra?.route_data as any)?.searchParams?.metadataTitle === testTitle
transactionEvent.contexts?.trace?.data?.['http.target'] ===
`/generation-functions?metadataTitle=${testTitle}&shouldThrowInGenerateMetadata=1`
);
});

Expand All @@ -54,14 +64,23 @@ test('Should send a transaction event for a generateViewport() function invokati

const transactionPromise = waitForTransaction('nextjs-14', async transactionEvent => {
return (
transactionEvent?.transaction === 'Page.generateViewport (/generation-functions)' &&
(transactionEvent.extra?.route_data as any)?.searchParams?.viewportThemeColor === testTitle
transactionEvent.contexts?.trace?.data?.['http.target'] ===
`/generation-functions?viewportThemeColor=${testTitle}`
);
});

await page.goto(`/generation-functions?viewportThemeColor=${testTitle}`);

expect(await transactionPromise).toBeDefined();
expect((await transactionPromise).spans).toContainEqual(
expect.objectContaining({
description: 'generateViewport /generation-functions/page',
origin: 'manual',
parent_span_id: expect.any(String),
span_id: expect.any(String),
status: 'ok',
trace_id: expect.any(String),
}),
);
});

test('Should send a transaction and an error event for a faulty generateViewport() function invokation', async ({
Expand All @@ -71,8 +90,8 @@ test('Should send a transaction and an error event for a faulty generateViewport

const transactionPromise = waitForTransaction('nextjs-14', async transactionEvent => {
return (
transactionEvent?.transaction === 'Page.generateViewport (/generation-functions)' &&
(transactionEvent.extra?.route_data as any)?.searchParams?.viewportThemeColor === testTitle
transactionEvent.contexts?.trace?.data?.['http.target'] ===
`/generation-functions?viewportThemeColor=${testTitle}&shouldThrowInGenerateViewport=1`
);
});

Expand All @@ -97,8 +116,8 @@ test('Should send a transaction event with correct status for a generateMetadata

const transactionPromise = waitForTransaction('nextjs-14', async transactionEvent => {
return (
transactionEvent?.transaction === 'Page.generateMetadata (/generation-functions/with-redirect)' &&
(transactionEvent.extra?.route_data as any)?.searchParams?.metadataTitle === testTitle
transactionEvent.contexts?.trace?.data?.['http.target'] ===
`/generation-functions/with-redirect?metadataTitle=${testTitle}`
);
});

Expand All @@ -114,8 +133,8 @@ test('Should send a transaction event with correct status for a generateMetadata

const transactionPromise = waitForTransaction('nextjs-14', async transactionEvent => {
return (
transactionEvent?.transaction === 'Page.generateMetadata (/generation-functions/with-notfound)' &&
(transactionEvent.extra?.route_data as any)?.searchParams?.metadataTitle === testTitle
transactionEvent.contexts?.trace?.data?.['http.target'] ===
`/generation-functions/with-notfound?metadataTitle=${testTitle}`
);
});

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

test('Should send a transaction with a fetch span', async ({ page }) => {
const transactionPromise = waitForTransaction('nextjs-14', async transactionEvent => {
return transactionEvent?.transaction === 'Page Server Component (/request-instrumentation)';
return transactionEvent?.transaction === 'GET /request-instrumentation';
});

await page.goto(`/request-instrumentation`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ export default async function Page() {
}

export async function generateMetadata() {
(await fetch('http://example.com/')).text();
(await fetch('http://example.com/', { cache: 'no-store' })).text();

return {
title: 'my title',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,15 @@ if (!testEnv) {
throw new Error('No test env defined');
}

const config = getPlaywrightConfig({
startCommand: testEnv === 'development' ? 'pnpm next dev -p 3030' : 'pnpm next start -p 3030',
port: 3030,
});
const config = getPlaywrightConfig(
{
startCommand: testEnv === 'development' ? 'pnpm next dev -p 3030' : 'pnpm next start -p 3030',
port: 3030,
},
{
// This comes with the risk of tests leaking into each other but the tests run quite slow so we should parallelize
workers: '100%',
},
);

export default config;
Original file line number Diff line number Diff line change
@@ -1,17 +1,9 @@
import { expect, test } from '@playwright/test';
import { waitForTransaction } from '@sentry-internal/test-utils';

test('all server component transactions should be attached to the pageload request span', async ({ page }) => {
const pageServerComponentTransactionPromise = waitForTransaction('nextjs-15', async transactionEvent => {
return transactionEvent?.transaction === 'Page Server Component (/pageload-tracing)';
});

const layoutServerComponentTransactionPromise = waitForTransaction('nextjs-15', async transactionEvent => {
return transactionEvent?.transaction === 'Layout Server Component (/pageload-tracing)';
});

const metadataTransactionPromise = waitForTransaction('nextjs-15', async transactionEvent => {
return transactionEvent?.transaction === 'Page.generateMetadata (/pageload-tracing)';
test('App router transactions should be attached to the pageload request span', async ({ page }) => {
const serverTransactionPromise = waitForTransaction('nextjs-15', async transactionEvent => {
return transactionEvent?.transaction === 'GET /pageload-tracing';
});

const pageloadTransactionPromise = waitForTransaction('nextjs-15', async transactionEvent => {
Expand All @@ -20,18 +12,13 @@ test('all server component transactions should be attached to the pageload reque

await page.goto(`/pageload-tracing`);

const [pageServerComponentTransaction, layoutServerComponentTransaction, metadataTransaction, pageloadTransaction] =
await Promise.all([
pageServerComponentTransactionPromise,
layoutServerComponentTransactionPromise,
metadataTransactionPromise,
pageloadTransactionPromise,
]);
const [serverTransaction, pageloadTransaction] = await Promise.all([
serverTransactionPromise,
pageloadTransactionPromise,
]);

const pageloadTraceId = pageloadTransaction.contexts?.trace?.trace_id;

expect(pageloadTraceId).toBeTruthy();
expect(pageServerComponentTransaction.contexts?.trace?.trace_id).toBe(pageloadTraceId);
expect(layoutServerComponentTransaction.contexts?.trace?.trace_id).toBe(pageloadTraceId);
expect(metadataTransaction.contexts?.trace?.trace_id).toBe(pageloadTraceId);
expect(serverTransaction.contexts?.trace?.trace_id).toBe(pageloadTraceId);
});
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@ import { waitForError, waitForTransaction } from '@sentry-internal/test-utils';

test('should not capture React-internal errors for PPR rendering', async ({ page }) => {
const pageServerComponentTransactionPromise = waitForTransaction('nextjs-15', async transactionEvent => {
return transactionEvent?.transaction === 'Page Server Component (/ppr-error/[param])';
return transactionEvent?.transaction === 'GET /ppr-error/[param]';
});

let errorEventReceived = false;
waitForError('nextjs-15', async transactionEvent => {
return transactionEvent?.transaction === 'Page Server Component (/ppr-error/[param])';
waitForError('nextjs-15', async errorEvent => {
return errorEvent?.transaction === 'Page Server Component (/ppr-error/[param])';
}).then(() => {
errorEventReceived = true;
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,19 @@ import { waitForError, waitForTransaction } from '@sentry-internal/test-utils';

test('should not capture serverside suspense errors', async ({ page }) => {
const pageServerComponentTransactionPromise = waitForTransaction('nextjs-15', async transactionEvent => {
return transactionEvent?.transaction === 'Page Server Component (/suspense-error)';
return transactionEvent?.transaction === 'GET /suspense-error';
});

let errorEvent;
waitForError('nextjs-15', async transactionEvent => {
return transactionEvent?.transaction === 'Page Server Component (/suspense-error)';
waitForError('nextjs-15', async errorEvent => {
return errorEvent?.transaction === 'Page Server Component (/suspense-error)';
}).then(event => {
errorEvent = event;
});

await page.goto(`/suspense-error`);

// Just to be a little bit more sure
await page.waitForTimeout(5000);

const pageServerComponentTransaction = await pageServerComponentTransactionPromise;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
"test:assert": "pnpm test:test-build && pnpm test:prod && pnpm test:dev"
},
"dependencies": {
"@next/font": "13.0.7",
"@sentry/nextjs": "latest || *",
"@types/node": "18.11.17",
"@types/react": "18.0.26",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,15 @@ if (!testEnv) {
throw new Error('No test env defined');
}

const config = getPlaywrightConfig({
startCommand: testEnv === 'development' ? 'pnpm next dev -p 3030' : 'pnpm next start -p 3030',
port: 3030,
});
const config = getPlaywrightConfig(
{
startCommand: testEnv === 'development' ? 'pnpm next dev -p 3030' : 'pnpm next start -p 3030',
port: 3030,
},
{
// This comes with the risk of tests leaking into each other but the tests run quite slow so we should parallelize
workers: '100%',
},
);

export default config;
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ test('Creates a navigation transaction for app router routes', async ({ page })

await page.goto(`/server-component/parameter/${randomRoute}`);
await clientPageloadTransactionPromise;
await page.getByText('Page (/server-component/parameter/[parameter])').isVisible();
await page.getByText('Page (/server-component/[parameter])').isVisible();

const clientNavigationTransactionPromise = waitForTransaction('nextjs-app-dir', transactionEvent => {
return (
Expand All @@ -39,7 +39,7 @@ test('Creates a navigation transaction for app router routes', async ({ page })

const serverComponentTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => {
return (
transactionEvent?.transaction === 'Page Server Component (/server-component/parameter/[...parameters])' &&
transactionEvent?.transaction === 'GET /server-component/parameter/foo/bar/baz' &&
(await clientNavigationTransactionPromise).contexts?.trace?.trace_id ===
transactionEvent.contexts?.trace?.trace_id
);
Expand Down
Loading
Loading