fix(tracing): Allow unsampled transactions to be findable by getTransaction() (#2952)

Adds a pointer to each span, pointing at that span's enclosing transaction, and changes `getTransaction()` to use it.

Author: lobsterkatie

packages/hub/src/scope.ts

@@ -217,10 +217,20 @@ export class Scope implements ScopeInterface {
    * @inheritDoc
    */
   public getTransaction(): Transaction | undefined {
-    const span = this.getSpan() as Span & { spanRecorder: { spans: Span[] } };
-    if (span && span.spanRecorder && span.spanRecorder.spans[0]) {
+    // often, this span will be a transaction, but it's not guaranteed to be
+    const span = this.getSpan() as undefined | (Span & { spanRecorder: { spans: Span[] } });
+
+    // try it the new way first
+    if (span?.transaction) {
+      return span?.transaction;
+    }
+
+    // fallback to the old way (known bug: this only finds transactions with sampled = true)
+    if (span?.spanRecorder?.spans[0]) {
       return span.spanRecorder.spans[0] as Transaction;
     }
+
+    // neither way found a transaction
     return undefined;
   }
 

packages/tracing/src/span.ts

@@ -1,5 +1,5 @@
 /* eslint-disable max-lines */
-import { Span as SpanInterface, SpanContext } from '@sentry/types';
+import { Span as SpanInterface, SpanContext, Transaction } from '@sentry/types';
 import { dropUndefinedKeys, timestampWithMs, uuid4 } from '@sentry/utils';
 
 import { SpanStatus } from './spanstatus';
@@ -99,6 +99,11 @@ export class Span implements SpanInterface, SpanContext {
    */
   public spanRecorder?: SpanRecorder;
 
+  /**
+   * @inheritDoc
+   */
+  public transaction?: Transaction;
+
   /**
    * You should never call the constructor manually, always use `hub.startSpan()`.
    * @internal
@@ -161,19 +166,21 @@ export class Span implements SpanInterface, SpanContext {
   public startChild(
     spanContext?: Pick<SpanContext, Exclude<keyof SpanContext, 'spanId' | 'sampled' | 'traceId' | 'parentSpanId'>>,
   ): Span {
-    const span = new Span({
+    const childSpan = new Span({
       ...spanContext,
       parentSpanId: this.spanId,
       sampled: this.sampled,
       traceId: this.traceId,
     });
 
-    span.spanRecorder = this.spanRecorder;
-    if (span.spanRecorder) {
-      span.spanRecorder.add(span);
+    childSpan.spanRecorder = this.spanRecorder;
+    if (childSpan.spanRecorder) {
+      childSpan.spanRecorder.add(childSpan);
     }
 
-    return span;
+    childSpan.transaction = this.transaction;
+
+    return childSpan;
   }
 
   /**

packages/tracing/src/transaction.ts

@@ -32,6 +32,9 @@ export class Transaction extends SpanClass implements TransactionInterface {
     this.name = transactionContext.name ? transactionContext.name : '';
 
     this._trimEnd = transactionContext.trimEnd;
+
+    // this is because transactions are also spans, and spans have a transaction pointer
+    this.transaction = this;
   }
 
   /**

packages/tracing/test/browser/browsertracing.test.ts

@@ -178,8 +178,7 @@ describe('BrowserTracing', () => {
         expect(mockBeforeNavigation).toHaveBeenCalledTimes(1);
       });
 
-      // TODO add this back in once getTransaction() returns sampled = false transactions, too
-      it.skip('creates a transaction with sampled = false if it returns undefined', () => {
+      it('creates a transaction with sampled = false if beforeNavigate returns undefined', () => {
         const mockBeforeNavigation = jest.fn().mockReturnValue(undefined);
         createBrowserTracing(true, {
           beforeNavigate: mockBeforeNavigation,
@@ -412,8 +411,7 @@ describe('BrowserTracing', () => {
     });
 
     describe('using the data', () => {
-      // TODO add this back in once getTransaction() returns sampled = false transactions, too
-      it.skip('uses the data for pageload transactions', () => {
+      it('uses the data for pageload transactions', () => {
         // make sampled false here, so we can see that it's being used rather than the tracesSampleRate-dictated one
         document.head.innerHTML = `<meta name="sentry-trace" content="12312012123120121231201212312012-1121201211212012-0">`;
 

packages/tracing/test/hub.test.ts

@@ -44,8 +44,7 @@ describe('Hub', () => {
       expect(hub.getScope()?.getTransaction()).toBe(transaction);
     });
 
-    // TODO add this back in once getTransaction() returns sampled = false transactions, too
-    it.skip('should find a transaction which has been set on the scope if sampled = false', () => {
+    it('should find a transaction which has been set on the scope if sampled = false', () => {
       const hub = new Hub(new BrowserClient({ tracesSampleRate: 1 }));
       const transaction = hub.startTransaction({ name: 'dogpark', sampled: false });
 
@@ -384,8 +383,7 @@ describe('Hub', () => {
         expect(extractTraceparentData(headers['sentry-trace'])!.parentSampled).toBe(true);
       });
 
-      // TODO add this back in once getTransaction() returns sampled = false transactions, too
-      it.skip('should propagate negative sampling decision to child transactions in XHR header', () => {
+      it('should propagate negative sampling decision to child transactions in XHR header', () => {
         const hub = new Hub(
           new BrowserClient({
             dsn: 'https://[email protected]/1121',

packages/types/src/span.ts

@@ -1,3 +1,5 @@
+import { Transaction } from './transaction';
+
 /** Interface holding all properties that can be set on a Span on creation. */
 export interface SpanContext {
   /**
@@ -84,6 +86,11 @@ export interface Span extends SpanContext {
    */
   data: { [key: string]: any };
 
+  /**
+   * The transaction containing this span
+   */
+  transaction?: Transaction;
+
   /**
    * Sets the finish timestamp on the current span.
    * @param endTimestamp Takes an endTimestamp if the end should not be the time when you call this function.