From bdbd01fc7457e63144c2382402ceb3895ab44139 Mon Sep 17 00:00:00 2001
From: iker-barriocanal <32816711+iker-barriocanal@users.noreply.github.com>
Date: Mon, 17 May 2021 15:47:45 +0200
Subject: [PATCH 01/19] First client performance monitoring steps

---
 packages/nextjs/src/index.client.ts           |  3 ++
 packages/nextjs/src/performance/client.ts     | 27 ++++++++++++
 .../nextjs/src/performance/routerWrapper.ts   | 41 +++++++++++++++++++
 3 files changed, 71 insertions(+)
 create mode 100644 packages/nextjs/src/performance/client.ts
 create mode 100644 packages/nextjs/src/performance/routerWrapper.ts

diff --git a/packages/nextjs/src/index.client.ts b/packages/nextjs/src/index.client.ts
index 1c792523148c..df987b51bb95 100644
--- a/packages/nextjs/src/index.client.ts
+++ b/packages/nextjs/src/index.client.ts
@@ -1,5 +1,6 @@
 import { configureScope, init as reactInit } from '@sentry/react';
 
+import { startClientPerfMonitoring } from './performance/client';
 import { MetadataBuilder } from './utils/metadataBuilder';
 import { NextjsOptions } from './utils/nextjsOptions';
 
@@ -14,4 +15,6 @@ export function init(options: NextjsOptions): void {
   configureScope(scope => {
     scope.setTag('runtime', 'browser');
   });
+
+  startClientPerfMonitoring();
 }
diff --git a/packages/nextjs/src/performance/client.ts b/packages/nextjs/src/performance/client.ts
new file mode 100644
index 000000000000..aacd2382e63c
--- /dev/null
+++ b/packages/nextjs/src/performance/client.ts
@@ -0,0 +1,27 @@
+import { TransactionContext } from '@sentry/types';
+
+import * as Sentry from '../index.client';
+import { wrapRouter } from './routerWrapper';
+
+export function startClientPerfMonitoring(): void {
+  wrapRouter();
+  startInitialTransaction();
+}
+
+function startInitialTransaction(): void {
+  const initialTransactionCtx = getInitialTransactionContext();
+  const transaction = Sentry.startTransaction(initialTransactionCtx);
+  Sentry.getCurrentHub()
+    .getScope()
+    ?.setSpan(transaction);
+}
+
+function getInitialTransactionContext(): TransactionContext {
+  return {
+    name: `GET ${window.location.pathname}`,
+    // Operation target format is `<protocol>.client`.
+    // `window.location.protocol` is `<protocol>:`, so remove the `:`.
+    op: `${window.location.protocol.slice(0, -1)}.client`,
+    sampled: true, // For testing purposes, remove for production
+  };
+}
diff --git a/packages/nextjs/src/performance/routerWrapper.ts b/packages/nextjs/src/performance/routerWrapper.ts
new file mode 100644
index 000000000000..6a73d622724e
--- /dev/null
+++ b/packages/nextjs/src/performance/routerWrapper.ts
@@ -0,0 +1,41 @@
+import { fill } from '@sentry/utils';
+import Router from 'next/router';
+
+/**
+ * Wraps the Next.js Router's methods to add performance monitoring.
+ * https://nextjs.org/docs/api-reference/next/router#router-object
+ */
+export function wrapRouter(): void {
+  // At this execution point, the router hasn't been created yet, so it cannot be wrapped.
+  // However, we can use the following callback to wrap it once it's been created.
+  Router.ready(() => {
+    // There should always be a router at this point; but checking again purely for types.
+    if (!Router.router) return;
+    const router = Router.router;
+    fill(Object.getPrototypeOf(router), 'push', pushWrapper);
+    // TODO: pop
+    // TODO: replace
+    // TODO: prefetch
+    // TODO: beforepopstate
+    // TODO: back
+    // TODO: reload
+  });
+}
+
+type RouterPush = () => Promise<boolean>;
+type WrappedRouterPush = RouterPush;
+
+/**
+ * Wraps the `push` of the Next.js client.
+ * https://nextjs.org/docs/api-reference/next/router#routerpush
+ */
+export function pushWrapper(originalPush: RouterPush): WrappedRouterPush {
+  // The additional arguments must have the same type as the original `push` function, see
+  // https://github.com/vercel/next.js/blob/da97a18dafc7799e63aa7985adc95f213c2bf5f3/packages/next/next-server/lib/router/router.ts#L763
+  // Not including it means leads to: `TypeError: Cannot read property 'auth' of undefined`, similar to
+  // https://github.com/vercel/next.js/issues/11513
+  const wrapper = function(this: any, ...args: any[]): Promise<boolean> {
+    return originalPush.call(this, ...args);
+  };
+  return wrapper;
+}

From d4f73b299124f4342ac749c926a1f62e86ba169f Mon Sep 17 00:00:00 2001
From: iker-barriocanal <32816711+iker-barriocanal@users.noreply.github.com>
Date: Tue, 18 May 2021 17:58:42 +0200
Subject: [PATCH 02/19] ref: Move towards router instrumentation

---
 packages/nextjs/src/index.client.ts           |  4 +-
 packages/nextjs/src/performance/client.ts     | 40 +++++------
 .../src/performance/nextRouterWrapper.ts      | 67 +++++++++++++++++++
 .../nextjs/src/performance/routerWrapper.ts   | 41 ------------
 4 files changed, 85 insertions(+), 67 deletions(-)
 create mode 100644 packages/nextjs/src/performance/nextRouterWrapper.ts
 delete mode 100644 packages/nextjs/src/performance/routerWrapper.ts

diff --git a/packages/nextjs/src/index.client.ts b/packages/nextjs/src/index.client.ts
index df987b51bb95..db1aba8413fe 100644
--- a/packages/nextjs/src/index.client.ts
+++ b/packages/nextjs/src/index.client.ts
@@ -1,10 +1,10 @@
 import { configureScope, init as reactInit } from '@sentry/react';
 
-import { startClientPerfMonitoring } from './performance/client';
 import { MetadataBuilder } from './utils/metadataBuilder';
 import { NextjsOptions } from './utils/nextjsOptions';
 
 export * from '@sentry/react';
