From 01f63866a6e583de171cb6638c0c466f4139e0d8 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 15 Feb 2024 11:37:27 +0100 Subject: [PATCH 1/2] ref: Make span types more robust (#10660) 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 https://github.com/getsentry/sentry-javascript/issues/10654 --- packages/core/src/tracing/span.ts | 10 +++--- packages/core/src/tracing/transaction.ts | 2 +- .../test/integrations/apollo-nestjs.test.ts | 7 ++-- .../tracing/test/integrations/apollo.test.ts | 7 ++-- .../tracing/test/integrations/graphql.test.ts | 7 ++-- .../test/integrations/node/mongo.test.ts | 7 ++-- .../test/integrations/node/postgres.test.ts | 9 ++--- packages/tracing/test/span.test.ts | 4 +-- packages/types/src/span.ts | 36 +++++++++---------- packages/types/src/transaction.ts | 4 +-- 10 files changed, 49 insertions(+), 44 deletions(-) diff --git a/packages/core/src/tracing/span.ts b/packages/core/src/tracing/span.ts index 1c178fc05fbe..5ded23cc386d 100644 --- 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>, - ): 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 index 709aa628f42e..a399137d1301 100644 --- 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> | undefined; diff --git a/packages/tracing/test/integrations/apollo-nestjs.test.ts b/packages/tracing/test/integrations/apollo-nestjs.test.ts index 7e9866146385..51f82f210e59 100644 --- 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 index ea861dcdec1f..5de017275080 100644 --- 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 index 06b9495d8061..7189a148130d 100644 --- 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 index 8caa5f35750f..4a42a096de69 100644 --- 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 index c94b9870907b..800657d20fa1 100644 --- 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 index ae13f42ea698..798cc2a8263c 100644 --- 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 index 6aa6ea1113f6..69704d497b8f 100644 --- 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 { * @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 { * * @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 { * 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 { * 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 { * * @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 index b297dd0ea9c2..fbcf8b38f02d 100644 --- 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. From f40cba0e86aefca96d96689514e1b0a450f59d74 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 21 Feb 2024 09:12:59 +0100 Subject: [PATCH 2/2] fix(tracing): Guard against missing `window.location` (#10659) We should also backport this to v7, probably. Closes https://github.com/getsentry/sentry-javascript/issues/10578 --- .../src/browser/browserTracingIntegration.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/tracing-internal/src/browser/browserTracingIntegration.ts b/packages/tracing-internal/src/browser/browserTracingIntegration.ts index b27575e147cf..d08ca952a0e0 100644 --- a/packages/tracing-internal/src/browser/browserTracingIntegration.ts +++ b/packages/tracing-internal/src/browser/browserTracingIntegration.ts @@ -257,7 +257,7 @@ export const browserTracingIntegration = ((_options: Partial { if (['interactive', 'complete'].includes(WINDOW.document.readyState)) { idleTransaction.sendAutoFinishSignal(); @@ -307,7 +307,7 @@ export const browserTracingIntegration = ((_options: Partial { @@ -335,7 +335,7 @@ export const browserTracingIntegration = ((_options: Partial { /** * This early return is there to account for some cases where a navigation transaction starts right after