Skip to content

feat(nextjs): Record transaction name source when creating transactions #5391

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 2 commits into from
Jul 8, 2022
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
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' }) },
);
});
});
});