fix(react): prevent duplicate spans in strict mode

packages/apm/src/integrations/tracing.ts

@@ -1,3 +1,4 @@
+// tslint:disable: max-file-line-count
 import { Hub } from '@sentry/hub';
 import { Event, EventProcessor, Integration, Severity, Span, SpanContext, TransactionContext } from '@sentry/types';
 import {
@@ -116,6 +117,7 @@ interface Activity {
 
 const global = getGlobalObject<Window>();
 const defaultTracingOrigins = ['localhost', /^\//];
+const SPAN_IGNORE_KEY = '__sentry_delete_span';
 
 /**
  * Tracing Integration
@@ -474,6 +476,11 @@ export class Tracing implements Integration {
             return span;
           }
 
+          // if a span is supposed to be ignored, don't add it to the transaction
+          if (span.data[SPAN_IGNORE_KEY]) {
+            return false;
+          }
+
           // We cancel all pending spans with status "cancelled" to indicate the idle transaction was finished early
           if (!span.endTimestamp) {
             span.endTimestamp = endTimestamp;
@@ -764,7 +771,7 @@ export class Tracing implements Integration {
   }
 
   /**
-   * Starts tracking for a specifc activity
+   * Starts tracking for a specific activity
    *
    * @param name Name of the activity, can be any string (Only used internally to identify the activity)
    * @param spanContext If provided a Span with the SpanContext will be created.
@@ -866,6 +873,31 @@ export class Tracing implements Integration {
       }, timeout);
     }
   }
+
+  /**
+   * Cancels an activity if it exists
+   */
+  public static cancelActivity(id: number): void {
+    if (!id) {
+      return;
+    }
+
+    const activity = Tracing._activities[id];
+
+    if (activity) {
+      Tracing._log(`[Tracing] cancelActivity ${activity.name}#${id}`);
+      if (activity.span) {
+        // Ignore the span in the transaction
+        activity.span.setData(SPAN_IGNORE_KEY, true);
+      }
+
+      // tslint:disable-next-line: no-dynamic-delete
+      delete Tracing._activities[id];
+
+      const count = Object.keys(Tracing._activities).length;
+      Tracing._log('[Tracing] activies count', count);
+    }
+  }
 }
 
 /**

packages/react/package.json

@@ -32,6 +32,7 @@
     "@types/hoist-non-react-statics": "^3.3.1",
     "@types/react": "^16.9.35",
     "jest": "^24.7.1",
+    "jsdom": "^16.2.2",
     "npm-run-all": "^4.1.2",
     "prettier": "^1.17.0",
     "prettier-check": "^2.0.0",

packages/react/src/errorboundary.tsx

@@ -11,13 +11,31 @@ export type FallbackRender = (fallback: {
 }) => React.ReactNode;
 
 export type ErrorBoundaryProps = {
+  /** If a Sentry report dialog should be rendered on error */
   showDialog?: boolean;
+  /**
+   * Options to be passed into the Sentry report dialog.
+   * No-op if {@link showDialog} is false.
+   */
   dialogOptions?: Sentry.ReportDialogOptions;
-  // tslint:disable-next-line: no-null-undefined-union
+  // tslint:disable no-null-undefined-union
+  /**
+   * A fallback component that gets rendered when the error boundary encounters an error.
+   *
+   * Can either provide a React Component, or a function that returns React Component as
+   * a valid fallback prop. If a function is provided, the function will be called with
+   * the error, the component stack, and an function that resets the error boundary on error.
+   *
+   */
   fallback?: React.ReactNode | FallbackRender;
+  // tslint:enable no-null-undefined-union
+  /** Called with the error boundary encounters an error */
   onError?(error: Error, componentStack: string): void;
+  /** Called on componentDidMount() */
   onMount?(): void;
+  /** Called if resetError() is called from the fallback render props function  */
   onReset?(error: Error | null, componentStack: string | null): void;
+  /** Called on componentWillUnmount() */
   onUnmount?(error: Error | null, componentStack: string | null): void;
 };
 
@@ -31,17 +49,21 @@ const INITIAL_STATE = {
   error: null,
 };
 
+/**
+ * A ErrorBoundary component that logs errors to Sentry.
+ * Requires React >= 16
+ */
 class ErrorBoundary extends React.Component<ErrorBoundaryProps, ErrorBoundaryState> {
   public state: ErrorBoundaryState = INITIAL_STATE;
 
   public componentDidCatch(error: Error, { componentStack }: React.ErrorInfo): void {
-    Sentry.captureException(error, { contexts: { react: { componentStack } } });
+    const eventId = Sentry.captureException(error, { contexts: { react: { componentStack } } });
     const { onError, showDialog, dialogOptions } = this.props;
     if (onError) {
       onError(error, componentStack);
     }
     if (showDialog) {
-      Sentry.showReportDialog(dialogOptions);
+      Sentry.showReportDialog({ ...dialogOptions, eventId });
     }
 
     // componentDidCatch is used over getDerivedStateFromError

packages/react/src/index.ts

@@ -1,4 +1,27 @@
+import { addGlobalEventProcessor, SDK_VERSION } from '@sentry/browser';
+
+function createReactEventProcessor(): void {
+  addGlobalEventProcessor(event => {
+    event.sdk = {
+      ...event.sdk,
+      name: 'sentry.javascript.react',
+      packages: [
+        ...((event.sdk && event.sdk.packages) || []),
+        {
+          name: 'npm:@sentry/react',
+          version: SDK_VERSION,
+        },
+      ],
+      version: SDK_VERSION,
+    };
+
+    return event;
+  });
+}
+
 export * from '@sentry/browser';
 
 export { Profiler, withProfiler, useProfiler } from './profiler';
 export { ErrorBoundary, withErrorBoundary } from './errorboundary';
+
+createReactEventProcessor();

packages/react/src/profiler.tsx

@@ -10,6 +10,11 @@ const TRACING_GETTER = ({
   id: 'Tracing',
 } as any) as IntegrationClass<Integration>;
 
+// https://stackoverflow.com/questions/52702466/detect-react-reactdom-development-production-build
+function isReactInDevMode(): boolean {
+  return '_self' in React.createElement('div');
+}
+
 /**
  *
  * Based on implementation from Preact:
@@ -39,21 +44,65 @@ function afterNextFrame(callback: Function): void {
   timeout = window.setTimeout(done, 100);
 }
 
+let profilerCount = 0;
+
+const profiledComponents: {
+  [key: string]: number;
+} = {};
+
+/**
+ * getInitActivity pushes activity based on React component mount
+ * @param name displayName of component that started activity
+ */
 const getInitActivity = (name: string): number | null => {
   const tracingIntegration = getCurrentHub().getIntegration(TRACING_GETTER);
 
-  if (tracingIntegration !== null) {
-    // tslint:disable-next-line:no-unsafe-any
-    return (tracingIntegration as any).constructor.pushActivity(name, {
-      description: `<${name}>`,
-      op: 'react',
-    });
+  if (tracingIntegration === null) {
+    logger.warn(
+      `Unable to profile component ${name} due to invalid Tracing Integration. Please make sure to setup the Tracing integration.`,
+    );
+
+    return null;
   }
 
-  logger.warn(
-    `Unable to profile component ${name} due to invalid Tracing Integration. Please make sure to setup the Tracing integration.`,
-  );
-  return null;
+  // tslint:disable-next-line:no-unsafe-any
+  const activity = (tracingIntegration as any).constructor.pushActivity(name, {
+    description: `<${name}>`,
+    op: 'react',
+  }) as number;
+
+  /**
+   * If an activity was already generated, this the component is in React.StrictMode.
+   * React.StrictMode will call constructors and setState hooks twice, effectively
+   * creating redundant spans for every render (ex. two <App /> spans, two <Link /> spans)
+   *
+   * React.StrictMode only has this behaviour in Development Mode
+   * See: https://reactjs.org/docs/strict-mode.html
+   *
+   * To account for this, we track all profiled components, and cancel activities that
+   * we recognize to be coming from redundant push activity calls. It is important to note
+   * that it is the first call to push activity that is invalid, as that is the one caused
+   * by React.StrictMode.
+   *
+   */
+  if (isReactInDevMode()) {
+    // We can make the guarantee here that if a redundant activity exists, it comes right
+    // before the current activity, hence having a profilerCount one less than the existing count.
+    const redundantActivity = profiledComponents[String(`${name}${profilerCount - 1}`)];
+
+    if (redundantActivity) {
+      // tslint:disable-next-line:no-unsafe-any
+      (tracingIntegration as any).constructor.cancelActivity(redundantActivity);
+    } else {
+      // If an redundant activity didn't exist, we can store the current activity to
+      // check later. We have to do this inside an else block because of the case of
+      // the edge case where two components may share a single components name.
+      profiledComponents[String(`${name}${profilerCount}`)] = activity;
+    }
+  }
+
+  profilerCount += 1;
+  return activity;
 };
 
 export type ProfilerProps = {
@@ -68,10 +117,13 @@ class Profiler extends React.Component<ProfilerProps> {
     this.activity = getInitActivity(this.props.name);
   }
 
+  // If a component mounted, we can finish the mount activity.
   public componentDidMount(): void {
     afterNextFrame(this.finishProfile);
   }
 
+  // Sometimes a component will unmount first, so we make
+  // sure to also finish the mount activity here.
   public componentWillUnmount(): void {
     afterNextFrame(this.finishProfile);
   }
@@ -94,8 +146,15 @@ class Profiler extends React.Component<ProfilerProps> {
   }
 }
 
-function withProfiler<P extends object>(WrappedComponent: React.ComponentType<P>): React.FC<P> {
-  const componentDisplayName = WrappedComponent.displayName || WrappedComponent.name || UNKNOWN_COMPONENT;
+/**
+ * withProfiler is a higher order component that wraps a
+ * component in a {@link Profiler} component.
+ *
+ * @param WrappedComponent component that is wrapped by Profiler
+ * @param name displayName of component being profiled
+ */
+function withProfiler<P extends object>(WrappedComponent: React.ComponentType<P>, name?: string): React.FC<P> {
+  const componentDisplayName = name || WrappedComponent.displayName || WrappedComponent.name || UNKNOWN_COMPONENT;
 
   const Wrapped: React.FC<P> = (props: P) => (
     <Profiler name={componentDisplayName}>
@@ -119,7 +178,7 @@ function withProfiler<P extends object>(WrappedComponent: React.ComponentType<P>
  * @param name displayName of component being profiled
  */
 function useProfiler(name: string): void {
-  const activity = getInitActivity(name);
+  const [activity] = React.useState(() => getInitActivity(name));
 
   React.useEffect(() => {
     afterNextFrame(() => {