Skip to content

Commit

Permalink
feat(nextjs): Enable nextjs otel spans pages router (#13960)
Browse files Browse the repository at this point in the history
  • Loading branch information
chargome authored Oct 17, 2024
1 parent a90d899 commit 74b68c7
Show file tree
Hide file tree
Showing 16 changed files with 206 additions and 244 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export function register() {
// We are doing a lot of events at once in this test app
bufferSize: 1000,
},
debug: false,
});
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
"@types/node": "18.11.17",
"@types/react": "18.0.26",
"@types/react-dom": "18.0.9",
"next": "13.2.0",
"next": "13.5.7",
"react": "18.2.0",
"react-dom": "18.2.0",
"typescript": "4.9.5"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,38 @@ test('should create a pageload transaction when the `pages` directory is used',
type: 'transaction',
});
});

test('should create a pageload transaction with correct name when an error occurs in getServerSideProps', async ({
page,
}) => {
const transactionPromise = waitForTransaction('nextjs-13', async transactionEvent => {
return (
transactionEvent.transaction === '/[param]/error-getServerSideProps' &&
transactionEvent.contexts?.trace?.op === 'pageload'
);
});

await page.goto(`/something/error-getServerSideProps`, { waitUntil: 'networkidle' });

const transaction = await transactionPromise;

expect(transaction).toMatchObject({
contexts: {
trace: {
data: {
'sentry.op': 'pageload',
'sentry.origin': 'auto.pageload.nextjs.pages_router_instrumentation',
'sentry.source': 'route',
},
op: 'pageload',
origin: 'auto.pageload.nextjs.pages_router_instrumentation',
},
},
transaction: '/[param]/error-getServerSideProps',
transaction_info: { source: 'route' },
type: 'transaction',
});

// Ensure the transaction name is not '/_error'
expect(transaction.transaction).not.toBe('/_error');
});
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ test('should propagate serverside `getInitialProps` trace to client', async ({ p

const serverTransactionPromise = waitForTransaction('nextjs-13', async transactionEvent => {
return (
transactionEvent.transaction === '/[param]/withInitialProps' &&
transactionEvent.transaction === 'GET /[param]/withInitialProps' &&
transactionEvent.contexts?.trace?.op === 'http.server'
);
});
Expand Down Expand Up @@ -47,7 +47,7 @@ test('should propagate serverside `getInitialProps` trace to client', async ({ p
status: 'ok',
},
},
transaction: '/[param]/withInitialProps',
transaction: 'GET /[param]/withInitialProps',
transaction_info: {
source: 'route',
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ test('Should record performance for getServerSideProps', async ({ page }) => {

const serverTransactionPromise = waitForTransaction('nextjs-13', async transactionEvent => {
return (
transactionEvent.transaction === '/[param]/withServerSideProps' &&
transactionEvent.transaction === 'GET /[param]/withServerSideProps' &&
transactionEvent.contexts?.trace?.op === 'http.server'
);
});
Expand Down Expand Up @@ -47,7 +47,7 @@ test('Should record performance for getServerSideProps', async ({ page }) => {
status: 'ok',
},
},
transaction: '/[param]/withServerSideProps',
transaction: 'GET /[param]/withServerSideProps',
transaction_info: {
source: 'route',
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ test('Should report an error event for errors thrown in getServerSideProps', asy

const transactionEventPromise = waitForTransaction('nextjs-13', transactionEvent => {
return (
transactionEvent.transaction === '/error-getServerSideProps' &&
transactionEvent.transaction === 'GET /[param]/error-getServerSideProps' &&
transactionEvent.contexts?.trace?.op === 'http.server'
);
});

await page.goto('/error-getServerSideProps');
await page.goto('/dogsaregreat/error-getServerSideProps');

expect(await errorEventPromise).toMatchObject({
contexts: {
Expand All @@ -40,7 +40,7 @@ test('Should report an error event for errors thrown in getServerSideProps', asy
url: expect.stringMatching(/^http.*\/error-getServerSideProps/),
},
timestamp: expect.any(Number),
transaction: 'getServerSideProps (/error-getServerSideProps)',
transaction: 'getServerSideProps (/[param]/error-getServerSideProps)',
});

expect(await transactionEventPromise).toMatchObject({
Expand All @@ -60,11 +60,11 @@ test('Should report an error event for errors thrown in getServerSideProps', asy
data: {
'http.response.status_code': 500,
'sentry.op': 'http.server',
'sentry.origin': 'auto.function.nextjs',
'sentry.origin': expect.stringMatching(/^(auto|auto\.http\.otel\.http)$/),
'sentry.source': 'route',
},
op: 'http.server',
origin: 'auto.function.nextjs',
origin: expect.stringMatching(/^(auto|auto\.http\.otel\.http)$/),
span_id: expect.any(String),
status: 'internal_error',
trace_id: expect.any(String),
Expand All @@ -80,8 +80,9 @@ test('Should report an error event for errors thrown in getServerSideProps', asy
},
start_timestamp: expect.any(Number),
timestamp: expect.any(Number),
transaction: '/error-getServerSideProps',
transaction_info: { source: 'route' },
transaction: 'GET /[param]/error-getServerSideProps',
// TODO: This test fails depending on the next version (next 13: 'custom', next >14: 'route')
// transaction_info: { source: 'custom' },
type: 'transaction',
});
});
Expand All @@ -95,11 +96,12 @@ test('Should report an error event for errors thrown in getServerSideProps in pa

const transactionEventPromise = waitForTransaction('nextjs-13', transactionEvent => {
return (
transactionEvent.transaction === '/customPageExtension' && transactionEvent.contexts?.trace?.op === 'http.server'
transactionEvent.transaction === 'GET /[param]/customPageExtension' &&
transactionEvent.contexts?.trace?.op === 'http.server'
);
});

await page.goto('/customPageExtension');
await page.goto('/123/customPageExtension');

expect(await errorEventPromise).toMatchObject({
contexts: {
Expand All @@ -126,7 +128,7 @@ test('Should report an error event for errors thrown in getServerSideProps in pa
url: expect.stringMatching(/^http.*\/customPageExtension/),
},
timestamp: expect.any(Number),
transaction: 'getServerSideProps (/customPageExtension)',
transaction: 'getServerSideProps (/[param]/customPageExtension)',
});

expect(await transactionEventPromise).toMatchObject({
Expand All @@ -146,11 +148,11 @@ test('Should report an error event for errors thrown in getServerSideProps in pa
data: {
'http.response.status_code': 500,
'sentry.op': 'http.server',
'sentry.origin': 'auto.function.nextjs',
'sentry.origin': expect.stringMatching(/^auto(\.http\.otel\.http)?$/),
'sentry.source': 'route',
},
op: 'http.server',
origin: 'auto.function.nextjs',
origin: expect.stringMatching(/^auto(\.http\.otel\.http)?$/),
span_id: expect.any(String),
status: 'internal_error',
trace_id: expect.any(String),
Expand All @@ -166,8 +168,9 @@ test('Should report an error event for errors thrown in getServerSideProps in pa
},
start_timestamp: expect.any(Number),
timestamp: expect.any(Number),
transaction: '/customPageExtension',
transaction_info: { source: 'route' },
transaction: 'GET /[param]/customPageExtension',
// TODO: This test fails depending on the next version (next 13: 'custom', next >14: 'route')
// transaction_info: { source: 'custom' },
type: 'transaction',
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ test('Will capture error for SSR rendering error with a connected trace (Class C

const serverComponentTransaction = waitForTransaction('nextjs-app-dir', async transactionEvent => {
return (
transactionEvent?.transaction === '/pages-router/ssr-error-class' &&
transactionEvent?.transaction === 'GET /pages-router/ssr-error-class' &&
(await errorEventPromise).contexts?.trace?.trace_id === transactionEvent.contexts?.trace?.trace_id
);
});
Expand All @@ -26,7 +26,7 @@ test('Will capture error for SSR rendering error with a connected trace (Functio

const ssrTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => {
return (
transactionEvent?.transaction === '/pages-router/ssr-error-fc' &&
transactionEvent?.transaction === 'GET /pages-router/ssr-error-fc' &&
(await errorEventPromise).contexts?.trace?.trace_id === transactionEvent.contexts?.trace?.trace_id
);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {
} from '@sentry/core';
import { WINDOW, startBrowserTracingNavigationSpan, startBrowserTracingPageLoadSpan } from '@sentry/react';
import type { Client, TransactionSource } from '@sentry/types';
import { browserPerformanceTimeOrigin, logger, stripUrlQueryAndFragment } from '@sentry/utils';
import { browserPerformanceTimeOrigin, logger, parseBaggageHeader, stripUrlQueryAndFragment } from '@sentry/utils';

import type { NEXT_DATA } from 'next/dist/shared/lib/utils';
import RouterImport from 'next/router';
Expand Down Expand Up @@ -106,7 +106,15 @@ function extractNextDataTagInformation(): NextDataTagInfo {
*/
export function pagesRouterInstrumentPageLoad(client: Client): void {
const { route, params, sentryTrace, baggage } = extractNextDataTagInformation();
const name = route || globalObject.location.pathname;
const parsedBaggage = parseBaggageHeader(baggage);
let name = route || globalObject.location.pathname;

// /_error is the fallback page for all errors. If there is a transaction name for /_error, use that instead
if (parsedBaggage && parsedBaggage['sentry-transaction'] && name === '/_error') {
name = parsedBaggage['sentry-transaction'];
// Strip any HTTP method from the span name
name = name.replace(/^(GET|POST|PUT|DELETE|PATCH|HEAD|OPTIONS|TRACE|CONNECT)\s+/i, '');
}

startBrowserTracingPageLoadSpan(
client,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ type Props = { [key: string]: unknown };
*/
export function wrapGetStaticPropsWithSentry(
origGetStaticPropsa: GetStaticProps<Props>,
parameterizedRoute: string,
_parameterizedRoute: string,
): GetStaticProps<Props> {
return new Proxy(origGetStaticPropsa, {
apply: async (wrappingTarget, thisArg, args: Parameters<GetStaticProps<Props>>) => {
Expand All @@ -23,10 +23,7 @@ export function wrapGetStaticPropsWithSentry(
}

const errorWrappedGetStaticProps = withErrorInstrumentation(wrappingTarget);
return callDataFetcherTraced(errorWrappedGetStaticProps, args, {
parameterizedRoute,
dataFetchingMethodName: 'getStaticProps',
});
return callDataFetcherTraced(errorWrappedGetStaticProps, args);
},
});
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { captureException, getCurrentScope, withIsolationScope } from '@sentry/core';
import { extractTraceparentData } from '@sentry/utils';
import { dropNextjsRootContext, escapeNextjsTracing } from '../utils/tracingUtils';

interface FunctionComponent {
(...args: unknown[]): unknown;
Expand All @@ -25,70 +24,64 @@ export function wrapPageComponentWithSentry(pageComponent: FunctionComponent | C
if (isReactClassComponent(pageComponent)) {
return class SentryWrappedPageComponent extends pageComponent {
public render(...args: unknown[]): unknown {
dropNextjsRootContext();
return escapeNextjsTracing(() => {
return withIsolationScope(() => {
const scope = getCurrentScope();
// We extract the sentry trace data that is put in the component props by datafetcher wrappers
const sentryTraceData =
typeof this.props === 'object' &&
this.props !== null &&
'_sentryTraceData' in this.props &&
typeof this.props._sentryTraceData === 'string'
? this.props._sentryTraceData
: undefined;
return withIsolationScope(() => {
const scope = getCurrentScope();
// We extract the sentry trace data that is put in the component props by datafetcher wrappers
const sentryTraceData =
typeof this.props === 'object' &&
this.props !== null &&
'_sentryTraceData' in this.props &&
typeof this.props._sentryTraceData === 'string'
? this.props._sentryTraceData
: undefined;

if (sentryTraceData) {
const traceparentData = extractTraceparentData(sentryTraceData);
scope.setContext('trace', {
span_id: traceparentData?.parentSpanId,
trace_id: traceparentData?.traceId,
});
}
if (sentryTraceData) {
const traceparentData = extractTraceparentData(sentryTraceData);
scope.setContext('trace', {
span_id: traceparentData?.parentSpanId,
trace_id: traceparentData?.traceId,
});
}

try {
return super.render(...args);
} catch (e) {
captureException(e, {
mechanism: {
handled: false,
},
});
throw e;
}
});
try {
return super.render(...args);
} catch (e) {
captureException(e, {
mechanism: {
handled: false,
},
});
throw e;
}
});
}
};
} else if (typeof pageComponent === 'function') {
return new Proxy(pageComponent, {
apply(target, thisArg, argArray: [{ _sentryTraceData?: string } | undefined]) {
dropNextjsRootContext();
return escapeNextjsTracing(() => {
return withIsolationScope(() => {
const scope = getCurrentScope();
// We extract the sentry trace data that is put in the component props by datafetcher wrappers
const sentryTraceData = argArray?.[0]?._sentryTraceData;
return withIsolationScope(() => {
const scope = getCurrentScope();
// We extract the sentry trace data that is put in the component props by datafetcher wrappers
const sentryTraceData = argArray?.[0]?._sentryTraceData;

if (sentryTraceData) {
const traceparentData = extractTraceparentData(sentryTraceData);
scope.setContext('trace', {
span_id: traceparentData?.parentSpanId,
trace_id: traceparentData?.traceId,
});
}
if (sentryTraceData) {
const traceparentData = extractTraceparentData(sentryTraceData);
scope.setContext('trace', {
span_id: traceparentData?.parentSpanId,
trace_id: traceparentData?.traceId,
});
}

try {
return target.apply(thisArg, argArray);
} catch (e) {
captureException(e, {
mechanism: {
handled: false,
},
});
throw e;
}
});
try {
return target.apply(thisArg, argArray);
} catch (e) {
captureException(e, {
mechanism: {
handled: false,
},
});
throw e;
}
});
},
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,5 @@
export const TRANSACTION_ATTR_SHOULD_DROP_TRANSACTION = 'sentry.drop_transaction';

export const TRANSACTION_ATTR_SENTRY_TRACE_BACKFILL = 'sentry.sentry_trace_backfill';

export const TRANSACTION_ATTR_SENTRY_ROUTE_BACKFILL = 'sentry.route_backfill';
Loading

0 comments on commit 74b68c7

Please sign in to comment.