ref: Make span types more robust (#10660)

mydea · mydea · commit 01f63866a6e5 · 2024-02-21T09:12:30.000+01:00
With more strict TS settings, these could lead to problems: 1. `startChild()` should return a span interface, to match the interface implementation. 2. All properties that are optional in span/transactions need to also accept `| undefined` to be really correct - otherwise when we extend this and set `undefined` explicitly it complains there. Fixes #10654
diff --git a/packages/core/src/tracing/span.ts b/packages/core/src/tracing/span.ts
@@ -105,16 +105,16 @@ export class Span implements SpanInterface {
 
   protected _traceId: string;
   protected _spanId: string;
-  protected _parentSpanId?: string;
+  protected _parentSpanId?: string | undefined;
   protected _sampled: boolean | undefined;
-  protected _name?: string;
+  protected _name?: string | undefined;
   protected _attributes: SpanAttributes;
   /** Epoch timestamp in seconds when the span started. */
   protected _startTime: number;
   /** Epoch timestamp in seconds when the span ended. */
-  protected _endTime?: number;
+  protected _endTime?: number | undefined;
   /** Internal keeper of the status */
-  protected _status?: SpanStatusType | string;
+  protected _status?: SpanStatusType | string | undefined;
 
   private _logMessage?: string;
 
@@ -385,7 +385,7 @@ export class Span implements SpanInterface {
    */
   public startChild(
     spanContext?: Pick<SpanContext, Exclude<keyof SpanContext, 'sampled' | 'traceId' | 'parentSpanId'>>,
-  ): Span {
+  ): SpanInterface {
     const childSpan = new Span({
       ...spanContext,
       parentSpanId: this._spanId,
diff --git a/packages/core/src/tracing/transaction.ts b/packages/core/src/tracing/transaction.ts
@@ -35,7 +35,7 @@ export class Transaction extends SpanClass implements TransactionInterface {
 
   private _contexts: Contexts;
 
-  private _trimEnd?: boolean;
+  private _trimEnd?: boolean | undefined;
 
   // DO NOT yet remove this property, it is used in a hack for v7 backwards compatibility.
   private _frozenDynamicSamplingContext: Readonly<Partial<DynamicSamplingContext>> | undefined;
diff --git a/packages/tracing/test/integrations/apollo-nestjs.test.ts b/packages/tracing/test/integrations/apollo-nestjs.test.ts
@@ -1,9 +1,10 @@
 /* eslint-disable deprecation/deprecation */
 /* eslint-disable @typescript-eslint/unbound-method */
-import { Hub, Scope } from '@sentry/core';
+import { Hub, Scope, Span as SpanClass } from '@sentry/core';
+import type { Span } from '@sentry/types';
 import { logger } from '@sentry/utils';
 
-import { Integrations, Span } from '../../src';
+import { Integrations } from '../../src';
 import { getTestClient } from '../testutils';
 
 type ApolloResolverGroup = {
@@ -79,7 +80,7 @@ describe('setupOnce', () => {
 
   beforeEach(() => {
     scope = new Scope();
-    parentSpan = new Span();
+    parentSpan = new SpanClass();
     childSpan = parentSpan.startChild();
     jest.spyOn(scope, 'getSpan').mockReturnValueOnce(parentSpan);
     jest.spyOn(scope, 'setSpan');
diff --git a/packages/tracing/test/integrations/apollo.test.ts b/packages/tracing/test/integrations/apollo.test.ts
@@ -1,9 +1,10 @@
 /* eslint-disable deprecation/deprecation */
 /* eslint-disable @typescript-eslint/unbound-method */
-import { Hub, Scope } from '@sentry/core';
+import { Hub, Scope, Span as SpanClass } from '@sentry/core';
+import type { Span } from '@sentry/types';
 import { logger } from '@sentry/utils';
 
-import { Integrations, Span } from '../../src';
+import { Integrations } from '../../src';
 import { getTestClient } from '../testutils';
 
 type ApolloResolverGroup = {
@@ -79,7 +80,7 @@ describe('setupOnce', () => {
 
   beforeEach(() => {
     scope = new Scope();
-    parentSpan = new Span();
+    parentSpan = new SpanClass();
     childSpan = parentSpan.startChild();
     jest.spyOn(scope, 'getSpan').mockReturnValueOnce(parentSpan);
     jest.spyOn(scope, 'setSpan');
diff --git a/packages/tracing/test/integrations/graphql.test.ts b/packages/tracing/test/integrations/graphql.test.ts
@@ -1,9 +1,10 @@
 /* eslint-disable deprecation/deprecation */
 /* eslint-disable @typescript-eslint/unbound-method */
-import { Hub, Scope } from '@sentry/core';
+import { Hub, Scope, Span as SpanClass } from '@sentry/core';
+import type { Span } from '@sentry/types';
 import { logger } from '@sentry/utils';
 
-import { Integrations, Span } from '../../src';
+import { Integrations } from '../../src';
 import { getTestClient } from '../testutils';
 
 const GQLExecute = {
@@ -41,7 +42,7 @@ describe('setupOnce', () => {
 
   beforeEach(() => {
     scope = new Scope();
-    parentSpan = new Span();
+    parentSpan = new SpanClass();
     childSpan = parentSpan.startChild();
     jest.spyOn(scope, 'getSpan').mockReturnValueOnce(parentSpan);
     jest.spyOn(scope, 'setSpan');
diff --git a/packages/tracing/test/integrations/node/mongo.test.ts b/packages/tracing/test/integrations/node/mongo.test.ts
@@ -1,9 +1,10 @@
 /* eslint-disable deprecation/deprecation */
 /* eslint-disable @typescript-eslint/unbound-method */
-import { Hub, Scope } from '@sentry/core';
+import { Hub, Scope, Span as SpanClass } from '@sentry/core';
+import type { Span } from '@sentry/types';
 import { logger } from '@sentry/utils';
 
-import { Integrations, Span } from '../../../src';
+import { Integrations } from '../../../src';
 import { getTestClient } from '../../testutils';
 
 class Collection {
@@ -63,7 +64,7 @@ describe('patchOperation()', () => {
 
   beforeEach(() => {
     scope = new Scope();
-    parentSpan = new Span();
+    parentSpan = new SpanClass();
     childSpan = parentSpan.startChild();
     testClient = getTestClient({});
     jest.spyOn(scope, 'getSpan').mockReturnValueOnce(parentSpan);
diff --git a/packages/tracing/test/integrations/node/postgres.test.ts b/packages/tracing/test/integrations/node/postgres.test.ts
@@ -1,10 +1,11 @@
 /* eslint-disable deprecation/deprecation */
 /* eslint-disable @typescript-eslint/unbound-method */
-import { Hub, Scope } from '@sentry/core';
+import { Hub, Scope, Span as SpanClass } from '@sentry/core';
+import type { Span } from '@sentry/types';
 import { loadModule, logger } from '@sentry/utils';
 import pg from 'pg';
 
-import { Integrations, Span } from '../../../src';
+import { Integrations } from '../../../src';
 import { getTestClient } from '../../testutils';
 
 class PgClient {
@@ -63,7 +64,7 @@ describe('setupOnce', () => {
 
     beforeEach(() => {
       scope = new Scope();
-      parentSpan = new Span();
+      parentSpan = new SpanClass();
       childSpan = parentSpan.startChild();
       jest.spyOn(scope, 'getSpan').mockReturnValueOnce(parentSpan);
       jest.spyOn(parentSpan, 'startChild').mockReturnValueOnce(childSpan);
@@ -134,7 +135,7 @@ describe('setupOnce', () => {
 
   it('does not attempt resolution when module is passed directly', async () => {
     const scope = new Scope();
-    jest.spyOn(scope, 'getSpan').mockReturnValueOnce(new Span());
+    jest.spyOn(scope, 'getSpan').mockReturnValueOnce(new SpanClass());
 
     new Integrations.Postgres({ module: mockModule }).setupOnce(
       () => undefined,
diff --git a/packages/tracing/test/span.test.ts b/packages/tracing/test/span.test.ts
@@ -57,8 +57,8 @@ describe('Span', () => {
       const span2 = transaction.startChild();
       const span3 = span2.startChild();
       span3.end();
-      expect(transaction.spanRecorder).toBe(span2.spanRecorder);
-      expect(transaction.spanRecorder).toBe(span3.spanRecorder);
+      expect(transaction.spanRecorder).toBe((span2 as Span).spanRecorder);
+      expect(transaction.spanRecorder).toBe((span3 as Span).spanRecorder);
     });
   });
 
diff --git a/packages/types/src/span.ts b/packages/types/src/span.ts
@@ -104,43 +104,43 @@ export interface SpanContext {
    *
    * @deprecated Use `name` instead.
    */
-  description?: string;
+  description?: string | undefined;
 
   /**
    * Human-readable identifier for the span. Alias for span.description.
    */
-  name?: string;
+  name?: string | undefined;
 
   /**
    * Operation of the Span.
    */
-  op?: string;
+  op?: string | undefined;
 
   /**
    * Completion status of the Span.
    * See: {@sentry/tracing SpanStatus} for possible values
    */
-  status?: string;
+  status?: string | undefined;
 
   /**
    * Parent Span ID
    */
-  parentSpanId?: string;
+  parentSpanId?: string | undefined;
 
   /**
    * Was this span chosen to be sent as part of the sample?
    */
-  sampled?: boolean;
+  sampled?: boolean | undefined;
 
   /**
    * Span ID
    */
-  spanId?: string;
+  spanId?: string | undefined;
 
   /**
    * Trace ID
    */
-  traceId?: string;
+  traceId?: string | undefined;
 
   /**
    * Tags of the Span.
@@ -162,22 +162,22 @@ export interface SpanContext {
   /**
    * Timestamp in seconds (epoch time) indicating when the span started.
    */
-  startTimestamp?: number;
+  startTimestamp?: number | undefined;
 
   /**
    * Timestamp in seconds (epoch time) indicating when the span ended.
    */
-  endTimestamp?: number;
+  endTimestamp?: number | undefined;
 
   /**
    * The instrumenter that created this span.
    */
-  instrumenter?: Instrumenter;
+  instrumenter?: Instrumenter | undefined;
 
   /**
    * The origin of the span, giving context about what created the span.
    */
-  origin?: SpanOrigin;
+  origin?: SpanOrigin | undefined;
 }
 
 /** Span holding trace_id, span_id */
@@ -194,7 +194,7 @@ export interface Span extends Omit<SpanContext, 'op' | 'status' | 'origin'> {
    * @deprecated Use `startSpan()` functions to set, `span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, 'op')
    * to update and `spanToJSON().op` to read the op instead
    */
-  op?: string;
+  op?: string | undefined;
 
   /**
    * The ID of the span.
@@ -207,7 +207,7 @@ export interface Span extends Omit<SpanContext, 'op' | 'status' | 'origin'> {
    *
    * @deprecated Use `spanToJSON(span).parent_span_id` instead.
    */
-  parentSpanId?: string;
+  parentSpanId?: string | undefined;
 
   /**
    * The ID of the trace.
@@ -219,7 +219,7 @@ export interface Span extends Omit<SpanContext, 'op' | 'status' | 'origin'> {
    * Was this span chosen to be sent as part of the sample?
    * @deprecated Use `isRecording()` instead.
    */
-  sampled?: boolean;
+  sampled?: boolean | undefined;
 
   /**
    * Timestamp in seconds (epoch time) indicating when the span started.
@@ -231,7 +231,7 @@ export interface Span extends Omit<SpanContext, 'op' | 'status' | 'origin'> {
    * Timestamp in seconds (epoch time) indicating when the span ended.
    * @deprecated Use `spanToJSON()` instead.
    */
-  endTimestamp?: number;
+  endTimestamp?: number | undefined;
 
   /**
    * Tags for the span.
@@ -271,14 +271,14 @@ export interface Span extends Omit<SpanContext, 'op' | 'status' | 'origin'> {
    *
    * @deprecated Use `.setStatus` to set or update and `spanToJSON()` to read the status.
    */
-  status?: string;
+  status?: string | undefined;
 
   /**
    * The origin of the span, giving context about what created the span.
    *
    * @deprecated Use `startSpan` function to set and `spanToJSON(span).origin` to read the origin instead.
    */
-  origin?: SpanOrigin;
+  origin?: SpanOrigin | undefined;
 
   /**
    * Get context data for this span.
diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts
@@ -21,12 +21,12 @@ export interface TransactionContext extends SpanContext {
    * 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.
    */
-  trimEnd?: boolean;
+  trimEnd?: boolean | undefined;
 
   /**
    * If this transaction has a parent, the parent's sampling decision
    */
-  parentSampled?: boolean;
+  parentSampled?: boolean | undefined;
 
   /**
    * Metadata associated with the transaction, for internal SDK use.
