ref: Add hub.startTransaction extension (getsentry#2626)

rhcarvalho · HazAT · web-flow · commit 727da280a006 · 2020-06-02T11:49:01.000+02:00
* ref: Check for missing name in startTransaction

Moving the check to the func where it matters eliminates the need for
the internal field _isTransaction and the downstream logic in startSpan.

* ref: Shorten log message

Shorter message equals smaller bundle size.
Limit ourselves to a single warning line.

The fix to eliminate the warning is to call
  startTransaction({name: "some name"})
so the setName advice is misplaced.

* ref: Add hub.startTransaction extension

Extract code that starts transactions from startSpan and deprecate
startSpan.

Move check of empty transaction name to right before sending the
transaction, because it is valid to start the transaction without a name
and set the name later with Transaction.setName().

* misc: Disable tslint deprecation check

* ref: Changes

* ref: Update docs

* fix: Typo

* ref: Small refactor

Co-authored-by: Daniel Griesser &lt;daniel.griesser.86@gmail.com&gt;
diff --git a/packages/apm/src/hubextensions.ts b/packages/apm/src/hubextensions.ts
@@ -1,4 +1,4 @@
-import { getMainCarrier, Hub } from '@sentry/hub';
+import { getCurrentHub, getMainCarrier, Hub } from '@sentry/hub';
 import { SpanContext, TransactionContext } from '@sentry/types';
 import { logger } from '@sentry/utils';
 
@@ -20,44 +20,49 @@ function traceHeaders(this: Hub): { [key: string]: string } {
 }
 
 /**
- * This functions starts a span. If there is already a `Span` on the Scope,
- * 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 context Properties with which the span should be created
+ * {@see Hub.startTransaction}
  */
-function startSpan(this: Hub, context: SpanContext | TransactionContext): Transaction | Span {
-  // This is our safeguard so people always get a Transaction in return.
-  // We set `_isTransaction: true` in {@link Sentry.startTransaction} to have a runtime check
-  // if the user really wanted to create a Transaction.
-  if ((context as TransactionContext)._isTransaction && !(context as TransactionContext).name) {
-    logger.warn('You are trying to start a Transaction but forgot to provide a `name` property.');
-    logger.warn('Will fall back to <unlabeled transaction>, use `transaction.setName()` to change it.');
-    (context as TransactionContext).name = '<unlabeled transaction>';
+function startTransaction(this: Hub, context: TransactionContext): Transaction {
+  const transaction = new Transaction(context, this);
+
+  const client = this.getClient();
+  // Roll the dice for sampling transaction, all child spans inherit the sampling decision.
+  if (transaction.sampled === undefined) {
+    const sampleRate = (client && client.getOptions().tracesSampleRate) || 0;
+    // if true = we want to have the transaction
+    // if false = we don't want to have it
+    // Math.random (inclusive of 0, but not 1)
+    transaction.sampled = Math.random() < sampleRate;
   }
 
-  if ((context as TransactionContext).name) {
-    // We are dealing with a Transaction
-    const transaction = new Transaction(context as TransactionContext, this);
+  // We only want to create a span list if we sampled the transaction
+  // If sampled == false, we will discard the span anyway, so we can save memory by not storing child spans
+  if (transaction.sampled) {
+    const experimentsOptions = (client && client.getOptions()._experiments) || {};
+    transaction.initSpanRecorder(experimentsOptions.maxSpans as number);
+  }
 
-    const client = this.getClient();
-    // 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;
-      // if true = we want to have the transaction
-      // if false = we don't want to have it
-      // Math.random (inclusive of 0, but not 1)
-      transaction.sampled = Math.random() < sampleRate;
-    }
+  return transaction;
+}
 
-    // We only want to create a span list if we sampled the transaction
-    // If sampled == false, we will discard the span anyway, so we can save memory by not storing child spans
-    if (transaction.sampled) {
-      const experimentsOptions = (client && client.getOptions()._experiments) || {};
-      transaction.initSpanRecorder(experimentsOptions.maxSpans as number);
-    }
+/**
+ * {@see Hub.startSpan}
+ */
+function startSpan(this: Hub, context: SpanContext): Transaction | Span {
+  /**
+   * @deprecated
+   * This is here to make sure we don't break users that relied on calling startSpan to create a transaction
+   * with the transaction poperty set.
+   */
+  if ((context as any).transaction !== undefined) {
+    logger.warn(`Use \`Sentry.startTransaction({name: ${(context as any).transaction}})\` to start a Transaction.`);
+    (context as TransactionContext).name = (context as any).transaction as string;
+  }
 
-    return transaction;
+  // We have the check of not undefined since we defined it's ok to start a transaction with an empty name
+  // tslint:disable-next-line: strict-type-predicates
+  if ((context as TransactionContext).name !== undefined) {
+    return getCurrentHub().startTransaction(context as TransactionContext);
   }
 
   const scope = this.getScope();
@@ -80,6 +85,9 @@ export function addExtensionMethods(): void {
   const carrier = getMainCarrier();
   if (carrier.__SENTRY__) {
     carrier.__SENTRY__.extensions = carrier.__SENTRY__.extensions || {};
+    if (!carrier.__SENTRY__.extensions.startTransaction) {
+      carrier.__SENTRY__.extensions.startTransaction = startTransaction;
+    }
     if (!carrier.__SENTRY__.extensions.startSpan) {
       carrier.__SENTRY__.extensions.startSpan = startSpan;
     }
diff --git a/packages/apm/src/span.ts b/packages/apm/src/span.ts
@@ -77,7 +77,7 @@ export class Span implements SpanInterface, SpanContext {
   public spanRecorder?: SpanRecorder;
 
   /**
-   * You should never call the custructor manually, always use `hub.startSpan()`.
+   * You should never call the constructor manually, always use `hub.startSpan()`.
    * @internal
    * @hideconstructor
    * @hidden
diff --git a/packages/apm/src/transaction.ts b/packages/apm/src/transaction.ts
@@ -46,7 +46,8 @@ export class Transaction extends SpanClass {
   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()`.
+   * This constructor should never be called manually. Those instrumenting tracing should use
+   * `Sentry.startTransaction()`, and internal methods should use `hub.startTransaction()`.
    * @internal
    * @hideconstructor
    * @hidden
@@ -92,6 +93,11 @@ export class Transaction extends SpanClass {
       return undefined;
     }
 
+    if (!this.name) {
+      logger.warn('Transaction has no name, falling back to `<unlabeled transaction>`.');
+      this.name = '<unlabeled transaction>';
+    }
+
     super.finish(endTimestamp);
 
     if (this.sampled !== true) {
diff --git a/packages/hub/src/hub.ts b/packages/hub/src/hub.ts
@@ -368,10 +368,17 @@ export class Hub implements HubInterface {
   /**
    * @inheritDoc
    */
-  public startSpan(context: SpanContext | TransactionContext): Transaction | Span {
+  public startSpan(context: SpanContext): Span {
     return this._callExtensionMethod('startSpan', context);
   }
 
+  /**
+   * @inheritDoc
+   */
+  public startTransaction(context: TransactionContext): Transaction {
+    return this._callExtensionMethod('startTransaction', context);
+  }
+
   /**
    * @inheritDoc
    */
diff --git a/packages/minimal/src/index.ts b/packages/minimal/src/index.ts
@@ -177,14 +177,12 @@ export function _callOnClient(method: string, ...args: any[]): void {
 }
 
 /**
- * This starts a Transaction and is considered the entry point to do manual tracing. You can add child spans
- * to a transactions. After that more children can be added to created spans to buld a tree structure.
- * This function returns a Transaction and you need to keep track of the instance yourself. When you call `.finsh()` on
- * a transaction it will be sent to Sentry.
+ * Starts a Transaction. This is the entry point to do manual tracing. You can
+ * add child spans to transactions. Spans themselves can have children, building
+ * a tree structure. This function returns a Transaction and you need to keep
+ * track of the instance yourself. When you call `.finish()` on the transaction
+ * it will be sent to Sentry.
  */
-export function startTransaction(transactionContext: TransactionContext): Transaction {
-  return callOnHub<Transaction>('startSpan', {
-    ...transactionContext,
-    _isTransaction: true,
-  });
+export function startTransaction(context: TransactionContext): Transaction {
+  return callOnHub('startTransaction', { ...context });
 }
diff --git a/packages/types/src/hub.ts b/packages/types/src/hub.ts
@@ -174,10 +174,26 @@ export interface Hub {
   traceHeaders(): { [key: string]: string };
 
   /**
-   * 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.
+   * This function starts a span. If there is already a `Span` on the Scope,
+   * the created Span with the SpanContext will have a reference to it and become it's child.
+   * Otherwise it'll create a new `Span`.
+   */
+  startSpan(context: SpanContext): Span;
+
+  /**
+   * Starts a new transaction and returns it. This is the entry point to manual
+   * tracing instrumentation.
+   *
+   * A tree structure can be built by adding child spans to the transaction, and
+   * child spans to other spans. To start a new child span within the transaction
+   * or any span, call the respective `.startChild()` method.
+   *
+   * Every child span must be finished before the transaction is finished,
+   * otherwise the unfinished spans are discarded.
    *
-   * @param context Properties with which the Transaction/Span should be created
+   * The transaction must be finished with a call to its `.finish()` method, at
+   * which point the transaction with all its finished child spans will be sent to
+   * Sentry.
    */
-  startSpan(context: SpanContext | TransactionContext): Transaction | Span;
+  startTransaction(context: TransactionContext): Transaction;
 }
diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts
@@ -12,13 +12,6 @@ export interface TransactionContext extends SpanContext {
    * transaction after a given "idle time" and we don't want this "idle time" to be part of the transaction.
    */
   trimEnd?: boolean;
-
-  /**
-   * This flag is internally used by {@link Sentry.startTransaction} and set there to determine this is really a
-   * Transaction. Since there is no type safety in JS we set it in the top level function to true to check later
-   * if the user really wanted to create a Transaction and we can log a warning if they forgot to set the name property.
-   */
-  _isTransaction?: boolean;
 }
 
 /**
