Skip to content

Commit

Permalink
feat(nextjs): Record transaction name source when creating transactio…
Browse files Browse the repository at this point in the history
…ns (#5391)

This adds information about the source of the transaction name to all transactions created by the nextjs SDK.
  • Loading branch information
lobsterkatie authored Jul 8, 2022
1 parent e97a639 commit 3b76a62
Show file tree
Hide file tree
Showing 8 changed files with 71 additions and 4 deletions.
8 changes: 7 additions & 1 deletion packages/nextjs/src/performance/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,16 @@ export function nextRouterInstrumentation(
// route name. Setting the transaction name after the transaction is started could lead
// to possible race conditions with the router, so this approach was taken.
if (startTransactionOnPageLoad) {
prevTransactionName = Router.route !== null ? stripUrlQueryAndFragment(Router.route) : global.location.pathname;
const pathIsRoute = Router.route !== null;

prevTransactionName = pathIsRoute ? stripUrlQueryAndFragment(Router.route) : global.location.pathname;
activeTransaction = startTransactionCb({
name: prevTransactionName,
op: 'pageload',
tags: DEFAULT_TAGS,
metadata: {
source: pathIsRoute ? 'route' : 'url',
},
});
}

Expand Down Expand Up @@ -105,6 +110,7 @@ function changeStateWrapper(originalChangeStateWrapper: RouterChangeState): Wrap
name: prevTransactionName,
op: 'navigation',
tags,
metadata: { source: 'route' },
});
}
return originalChangeStateWrapper.call(this, method, url, as, options, ...args);
Expand Down
10 changes: 9 additions & 1 deletion packages/nextjs/src/utils/instrumentServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,14 @@ function makeWrappedReqHandler(origReqHandler: ReqHandler): WrappedReqHandler {
{
name: `${namePrefix}${reqPath}`,
op: 'http.server',
metadata: { requestPath: reqPath, baggage },
metadata: {
baggage,
requestPath: reqPath,
// TODO: Investigate if there's a way to tell if this is a dynamic route, so that we can make this more
// like `source: isDynamicRoute? 'url' : 'route'`
// TODO: What happens when `withSentry` is used also? Which values of `name` and `source` win?
source: 'url',
},
...traceparentData,
},
// Extra context passed to the `tracesSampler` (Note: We're combining `nextReq` and `req` this way in order
Expand Down Expand Up @@ -326,6 +333,7 @@ function makeWrappedMethodForGettingParameterizedPath(
if (transaction && transaction.metadata.requestPath) {
const origPath = transaction.metadata.requestPath;
transaction.name = transaction.name.replace(origPath, parameterizedPath);
transaction.setMetadata({ source: 'route' });
}

return origMethod.call(this, parameterizedPath, ...args);
Expand Down
4 changes: 2 additions & 2 deletions packages/nextjs/src/utils/withSentry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler =
// Replace with placeholder
if (req.query) {
// TODO get this from next if possible, to avoid accidentally replacing non-dynamic parts of the path if
// they match dynamic parts
// they happen to match the values of any of the dynamic parts
for (const [key, value] of Object.entries(req.query)) {
reqPath = reqPath.replace(`${value}`, `[${key}]`);
}
Expand All @@ -72,7 +72,7 @@ export const withSentry = (origHandler: NextApiHandler): WrappedNextApiHandler =
name: `${reqMethod}${reqPath}`,
op: 'http.server',
...traceparentData,
metadata: { baggage },
metadata: { baggage, source: 'route' },
},
// extra context passed to the `tracesSampler`
{ request: req },
Expand Down
3 changes: 3 additions & 0 deletions packages/nextjs/test/integration/test/server/tracing200.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ module.exports = async ({ url: urlBase, argv }) => {
},
},
transaction: 'GET /api/users',
transaction_info: {
source: 'route',
},
type: 'transaction',
request: {
url,
Expand Down
3 changes: 3 additions & 0 deletions packages/nextjs/test/integration/test/server/tracing500.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ module.exports = async ({ url: urlBase, argv }) => {
},
},
transaction: 'GET /api/broken',
transaction_info: {
source: 'route',
},
type: 'transaction',
request: {
url,
Expand Down
3 changes: 3 additions & 0 deletions packages/nextjs/test/integration/test/server/tracingHttp.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ module.exports = async ({ url: urlBase, argv }) => {
},
],
transaction: 'GET /api/http',
transaction_info: {
source: 'route',
},
type: 'transaction',
request: {
url,
Expand Down
18 changes: 18 additions & 0 deletions packages/nextjs/test/performance/client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ describe('client', () => {
tags: {
'routing.instrumentation': 'next-router',
},
metadata: {
source: 'route',
},
});
});

Expand All @@ -68,6 +71,9 @@ describe('client', () => {
method: 'pushState',
'routing.instrumentation': 'next-router',
},
metadata: {
source: 'route',
},
},
],
[
Expand All @@ -81,6 +87,9 @@ describe('client', () => {
method: 'replaceState',
'routing.instrumentation': 'next-router',
},
metadata: {
source: 'route',
},
},
],
[
Expand All @@ -94,6 +103,9 @@ describe('client', () => {
method: 'pushState',
'routing.instrumentation': 'next-router',
},
metadata: {
source: 'route',
},
},
],
];
Expand All @@ -120,6 +132,9 @@ describe('client', () => {
name: '/login',
op: 'navigation',
tags: { from: '/[user]/posts/[id]', method: 'pushState', 'routing.instrumentation': 'next-router' },
metadata: {
source: 'route',
},
});

Router.router?.changeState('pushState', '/login', '/login', {});
Expand All @@ -131,6 +146,9 @@ describe('client', () => {
name: '/posts/[id]',
op: 'navigation',
tags: { from: '/login', method: 'pushState', 'routing.instrumentation': 'next-router' },
metadata: {
source: 'route',
},
});
});
});
Expand Down
26 changes: 26 additions & 0 deletions packages/nextjs/test/utils/withSentry.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import * as hub from '@sentry/hub';
import * as Sentry from '@sentry/node';
import { Client, ClientOptions } from '@sentry/types';
import * as utils from '@sentry/utils';
import { NextApiHandler, NextApiRequest, NextApiResponse } from 'next';

Expand Down Expand Up @@ -43,6 +45,7 @@ const flushSpy = jest.spyOn(Sentry, 'flush').mockImplementation(async () => {
await sleep(FLUSH_DURATION);
return true;
});
const startTransactionSpy = jest.spyOn(Sentry, 'startTransaction');

describe('withSentry', () => {
let req: NextApiRequest, res: NextApiResponse;
Expand Down Expand Up @@ -99,4 +102,27 @@ describe('withSentry', () => {
expect(loggerSpy).toHaveBeenCalledWith('Done flushing events');
});
});

describe('tracing', () => {
it('starts a transaction when tracing is enabled', async () => {
jest
.spyOn(hub.Hub.prototype, 'getClient')
.mockReturnValueOnce({ getOptions: () => ({ tracesSampleRate: 1 } as ClientOptions) } as Client);

await callWrappedHandler(wrappedHandlerNoError, req, res);

expect(startTransactionSpy).toHaveBeenCalledWith(
{
name: 'GET http://dogs.are.great',
op: 'http.server',

metadata: {
baggage: expect.any(Array),
source: 'route',
},
},
{ request: expect.objectContaining({ url: 'http://dogs.are.great' }) },
);
});
});
});

0 comments on commit 3b76a62

Please sign in to comment.