+export { nextRouterInstrumentation } from './performance/client';
 
 /** Inits the Sentry NextJS SDK on the browser with the React SDK. */
 export function init(options: NextjsOptions): void {
@@ -15,6 +15,4 @@ export function init(options: NextjsOptions): void {
   configureScope(scope => {
     scope.setTag('runtime', 'browser');
   });
-
-  startClientPerfMonitoring();
 }
diff --git a/packages/nextjs/src/performance/client.ts b/packages/nextjs/src/performance/client.ts
index aacd2382e63c..ad2cf8f1a6af 100644
--- a/packages/nextjs/src/performance/client.ts
+++ b/packages/nextjs/src/performance/client.ts
@@ -1,27 +1,21 @@
-import { TransactionContext } from '@sentry/types';
+import { Transaction, TransactionContext } from '@sentry/types';
 
-import * as Sentry from '../index.client';
-import { wrapRouter } from './routerWrapper';
+import { wrapRouter } from './nextRouterWrapper';
 
-export function startClientPerfMonitoring(): void {
-  wrapRouter();
-  startInitialTransaction();
+export function nextRouterInstrumentation(
+  startTransaction: (context: TransactionContext) => Transaction | undefined,
+  startTransactionOnPageLoad: boolean = true,
+  startTransactionOnLocationChange: boolean = true,
+): void {
+  wrapRouter(startTransaction, startTransactionOnLocationChange);
+  if (startTransactionOnPageLoad) {
+    startTransaction({
+      name: window.location.pathname,
+      op: 'pageload',
+    });
+  }
 }
 
-function startInitialTransaction(): void {
-  const initialTransactionCtx = getInitialTransactionContext();
-  const transaction = Sentry.startTransaction(initialTransactionCtx);
-  Sentry.getCurrentHub()
-    .getScope()
-    ?.setSpan(transaction);
-}
-
-function getInitialTransactionContext(): TransactionContext {
-  return {
-    name: `GET ${window.location.pathname}`,
-    // Operation target format is `<protocol>.client`.
-    // `window.location.protocol` is `<protocol>:`, so remove the `:`.
-    op: `${window.location.protocol.slice(0, -1)}.client`,
-    sampled: true, // For testing purposes, remove for production
-  };
-}
+// TODO: check for the info we can get from the router events,
+// note that it may interfere with the wrapping
+// https://nextjs.org/docs/api-reference/next/router#routerevents
diff --git a/packages/nextjs/src/performance/nextRouterWrapper.ts b/packages/nextjs/src/performance/nextRouterWrapper.ts
new file mode 100644
index 000000000000..e89a6f0012f9
--- /dev/null
+++ b/packages/nextjs/src/performance/nextRouterWrapper.ts
@@ -0,0 +1,67 @@
+/* eslint-disable no-console */
+import { Transaction, TransactionContext } from '@sentry/types';
+import { fill } from '@sentry/utils';
+import Router from 'next/router';
+
+let activeTransaction: Transaction | undefined = undefined;
+let prevTransactionId: string | undefined = undefined;
+let startTransaction: StartTransactionCb | undefined = undefined;
+
+type StartTransactionCb = (context: TransactionContext) => Transaction | undefined;
+
+/**
+ * Wraps the Next.js Router's methods to add performance monitoring.
+ * https://nextjs.org/docs/api-reference/next/router#router-object
+ */
+export function wrapRouter(startTransactionCb: StartTransactionCb, _startTransactionOnLocationChange: boolean): void {
+  // TODO: if we aren't going to start transactions on location change, stop at this point.
+  // Created spans don't have any transaction to attach to.
+
+  startTransaction = startTransactionCb;
+
+  // At this execution point, the router hasn't been created yet, so it cannot be wrapped.
+  // However, we can use the following callback to wrap it once it's been created.
+  Router.ready(() => {
+    // There should always be a router at this point; but checking again purely for types.
+    if (!Router.router) return;
+    const router = Router.router;
+    fill(Object.getPrototypeOf(router), 'push', pushWrapper); // create transactions
+    // TODO: replace - create transactions
+    // TODO: back - create transactions
+    // TODO: reload - create transactions
+    // TODO: beforepopstate - create span
+    // TODO: prefetch - ignore it, outside of the page load
+  });
+}
+
+type RouterPush = () => Promise<boolean>;
+type WrappedRouterPush = RouterPush;
+
+/**
+ * Wraps the `push` of the Next.js client.
+ * https://nextjs.org/docs/api-reference/next/router#routerpush
+ */
+export function pushWrapper(originalPush: RouterPush): WrappedRouterPush {
+  // The additional arguments must have the same type as the original `push` function, see
+  // https://github.com/vercel/next.js/blob/da97a18dafc7799e63aa7985adc95f213c2bf5f3/packages/next/next-server/lib/router/router.ts#L763
+  // Not including it means leads to: `TypeError: Cannot read property 'auth' of undefined`, similar to
+  // https://github.com/vercel/next.js/issues/11513
+  const wrapper = function(this: any, ...args: any[]): Promise<boolean> {
+    if (activeTransaction) {
+      activeTransaction.finish();
+    }
+    if (!startTransaction) {
+      // This should never happen, it's only for type checking bla bla bla
+      return Promise.resolve(false);
+    }
+    activeTransaction = startTransaction({
+      name: args[0], // url for now, it should be prevTransactionId
+      op: 'navigation',
+    });
+    prevTransactionId = args[0]; // TODO: we might want to normalize the url in the future
+    console.log(prevTransactionId);
+
+    return originalPush.call(this, ...args);
+  };
+  return wrapper;
+}
diff --git a/packages/nextjs/src/performance/routerWrapper.ts b/packages/nextjs/src/performance/routerWrapper.ts
deleted file mode 100644
index 6a73d622724e..000000000000
--- a/packages/nextjs/src/performance/routerWrapper.ts
+++ /dev/null
@@ -1,41 +0,0 @@
-import { fill } from '@sentry/utils';
-import Router from 'next/router';
-
-/**
- * Wraps the Next.js Router's methods to add performance monitoring.
- * https://nextjs.org/docs/api-reference/next/router#router-object
- */
-export function wrapRouter(): void {
-  // At this execution point, the router hasn't been created yet, so it cannot be wrapped.
-  // However, we can use the following callback to wrap it once it's been created.
-  Router.ready(() => {
-    // There should always be a router at this point; but checking again purely for types.
-    if (!Router.router) return;
-    const router = Router.router;
-    fill(Object.getPrototypeOf(router), 'push', pushWrapper);
-    // TODO: pop
-    // TODO: replace
-    // TODO: prefetch
-    // TODO: beforepopstate
-    // TODO: back
-    // TODO: reload
-  });
-}
-
-type RouterPush = () => Promise<boolean>;
-type WrappedRouterPush = RouterPush;
-
-/**
- * Wraps the `push` of the Next.js client.
- * https://nextjs.org/docs/api-reference/next/router#routerpush
- */
-export function pushWrapper(originalPush: RouterPush): WrappedRouterPush {
-  // The additional arguments must have the same type as the original `push` function, see
-  // https://github.com/vercel/next.js/blob/da97a18dafc7799e63aa7985adc95f213c2bf5f3/packages/next/next-server/lib/router/router.ts#L763
-  // Not including it means leads to: `TypeError: Cannot read property 'auth' of undefined`, similar to
-  // https://github.com/vercel/next.js/issues/11513
-  const wrapper = function(this: any, ...args: any[]): Promise<boolean> {
-    return originalPush.call(this, ...args);
-  };
-  return wrapper;
-}

From a9ec3e5f61058c2084d4709777fb2fd78434a933 Mon Sep 17 00:00:00 2001
From: iker-barriocanal <32816711+iker-barriocanal@users.noreply.github.com>
Date: Tue, 18 May 2021 18:09:38 +0200
Subject: [PATCH 03/19] Wrap more methods in the router

---
 .../src/performance/nextRouterWrapper.ts      | 54 ++++++++++++++++---
 1 file changed, 47 insertions(+), 7 deletions(-)

diff --git a/packages/nextjs/src/performance/nextRouterWrapper.ts b/packages/nextjs/src/performance/nextRouterWrapper.ts
index e89a6f0012f9..c8eceaac07de 100644
--- a/packages/nextjs/src/performance/nextRouterWrapper.ts
+++ b/packages/nextjs/src/performance/nextRouterWrapper.ts
@@ -24,13 +24,13 @@ export function wrapRouter(startTransactionCb: StartTransactionCb, _startTransac
   Router.ready(() => {
     // There should always be a router at this point; but checking again purely for types.
     if (!Router.router) return;
-    const router = Router.router;
-    fill(Object.getPrototypeOf(router), 'push', pushWrapper); // create transactions
-    // TODO: replace - create transactions
-    // TODO: back - create transactions
-    // TODO: reload - create transactions
-    // TODO: beforepopstate - create span
-    // TODO: prefetch - ignore it, outside of the page load
+    const routerPrototype = Object.getPrototypeOf(Router.router);
+    fill(routerPrototype, 'push', pushWrapper); // create transactions
+    fill(routerPrototype, 'replace', replaceWrapper); // create transactions
+    fill(routerPrototype, 'back', backWrapper); // create transactions
+    fill(routerPrototype, 'reload', reloadWrapper); // create transactions
+    fill(routerPrototype, 'beforePopState', beforePopStateWrapper); // create spans
+    // Ignore `prefetch`, since its outside of the page load
   });
 }
 
@@ -65,3 +65,43 @@ export function pushWrapper(originalPush: RouterPush): WrappedRouterPush {
   };
   return wrapper;
 }
+
+type RouterReplace = () => Promise<boolean>;
+type WrappedRouterReplace = RouterReplace;
+
+function replaceWrapper(originalReplace: RouterReplace): WrappedRouterReplace {
+  const wrapper = function(this: any, ...args: any[]): Promise<boolean> {
+    return originalReplace.apply(this, args);
+  };
+  return wrapper;
+}
+
+type RouterBack = () => Promise<boolean>;
+type WrappedRouterBack = RouterReplace;
+
+function backWrapper(originalBack: RouterBack): WrappedRouterBack {
+  const wrapper = function(this: any, ...args: any[]): Promise<boolean> {
+    return originalBack.apply(this, args);
+  };
+  return wrapper;
+}
+
+type RouterReload = () => Promise<boolean>;
+type WrappedRouterReload = RouterReplace;
+
+function reloadWrapper(originalReload: RouterReload): WrappedRouterReload {
+  const wrapper = function(this: any, ...args: any[]): Promise<boolean> {
+    return originalReload.apply(this, args);
+  };
+  return wrapper;
+}
+
+type RouterBeforePopState = () => Promise<boolean>;
+type WrappedRouterBeforePopState = RouterReplace;
+
+function beforePopStateWrapper(originalBeforePopState: RouterBeforePopState): WrappedRouterBeforePopState {
+  const wrapper = function(this: any, ...args: any[]): Promise<boolean> {
+    return originalBeforePopState.apply(this, args);
+  };
+  return wrapper;
+}

From 7c57e6a4fcf21746b7d125896b6787fbef6703c9 Mon Sep 17 00:00:00 2001
From: iker-barriocanal <32816711+iker-barriocanal@users.noreply.github.com>
Date: Wed, 19 May 2021 11:42:05 +0200
Subject: [PATCH 04/19] Remove unnecessary comments

---
 .../src/performance/nextRouterWrapper.ts      | 25 ++++++++-----------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/packages/nextjs/src/performance/nextRouterWrapper.ts b/packages/nextjs/src/performance/nextRouterWrapper.ts
index c8eceaac07de..4ab9f53a098b 100644
--- a/packages/nextjs/src/performance/nextRouterWrapper.ts
+++ b/packages/nextjs/src/performance/nextRouterWrapper.ts
@@ -4,7 +4,7 @@ import { fill } from '@sentry/utils';
 import Router from 'next/router';
 
 let activeTransaction: Transaction | undefined = undefined;
-let prevTransactionId: string | undefined = undefined;
+// let prevTransactionId: string | undefined = undefined;
 let startTransaction: StartTransactionCb | undefined = undefined;
 
 type StartTransactionCb = (context: TransactionContext) => Transaction | undefined;
@@ -13,17 +13,16 @@ type StartTransactionCb = (context: TransactionContext) => Transaction | undefin
  * Wraps the Next.js Router's methods to add performance monitoring.
  * https://nextjs.org/docs/api-reference/next/router#router-object
  */
-export function wrapRouter(startTransactionCb: StartTransactionCb, _startTransactionOnLocationChange: boolean): void {
-  // TODO: if we aren't going to start transactions on location change, stop at this point.
-  // Created spans don't have any transaction to attach to.
+export function wrapRouter(startTransactionCb: StartTransactionCb, startTransactionOnLocationChange: boolean): void {
+  // Spans that aren't attached to any transaction are lost; so if transactions aren't
+  // created, no need to wrap the router.
+  if (!startTransactionOnLocationChange) return;
 
   startTransaction = startTransactionCb;
 
   // At this execution point, the router hasn't been created yet, so it cannot be wrapped.
   // However, we can use the following callback to wrap it once it's been created.
   Router.ready(() => {
-    // There should always be a router at this point; but checking again purely for types.
-    if (!Router.router) return;
     const routerPrototype = Object.getPrototypeOf(Router.router);
     fill(routerPrototype, 'push', pushWrapper); // create transactions
     fill(routerPrototype, 'replace', replaceWrapper); // create transactions
@@ -37,11 +36,7 @@ export function wrapRouter(startTransactionCb: StartTransactionCb, _startTransac
 type RouterPush = () => Promise<boolean>;
 type WrappedRouterPush = RouterPush;
 
-/**
- * Wraps the `push` of the Next.js client.
- * https://nextjs.org/docs/api-reference/next/router#routerpush
- */
-export function pushWrapper(originalPush: RouterPush): WrappedRouterPush {
+function pushWrapper(originalPush: RouterPush): WrappedRouterPush {
   // The additional arguments must have the same type as the original `push` function, see
   // https://github.com/vercel/next.js/blob/da97a18dafc7799e63aa7985adc95f213c2bf5f3/packages/next/next-server/lib/router/router.ts#L763
   // Not including it means leads to: `TypeError: Cannot read property 'auth' of undefined`, similar to
@@ -51,15 +46,15 @@ export function pushWrapper(originalPush: RouterPush): WrappedRouterPush {
       activeTransaction.finish();
     }
     if (!startTransaction) {
-      // This should never happen, it's only for type checking bla bla bla
+      // This should never happen, adding it for type checking purposes.
       return Promise.resolve(false);
     }
+
+    const toUrl = args[0]; // TODO: normalize the URL?
     activeTransaction = startTransaction({
-      name: args[0], // url for now, it should be prevTransactionId
+      name: toUrl,
       op: 'navigation',
     });
-    prevTransactionId = args[0]; // TODO: we might want to normalize the url in the future
-    console.log(prevTransactionId);
 
     return originalPush.call(this, ...args);
   };

From ac90c43c3067f9bf135b60d1cb522c1e9e191818 Mon Sep 17 00:00:00 2001
From: iker-barriocanal <32816711+iker-barriocanal@users.noreply.github.com>
Date: Wed, 19 May 2021 12:11:52 +0200
Subject: [PATCH 05/19] Sort wrapping calls

---
 packages/nextjs/src/performance/nextRouterWrapper.ts | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/packages/nextjs/src/performance/nextRouterWrapper.ts b/packages/nextjs/src/performance/nextRouterWrapper.ts
index 4ab9f53a098b..5560a7f043cc 100644
--- a/packages/nextjs/src/performance/nextRouterWrapper.ts
+++ b/packages/nextjs/src/performance/nextRouterWrapper.ts
@@ -26,10 +26,10 @@ export function wrapRouter(startTransactionCb: StartTransactionCb, startTransact
     const routerPrototype = Object.getPrototypeOf(Router.router);
     fill(routerPrototype, 'push', pushWrapper); // create transactions
     fill(routerPrototype, 'replace', replaceWrapper); // create transactions
+    // Ignore `prefetch`, since its outside of the page load
+    fill(routerPrototype, 'beforePopState', beforePopStateWrapper); // create spans
     fill(routerPrototype, 'back', backWrapper); // create transactions
     fill(routerPrototype, 'reload', reloadWrapper); // create transactions
-    fill(routerPrototype, 'beforePopState', beforePopStateWrapper); // create spans
-    // Ignore `prefetch`, since its outside of the page load
   });
 }
 

From 0175ab2a271ab5ec91b833239ada6ac0b742ac20 Mon Sep 17 00:00:00 2001
From: iker-barriocanal <32816711+iker-barriocanal@users.noreply.github.com>
Date: Wed, 19 May 2021 12:15:14 +0200
Subject: [PATCH 06/19] Wrap transaction creation in a function

---
 .../src/performance/nextRouterWrapper.ts      | 31 ++++++++++---------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/packages/nextjs/src/performance/nextRouterWrapper.ts b/packages/nextjs/src/performance/nextRouterWrapper.ts
index 5560a7f043cc..ea7ce3416dda 100644
--- a/packages/nextjs/src/performance/nextRouterWrapper.ts
+++ b/packages/nextjs/src/performance/nextRouterWrapper.ts
@@ -33,6 +33,22 @@ export function wrapRouter(startTransactionCb: StartTransactionCb, startTransact
   });
 }
 
+/**
+ * Closes previous transaction (if required) and starts a new one.
+ */
+function startNewTransaction(toUrl: any, transactionOp: string = 'navigation'): void {
+  if (activeTransaction) activeTransaction.finish();
+  if (!startTransaction) {
+    // This should never happen, adding it for type checking purposes.
+    return;
+  }
+
+  activeTransaction = startTransaction({
+    name: toUrl, // TODO: should this be normalized?
+    op: transactionOp,
+  });
+}
+
 type RouterPush = () => Promise<boolean>;
 type WrappedRouterPush = RouterPush;
 
@@ -42,20 +58,7 @@ function pushWrapper(originalPush: RouterPush): WrappedRouterPush {
   // Not including it means leads to: `TypeError: Cannot read property 'auth' of undefined`, similar to
   // https://github.com/vercel/next.js/issues/11513
   const wrapper = function(this: any, ...args: any[]): Promise<boolean> {
-    if (activeTransaction) {
-      activeTransaction.finish();
-    }
-    if (!startTransaction) {
-      // This should never happen, adding it for type checking purposes.
-      return Promise.resolve(false);
-    }
-
-    const toUrl = args[0]; // TODO: normalize the URL?
-    activeTransaction = startTransaction({
-      name: toUrl,
-      op: 'navigation',
-    });
-
+    startNewTransaction(args[0]); // The URL being pushed
     return originalPush.call(this, ...args);
   };
   return wrapper;

From 9ba456e677cd39082c37bcc6b5ed9d3e2a2f1e5a Mon Sep 17 00:00:00 2001
From: iker-barriocanal <32816711+iker-barriocanal@users.noreply.github.com>
Date: Wed, 19 May 2021 12:16:37 +0200
Subject: [PATCH 07/19] Move `beforePopState` wrapping to match order

The order is determined by the exposed `wrapRouter` function.
This function fills the router in the same order as Next.js docs, for clarity.
---
 .../src/performance/nextRouterWrapper.ts      | 22 ++++++++++---------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/packages/nextjs/src/performance/nextRouterWrapper.ts b/packages/nextjs/src/performance/nextRouterWrapper.ts
index ea7ce3416dda..bb698cc6b78e 100644
--- a/packages/nextjs/src/performance/nextRouterWrapper.ts
+++ b/packages/nextjs/src/performance/nextRouterWrapper.ts
@@ -74,6 +74,17 @@ function replaceWrapper(originalReplace: RouterReplace): WrappedRouterReplace {
   return wrapper;
 }
 
+type RouterBeforePopState = () => Promise<boolean>;
+type WrappedRouterBeforePopState = RouterReplace;
+
+function beforePopStateWrapper(originalBeforePopState: RouterBeforePopState): WrappedRouterBeforePopState {
+  const wrapper = function(this: any, ...args: any[]): Promise<boolean> {
+    // TODO: create a span
+    return originalBeforePopState.apply(this, args);
+  };
+  return wrapper;
+}
+
 type RouterBack = () => Promise<boolean>;
 type WrappedRouterBack = RouterReplace;
 
@@ -89,17 +100,8 @@ type WrappedRouterReload = RouterReplace;
 
 function reloadWrapper(originalReload: RouterReload): WrappedRouterReload {
   const wrapper = function(this: any, ...args: any[]): Promise<boolean> {
+    // TODO
     return originalReload.apply(this, args);
   };
   return wrapper;
 }
-
-type RouterBeforePopState = () => Promise<boolean>;
-type WrappedRouterBeforePopState = RouterReplace;
-
-function beforePopStateWrapper(originalBeforePopState: RouterBeforePopState): WrappedRouterBeforePopState {
-  const wrapper = function(this: any, ...args: any[]): Promise<boolean> {
-    return originalBeforePopState.apply(this, args);
-  };
-  return wrapper;
-}

From 6b27974884dd1e5101f2b13247bc1bc7ebea0971 Mon Sep 17 00:00:00 2001
From: iker-barriocanal <32816711+iker-barriocanal@users.noreply.github.com>
Date: Wed, 19 May 2021 12:19:14 +0200
Subject: [PATCH 08/19] Start transactions on `replace`

---
 packages/nextjs/src/performance/nextRouterWrapper.ts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/packages/nextjs/src/performance/nextRouterWrapper.ts b/packages/nextjs/src/performance/nextRouterWrapper.ts
index bb698cc6b78e..95da9ad0c757 100644
--- a/packages/nextjs/src/performance/nextRouterWrapper.ts
+++ b/packages/nextjs/src/performance/nextRouterWrapper.ts
@@ -69,6 +69,7 @@ type WrappedRouterReplace = RouterReplace;
 
 function replaceWrapper(originalReplace: RouterReplace): WrappedRouterReplace {
   const wrapper = function(this: any, ...args: any[]): Promise<boolean> {
+    startNewTransaction(args[0]); // The replacer URL
     return originalReplace.apply(this, args);
   };
   return wrapper;
@@ -90,6 +91,7 @@ type WrappedRouterBack = RouterReplace;
 
 function backWrapper(originalBack: RouterBack): WrappedRouterBack {
   const wrapper = function(this: any, ...args: any[]): Promise<boolean> {
+    // TODO
     return originalBack.apply(this, args);
   };
   return wrapper;

From ba8bee40c0f3e38a39e8e0beb401538e1d205723 Mon Sep 17 00:00:00 2001
From: iker-barriocanal <32816711+iker-barriocanal@users.noreply.github.com>
Date: Wed, 19 May 2021 17:10:23 +0200
Subject: [PATCH 09/19] Move towards only wrapping `changeState`

As the docstring say, wrapping `changeState` in the Router should be enough to get performance
monitoring on the frontend. The previous approach was based on wrapping all the exposed Router
methods users could use; but these are using the `changeState` underneath (it seems router actions
always call it but not 100% about it, although it _may_ work implementing far less complexity).

The approach and some code has been taken from
https://github.com/getsentry/sentry-javascript/tree/abhi/next-client-names
---
 packages/nextjs/src/performance/client.ts | 73 ++++++++++++++++++++---
 1 file changed, 65 insertions(+), 8 deletions(-)

diff --git a/packages/nextjs/src/performance/client.ts b/packages/nextjs/src/performance/client.ts
index ad2cf8f1a6af..f946b0bc1bfc 100644
--- a/packages/nextjs/src/performance/client.ts
+++ b/packages/nextjs/src/performance/client.ts
@@ -1,21 +1,78 @@
-import { Transaction, TransactionContext } from '@sentry/types';
+import { Primitive, Transaction, TransactionContext } from '@sentry/types';
+import { fill } from '@sentry/utils';
+import Router from 'next/router';
 
-import { wrapRouter } from './nextRouterWrapper';
+type StartTransactionCb = (context: TransactionContext) => Transaction | undefined;
+
+let activeTransaction: Transaction | undefined = undefined;
+let prevTransactionId: string | undefined = undefined;
+let startTransaction: StartTransactionCb | undefined = undefined;
 
 export function nextRouterInstrumentation(
-  startTransaction: (context: TransactionContext) => Transaction | undefined,
+  startTransactionCb: StartTransactionCb,
   startTransactionOnPageLoad: boolean = true,
   startTransactionOnLocationChange: boolean = true,
 ): void {
-  wrapRouter(startTransaction, startTransactionOnLocationChange);
+  // Spans that aren't attached to any transaction are lost; so if transactions aren't
+  // created (besides potentially the onpageload transaction), no need to wrap the router.
+  if (startTransactionOnLocationChange) {
+    startTransaction = startTransactionCb;
+    Router.ready(() => {
+      const routerPrototype = Object.getPrototypeOf(Router);
+      // TODO: fill `beforePopState`
+
+      // `withRouter` uses `useRouter` underneath:
+      // https://github.com/vercel/next.js/blob/de42719619ae69fbd88e445100f15701f6e1e100/packages/next/client/with-router.tsx#L21
+      // Router events also use the router:
+      // https://github.com/vercel/next.js/blob/de42719619ae69fbd88e445100f15701f6e1e100/packages/next/client/router.ts#L92
+      // `Router.changeState` handles the router state changes, so it may be enough to only wrap it
+      // (instead of wrapping all of the Router's functions).
+      fill(routerPrototype, 'changeState', changeStateWrapper);
+    });
+  }
+
   if (startTransactionOnPageLoad) {
-    startTransaction({
+    startTransactionCb({
       name: window.location.pathname,
       op: 'pageload',
     });
   }
 }
 
-// TODO: check for the info we can get from the router events,
-// note that it may interfere with the wrapping
-// https://nextjs.org/docs/api-reference/next/router#routerevents
+type RouterChangeState = (method: string, url: string, as: string, options: Record<string, any>) => void;
+type WrappedRouterChangeState = RouterChangeState;
+
+export function changeStateWrapper(originalChangeStateWrapper: RouterChangeState): WrappedRouterChangeState {
+  const wrapper = function(
+    this: any,
+    method: string,
+    url: string,
+    as: string,
+    options: Record<string, any>,
+    // At the moment there are no additional arguments (meaning the rest parameter is empty).
+    // This is meant to protect from the Next.js API being expanded and the SDK break apps down.
+    ...args: any[]
+  ): Promise<boolean> {
+    if (activeTransaction) {
+      activeTransaction.finish();
+    }
+    if (startTransaction !== undefined) {
+      const tags: Record<string, Primitive> = {
+        'routing.instrumentation': 'next-router',
+        method,
+        ...options,
+      };
+      if (prevTransactionId) {
+        tags.from = prevTransactionId;
+      }
+      activeTransaction = startTransaction({
+        name: url,
+        op: 'navigation',
+        tags,
+      });
+      prevTransactionId = url;
+    }
+    return originalChangeStateWrapper.call(this, method, url, as, options, ...args);
+  };
+  return wrapper;
+}

From 26f03cfb362709e4893dd34c376f22d6330fa373 Mon Sep 17 00:00:00 2001
From: iker-barriocanal <32816711+iker-barriocanal@users.noreply.github.com>
Date: Wed, 19 May 2021 17:22:22 +0200
Subject: [PATCH 10/19] Default import Next.js router

---
 packages/nextjs/src/performance/client.ts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/packages/nextjs/src/performance/client.ts b/packages/nextjs/src/performance/client.ts
index f946b0bc1bfc..14d81ca6a2fa 100644
--- a/packages/nextjs/src/performance/client.ts
+++ b/packages/nextjs/src/performance/client.ts
@@ -1,6 +1,6 @@
 import { Primitive, Transaction, TransactionContext } from '@sentry/types';
 import { fill } from '@sentry/utils';
-import Router from 'next/router';
+import { default as Router } from 'next/router';
 
 type StartTransactionCb = (context: TransactionContext) => Transaction | undefined;
 

From 98a93d41436daa37ecc97dfe56147b5daa6e227d Mon Sep 17 00:00:00 2001
From: iker-barriocanal <32816711+iker-barriocanal@users.noreply.github.com>
Date: Wed, 19 May 2021 17:23:12 +0200
Subject: [PATCH 11/19] Remove unnecessary export

---
 packages/nextjs/src/performance/client.ts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/packages/nextjs/src/performance/client.ts b/packages/nextjs/src/performance/client.ts
index 14d81ca6a2fa..b6e3b23ebe2e 100644
--- a/packages/nextjs/src/performance/client.ts
+++ b/packages/nextjs/src/performance/client.ts
@@ -42,7 +42,7 @@ export function nextRouterInstrumentation(
 type RouterChangeState = (method: string, url: string, as: string, options: Record<string, any>) => void;
 type WrappedRouterChangeState = RouterChangeState;
 
-export function changeStateWrapper(originalChangeStateWrapper: RouterChangeState): WrappedRouterChangeState {
+function changeStateWrapper(originalChangeStateWrapper: RouterChangeState): WrappedRouterChangeState {
   const wrapper = function(
     this: any,
     method: string,

From 639779a416d6361a60e414daa0cf8dc631b5e19d Mon Sep 17 00:00:00 2001
From: iker-barriocanal <32816711+iker-barriocanal@users.noreply.github.com>
Date: Wed, 19 May 2021 18:17:01 +0200
Subject: [PATCH 12/19] Initial work on filling `beforePopState`

---
 packages/nextjs/src/performance/client.ts | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/packages/nextjs/src/performance/client.ts b/packages/nextjs/src/performance/client.ts
index b6e3b23ebe2e..e6623f044256 100644
--- a/packages/nextjs/src/performance/client.ts
+++ b/packages/nextjs/src/performance/client.ts
@@ -1,3 +1,4 @@
+/* eslint-disable no-console */
 import { Primitive, Transaction, TransactionContext } from '@sentry/types';
 import { fill } from '@sentry/utils';
 import { default as Router } from 'next/router';
@@ -18,8 +19,9 @@ export function nextRouterInstrumentation(
   if (startTransactionOnLocationChange) {
     startTransaction = startTransactionCb;
     Router.ready(() => {
-      const routerPrototype = Object.getPrototypeOf(Router);
+      const routerPrototype = Object.getPrototypeOf(Router.router);
       // TODO: fill `beforePopState`
+      fill(routerPrototype, 'beforePopState', beforePopStateWrapper);
 
       // `withRouter` uses `useRouter` underneath:
       // https://github.com/vercel/next.js/blob/de42719619ae69fbd88e445100f15701f6e1e100/packages/next/client/with-router.tsx#L21
@@ -76,3 +78,17 @@ function changeStateWrapper(originalChangeStateWrapper: RouterChangeState): Wrap
   };
   return wrapper;
 }
+
+// Next.js only cares when `beforePopState` returns `false`, but it can actually return anything.
+// https://nextjs.org/docs/api-reference/next/router#routerbeforepopstate
+type RouterBeforePopState = () => boolean | any;
+type WrappedRouterBeforePopState = RouterBeforePopState;
+
+function beforePopStateWrapper(originalBeforePopState: RouterBeforePopState): WrappedRouterBeforePopState {
+  console.log('beforePopStateWrapper 1');
+  const wrapper = function(this: any, ...args: any[]): any {
+    console.log('beforePopStateWrapper 2');
+    return originalBeforePopState.apply(this, args);
+  };
+  return wrapper;
+}

From 54c99f50c119516f6124caae752cdd0728bf3b3e Mon Sep 17 00:00:00 2001
From: Abhijeet Prasad <aprasad@sentry.io>
Date: Wed, 19 May 2021 15:48:27 -0400
Subject: [PATCH 13/19] fix: Remove query params

---
 packages/nextjs/src/performance/client.ts     | 109 ++++++++++++------
 .../nextjs/test/performance/client.test.ts    |  20 ++++
 2 files changed, 94 insertions(+), 35 deletions(-)
 create mode 100644 packages/nextjs/test/performance/client.test.ts

diff --git a/packages/nextjs/src/performance/client.ts b/packages/nextjs/src/performance/client.ts
index e6623f044256..2a579effc326 100644
--- a/packages/nextjs/src/performance/client.ts
+++ b/packages/nextjs/src/performance/client.ts
@@ -1,28 +1,52 @@
-/* eslint-disable no-console */
+/* eslint-disable @typescript-eslint/no-explicit-any */
 import { Primitive, Transaction, TransactionContext } from '@sentry/types';
 import { fill } from '@sentry/utils';
 import { default as Router } from 'next/router';
 
 type StartTransactionCb = (context: TransactionContext) => Transaction | undefined;
 
+const DEFAULT_TAGS = Object.freeze({
+  'routing.instrumentation': 'next-router',
+});
+
+const QUERY_PARAM_REGEX = /\?(.*)/;
+
 let activeTransaction: Transaction | undefined = undefined;
 let prevTransactionId: string | undefined = undefined;
 let startTransaction: StartTransactionCb | undefined = undefined;
 
+/**
+ * Creates routing instrumention for Next Router. Only supported for
+ * client side routing. Works for Next >= 10.
+ *
+ * Leverages the SingletonRouter from the `next/router` to
+ * generate pageload/navigation transactions and parameterize
+ * transaction names.
+ */
 export function nextRouterInstrumentation(
   startTransactionCb: StartTransactionCb,
   startTransactionOnPageLoad: boolean = true,
   startTransactionOnLocationChange: boolean = true,
 ): void {
-  // Spans that aren't attached to any transaction are lost; so if transactions aren't
-  // created (besides potentially the onpageload transaction), no need to wrap the router.
-  if (startTransactionOnLocationChange) {
-    startTransaction = startTransactionCb;
-    Router.ready(() => {
-      const routerPrototype = Object.getPrototypeOf(Router.router);
-      // TODO: fill `beforePopState`
-      fill(routerPrototype, 'beforePopState', beforePopStateWrapper);
+  startTransaction = startTransactionCb;
+  Router.ready(() => {
+    const routerPrototype = Object.getPrototypeOf(Router.router);
 
+    // We can only start the pageload transaction when we have access to the parameterized
+    // 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) {
+      prevTransactionId = removeQueryParams(Router.route);
+      activeTransaction = startTransactionCb({
+        name: prevTransactionId,
+        op: 'pageload',
+        tags: DEFAULT_TAGS,
+      });
+    }
+
+    // Spans that aren't attached to any transaction are lost; so if transactions aren't
+    // created (besides potentially the onpageload transaction), no need to wrap the router.
+    if (startTransactionOnLocationChange) {
       // `withRouter` uses `useRouter` underneath:
       // https://github.com/vercel/next.js/blob/de42719619ae69fbd88e445100f15701f6e1e100/packages/next/client/with-router.tsx#L21
       // Router events also use the router:
@@ -30,49 +54,58 @@ export function nextRouterInstrumentation(
       // `Router.changeState` handles the router state changes, so it may be enough to only wrap it
       // (instead of wrapping all of the Router's functions).
       fill(routerPrototype, 'changeState', changeStateWrapper);
-    });
-  }
 
-  if (startTransactionOnPageLoad) {
-    startTransactionCb({
-      name: window.location.pathname,
-      op: 'pageload',
-    });
-  }
+      // TODO: fill `beforePopState`
+      // fill(routerPrototype, 'beforePopState', beforePopStateWrapper);
+    }
+  });
 }
 
-type RouterChangeState = (method: string, url: string, as: string, options: Record<string, any>) => void;
+type RouterChangeState = (
+  method: string,
+  url: string,
+  as: string,
+  options: Record<string, any>,
+  ...args: any[]
+) => void;
 type WrappedRouterChangeState = RouterChangeState;
 
+/**
+ * Wraps Router.changeState()
+ * https://github.com/vercel/next.js/blob/da97a18dafc7799e63aa7985adc95f213c2bf5f3/packages/next/next-server/lib/router/router.ts#L1204
+ * Start a navigation transaction every time the router changes state.
+ */
 function changeStateWrapper(originalChangeStateWrapper: RouterChangeState): WrappedRouterChangeState {
   const wrapper = function(
     this: any,
     method: string,
+    // The parameterized url, ex. posts/[id]/[comment]
     url: string,
+    // The actual url, ex. posts/85/my-comment
     as: string,
     options: Record<string, any>,
     // At the moment there are no additional arguments (meaning the rest parameter is empty).
-    // This is meant to protect from the Next.js API being expanded and the SDK break apps down.
+    // This is meant to protect from future additions to Next.js API, especially since this is an
+    // internal API.
     ...args: any[]
   ): Promise<boolean> {
-    if (activeTransaction) {
-      activeTransaction.finish();
-    }
     if (startTransaction !== undefined) {
+      if (activeTransaction) {
+        activeTransaction.finish();
+      }
       const tags: Record<string, Primitive> = {
-        'routing.instrumentation': 'next-router',
+        ...DEFAULT_TAGS,
         method,
-        ...options,
       };
       if (prevTransactionId) {
         tags.from = prevTransactionId;
       }
+      prevTransactionId = removeQueryParams(url);
       activeTransaction = startTransaction({
-        name: url,
+        name: prevTransactionId,
         op: 'navigation',
         tags,
       });
-      prevTransactionId = url;
     }
     return originalChangeStateWrapper.call(this, method, url, as, options, ...args);
   };
@@ -81,14 +114,20 @@ function changeStateWrapper(originalChangeStateWrapper: RouterChangeState): Wrap
 
 // Next.js only cares when `beforePopState` returns `false`, but it can actually return anything.
 // https://nextjs.org/docs/api-reference/next/router#routerbeforepopstate
-type RouterBeforePopState = () => boolean | any;
-type WrappedRouterBeforePopState = RouterBeforePopState;
+// type RouterBeforePopState = () => boolean | any;
+// type WrappedRouterBeforePopState = RouterBeforePopState;
 
-function beforePopStateWrapper(originalBeforePopState: RouterBeforePopState): WrappedRouterBeforePopState {
-  console.log('beforePopStateWrapper 1');
-  const wrapper = function(this: any, ...args: any[]): any {
-    console.log('beforePopStateWrapper 2');
-    return originalBeforePopState.apply(this, args);
-  };
-  return wrapper;
+/* eslint-disable no-console */
+// function beforePopStateWrapper(originalBeforePopState: RouterBeforePopState): WrappedRouterBeforePopState {
+//   console.log('beforePopStateWrapper 1');
+//   const wrapper = function(this: any, ...args: any[]): any {
+//     console.log('beforePopStateWrapper 2', args);
+//     return originalBeforePopState.apply(this, args);
+//   };
+//   return wrapper;
+// }
+/* eslint-enable no-console */
+
+export function removeQueryParams(route: string): string {
+  return route.replace(QUERY_PARAM_REGEX, '');
 }
diff --git a/packages/nextjs/test/performance/client.test.ts b/packages/nextjs/test/performance/client.test.ts
new file mode 100644
index 000000000000..d454d33948bf
--- /dev/null
+++ b/packages/nextjs/test/performance/client.test.ts
@@ -0,0 +1,20 @@
+import { removeQueryParams } from '../../src/performance/client';
+
+// [in, out]
+type Table = Array<{ in: string; out: string }>;
+
+describe('client', () => {
+  describe('removeQueryParams', () => {
+    it('removes query params from an url', () => {
+      const table: Table = [
+        { in: '/posts/[id]/[comment]?name=ferret&color=purple', out: '/posts/[id]/[comment]' },
+        { in: '/posts/[id]/[comment]?', out: '/posts/[id]/[comment]' },
+        { in: '/about?', out: '/about' },
+      ];
+
+      table.forEach(test => {
+        expect(removeQueryParams(test.in)).toEqual(test.out);
+      });
+    });
+  });
+});

From f6f83d8364063ad21e687fa1d5aeb890ea0bbe61 Mon Sep 17 00:00:00 2001
From: Abhijeet Prasad <aprasad@sentry.io>
Date: Wed, 19 May 2021 17:11:37 -0400
Subject: [PATCH 14/19] test: Add tests + mocks for pageload

---
 packages/nextjs/src/performance/client.ts     | 28 ++--------
 .../src/performance/nextRouterWrapper.ts      |  2 +
 .../nextjs/test/performance/client.test.ts    | 53 ++++++++++++++++++-
 3 files changed, 58 insertions(+), 25 deletions(-)

diff --git a/packages/nextjs/src/performance/client.ts b/packages/nextjs/src/performance/client.ts
index 2a579effc326..0349c2ceba67 100644
--- a/packages/nextjs/src/performance/client.ts
+++ b/packages/nextjs/src/performance/client.ts
@@ -1,8 +1,10 @@
 /* eslint-disable @typescript-eslint/no-explicit-any */
 import { Primitive, Transaction, TransactionContext } from '@sentry/types';
-import { fill } from '@sentry/utils';
+import { fill, getGlobalObject } from '@sentry/utils';
 import { default as Router } from 'next/router';
 
+const global = getGlobalObject<Window>();
+
 type StartTransactionCb = (context: TransactionContext) => Transaction | undefined;
 
 const DEFAULT_TAGS = Object.freeze({
@@ -30,13 +32,11 @@ export function nextRouterInstrumentation(
 ): void {
   startTransaction = startTransactionCb;
   Router.ready(() => {
-    const routerPrototype = Object.getPrototypeOf(Router.router);
-
     // We can only start the pageload transaction when we have access to the parameterized
     // 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) {
-      prevTransactionId = removeQueryParams(Router.route);
+      prevTransactionId = Router.route !== null ? removeQueryParams(Router.route) : global.location.pathname;
       activeTransaction = startTransactionCb({
         name: prevTransactionId,
         op: 'pageload',
@@ -53,10 +53,8 @@ export function nextRouterInstrumentation(
       // https://github.com/vercel/next.js/blob/de42719619ae69fbd88e445100f15701f6e1e100/packages/next/client/router.ts#L92
       // `Router.changeState` handles the router state changes, so it may be enough to only wrap it
       // (instead of wrapping all of the Router's functions).
+      const routerPrototype = Object.getPrototypeOf(Router.router);
       fill(routerPrototype, 'changeState', changeStateWrapper);
-
-      // TODO: fill `beforePopState`
-      // fill(routerPrototype, 'beforePopState', beforePopStateWrapper);
     }
   });
 }
@@ -112,22 +110,6 @@ function changeStateWrapper(originalChangeStateWrapper: RouterChangeState): Wrap
   return wrapper;
 }
 
-// Next.js only cares when `beforePopState` returns `false`, but it can actually return anything.
-// https://nextjs.org/docs/api-reference/next/router#routerbeforepopstate
-// type RouterBeforePopState = () => boolean | any;
-// type WrappedRouterBeforePopState = RouterBeforePopState;
-
-/* eslint-disable no-console */
-// function beforePopStateWrapper(originalBeforePopState: RouterBeforePopState): WrappedRouterBeforePopState {
-//   console.log('beforePopStateWrapper 1');
-//   const wrapper = function(this: any, ...args: any[]): any {
-//     console.log('beforePopStateWrapper 2', args);
-//     return originalBeforePopState.apply(this, args);
-//   };
-//   return wrapper;
-// }
-/* eslint-enable no-console */
-
 export function removeQueryParams(route: string): string {
   return route.replace(QUERY_PARAM_REGEX, '');
 }
diff --git a/packages/nextjs/src/performance/nextRouterWrapper.ts b/packages/nextjs/src/performance/nextRouterWrapper.ts
index 95da9ad0c757..a3b205c7a624 100644
--- a/packages/nextjs/src/performance/nextRouterWrapper.ts
+++ b/packages/nextjs/src/performance/nextRouterWrapper.ts
@@ -75,6 +75,8 @@ function replaceWrapper(originalReplace: RouterReplace): WrappedRouterReplace {
   return wrapper;
 }
 
+// Next.js only cares when `beforePopState` returns `false`, but it can actually return anything.
+// https://nextjs.org/docs/api-reference/next/router#routerbeforepopstate
 type RouterBeforePopState = () => Promise<boolean>;
 type WrappedRouterBeforePopState = RouterReplace;
 
diff --git a/packages/nextjs/test/performance/client.test.ts b/packages/nextjs/test/performance/client.test.ts
index d454d33948bf..ec438511d837 100644
--- a/packages/nextjs/test/performance/client.test.ts
+++ b/packages/nextjs/test/performance/client.test.ts
@@ -1,10 +1,59 @@
-import { removeQueryParams } from '../../src/performance/client';
+import { default as Router } from 'next/router';
+
+import { nextRouterInstrumentation, removeQueryParams } from '../../src/performance/client';
+
+let readyCalled = false;
+jest.mock('next/router', () => ({
+  default: {
+    router: {
+      changeState: jest.fn(),
+    },
+    route: '/[user]/posts/[id]',
+    readyCallbacks: [],
+    ready(cb: () => void) {
+      readyCalled = true;
+      return cb();
+    },
+  },
+}));
 
 // [in, out]
 type Table = Array<{ in: string; out: string }>;
 
+beforeEach(() => {
+  readyCalled = false;
+});
+
 describe('client', () => {
-  describe('removeQueryParams', () => {
+  describe('nextRouterInstrumentation', () => {
+    it('waits for Router.ready()', () => {
+      const mockStartTransaction = jest.fn();
+      expect(readyCalled).toBe(false);
+      nextRouterInstrumentation(mockStartTransaction);
+      expect(readyCalled).toBe(true);
+    });
+
+    it('creates a pageload transaction', () => {
+      const mockStartTransaction = jest.fn();
+      nextRouterInstrumentation(mockStartTransaction);
+      expect(mockStartTransaction).toHaveBeenCalledTimes(1);
+      expect(mockStartTransaction).toHaveBeenLastCalledWith({
+        name: '/[user]/posts/[id]',
+        op: 'pageload',
+        tags: {
+          'routing.instrumentation': 'next-router',
+        },
+      });
+    });
+
+    it('does not create a pageload transaction if option not given', () => {
+      const mockStartTransaction = jest.fn();
+      nextRouterInstrumentation(mockStartTransaction, false);
+      expect(mockStartTransaction).toHaveBeenCalledTimes(0);
+    });
+  });
+
+  describe('removeQueryParams()', () => {
     it('removes query params from an url', () => {
       const table: Table = [
         { in: '/posts/[id]/[comment]?name=ferret&color=purple', out: '/posts/[id]/[comment]' },

From 73bbec348b70737c87309f2ad50523753c811921 Mon Sep 17 00:00:00 2001
From: Abhijeet Prasad <aprasad@sentry.io>
Date: Wed, 19 May 2021 21:17:24 -0400
Subject: [PATCH 15/19] test: Add table tests for navigation

---
 .../nextjs/test/performance/client.test.ts    | 83 +++++++++++++++----
 1 file changed, 66 insertions(+), 17 deletions(-)

diff --git a/packages/nextjs/test/performance/client.test.ts b/packages/nextjs/test/performance/client.test.ts
index ec438511d837..d2b55ceefd7e 100644
--- a/packages/nextjs/test/performance/client.test.ts
+++ b/packages/nextjs/test/performance/client.test.ts
@@ -3,26 +3,24 @@ import { default as Router } from 'next/router';
 import { nextRouterInstrumentation, removeQueryParams } from '../../src/performance/client';
 
 let readyCalled = false;
-jest.mock('next/router', () => ({
-  default: {
-    router: {
-      changeState: jest.fn(),
+jest.mock('next/router', () => {
+  const router = {};
+  Object.setPrototypeOf(router, { changeState: () => undefined });
+  return {
+    default: {
+      router,
+      route: '/[user]/posts/[id]',
+      readyCallbacks: [],
+      ready(cb: () => void) {
+        readyCalled = true;
+        return cb();
+      },
     },
-    route: '/[user]/posts/[id]',
-    readyCallbacks: [],
-    ready(cb: () => void) {
-      readyCalled = true;
-      return cb();
-    },
-  },
-}));
+  };
+});
 
 // [in, out]
-type Table = Array<{ in: string; out: string }>;
-
-beforeEach(() => {
-  readyCalled = false;
-});
+type Table<I = string, O = string> = Array<{ in: I; out: O }>;
 
 describe('client', () => {
   describe('nextRouterInstrumentation', () => {
@@ -51,6 +49,57 @@ describe('client', () => {
       nextRouterInstrumentation(mockStartTransaction, false);
       expect(mockStartTransaction).toHaveBeenCalledTimes(0);
     });
+
+    it('creates navigation transactions', () => {
+      const mockStartTransaction = jest.fn();
+      nextRouterInstrumentation(mockStartTransaction, false);
+      expect(mockStartTransaction).toHaveBeenCalledTimes(0);
+
+      const table: Table<Array<string | unknown>, Record<string, unknown>> = [
+        {
+          in: ['pushState', '/posts/[id]', '/posts/32', {}],
+          out: {
+            name: '/posts/[id]',
+            op: 'navigation',
+            tags: {
+              from: '/posts/[id]',
+              method: 'pushState',
+              'routing.instrumentation': 'next-router',
+            },
+          },
+        },
+        {
+          in: ['replaceState', '/posts/[id]?name=cat', '/posts/32?name=cat', {}],
+          out: {
+            name: '/posts/[id]',
+            op: 'navigation',
+            tags: {
+              from: '/posts/[id]',
+              method: 'replaceState',
+              'routing.instrumentation': 'next-router',
+            },
+          },
+        },
+        {
+          in: ['pushState', '/about', '/about', {}],
+          out: {
+            name: '/about',
+            op: 'navigation',
+            tags: {
+              from: '/about',
+              method: 'pushState',
+              'routing.instrumentation': 'next-router',
+            },
+          },
+        },
+      ];
+
+      table.forEach(test => {
+        // @ts-ignore changeState can be called with array spread
+        Router.router?.changeState(...test.in);
+        expect(mockStartTransaction).toHaveBeenLastCalledWith(test.out);
+      });
+    });
   });
 
   describe('removeQueryParams()', () => {

From f85e1ad682185e4ec6b1f4ebeff602650b81a588 Mon Sep 17 00:00:00 2001
From: iker-barriocanal <32816711+iker-barriocanal@users.noreply.github.com>
Date: Thu, 20 May 2021 13:02:18 +0200
Subject: [PATCH 16/19] Add changeState options back to transaction tags

---
 packages/nextjs/src/performance/client.ts | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/packages/nextjs/src/performance/client.ts b/packages/nextjs/src/performance/client.ts
index 0349c2ceba67..9b3f44e47428 100644
--- a/packages/nextjs/src/performance/client.ts
+++ b/packages/nextjs/src/performance/client.ts
@@ -46,16 +46,16 @@ export function nextRouterInstrumentation(
 
     // Spans that aren't attached to any transaction are lost; so if transactions aren't
     // created (besides potentially the onpageload transaction), no need to wrap the router.
-    if (startTransactionOnLocationChange) {
-      // `withRouter` uses `useRouter` underneath:
-      // https://github.com/vercel/next.js/blob/de42719619ae69fbd88e445100f15701f6e1e100/packages/next/client/with-router.tsx#L21
-      // Router events also use the router:
-      // https://github.com/vercel/next.js/blob/de42719619ae69fbd88e445100f15701f6e1e100/packages/next/client/router.ts#L92
-      // `Router.changeState` handles the router state changes, so it may be enough to only wrap it
-      // (instead of wrapping all of the Router's functions).
-      const routerPrototype = Object.getPrototypeOf(Router.router);
-      fill(routerPrototype, 'changeState', changeStateWrapper);
-    }
+    if (!startTransactionOnLocationChange) return;
+
+    // `withRouter` uses `useRouter` underneath:
+    // https://github.com/vercel/next.js/blob/de42719619ae69fbd88e445100f15701f6e1e100/packages/next/client/with-router.tsx#L21
+    // Router events also use the router:
+    // https://github.com/vercel/next.js/blob/de42719619ae69fbd88e445100f15701f6e1e100/packages/next/client/router.ts#L92
+    // `Router.changeState` handles the router state changes, so it may be enough to only wrap it
+    // (instead of wrapping all of the Router's functions).
+    const routerPrototype = Object.getPrototypeOf(Router.router);
+    fill(routerPrototype, 'changeState', changeStateWrapper);
   });
 }
 
@@ -94,6 +94,7 @@ function changeStateWrapper(originalChangeStateWrapper: RouterChangeState): Wrap
       const tags: Record<string, Primitive> = {
         ...DEFAULT_TAGS,
         method,
+        ...options,
       };
       if (prevTransactionId) {
         tags.from = prevTransactionId;

From fbf25c2ae0163e37a49bad3449e366ed6101f6f7 Mon Sep 17 00:00:00 2001
From: iker-barriocanal <32816711+iker-barriocanal@users.noreply.github.com>
Date: Thu, 20 May 2021 15:25:51 +0200
Subject: [PATCH 17/19] Remove unnecessary comments

---
 packages/nextjs/src/performance/client.ts       | 1 -
 packages/nextjs/test/performance/client.test.ts | 1 -
 2 files changed, 2 deletions(-)

diff --git a/packages/nextjs/src/performance/client.ts b/packages/nextjs/src/performance/client.ts
index 9b3f44e47428..2930c4e97e56 100644
--- a/packages/nextjs/src/performance/client.ts
+++ b/packages/nextjs/src/performance/client.ts
@@ -1,4 +1,3 @@
-/* eslint-disable @typescript-eslint/no-explicit-any */
 import { Primitive, Transaction, TransactionContext } from '@sentry/types';
 import { fill, getGlobalObject } from '@sentry/utils';
 import { default as Router } from 'next/router';
diff --git a/packages/nextjs/test/performance/client.test.ts b/packages/nextjs/test/performance/client.test.ts
index d2b55ceefd7e..46ec3e60f051 100644
--- a/packages/nextjs/test/performance/client.test.ts
+++ b/packages/nextjs/test/performance/client.test.ts
@@ -19,7 +19,6 @@ jest.mock('next/router', () => {
   };
 });
 
-// [in, out]
 type Table<I = string, O = string> = Array<{ in: I; out: O }>;
 
 describe('client', () => {

From 97ac62f1c71c71d390a46926ecc0a046c006ab05 Mon Sep 17 00:00:00 2001
From: iker-barriocanal <32816711+iker-barriocanal@users.noreply.github.com>
Date: Thu, 20 May 2021 15:27:20 +0200
Subject: [PATCH 18/19] Delete old wrapping approach

---
 .../src/performance/nextRouterWrapper.ts      | 111 ------------------
 1 file changed, 111 deletions(-)
 delete mode 100644 packages/nextjs/src/performance/nextRouterWrapper.ts

diff --git a/packages/nextjs/src/performance/nextRouterWrapper.ts b/packages/nextjs/src/performance/nextRouterWrapper.ts
deleted file mode 100644
index a3b205c7a624..000000000000
--- a/packages/nextjs/src/performance/nextRouterWrapper.ts
+++ /dev/null
@@ -1,111 +0,0 @@
-/* eslint-disable no-console */
-import { Transaction, TransactionContext } from '@sentry/types';
-import { fill } from '@sentry/utils';
-import Router from 'next/router';
-
-let activeTransaction: Transaction | undefined = undefined;
-// let prevTransactionId: string | undefined = undefined;
-let startTransaction: StartTransactionCb | undefined = undefined;
-
-type StartTransactionCb = (context: TransactionContext) => Transaction | undefined;
-
-/**
- * Wraps the Next.js Router's methods to add performance monitoring.
- * https://nextjs.org/docs/api-reference/next/router#router-object
- */
-export function wrapRouter(startTransactionCb: StartTransactionCb, startTransactionOnLocationChange: boolean): void {
-  // Spans that aren't attached to any transaction are lost; so if transactions aren't
-  // created, no need to wrap the router.
-  if (!startTransactionOnLocationChange) return;
-
-  startTransaction = startTransactionCb;
-
-  // At this execution point, the router hasn't been created yet, so it cannot be wrapped.
-  // However, we can use the following callback to wrap it once it's been created.
-  Router.ready(() => {
-    const routerPrototype = Object.getPrototypeOf(Router.router);
-    fill(routerPrototype, 'push', pushWrapper); // create transactions
-    fill(routerPrototype, 'replace', replaceWrapper); // create transactions
-    // Ignore `prefetch`, since its outside of the page load
-    fill(routerPrototype, 'beforePopState', beforePopStateWrapper); // create spans
-    fill(routerPrototype, 'back', backWrapper); // create transactions
-    fill(routerPrototype, 'reload', reloadWrapper); // create transactions
-  });
-}
-
-/**
- * Closes previous transaction (if required) and starts a new one.
- */
-function startNewTransaction(toUrl: any, transactionOp: string = 'navigation'): void {
-  if (activeTransaction) activeTransaction.finish();
-  if (!startTransaction) {
-    // This should never happen, adding it for type checking purposes.
-    return;
-  }
-
-  activeTransaction = startTransaction({
-    name: toUrl, // TODO: should this be normalized?
-    op: transactionOp,
-  });
-}
-
-type RouterPush = () => Promise<boolean>;
-type WrappedRouterPush = RouterPush;
-
-function pushWrapper(originalPush: RouterPush): WrappedRouterPush {
-  // The additional arguments must have the same type as the original `push` function, see
-  // https://github.com/vercel/next.js/blob/da97a18dafc7799e63aa7985adc95f213c2bf5f3/packages/next/next-server/lib/router/router.ts#L763
-  // Not including it means leads to: `TypeError: Cannot read property 'auth' of undefined`, similar to
-  // https://github.com/vercel/next.js/issues/11513
-  const wrapper = function(this: any, ...args: any[]): Promise<boolean> {
-    startNewTransaction(args[0]); // The URL being pushed
-    return originalPush.call(this, ...args);
-  };
-  return wrapper;
-}
-
-type RouterReplace = () => Promise<boolean>;
-type WrappedRouterReplace = RouterReplace;
-
-function replaceWrapper(originalReplace: RouterReplace): WrappedRouterReplace {
-  const wrapper = function(this: any, ...args: any[]): Promise<boolean> {
-    startNewTransaction(args[0]); // The replacer URL
-    return originalReplace.apply(this, args);
-  };
-  return wrapper;
-}
-
-// Next.js only cares when `beforePopState` returns `false`, but it can actually return anything.
-// https://nextjs.org/docs/api-reference/next/router#routerbeforepopstate
-type RouterBeforePopState = () => Promise<boolean>;
-type WrappedRouterBeforePopState = RouterReplace;
-
-function beforePopStateWrapper(originalBeforePopState: RouterBeforePopState): WrappedRouterBeforePopState {
-  const wrapper = function(this: any, ...args: any[]): Promise<boolean> {
-    // TODO: create a span
-    return originalBeforePopState.apply(this, args);
-  };
-  return wrapper;
-}
-
-type RouterBack = () => Promise<boolean>;
-type WrappedRouterBack = RouterReplace;
-
-function backWrapper(originalBack: RouterBack): WrappedRouterBack {
-  const wrapper = function(this: any, ...args: any[]): Promise<boolean> {
-    // TODO
-    return originalBack.apply(this, args);
-  };
-  return wrapper;
-}
-
-type RouterReload = () => Promise<boolean>;
-type WrappedRouterReload = RouterReplace;
-
-function reloadWrapper(originalReload: RouterReload): WrappedRouterReload {
-  const wrapper = function(this: any, ...args: any[]): Promise<boolean> {
-    // TODO
-    return originalReload.apply(this, args);
-  };
-  return wrapper;
-}

From d4e39c1df45eefc7c3fc9fb4c546bab9791c4d77 Mon Sep 17 00:00:00 2001
From: iker-barriocanal <32816711+iker-barriocanal@users.noreply.github.com>
Date: Thu, 20 May 2021 16:16:18 +0200
Subject: [PATCH 19/19] Rename transaction name variable

---
 packages/nextjs/src/performance/client.ts | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/packages/nextjs/src/performance/client.ts b/packages/nextjs/src/performance/client.ts
index 2930c4e97e56..e33e3243db47 100644
--- a/packages/nextjs/src/performance/client.ts
+++ b/packages/nextjs/src/performance/client.ts
@@ -13,7 +13,7 @@ const DEFAULT_TAGS = Object.freeze({
 const QUERY_PARAM_REGEX = /\?(.*)/;
 
 let activeTransaction: Transaction | undefined = undefined;
-let prevTransactionId: string | undefined = undefined;
+let prevTransactionName: string | undefined = undefined;
 let startTransaction: StartTransactionCb | undefined = undefined;
 
 /**
@@ -35,9 +35,9 @@ 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) {
-      prevTransactionId = Router.route !== null ? removeQueryParams(Router.route) : global.location.pathname;
+      prevTransactionName = Router.route !== null ? removeQueryParams(Router.route) : global.location.pathname;
       activeTransaction = startTransactionCb({
-        name: prevTransactionId,
+        name: prevTransactionName,
         op: 'pageload',
         tags: DEFAULT_TAGS,
       });
@@ -95,12 +95,12 @@ function changeStateWrapper(originalChangeStateWrapper: RouterChangeState): Wrap
         method,
         ...options,
       };
-      if (prevTransactionId) {
-        tags.from = prevTransactionId;
+      if (prevTransactionName) {
+        tags.from = prevTransactionName;
       }
-      prevTransactionId = removeQueryParams(url);
+      prevTransactionName = removeQueryParams(url);
       activeTransaction = startTransaction({
-        name: prevTransactionId,
+        name: prevTransactionName,
         op: 'navigation',
         tags,
       });