more idle transaction refactor

Author: AbhiPrasad

packages/tracing/src/idletransaction.ts

@@ -56,6 +56,9 @@ export class IdleTransaction extends Transaction {
 
   private _heartbeatCounter: number = 0;
 
+  // We use heartbeat if we finished a transaction
+  private _finished: boolean = false;
+
   private _finishCallback: Function | undefined = undefined;
 
   private readonly _idleHub: Hub | undefined;
@@ -81,6 +84,10 @@ export class IdleTransaction extends Transaction {
    */
   private _beat(): void {
     clearTimeout(this._heartbeatTimer);
+    // We should not be running heartbeat if the idle transaction is finished.
+    if (this._finished) {
+      return;
+    }
     const keys = Object.keys(this.activities);
     const heartbeatString = keys.length ? keys.reduce((prev: string, current: string) => prev + current) : '';
 
@@ -122,7 +129,8 @@ export class IdleTransaction extends Transaction {
     if (this._idleHub) {
       const scope = this._idleHub.getScope();
       if (scope) {
-        if (scope.getSpan() === this) {
+        const span = scope.getSpan();
+        if (span && span.spanId === this.spanId) {
           scope.setSpan(undefined);
         }
       }
@@ -163,6 +171,7 @@ export class IdleTransaction extends Transaction {
 
       logger.log('[Tracing] flushing IdleTransaction');
       this.finish();
+      this._finished = true;
       this._resetActiveTransaction();
     } else {
       logger.log('[Tracing] No active IdleTransaction');
@@ -200,7 +209,8 @@ export class IdleTransaction extends Transaction {
   }
 
   /**
-   * Register a callback function that gets excecuted before the transaction finishes
+   * Register a callback function that gets excecuted before the transaction finishes.
+   * Useful for cleanup or if you want to add any additional spans based on current context.
    */
   public beforeFinish(callback: (transactionSpan: IdleTransaction) => void): void {
     this._finishCallback = callback;

packages/tracing/src/integrations/browsertracing.ts

@@ -1,22 +1,14 @@
 import { Hub } from '@sentry/hub';
-import { Event, EventProcessor, Integration, Severity, TransactionContext } from '@sentry/types';
+import { Event, EventProcessor, Integration, Severity } from '@sentry/types';
 import { logger, safeJoin } from '@sentry/utils';
 
+import { IdleTransaction } from '../idletransaction';
 import { SpanStatus } from '../spanstatus';
-import { Transaction } from '../transaction';
-
-import {
-  RoutingInstrumentationClass,
-  RoutingInstrumentation,
-  TracingRouterOptions,
-  TracingRouter,
-} from './tracing/router';
+
+import { RoutingInstrumentationClass, TracingRouter, TracingRouterOptions } from './tracing/router';
 import { Location as LocationType } from './tracing/types';
 
 /**
- * TODO: Figure out Tracing._resetActiveTransaction()
- *  - No clue :/
- *  - Maybe on finish, all transactions should remove themselves off the scope?
  * TODO: Figure out Tracing.finishIdleTransaction()
  *  - Need beforeFinish() transaction hook here
  * TODO: Figure out both XHR and Fetch tracing
@@ -148,7 +140,11 @@ export class BrowserTracing implements Integration {
       startTransactionOnPageLoad,
     });
 
-    routerTracing.init(hub, idleTimeout);
+    const beforeFinish = (transactionSpan: IdleTransaction): void => {
+      // BrowserTracing._beforeFinish(transactionSpan);
+    };
+
+    routerTracing.init(hub, idleTimeout, beforeFinish);
 
     // This EventProcessor makes sure that the transaction is not longer than maxTransactionDuration
     addGlobalEventProcessor((event: Event) => {
@@ -185,6 +181,15 @@ export class BrowserTracing implements Integration {
     });
   }
 
+  /**
+   * Called before the idle transaction finishes
+   * {@see IdleTransaction.beforeFinish}
+   */
+  // private static _beforeFinish(transaction: IdleTransaction): void {
+  //   this._checkIfOutdated;
+  //   //this._addPerformanceEntries(transaction);
+  // }
+
   /**
    * Uses logger.log to log things in the SDK or as breadcrumbs if defined in options
    */

packages/tracing/src/integrations/tracing/backgroundtab.ts

@@ -55,6 +55,11 @@ export interface RoutingInstrumentation {
    * init the routing instrumentation
    */
   init(hub: Hub, idleTimeout: number, beforeFinish?: (transactionSpan: IdleTransaction) => void): void;
+
+  /**
+   * Start an idle transaction. Called by init().
+   */
+  startIdleTransaction(hub: Hub, op: 'pageload' | 'navigation', idleTimeout: number): IdleTransaction | undefined;
 }
 
 export type RoutingInstrumentationClass = new (_options?: TracingRouterOptions) => RoutingInstrumentation;
@@ -72,8 +77,14 @@ export class TracingRouter implements RoutingInstrumentation {
     }
   }
 
-  /** JSDOC */
-  private _startIdleTransaction(hub: Hub, op: string, idleTimeout: number): IdleTransaction | undefined {
+  /**
+   * Starts an idle transaction.
+   */
+  public startIdleTransaction(
+    hub: Hub,
+    op: 'pageload' | 'navigation',
+    idleTimeout: number,
+  ): IdleTransaction | undefined {
     if (!global || !global.location || !hub) {
       return undefined;
     }
@@ -107,7 +118,7 @@ export class TracingRouter implements RoutingInstrumentation {
    */
   public init(hub: Hub, idleTimeout: number, beforeFinish?: (transactionSpan: IdleTransaction) => void): void {
     if (this.options.startTransactionOnPageLoad) {
-      this._activeTransaction = this._startIdleTransaction(hub, 'pageload', idleTimeout);
+      this._activeTransaction = this.startIdleTransaction(hub, 'pageload', idleTimeout);
       if (this._activeTransaction && beforeFinish) {
         this._activeTransaction.beforeFinish(beforeFinish);
       }
@@ -119,7 +130,7 @@ export class TracingRouter implements RoutingInstrumentation {
           if (this._activeTransaction) {
             this._activeTransaction.finish(timestampWithMs());
           }
-          this._activeTransaction = this._startIdleTransaction(hub, 'navigation', idleTimeout);
+          this._activeTransaction = this.startIdleTransaction(hub, 'navigation', idleTimeout);
           if (this._activeTransaction && beforeFinish) {
             this._activeTransaction.beforeFinish(beforeFinish);
           }

packages/tracing/src/integrations/tracing/router.ts

@@ -11,6 +11,27 @@ describe('IdleTransaction', () => {
     hub = new Hub(new BrowserClient({ tracesSampleRate: 1 }));
   });
 
+  it('sets the transaction on the scope on creation', () => {
+    const transaction = new IdleTransaction({ name: 'foo' }, hub, 1000);
+    transaction.initSpanRecorder(10);
+
+    hub.configureScope(s => {
+      expect(s.getTransaction()).toBe(transaction);
+    });
+  });
+
+  it('removes transaction from scope on finish', () => {
+    const transaction = new IdleTransaction({ name: 'foo' }, hub, 1000);
+    transaction.initSpanRecorder(10);
+
+    transaction.finish();
+    jest.runAllTimers();
+
+    hub.configureScope(s => {
+      expect(s.getTransaction()).toBe(undefined);
+    });
+  });
+
   it('push and pops activities', () => {
     const mockFinish = jest.fn();
     const transaction = new IdleTransaction({ name: 'foo' }, hub, 1000);
@@ -25,8 +46,16 @@ describe('IdleTransaction', () => {
 
     span.finish();
     expect(transaction.activities).toMatchObject({});
+
     jest.runOnlyPendingTimers();
+    expect(mockFinish).toHaveBeenCalledTimes(1);
 
+    // After we run all timers, we still want to make sure that
+    // transaction.finish() is only called once. This tells us that the
+    // heartbeat was stopped and that the `_finished` class
+    // property was used properly. This allows us to know if the
+    // heartbeat was correctly shut down.
+    jest.runAllTimers();
     expect(mockFinish).toHaveBeenCalledTimes(1);
   });