Rework render span

AbhiPrasad · AbhiPrasad · commit cb2ce415bb79 · 2020-06-17T10:12:51.000-04:00
diff --git a/packages/apm/src/integrations/tracing.ts b/packages/apm/src/integrations/tracing.ts
@@ -776,7 +776,7 @@ export class Tracing implements Integration {
     spanContext?: SpanContext,
     options?: {
       autoPopAfter?: number;
-    },∂
+    },
   ): number {
     const activeTransaction = Tracing._activeTransaction;
 
diff --git a/packages/react/src/profiler.tsx b/packages/react/src/profiler.tsx
@@ -83,7 +83,7 @@ export type ProfilerProps = {
   // 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. False by default.
+  // 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;
@@ -105,7 +105,7 @@ class Profiler extends React.Component<ProfilerProps> {
 
   public static defaultProps: Partial<ProfilerProps> = {
     disabled: false,
-    hasRenderSpan: false,
+    hasRenderSpan: true,
     hasUpdateSpan: true,
   };
 
@@ -129,17 +129,6 @@ class Profiler extends React.Component<ProfilerProps> {
     this.mountSpan = getActivitySpan(this.mountActivity);
     popActivity(this.mountActivity);
     this.mountActivity = null;
-
-    const { name, hasRenderSpan = false } = this.props;
-
-    // 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.
-    if (this.mountSpan && hasRenderSpan) {
-      this.renderSpan = this.mountSpan.startChild({
-        description: `<${name}>`,
-        op: `react.render`,
-      });
-    }
   }
 
   public componentDidUpdate({ updateProps, hasUpdateSpan = true }: ProfilerProps): void {
@@ -170,8 +159,17 @@ class Profiler extends React.Component<ProfilerProps> {
   // 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 {
-    if (this.renderSpan) {
-      this.renderSpan.finish();
+    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,
+      });
     }
   }
 
@@ -219,9 +217,9 @@ function withProfiler<P extends object>(
  */
 function useProfiler(
   name: string,
-  options?: {
-    disabled?: boolean;
-    hasRenderSpan?: boolean;
+  options: { disabled?: boolean; hasRenderSpan?: boolean } = {
+    disabled: false,
+    hasRenderSpan: true,
   },
 ): void {
   const [mountActivity] = React.useState(() => {
@@ -241,17 +239,14 @@ function useProfiler(
     const mountSpan = getActivitySpan(mountActivity);
     popActivity(mountActivity);
 
-    const renderSpan =
-      mountSpan && options && options.hasRenderSpan
-        ? mountSpan.startChild({
-            description: `<${name}>`,
-            op: `react.render`,
-          })
-        : undefined;
-
     return () => {
-      if (renderSpan) {
-        renderSpan.finish();
+      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
@@ -12,8 +12,7 @@ SENTRY_APM_SAMPLING = 1
 )*/
 const TEST_SPAN_ID = '518999beeceb49af';
 
-const mockSpanFinish = jest.fn();
-const mockStartChild = jest.fn((spanArgs: SpanContext) => ({ ...spanArgs, finish: mockSpanFinish }));
+const mockStartChild = jest.fn((spanArgs: SpanContext) => ({ ...spanArgs }));
 const TEST_SPAN = {
   spanId: TEST_SPAN_ID,
   startChild: mockStartChild,
@@ -57,7 +56,6 @@ beforeEach(() => {
   mockLoggerWarn.mockClear();
   mockGetActivitySpan.mockClear();
   mockStartChild.mockClear();
-  mockSpanFinish.mockClear();
 });
 
 describe('withProfiler', () => {
@@ -111,26 +109,30 @@ describe('withProfiler', () => {
   });
 
   describe('render span', () => {
-    it('does not get created by default', () => {
+    it('is created on unmount', () => {
       const ProfiledComponent = withProfiler(() => <h1>Testing</h1>);
       expect(mockStartChild).toHaveBeenCalledTimes(0);
-      render(<ProfiledComponent />);
-      expect(mockStartChild).toHaveBeenCalledTimes(0);
-    });
 
-    it('is created when given hasRenderSpan option', () => {
-      const ProfiledComponent = withProfiler(() => <h1>Testing</h1>, { hasRenderSpan: true });
-      expect(mockStartChild).toHaveBeenCalledTimes(0);
       const component = render(<ProfiledComponent />);
+      component.unmount();
+
       expect(mockStartChild).toHaveBeenCalledTimes(1);
-      expect(mockStartChild).toHaveBeenLastCalledWith({
-        description: `<${UNKNOWN_COMPONENT}>`,
-        op: 'react.render',
-      });
+      expect(mockStartChild).toHaveBeenLastCalledWith(
+        expect.objectContaining({
+          description: `<${UNKNOWN_COMPONENT}>`,
+          op: 'react.render',
+        }),
+      );
+    });
 
-      expect(mockSpanFinish).toHaveBeenCalledTimes(0);
+    it('is not created if hasRenderSpan is false', () => {
+      const ProfiledComponent = withProfiler(() => <h1>Testing</h1>, { hasRenderSpan: false });
+      expect(mockStartChild).toHaveBeenCalledTimes(0);
+
+      const component = render(<ProfiledComponent />);
       component.unmount();
-      expect(mockSpanFinish).toHaveBeenCalledTimes(1);
+
+      expect(mockStartChild).toHaveBeenCalledTimes(0);
     });
   });
 
@@ -207,25 +209,27 @@ describe('useProfiler()', () => {
   });
 
   describe('render span', () => {
-    it('does not get created by default', () => {
+    it('does not get created when hasRenderSpan is false', () => {
       // tslint:disable-next-line: no-void-expression
-      renderHook(() => useProfiler('Example'));
+      const component = renderHook(() => useProfiler('Example', { hasRenderSpan: false }));
+      expect(mockStartChild).toHaveBeenCalledTimes(0);
+      component.unmount();
       expect(mockStartChild).toHaveBeenCalledTimes(0);
     });
 
-    it('is created when given hasRenderSpan option', () => {
+    it('is created by default', () => {
       // tslint:disable-next-line: no-void-expression
-      const component = renderHook(() => useProfiler('Example', { hasRenderSpan: true }));
-
-      expect(mockStartChild).toHaveBeenCalledTimes(1);
-      expect(mockStartChild).toHaveBeenLastCalledWith({
-        description: '<Example>',
-        op: 'react.render',
-      });
+      const component = renderHook(() => useProfiler('Example'));
 
-      expect(mockSpanFinish).toHaveBeenCalledTimes(0);
+      expect(mockStartChild).toHaveBeenCalledTimes(0);
       component.unmount();
-      expect(mockSpanFinish).toHaveBeenCalledTimes(1);
+      expect(mockStartChild).toHaveBeenCalledTimes(1);
+      expect(mockStartChild).toHaveBeenLastCalledWith(
+        expect.objectContaining({
+          description: '<Example>',
+          op: 'react.render',
+        }),
+      );
     });
   });
 });
