feat: Add instrumentation for React Router v3

Author: AbhiPrasad

packages/react/package.json

@@ -28,21 +28,19 @@
     "react-dom": "15.x || 16.x"
   },
   "devDependencies": {
-    "@sentry/tracing": "5.19.2",
     "@testing-library/react": "^10.0.6",
     "@testing-library/react-hooks": "^3.3.0",
-    "@types/history": "^4.7.6",
     "@types/hoist-non-react-statics": "^3.3.1",
     "@types/react": "^16.9.35",
-    "@types/react-router-3": "npm:@types/react-router@3.0.20",
+    "@types/react-router-3": "npm:@types/react-router@^3.2.0",
     "jest": "^24.7.1",
     "jsdom": "^16.2.2",
     "npm-run-all": "^4.1.2",
     "prettier": "^1.17.0",
     "prettier-check": "^2.0.0",
     "react": "^16.0.0",
     "react-dom": "^16.0.0",
-    "react-router-3": "npm:[email protected]",
+    "react-router-3": "npm:react-router@^3.2.0",
     "react-test-renderer": "^16.13.1",
     "redux": "^4.0.5",
     "rimraf": "^2.6.3",

packages/react/src/index.ts

@@ -26,6 +26,6 @@ export * from '@sentry/browser';
 export { Profiler, withProfiler, useProfiler } from './profiler';
 export { ErrorBoundary, withErrorBoundary } from './errorboundary';
 export { createReduxEnhancer } from './redux';
-export { reactRouterV3Instrumenation } from './reactrouter';
+export { reactRouterV3Instrumentation } from './reactrouter';
 
 createReactEventProcessor();

packages/react/src/reactrouter.tsx

@@ -1,70 +1,81 @@
 import { Transaction, TransactionContext } from '@sentry/types';
+import { getGlobalObject } from '@sentry/utils';
 
-type routingInstrumentation = (
-  startTransaction: (context: TransactionContext) => Transaction,
+type ReactRouterInstrumentation = <T extends Transaction>(
+  startTransaction: (context: TransactionContext) => T | undefined,
   startTransactionOnPageLoad?: boolean,
   startTransactionOnLocationChange?: boolean,
 ) => void;
 
-// Many of the types below had to be mocked out to lower bundle size.
-type PlainRoute = { path: string; childRoutes: PlainRoute[] };
+// Many of the types below had to be mocked out to prevent typescript issues
+// these types are required for correct functionality.
 
-type Match = (
-  props: { location: Location; routes: PlainRoute[] },
-  cb: (error?: Error, _redirectLocation?: Location, renderProps?: { routes?: PlainRoute[] }) => void,
+export type Route = { path?: string; childRoutes?: Route[] };
+
+export type Match = (
+  props: { location: Location; routes: Route[] },
+  cb: (error?: Error, _redirectLocation?: Location, renderProps?: { routes?: Route[] }) => void,
 ) => void;
 
 type Location = {
   pathname: string;
-  action: 'PUSH' | 'REPLACE' | 'POP';
-  key: string;
+  action?: 'PUSH' | 'REPLACE' | 'POP';
 } & Record<string, any>;
 
 type History = {
-  location: Location;
-  listen(cb: (location: Location) => void): void;
+  location?: Location;
+  listen?(cb: (location: Location) => void): void;
 } & Record<string, any>;
 
+const global = getGlobalObject<Window>();
+
 /**
  * Creates routing instrumentation for React Router v3
+ * Works for React Router >= 3.2.0 and < 4.0.0
  *
  * @param history object from the `history` library
  * @param routes a list of all routes, should be
  * @param match `Router.match` utility
  */
-export function reactRouterV3Instrumenation(
+export function reactRouterV3Instrumentation(
   history: History,
-  routes: PlainRoute[],
+  routes: Route[],
   match: Match,
-): routingInstrumentation {
+): ReactRouterInstrumentation {
   return (
     startTransaction: (context: TransactionContext) => Transaction | undefined,
     startTransactionOnPageLoad: boolean = true,
     startTransactionOnLocationChange: boolean = true,
   ) => {
     let activeTransaction: Transaction | undefined;
-    let name = normalizeTransactionName(routes, history.location, match);
-    if (startTransactionOnPageLoad) {
+    let prevName: string | undefined;
+
+    if (startTransactionOnPageLoad && global && global.location) {
+      // Have to use global.location because history.location might not be defined.
+      prevName = normalizeTransactionName(routes, global.location, match);
       activeTransaction = startTransaction({
-        name,
+        name: prevName,
         op: 'pageload',
         tags: {
-          from: name,
-          routingInstrumentation: 'react-router-v3',
+          'routing.instrumentation': 'react-router-v3',
         },
       });
     }
 
-    if (startTransactionOnLocationChange) {
+    if (startTransactionOnLocationChange && history.listen) {
       history.listen(location => {
         if (location.action === 'PUSH') {
           if (activeTransaction) {
             activeTransaction.finish();
           }
-          const tags = { from: name, routingInstrumentation: 'react-router-v3' };
-          name = normalizeTransactionName(routes, history.location, match);
+          const tags: Record<string, string> = { 'routing.instrumentation': 'react-router-v3' };
+          if (prevName) {
+            tags.from = prevName;
+          }
+
+          prevName = normalizeTransactionName(routes, location, match);
           activeTransaction = startTransaction({
-            name,
+            name: prevName,
             op: 'navigation',
             tags,
           });
@@ -74,41 +85,47 @@ export function reactRouterV3Instrumenation(
   };
 }
 
-export function normalizeTransactionName(appRoutes: PlainRoute[], location: Location, match: Match): string {
-  const defaultName = location.pathname;
+/**
+ * Normalize transaction names using `Router.match`
+ */
+function normalizeTransactionName(appRoutes: Route[], location: Location, match: Match): string {
+  let name = location.pathname;
   match(
     {
       location,
       routes: appRoutes,
     },
-    (error?: Error, _redirectLocation?: Location, renderProps?: { routes?: PlainRoute[] }) => {
+    (error, _redirectLocation, renderProps) => {
       if (error || !renderProps) {
-        return defaultName;
+        return name;
       }
 
       const routePath = getRouteStringFromRoutes(renderProps.routes || []);
-
       if (routePath.length === 0 || routePath === '/*') {
-        return defaultName;
+        return name;
       }
 
-      return routePath;
+      name = routePath;
+      return name;
     },
   );
-
-  return defaultName;
+  return name;
 }
 
-function getRouteStringFromRoutes(routes: PlainRoute[]): string {
-  if (!Array.isArray(routes)) {
+/**
+ * Generate route name from array of routes
+ */
+function getRouteStringFromRoutes(routes: Route[]): string {
+  if (!Array.isArray(routes) || routes.length === 0) {
     return '';
   }
 
-  const routesWithPaths: PlainRoute[] = routes.filter((route: PlainRoute) => !!route.path);
+  const routesWithPaths: Route[] = routes.filter((route: Route) => !!route.path);
 
   let index = -1;
-  for (let x = routesWithPaths.length; x >= 0; x--) {
-    if (routesWithPaths[x].path.startsWith('/')) {
+  for (let x = routesWithPaths.length - 1; x >= 0; x--) {
+    const route = routesWithPaths[x];
+    if (route.path && route.path.startsWith('/')) {
       index = x;
       break;
     }

packages/react/test/reactrouter.test.tsx

@@ -0,0 +1,132 @@
+import { render } from '@testing-library/react';
+import * as React from 'react';
+import { createMemoryHistory, createRoutes, IndexRoute, match, Route, Router } from 'react-router-3';
+
+import { Match, reactRouterV3Instrumentation, Route as RouteType } from '../src/reactrouter';
+
+// Have to manually set types because we are using package-alias
+declare module 'react-router-3' {
+  type History = { replace: Function; push: Function };
+  export function createMemoryHistory(): History;
+  export const Router: React.ComponentType<{ history: History }>;
+  export const Route: React.ComponentType<{ path: string; component: React.ComponentType<any> }>;
+  export const IndexRoute: React.ComponentType<{ component: React.ComponentType<any> }>;
+  export const match: Match;
+  export const createRoutes: (routes: any) => RouteType[];
+}
+
+const App: React.FC = ({ children }) => <div>{children}</div>;
+
+function Home(): JSX.Element {
+  return <div>Home</div>;
+}
+
+function About(): JSX.Element {
+  return <div>About</div>;
+}
+
+function Features(): JSX.Element {
+  return <div>Features</div>;
+}
+
+function Users({ params }: { params: Record<string, string> }): JSX.Element {
+  return <div>{params.userid}</div>;
+}
+
+describe('React Router V3', () => {
+  const routes = (
+    <Route path="/" component={App}>
+      <IndexRoute component={Home} />
+      <Route path="about" component={About} />
+      <Route path="features" component={Features} />
+      <Route path="users/:userid" component={Users} />
+    </Route>
+  );
+  const history = createMemoryHistory();
+
+  const instrumentationRoutes = createRoutes(routes);
+  const instrumentation = reactRouterV3Instrumentation(history, instrumentationRoutes, match);
+
+  it('starts a pageload transaction when instrumentation is started', () => {
+    const mockStartTransaction = jest.fn();
+    instrumentation(mockStartTransaction);
+    expect(mockStartTransaction).toHaveBeenCalledTimes(1);
+    expect(mockStartTransaction).toHaveBeenLastCalledWith({
+      name: '/',
+      op: 'pageload',
+      tags: { 'routing.instrumentation': 'react-router-v3' },
+    });
+  });
+
+  it('does not start pageload transaction if option is false', () => {
+    const mockStartTransaction = jest.fn();
+    instrumentation(mockStartTransaction, false);
+    expect(mockStartTransaction).toHaveBeenCalledTimes(0);
+  });
+
+  it('starts a navigation transaction', () => {
+    const mockStartTransaction = jest.fn();
+    instrumentation(mockStartTransaction);
+    render(<Router history={history}>{routes}</Router>);
+
+    history.push('/about');
+    expect(mockStartTransaction).toHaveBeenCalledTimes(2);
+    expect(mockStartTransaction).toHaveBeenLastCalledWith({
+      name: '/about',
+      op: 'navigation',
+      tags: { from: '/', 'routing.instrumentation': 'react-router-v3' },
+    });
+
+    history.push('/features');
+    expect(mockStartTransaction).toHaveBeenCalledTimes(3);
+    expect(mockStartTransaction).toHaveBeenLastCalledWith({
+      name: '/features',
+      op: 'navigation',
+      tags: { from: '/about', 'routing.instrumentation': 'react-router-v3' },
+    });
+  });
+
+  it('does not start a transaction if option is false', () => {
+    const mockStartTransaction = jest.fn();
+    instrumentation(mockStartTransaction, true, false);
+    render(<Router history={history}>{routes}</Router>);
+    expect(mockStartTransaction).toHaveBeenCalledTimes(1);
+  });
+
+  it('only starts a navigation transaction on push', () => {
+    const mockStartTransaction = jest.fn();
+    instrumentation(mockStartTransaction);
+    render(<Router history={history}>{routes}</Router>);
+
+    history.replace('hello');
+    expect(mockStartTransaction).toHaveBeenCalledTimes(1);
+  });
+
+  it('finishes a transaction on navigation', () => {
+    const mockFinish = jest.fn();
+    const mockStartTransaction = jest.fn().mockReturnValue({ finish: mockFinish });
+    instrumentation(mockStartTransaction);
+    render(<Router history={history}>{routes}</Router>);
+    expect(mockStartTransaction).toHaveBeenCalledTimes(1);
+
+    history.push('/features');
+    expect(mockFinish).toHaveBeenCalledTimes(1);
+    expect(mockStartTransaction).toHaveBeenCalledTimes(2);
+  });
+
+  it('normalizes transaction names', () => {
+    const mockStartTransaction = jest.fn();
+    instrumentation(mockStartTransaction);
+    const { container } = render(<Router history={history}>{routes}</Router>);
+
+    history.push('/users/123');
+    expect(container.innerHTML).toContain('123');
+
+    expect(mockStartTransaction).toHaveBeenCalledTimes(2);
+    expect(mockStartTransaction).toHaveBeenLastCalledWith({
+      name: '/users/:userid',
+      op: 'navigation',
+      tags: { from: '/', 'routing.instrumentation': 'react-router-v3' },
+    });
+  });
+});