ref: CodeReview

Author: HazAT

CHANGELOG.md

@@ -16,7 +16,7 @@
 - [core] fix: sent_at for envelope headers to use same clock #2597
 - [apm] fix: Improve bundle size by moving span status to @sentry/apm #2589
 - [apm] feat: No longer discard transactions instead mark them deadline exceeded #2588
-- [apm] feat: Introduce `Sentry.startTransaction` and `startChild` #2600
+- [apm] feat: Introduce `Sentry.startTransaction` and `Transaction.startChild` #2600
 
 ## 5.15.5
 

packages/apm/src/hubextensions.ts

@@ -25,27 +25,25 @@ function traceHeaders(): { [key: string]: string } {
  * the created Span with the SpanContext will have a reference to it and become it's child.
  * Otherwise it'll crete a new `Span`.
  *
- * @param spanContext Properties with which the span should be created
+ * @param context Properties with which the span should be created
  */
-function startSpan(spanOrTransactionContext: SpanContext | TransactionContext): Transaction | Span {
+function startSpan(context: SpanContext | TransactionContext): Transaction | Span {
   // @ts-ignore
   const hub = this as Hub;
   const scope = hub.getScope();
   const client = hub.getClient();
 
-  let newSpanContext = spanOrTransactionContext;
+  let newSpanContext = context;
 
-  // We create an empty Span in case there is no scope on the hub
-  let parentSpan = new Span();
   if (scope) {
     // If there is a Span on the Scope we use the span_id / trace_id
     // To define the parent <-> child relationship
-    parentSpan = scope.getSpan() as Span;
+    const parentSpan = scope.getSpan();
     if (parentSpan) {
       const { trace_id } = parentSpan.getTraceContext();
       newSpanContext = {
         traceId: trace_id,
-        ...spanOrTransactionContext,
+        ...context,
       };
     }
   }
@@ -57,7 +55,7 @@ function startSpan(spanOrTransactionContext: SpanContext | TransactionContext):
     // We only roll the dice on sampling for root spans of transactions because all child spans inherit this state
     if (transaction.sampled === undefined) {
       const sampleRate = (client && client.getOptions().tracesSampleRate) || 0;
-      transaction.sampled = Math.random() < sampleRate;
+      transaction.sampled = Math.random() <= sampleRate;
     }
 
     // We only want to create a span list if we sampled the transaction

packages/apm/src/integrations/express.ts

@@ -1,8 +1,12 @@
-import { Integration } from '@sentry/types';
+import { Integration, Transaction } from '@sentry/types';
 import { logger } from '@sentry/utils';
 // tslint:disable-next-line:no-implicit-dependencies
 import { Application, ErrorRequestHandler, NextFunction, Request, RequestHandler, Response } from 'express';
 
+interface SentryTracingResponse {
+  __sentry_transaction?: Transaction;
+}
+
 /**
  * Express integration
  *
@@ -61,61 +65,65 @@ function wrap(fn: Function): RequestHandler | ErrorRequestHandler {
 
   switch (arrity) {
     case 2: {
-      return function(this: NodeJS.Global, _req: Request, res: Response): any {
-        // tslint:disable: no-unsafe-any
-        const transaction = (res as any).getTransaction && (res as any).getTransaction();
+      return function(this: NodeJS.Global, _req: Request, res: Response & SentryTracingResponse): any {
+        const transaction = res.__sentry_transaction;
         if (transaction) {
           const span = transaction.startChild({
             description: fn.name,
             op: 'middleware',
           });
           res.once('finish', () => {
-            span.finish();
+            if (span) {
+              span.finish();
+            }
           });
         }
-        // tslint:enable: no-unsafe-any
         return fn.apply(this, arguments);
       };
     }
     case 3: {
-      return function(this: NodeJS.Global, req: Request, res: Response, next: NextFunction): any {
-        // tslint:disable: no-unsafe-any
-        const transaction = (res as any).getTransaction && (res as any).getTransaction();
-        if (transaction) {
-          const span = transaction.startChild({
+      return function(
+        this: NodeJS.Global,
+        req: Request,
+        res: Response & SentryTracingResponse,
+        next: NextFunction,
+      ): any {
+        const transaction = res.__sentry_transaction;
+        const span =
+          transaction &&
+          transaction.startChild({
             description: fn.name,
             op: 'middleware',
           });
-          fn.call(this, req, res, function(this: NodeJS.Global): any {
+        fn.call(this, req, res, function(this: NodeJS.Global): any {
+          if (span) {
             span.finish();
-            return next.apply(this, arguments);
-          });
-        } else {
-          fn.call(this, req, res, function(this: NodeJS.Global): any {
-            return next.apply(this, arguments);
-          });
-        }
-        // tslint:enable: no-unsafe-any
+          }
+          return next.apply(this, arguments);
+        });
       };
     }
     case 4: {
-      return function(this: NodeJS.Global, err: any, req: Request, res: Response, next: NextFunction): any {
-        // tslint:disable: no-unsafe-any
-        const transaction = (res as any).getTransaction && (res as any).getTransaction();
-        if (transaction) {
-          const span = transaction.startChild({
+      return function(
+        this: NodeJS.Global,
+        err: any,
+        req: Request,
+        res: Response & SentryTracingResponse,
+        next: NextFunction,
+      ): any {
+        const transaction = res.__sentry_transaction;
+        const span =
+          transaction &&
+          transaction.startChild({
             description: fn.name,
             op: 'middleware',
           });
-          fn.call(this, err, req, res, function(this: NodeJS.Global): any {
+        fn.call(this, err, req, res, function(this: NodeJS.Global): any {
+          if (span) {
             span.finish();
-            return next.apply(this, arguments);
-          });
-        } else {
-          fn.call(this, err, req, res, function(this: NodeJS.Global): any {
-            return next.apply(this, arguments);
-          });
-        }
+          }
+          return next.apply(this, arguments);
+        });
       };
     }
     default: {

packages/apm/src/integrations/tracing.ts

@@ -1,5 +1,5 @@
 import { Hub } from '@sentry/hub';
-import { Event, EventProcessor, Integration, Severity, Span, SpanContext } from '@sentry/types';
+import { Event, EventProcessor, Integration, Severity, Span, SpanContext, TransactionContext } from '@sentry/types';
 import {
   addInstrumentationHandler,
   getGlobalObject,
@@ -71,7 +71,7 @@ interface TracingOptions {
   maxTransactionDuration: number;
 
   /**
-   * Flag Transactions where tabs moved to background with "cancelled".Browser background tab timing is
+   * Flag Transactions where tabs moved to background with "cancelled". Browser background tab timing is
    * not suited towards doing precise measurements of operations. Background transaction can mess up your
    * statistics in non deterministic ways that's why we by default recommend leaving this opition enabled.
    *
@@ -205,7 +205,8 @@ export class Tracing implements Integration {
     // Starting pageload transaction
     if (global.location && global.location.href) {
       // Use `${global.location.href}` as transaction name
-      Tracing.startIdleTransaction(global.location.href, {
+      Tracing.startIdleTransaction({
+        name: global.location.href,
         op: 'pageload',
       });
     }
@@ -412,13 +413,13 @@ export class Tracing implements Integration {
   /**
    * Starts a Transaction waiting for activity idle to finish
    */
-  public static startIdleTransaction(name: string, spanContext?: SpanContext): Span | undefined {
+  public static startIdleTransaction(transactionContext: TransactionContext): Transaction | undefined {
     // If we already have an active transaction it means one of two things
     // a) The user did rapid navigation changes and didn't wait until the transaction was finished
     // b) A activity wasn't popped correctly and therefore the transaction is stalling
     Tracing.finishIdleTransaction();
 
-    Tracing._log('[Tracing] startIdleTransaction, name:', name);
+    Tracing._log('[Tracing] startIdleTransaction, name:', transactionContext.name);
 
     const _getCurrentHub = Tracing._getCurrentHub;
     if (!_getCurrentHub) {
@@ -431,8 +432,8 @@ export class Tracing implements Integration {
     }
 
     Tracing._activeTransaction = hub.startSpan({
-      ...spanContext,
-      name,
+      trimEnd: true,
+      ...transactionContext,
     }) as Transaction;
 
     // The reason we do this here is because of cached responses
@@ -453,7 +454,7 @@ export class Tracing implements Integration {
     if (active) {
       Tracing._addPerformanceEntries(active);
       Tracing._log('[Tracing] finishIdleTransaction', active.name);
-      active.finish(undefined, /*trimEnd*/ true);
+      active.finish();
       Tracing._resetActiveTransaction();
     }
   }
@@ -860,7 +861,8 @@ function fetchCallback(handlerData: { [key: string]: any }): void {
  */
 function historyCallback(_: { [key: string]: any }): void {
   if (Tracing.options.startTransactionOnLocationChange && global && global.location) {
-    Tracing.startIdleTransaction(global.location.href, {
+    Tracing.startIdleTransaction({
+      name: global.location.href,
       op: 'navigation',
     });
   }

packages/apm/src/transaction.ts

@@ -26,7 +26,7 @@ export class SpanRecorder {
    * start_timestamp).
    */
   public add(span: SpanClass): void {
-    if (this.spans.length > this._maxlen) {
+    if (this.spans.length >= this._maxlen) {
       span.spanRecorder = undefined;
     } else {
       this.spans.push(span);
@@ -43,6 +43,8 @@ export class Transaction extends SpanClass {
 
   public name?: string;
 
+  private readonly _trimEnd?: boolean;
+
   /**
    * This constructor should never be called manually. Those instrumenting tracing should use `Stentry.startTransaction()`, and internal methods should use `hub.startSpan()`.
    * @internal
@@ -59,6 +61,8 @@ export class Transaction extends SpanClass {
     if (transactionContext.name) {
       this.name = transactionContext.name;
     }
+
+    this._trimEnd = transactionContext.trimEnd;
   }
 
   /**
@@ -80,16 +84,9 @@ export class Transaction extends SpanClass {
   }
 
   /**
-   * Sets the finish timestamp on the current span.
-   *
-   * @inheritdoc
-   *
-   * @param trimEnd If true, sets the end timestamp of the transaction to the highest timestamp of child spans, trimming
-   * the duration of the transaction. This is useful to discard extra time in the transaction that is not
-   * accounted for in child spans, like what happens in the idle transaction Tracing integration, where we finish the
-   * transaction after a given "idle time" and we don't want this "idle time" to be part of the transaction.
+   * @inheritDoc
    */
-  public finish(endTimestamp?: number, trimEnd: boolean = false): string | undefined {
+  public finish(endTimestamp?: number): string | undefined {
     // This transaction is already finished, so we should not flush it again.
     if (this.endTimestamp !== undefined) {
       return undefined;
@@ -105,7 +102,7 @@ export class Transaction extends SpanClass {
 
     const finishedSpans = this.spanRecorder ? this.spanRecorder.spans.filter(s => s !== this && s.endTimestamp) : [];
 
-    if (trimEnd && finishedSpans.length > 0) {
+    if (this._trimEnd && finishedSpans.length > 0) {
       this.endTimestamp = finishedSpans.reduce((prev: SpanClass, current: SpanClass) => {
         if (prev.endTimestamp && current.endTimestamp) {
           return prev.endTimestamp > current.endTimestamp ? prev : current;

packages/hub/src/hub.ts

@@ -368,8 +368,8 @@ export class Hub implements HubInterface {
   /**
    * @inheritDoc
    */
-  public startSpan(spanOrTransactionContext: SpanContext | TransactionContext): Transaction | Span {
-    return this._callExtensionMethod('startSpan', spanOrTransactionContext);
+  public startSpan(context: SpanContext | TransactionContext): Transaction | Span {
+    return this._callExtensionMethod('startSpan', context);
   }
 
   /**

packages/node/src/handlers.ts

@@ -53,21 +53,17 @@ export function tracingHandler(): (
       traceId,
     });
 
-    if (transaction) {
-      // We put the transaction on the scope so users can attach children to it
-      getCurrentHub().configureScope(scope => {
-        scope.setSpan(transaction);
-      });
-      // We also set a function getTransaction on the response so people can grab the transaction there to add
-      // spans to it
-      (res as any).getTransaction = () => transaction;
-    }
+    // We put the transaction on the scope so users can attach children to it
+    getCurrentHub().configureScope(scope => {
+      scope.setSpan(transaction);
+    });
+    // We also set __sentry_transaction on the response so people can grab the transaction there to add
+    // spans to it later.
+    (res as any).__sentry_transaction = transaction;
 
     res.once('finish', () => {
-      if (transaction) {
-        transaction.setHttpStatus(res.statusCode);
-        transaction.finish();
-      }
+      transaction.setHttpStatus(res.statusCode);
+      transaction.finish();
     });
 
     next();
@@ -303,7 +299,6 @@ export function parseRequest(
   }
 
   if (options.transaction && !event.transaction) {
-    // TODO: Use the real transaction here
     const transaction = extractTransaction(req, options.transaction);
     if (transaction) {
       event.transaction = transaction;

packages/types/src/hub.ts

@@ -177,7 +177,7 @@ export interface Hub {
    * This functions starts either a Span or a Transaction (depending on the argument passed).
    * If there is a Span on the Scope we use the `trace_id` for all other created Transactions / Spans as a reference.
    *
-   * @param spanOrTransactionContext Properties with which the Transaction/Span should be created
+   * @param context Properties with which the Transaction/Span should be created
    */
-  startSpan(spanOrTransactionContext: SpanContext | TransactionContext): Transaction | Span;
+  startSpan(context: SpanContext | TransactionContext): Transaction | Span;
 }