ref: Rewrite React Profiler (#2677)

AbhiPrasad · web-flow · commit 1f4772ea731a · 2020-06-18T09:40:27.000-04:00
* ref(react): Refactor Profiler to account for update and render spans
* feat(apm): Add ability to get span from activity using `getActivitySpan`
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,8 @@
 - [core] fix: Call `bindClient` when creating new `Hub` to make integrations work automatically (#2665)
 - [gatsby] feat: Add @sentry/gatsby package (#2652)
 - [core] ref: Rename `whitelistUrls/blacklistUrls` to `allowUrls/denyUrls`
+- [react] ref: Refactor Profiler to account for update and render (#2677)
+- [tracing] feat: Add ability to get span from activity using `getActivitySpan` (#2677)
 
 ## 5.17.0
 
diff --git a/packages/apm/src/integrations/tracing.ts b/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 {
@@ -817,6 +818,9 @@ export class Tracing implements Integration {
 
   /**
    * Removes activity and finishes the span in case there is one
+   * @param id the id of the activity being removed
+   * @param spanData span data that can be updated
+   *
    */
   public static popActivity(id: number, spanData?: { [key: string]: any }): void {
     // The !id is on purpose to also fail with 0
@@ -866,6 +870,20 @@ export class Tracing implements Integration {
       }, timeout);
     }
   }
+
+  /**
+   * Get span based on activity id
+   */
+  public static getActivitySpan(id: number): Span | undefined {
+    if (!id) {
+      return undefined;
+    }
+    const activity = Tracing._activities[id];
+    if (activity) {
+      return activity.span;
+    }
+    return undefined;
+  }
 }
 
 /**
diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts
@@ -1,22 +1,24 @@
 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,
-    };
+  if (addGlobalEventProcessor) {
+    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;
-  });
+      return event;
+    });
+  }
 }
 
 export * from '@sentry/browser';
diff --git a/packages/react/src/profiler.tsx b/packages/react/src/profiler.tsx
@@ -1,6 +1,6 @@
 import { getCurrentHub } from '@sentry/browser';
-import { Integration, IntegrationClass } from '@sentry/types';
-import { logger } from '@sentry/utils';
+import { Integration, IntegrationClass, Span } from '@sentry/types';
+import { logger, timestampWithMs } from '@sentry/utils';
 import * as hoistNonReactStatic from 'hoist-non-react-statics';
 import * as React from 'react';
 
@@ -10,91 +10,167 @@ const TRACING_GETTER = ({
   id: 'Tracing',
 } as any) as IntegrationClass<Integration>;
 
+let globalTracingIntegration: Integration | null = null;
+const getTracingIntegration = () => {
+  if (globalTracingIntegration) {
+    return globalTracingIntegration;
+  }
+
+  globalTracingIntegration = getCurrentHub().getIntegration(TRACING_GETTER);
+  return globalTracingIntegration;
+};
+
 /**
- *
- * Based on implementation from Preact:
- * https:github.com/preactjs/preact/blob/9a422017fec6dab287c77c3aef63c7b2fef0c7e1/hooks/src/index.js#L301-L313
- *
- * Schedule a callback to be invoked after the browser has a chance to paint a new frame.
- * Do this by combining requestAnimationFrame (rAF) + setTimeout to invoke a callback after
- * the next browser frame.
- *
- * Also, schedule a timeout in parallel to the the rAF to ensure the callback is invoked
- * even if RAF doesn't fire (for example if the browser tab is not visible)
- *
- * This is what we use to tell if a component activity has finished
- *
+ * Warn if tracing integration not configured. Will only warn once.
  */
-function afterNextFrame(callback: Function): void {
-  let timeout: number | undefined;
-  let raf: number;
-
-  const done = () => {
-    window.clearTimeout(timeout);
-    window.cancelAnimationFrame(raf);
-    window.setTimeout(callback);
-  };
-
-  raf = window.requestAnimationFrame(done);
-  timeout = window.setTimeout(done, 100);
+function warnAboutTracing(name: string): void {
+  if (globalTracingIntegration === null) {
+    logger.warn(
+      `Unable to profile component ${name} due to invalid Tracing Integration. Please make sure the Tracing integration is setup properly.`,
+    );
+  }
 }
 
 /**
- * getInitActivity pushes activity based on React component mount
+ * pushActivity creates an new react activity.
+ * Is a no-op if Tracing integration is not valid
  * @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',
-    });
+function pushActivity(name: string, op: string): number | null {
+  if (globalTracingIntegration === null) {
+    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
+  return (globalTracingIntegration as any).constructor.pushActivity(name, {
+    description: `<${name}>`,
+    op: `react.${op}`,
+  });
+}
+
+/**
+ * popActivity removes a React activity.
+ * Is a no-op if Tracing integration is not valid.
+ * @param activity id of activity that is being popped
+ */
+function popActivity(activity: number | null): void {
+  if (activity === null || globalTracingIntegration === null) {
+    return;
+  }
+
+  // tslint:disable-next-line:no-unsafe-any
+  (globalTracingIntegration as any).constructor.popActivity(activity);
+}
+
+/**
+ * Obtain a span given an activity id.
+ * Is a no-op if Tracing integration is not valid.
+ * @param activity activity id associated with obtained span
+ */
+function getActivitySpan(activity: number | null): Span | undefined {
+  if (activity === null || globalTracingIntegration === null) {
+    return undefined;
+  }
+
+  // tslint:disable-next-line:no-unsafe-any
+  return (globalTracingIntegration as any).constructor.getActivitySpan(activity) as Span | undefined;
+}
 
 export type ProfilerProps = {
+  // The name of the component being profiled.
   name: string;
+  // If the Profiler is disabled. False by default. This is useful if you want to disable profilers
+  // in certain environments.
+  disabled?: boolean;
+  // If time component is on page should be displayed as spans. True by default.
+  hasRenderSpan?: boolean;
+  // If component updates should be displayed as spans. True by default.
+  hasUpdateSpan?: boolean;
+  // props given to component being profiled.
+  updateProps: { [key: string]: any };
 };
 
+/**
+ * The Profiler component leverages Sentry's Tracing integration to generate
+ * spans based on component lifecycles.
+ */
 class Profiler extends React.Component<ProfilerProps> {
-  public activity: number | null;
+  // The activity representing how long it takes to mount a component.
+  public mountActivity: number | null = null;
+  // The span of the mount activity
+  public mountSpan: Span | undefined = undefined;
+  // The span of the render
+  public renderSpan: Span | undefined = undefined;
+
+  public static defaultProps: Partial<ProfilerProps> = {
+    disabled: false,
+    hasRenderSpan: true,
+    hasUpdateSpan: true,
+  };
+
   public constructor(props: ProfilerProps) {
     super(props);
+    const { name, disabled = false } = this.props;
 
-    this.activity = getInitActivity(this.props.name);
+    if (disabled) {
+      return;
+    }
+
+    if (getTracingIntegration()) {
+      this.mountActivity = pushActivity(name, 'mount');
+    } else {
+      warnAboutTracing(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);
+    this.mountSpan = getActivitySpan(this.mountActivity);
+    popActivity(this.mountActivity);
+    this.mountActivity = null;
   }
 
-  public finishProfile = () => {
-    if (!this.activity) {
-      return;
+  public componentDidUpdate({ updateProps, hasUpdateSpan = true }: ProfilerProps): void {
+    // Only generate an update span if hasUpdateSpan is true, if there is a valid mountSpan,
+    // and if the updateProps have changed. It is ok to not do a deep equality check here as it is expensive.
+    // We are just trying to give baseline clues for further investigation.
+    if (hasUpdateSpan && this.mountSpan && updateProps !== this.props.updateProps) {
+      // See what props haved changed between the previous props, and the current props. This is
+      // set as data on the span. We just store the prop keys as the values could be potenially very large.
+      const changedProps = Object.keys(updateProps).filter(k => updateProps[k] !== this.props.updateProps[k]);
+      if (changedProps.length > 0) {
+        // The update span is a point in time span with 0 duration, just signifying that the component
+        // has been updated.
+        const now = timestampWithMs();
+        this.mountSpan.startChild({
+          data: {
+            changedProps,
+          },
+          description: `<${this.props.name}>`,
+          endTimestamp: now,
+          op: `react.update`,
+          startTimestamp: now,
+        });
+      }
     }
+  }
 
-    const tracingIntegration = getCurrentHub().getIntegration(TRACING_GETTER);
-    if (tracingIntegration !== null) {
-      // tslint:disable-next-line:no-unsafe-any
-      (tracingIntegration as any).constructor.popActivity(this.activity);
-      this.activity = null;
+  // If a component is unmounted, we can say it is no longer on the screen.
+  // This means we can finish the span representing the component render.
+  public componentWillUnmount(): void {
+    const { name, hasRenderSpan = true } = this.props;
+
+    if (this.mountSpan && hasRenderSpan) {
+      // If we were able to obtain the spanId of the mount activity, we should set the
+      // next activity as a child to the component mount activity.
+      this.mountSpan.startChild({
+        description: `<${name}>`,
+        endTimestamp: timestampWithMs(),
+        op: `react.render`,
+        startTimestamp: this.mountSpan.endTimestamp,
+      });
     }
-  };
+  }
 
   public render(): React.ReactNode {
     return this.props.children;
@@ -103,16 +179,22 @@ class Profiler extends React.Component<ProfilerProps> {
 
 /**
  * withProfiler is a higher order component that wraps a
- * component in a {@link Profiler} component.
+ * component in a {@link Profiler} component. It is recommended that
+ * the higher order component be used over the regular {@link Profiler} component.
  *
  * @param WrappedComponent component that is wrapped by Profiler
- * @param name displayName of component being profiled
+ * @param options the {@link ProfilerProps} you can pass into the Profiler
  */
-function withProfiler<P extends object>(WrappedComponent: React.ComponentType<P>, name?: string): React.FC<P> {
-  const componentDisplayName = name || WrappedComponent.displayName || WrappedComponent.name || UNKNOWN_COMPONENT;
+function withProfiler<P extends object>(
+  WrappedComponent: React.ComponentType<P>,
+  // We do not want to have `updateProps` given in options, it is instead filled through the HOC.
+  options?: Pick<Partial<ProfilerProps>, Exclude<keyof ProfilerProps, 'updateProps'>>,
+): React.FC<P> {
+  const componentDisplayName =
+    (options && options.name) || WrappedComponent.displayName || WrappedComponent.name || UNKNOWN_COMPONENT;
 
   const Wrapped: React.FC<P> = (props: P) => (
-    <Profiler name={componentDisplayName}>
+    <Profiler {...options} name={componentDisplayName} updateProps={props}>
       <WrappedComponent {...props} />
     </Profiler>
   );
@@ -132,17 +214,40 @@ function withProfiler<P extends object>(WrappedComponent: React.ComponentType<P>
  * Requires React 16.8 or above.
  * @param name displayName of component being profiled
  */
-function useProfiler(name: string): void {
-  const [activity] = React.useState(() => getInitActivity(name));
+function useProfiler(
+  name: string,
+  options: { disabled?: boolean; hasRenderSpan?: boolean } = {
+    disabled: false,
+    hasRenderSpan: true,
+  },
+): void {
+  const [mountActivity] = React.useState(() => {
+    if (options && options.disabled) {
+      return null;
+    }
+
+    if (getTracingIntegration()) {
+      return pushActivity(name, 'mount');
+    }
+
+    warnAboutTracing(name);
+    return null;
+  });
 
   React.useEffect(() => {
-    afterNextFrame(() => {
-      const tracingIntegration = getCurrentHub().getIntegration(TRACING_GETTER);
-      if (tracingIntegration !== null) {
-        // tslint:disable-next-line:no-unsafe-any
-        (tracingIntegration as any).constructor.popActivity(activity);
+    const mountSpan = getActivitySpan(mountActivity);
+    popActivity(mountActivity);
+
+    return () => {
+      if (mountSpan && options.hasRenderSpan) {
+        mountSpan.startChild({
+          description: `<${name}>`,
+          endTimestamp: timestampWithMs(),
+          op: `react.render`,
+          startTimestamp: mountSpan.endTimestamp,
+        });
       }
-    });
+    };
   }, []);
 }
 
diff --git a/packages/react/test/profiler.test.tsx b/packages/react/test/profiler.test.tsx
