From dcfc7c76740f5575e624e4a5a0187f08d0bd5605 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 23 Jan 2024 14:32:55 -0500 Subject: [PATCH] feat(tracing): Deprecate transaction-related options for `BrowserTracing` --- MIGRATION.md | 10 + packages/core/src/tracing/trace.ts | 113 +----------- .../src/browser/browsertracing.ts | 120 +++++++++--- .../test/browser/browsertracing.test.ts | 174 ++++++++++++------ packages/types/src/index.ts | 1 + packages/types/src/startSpanOptions.ts | 104 +++++++++++ 6 files changed, 326 insertions(+), 196 deletions(-) create mode 100644 packages/types/src/startSpanOptions.ts diff --git a/MIGRATION.md b/MIGRATION.md index 0253907e3278..f605a3b03075 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -10,6 +10,16 @@ npx @sentry/migr8@latest This will let you select which updates to run, and automatically update your code. Make sure to still review all code changes! +## Deprecate transaction-related options to `BrowserTracing` + +When configuring the `BrowserTracing` integration, some options have been renamed: + +- `startTransactionOnPageLoad` --> `spanOnPageLoad` +- `startTransactionOnLocationChange` --> `spanOnLocationChange` +- `markBackgroundTransactions` --> `markBackgroundSpan` + +Also, `beforeNavigate` is replaced with a `beforeStartSpan` callback, which receives `StartSpanOptions`. + ## Deprecate using `getClient()` to check if the SDK was initialized In v8, `getClient()` will stop returning `undefined` if `Sentry.init()` was not called. For cases where this may be used diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index eb070510a3b7..885cbd7c9d08 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -1,15 +1,5 @@ -import type { - Instrumenter, - Primitive, - Scope, - Span, - SpanTimeInput, - TransactionContext, - TransactionMetadata, -} from '@sentry/types'; -import type { SpanAttributes } from '@sentry/types'; -import type { SpanOrigin } from '@sentry/types'; -import type { TransactionSource } from '@sentry/types'; +import type { Span, SpanTimeInput, StartSpanOptions, TransactionContext } from '@sentry/types'; + import { dropUndefinedKeys, logger, tracingContextFromHeaders } from '@sentry/utils'; import { DEBUG_BUILD } from '../debug-build'; @@ -20,105 +10,6 @@ import { handleCallbackErrors } from '../utils/handleCallbackErrors'; import { hasTracingEnabled } from '../utils/hasTracingEnabled'; import { spanTimeInputToSeconds, spanToJSON } from '../utils/spanUtils'; -interface StartSpanOptions extends TransactionContext { - /** A manually specified start time for the created `Span` object. */ - startTime?: SpanTimeInput; - - /** If defined, start this span off this scope instead off the current scope. */ - scope?: Scope; - - /** The name of the span. */ - name: string; - - /** An op for the span. This is a categorization for spans. */ - op?: string; - - /** The origin of the span - if it comes from auto instrumenation or manual instrumentation. */ - origin?: SpanOrigin; - - /** Attributes for the span. */ - attributes?: SpanAttributes; - - // All remaining fields are deprecated - - /** - * @deprecated Manually set the end timestamp instead. - */ - trimEnd?: boolean; - - /** - * @deprecated This cannot be set manually anymore. - */ - parentSampled?: boolean; - - /** - * @deprecated Use attributes or set data on scopes instead. - */ - metadata?: Partial; - - /** - * The name thingy. - * @deprecated Use `name` instead. - */ - description?: string; - - /** - * @deprecated Use `span.setStatus()` instead. - */ - status?: string; - - /** - * @deprecated Use `scope` instead. - */ - parentSpanId?: string; - - /** - * @deprecated You cannot manually set the span to sampled anymore. - */ - sampled?: boolean; - - /** - * @deprecated You cannot manually set the spanId anymore. - */ - spanId?: string; - - /** - * @deprecated You cannot manually set the traceId anymore. - */ - traceId?: string; - - /** - * @deprecated Use an attribute instead. - */ - source?: TransactionSource; - - /** - * @deprecated Use attributes or set tags on the scope instead. - */ - tags?: { [key: string]: Primitive }; - - /** - * @deprecated Use attributes instead. - */ - // eslint-disable-next-line @typescript-eslint/no-explicit-any - data?: { [key: string]: any }; - - /** - * @deprecated Use `startTime` instead. - */ - startTimestamp?: number; - - /** - * @deprecated Use `span.end()` instead. - */ - endTimestamp?: number; - - /** - * @deprecated You cannot set the instrumenter manually anymore. - */ - instrumenter?: Instrumenter; -} - /** * Wraps a function with a transaction/span and finishes the span after the function is done. * diff --git a/packages/tracing-internal/src/browser/browsertracing.ts b/packages/tracing-internal/src/browser/browsertracing.ts index e9f61c73c0f3..a2d89b5c7c28 100644 --- a/packages/tracing-internal/src/browser/browsertracing.ts +++ b/packages/tracing-internal/src/browser/browsertracing.ts @@ -1,4 +1,4 @@ -/* eslint-disable max-lines */ +/* eslint-disable max-lines, complexity */ import type { Hub, IdleTransaction } from '@sentry/core'; import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, @@ -9,7 +9,14 @@ import { spanToJSON, startIdleTransaction, } from '@sentry/core'; -import type { EventProcessor, Integration, Transaction, TransactionContext, TransactionSource } from '@sentry/types'; +import type { + EventProcessor, + Integration, + StartSpanOptions, + Transaction, + TransactionContext, + TransactionSource, +} from '@sentry/types'; import { getDomElement, logger, tracingContextFromHeaders } from '@sentry/utils'; import { DEBUG_BUILD } from '../common/debug-build'; @@ -60,27 +67,48 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions { heartbeatInterval: number; /** - * Flag to enable/disable creation of `navigation` transaction on history changes. + * If a span should be created on location (history) changes. + * Default: true + */ + spanOnLocationChange: boolean; + + /** + * If a span should be created on pageload. + * Default: true + */ + spanOnPageLoad: boolean; + + /** + * Flag spans where tabs moved to background with "cancelled". Browser background tab timing is + * not suited towards doing precise measurements of operations. By default, we recommend that this option + * be enabled as background transactions can mess up your statistics in nondeterministic ways. * * Default: true */ - startTransactionOnLocationChange: boolean; + markBackgroundSpan: boolean; + + /** + * Flag to enable/disable creation of `navigation` transaction on history changes. + * Default: true + * @deprecated Configure `spanOnLocationChange` instead. + */ + startTransactionOnLocationChange?: boolean; /** * Flag to enable/disable creation of `pageload` transaction on first pageload. - * * Default: true + * @deprecated Configure `spanOnPageLoad` instead. */ - startTransactionOnPageLoad: boolean; + startTransactionOnPageLoad?: boolean; /** * Flag Transactions where tabs moved to background with "cancelled". Browser background tab timing is * not suited towards doing precise measurements of operations. By default, we recommend that this option * be enabled as background transactions can mess up your statistics in nondeterministic ways. - * * Default: true + * @deprecated Configure `markBackgroundSpan` instead. */ - markBackgroundTransactions: boolean; + markBackgroundTransactions?: boolean; /** * If true, Sentry will capture long tasks and add them to the corresponding transaction. @@ -117,6 +145,12 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions { onStartRouteTransaction: (t: Transaction | undefined, ctx: TransactionContext, getCurrentHub: () => Hub) => void; }>; + /** + * A callback which is called before a span for a pageload or navigation is started. + * It receives the options passed to `startSpan`, and expects to return an updated options object. + */ + beforeStartSpan?: (options: StartSpanOptions) => StartSpanOptions; + /** * beforeNavigate is called before a pageload/navigation transaction is created and allows users to modify transaction * context data, or drop the transaction entirely (by setting `sampled = false` in the context). @@ -126,6 +160,8 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions { * @param context: The context data which will be passed to `startTransaction` by default * * @returns A (potentially) modified context object, with `sampled = false` if the transaction should be dropped. + * + * @deprecated Use `beforeStartSpan` instead. */ beforeNavigate?(this: void, context: TransactionContext): TransactionContext | undefined; @@ -143,10 +179,10 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions { const DEFAULT_BROWSER_TRACING_OPTIONS: BrowserTracingOptions = { ...TRACING_DEFAULTS, - markBackgroundTransactions: true, routingInstrumentation: instrumentRoutingWithDefaults, - startTransactionOnLocationChange: true, - startTransactionOnPageLoad: true, + spanOnLocationChange: true, + spanOnPageLoad: true, + markBackgroundSpan: true, enableLongTask: true, _experiments: {}, ...defaultRequestInstrumentationOptions, @@ -182,7 +218,7 @@ export class BrowserTracing implements Integration { private _hasSetTracePropagationTargets: boolean; - public constructor(_options?: Partial) { + public constructor(_options: Partial = {}) { this.name = BROWSER_TRACING_INTEGRATION_ID; this._hasSetTracePropagationTargets = false; @@ -190,12 +226,34 @@ export class BrowserTracing implements Integration { if (DEBUG_BUILD) { this._hasSetTracePropagationTargets = !!( - _options && // eslint-disable-next-line deprecation/deprecation (_options.tracePropagationTargets || _options.tracingOrigins) ); } + // Migrate legacy options + // TODO v8: Remove this + /* eslint-disable deprecation/deprecation */ + if (typeof _options.startTransactionOnPageLoad === 'boolean') { + _options.spanOnPageLoad = _options.startTransactionOnPageLoad; + } + if (typeof _options.startTransactionOnLocationChange === 'boolean') { + _options.spanOnLocationChange = _options.startTransactionOnLocationChange; + } + if (typeof _options.markBackgroundTransactions === 'boolean') { + _options.markBackgroundSpan = _options.markBackgroundTransactions; + } + /* eslint-enable deprecation/deprecation */ + + // TODO (v8): remove this block after tracingOrigins is removed + // Set tracePropagationTargets to tracingOrigins if specified by the user + // In case both are specified, tracePropagationTargets takes precedence + // eslint-disable-next-line deprecation/deprecation + if (!_options.tracePropagationTargets && _options.tracingOrigins) { + // eslint-disable-next-line deprecation/deprecation + _options.tracePropagationTargets = _options.tracingOrigins; + } + this.options = { ...DEFAULT_BROWSER_TRACING_OPTIONS, ..._options, @@ -207,15 +265,6 @@ export class BrowserTracing implements Integration { this.options.enableLongTask = this.options._experiments.enableLongTask; } - // TODO (v8): remove this block after tracingOrigins is removed - // Set tracePropagationTargets to tracingOrigins if specified by the user - // In case both are specified, tracePropagationTargets takes precedence - // eslint-disable-next-line deprecation/deprecation - if (_options && !_options.tracePropagationTargets && _options.tracingOrigins) { - // eslint-disable-next-line deprecation/deprecation - this.options.tracePropagationTargets = _options.tracingOrigins; - } - this._collectWebVitals = startTrackingWebVitals(); if (this.options.enableLongTask) { startTrackingLongTasks(); @@ -237,9 +286,9 @@ export class BrowserTracing implements Integration { const { routingInstrumentation: instrumentRouting, - startTransactionOnLocationChange, - startTransactionOnPageLoad, - markBackgroundTransactions, + spanOnPageLoad, + spanOnLocationChange, + markBackgroundSpan, traceFetch, traceXHR, shouldCreateSpanForRequest, @@ -275,11 +324,11 @@ export class BrowserTracing implements Integration { return transaction; }, - startTransactionOnPageLoad, - startTransactionOnLocationChange, + spanOnPageLoad, + spanOnLocationChange, ); - if (markBackgroundTransactions) { + if (markBackgroundSpan) { registerBackgroundTabDetection(); } @@ -306,7 +355,14 @@ export class BrowserTracing implements Integration { const hub = this._getCurrentHub(); - const { beforeNavigate, idleTimeout, finalTimeout, heartbeatInterval } = this.options; + const { + // eslint-disable-next-line deprecation/deprecation + beforeNavigate, + beforeStartSpan, + idleTimeout, + finalTimeout, + heartbeatInterval, + } = this.options; const isPageloadTransaction = context.op === 'pageload'; @@ -328,7 +384,11 @@ export class BrowserTracing implements Integration { trimEnd: true, }; - const modifiedContext = typeof beforeNavigate === 'function' ? beforeNavigate(expandedContext) : expandedContext; + const contextAfterProcessing = beforeStartSpan ? beforeStartSpan(expandedContext) : expandedContext; + + const modifiedContext = + // eslint-disable-next-line deprecation/deprecation + typeof beforeNavigate === 'function' ? beforeNavigate(contextAfterProcessing) : contextAfterProcessing; // For backwards compatibility reasons, beforeNavigate can return undefined to "drop" the transaction (prevent it // from being sent to Sentry). diff --git a/packages/tracing-internal/test/browser/browsertracing.test.ts b/packages/tracing-internal/test/browser/browsertracing.test.ts index b9830b8d754c..0892169c28a2 100644 --- a/packages/tracing-internal/test/browser/browsertracing.test.ts +++ b/packages/tracing-internal/test/browser/browsertracing.test.ts @@ -85,61 +85,6 @@ conditionalTest({ min: 10 })('BrowserTracing', () => { return instance; } - // These are important enough to check with a test as incorrect defaults could - // break a lot of users' configurations. - it('is created with default settings', () => { - const browserTracing = createBrowserTracing(); - - expect(browserTracing.options).toEqual({ - enableLongTask: true, - _experiments: {}, - ...TRACING_DEFAULTS, - markBackgroundTransactions: true, - routingInstrumentation: instrumentRoutingWithDefaults, - startTransactionOnLocationChange: true, - startTransactionOnPageLoad: true, - ...defaultRequestInstrumentationOptions, - }); - }); - - it('is allows to disable enableLongTask via _experiments', () => { - const browserTracing = createBrowserTracing(false, { - _experiments: { - enableLongTask: false, - }, - }); - - expect(browserTracing.options).toEqual({ - enableLongTask: false, - ...TRACING_DEFAULTS, - markBackgroundTransactions: true, - routingInstrumentation: instrumentRoutingWithDefaults, - startTransactionOnLocationChange: true, - startTransactionOnPageLoad: true, - ...defaultRequestInstrumentationOptions, - _experiments: { - enableLongTask: false, - }, - }); - }); - - it('is allows to disable enableLongTask', () => { - const browserTracing = createBrowserTracing(false, { - enableLongTask: false, - }); - - expect(browserTracing.options).toEqual({ - enableLongTask: false, - _experiments: {}, - ...TRACING_DEFAULTS, - markBackgroundTransactions: true, - routingInstrumentation: instrumentRoutingWithDefaults, - startTransactionOnLocationChange: true, - startTransactionOnPageLoad: true, - ...defaultRequestInstrumentationOptions, - }); - }); - /** * All of these tests under `describe('route transaction')` are tested with * `browserTracing.options = { routingInstrumentation: customInstrumentRouting }`, @@ -387,6 +332,71 @@ conditionalTest({ min: 10 })('BrowserTracing', () => { }); }); + describe('beforeStartSpan', () => { + it('is called on transaction creation', () => { + const beforeStartSpan = jest.fn().mockReturnValue({ name: 'here/is/my/path' }); + createBrowserTracing(true, { + beforeStartSpan, + routingInstrumentation: customInstrumentRouting, + }); + const transaction = getActiveTransaction(hub) as IdleTransaction; + expect(transaction).toBeDefined(); + + expect(beforeStartSpan).toHaveBeenCalledTimes(1); + }); + + it('can override default context values', () => { + const beforeStartSpan = jest.fn(ctx => ({ + ...ctx, + op: 'not-pageload', + })); + createBrowserTracing(true, { + beforeStartSpan, + routingInstrumentation: customInstrumentRouting, + }); + const transaction = getActiveTransaction(hub) as IdleTransaction; + expect(transaction).toBeDefined(); + expect(transaction.op).toBe('not-pageload'); + + expect(beforeStartSpan).toHaveBeenCalledTimes(1); + }); + + it("sets transaction name source to `'custom'` if name is changed", () => { + const beforeStartSpan = jest.fn(ctx => ({ + ...ctx, + name: 'newName', + })); + createBrowserTracing(true, { + beforeStartSpan, + routingInstrumentation: customInstrumentRouting, + }); + const transaction = getActiveTransaction(hub) as IdleTransaction; + expect(transaction).toBeDefined(); + expect(transaction.name).toBe('newName'); + expect(transaction.metadata.source).toBe('custom'); + + expect(beforeStartSpan).toHaveBeenCalledTimes(1); + }); + + it('sets transaction name source to default `url` if name is not changed', () => { + const beforeStartSpan = jest.fn(ctx => ({ + ...ctx, + })); + createBrowserTracing(true, { + beforeStartSpan, + routingInstrumentation: (customStartTransaction: (obj: any) => void) => { + customStartTransaction({ name: 'a/path', op: 'pageload', metadata: { source: 'url' } }); + }, + }); + const transaction = getActiveTransaction(hub) as IdleTransaction; + expect(transaction).toBeDefined(); + expect(transaction.name).toBe('a/path'); + expect(transaction.metadata.source).toBe('url'); + + expect(beforeStartSpan).toHaveBeenCalledTimes(1); + }); + }); + it('sets transaction context from sentry-trace header', () => { const name = 'sentry-trace'; const content = '126de09502ae4e0fb26c6967190756a4-b6e54397b12a2a0f-1'; @@ -699,4 +709,58 @@ conditionalTest({ min: 10 })('BrowserTracing', () => { ); }); }); + + describe('options', () => { + // These are important enough to check with a test as incorrect defaults could + // break a lot of users' configurations. + it('is created with default settings', () => { + const browserTracing = createBrowserTracing(); + + expect(browserTracing.options).toEqual({ + enableLongTask: true, + _experiments: {}, + ...TRACING_DEFAULTS, + routingInstrumentation: instrumentRoutingWithDefaults, + spanOnLocationChange: true, + spanOnPageLoad: true, + markBackgroundSpan: true, + ...defaultRequestInstrumentationOptions, + }); + }); + + it('handles legacy `startTransactionOnLocationChange` option', () => { + const integration = new BrowserTracing({ startTransactionOnLocationChange: false }); + expect(integration.options.spanOnLocationChange).toBe(false); + }); + + it('handles legacy `startTransactionOnPageLoad` option', () => { + const integration = new BrowserTracing({ startTransactionOnPageLoad: false }); + expect(integration.options.spanOnPageLoad).toBe(false); + }); + + it('handles legacy `markBackgroundTransactions` option', () => { + const integration = new BrowserTracing({ markBackgroundTransactions: false }); + expect(integration.options.markBackgroundSpan).toBe(false); + }); + + it('allows to disable enableLongTask via _experiments', () => { + const browserTracing = createBrowserTracing(false, { + _experiments: { + enableLongTask: false, + }, + }); + + expect(browserTracing.options.enableLongTask).toBe(false); + expect(browserTracing.options._experiments.enableLongTask).toBe(false); + }); + + it('allows to disable enableLongTask', () => { + const browserTracing = createBrowserTracing(false, { + enableLongTask: false, + }); + + expect(browserTracing.options.enableLongTask).toBe(false); + expect(browserTracing.options._experiments.enableLongTask).toBe(undefined); + }); + }); }); diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 7f9d66c904fa..ca4c976db1c8 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -100,6 +100,7 @@ export type { SpanContextData, TraceFlag, } from './span'; +export type { StartSpanOptions } from './startSpanOptions'; export type { StackFrame } from './stackframe'; export type { Stacktrace, StackParser, StackLineParser, StackLineParserFn } from './stacktrace'; export type { TextEncoderInternal } from './textencoder'; diff --git a/packages/types/src/startSpanOptions.ts b/packages/types/src/startSpanOptions.ts new file mode 100644 index 000000000000..49f81250008c --- /dev/null +++ b/packages/types/src/startSpanOptions.ts @@ -0,0 +1,104 @@ +import type { Instrumenter } from './instrumenter'; +import type { Primitive } from './misc'; +import type { Scope } from './scope'; +import type { SpanAttributes, SpanOrigin, SpanTimeInput } from './span'; +import type { TransactionContext, TransactionMetadata, TransactionSource } from './transaction'; + +export interface StartSpanOptions extends TransactionContext { + /** A manually specified start time for the created `Span` object. */ + startTime?: SpanTimeInput; + + /** If defined, start this span off this scope instead off the current scope. */ + scope?: Scope; + + /** The name of the span. */ + name: string; + + /** An op for the span. This is a categorization for spans. */ + op?: string; + + /** The origin of the span - if it comes from auto instrumenation or manual instrumentation. */ + origin?: SpanOrigin; + + /** Attributes for the span. */ + attributes?: SpanAttributes; + + // All remaining fields are deprecated + + /** + * @deprecated Manually set the end timestamp instead. + */ + trimEnd?: boolean; + + /** + * @deprecated This cannot be set manually anymore. + */ + parentSampled?: boolean; + + /** + * @deprecated Use attributes or set data on scopes instead. + */ + metadata?: Partial; + + /** + * The name thingy. + * @deprecated Use `name` instead. + */ + description?: string; + + /** + * @deprecated Use `span.setStatus()` instead. + */ + status?: string; + + /** + * @deprecated Use `scope` instead. + */ + parentSpanId?: string; + + /** + * @deprecated You cannot manually set the span to sampled anymore. + */ + sampled?: boolean; + + /** + * @deprecated You cannot manually set the spanId anymore. + */ + spanId?: string; + + /** + * @deprecated You cannot manually set the traceId anymore. + */ + traceId?: string; + + /** + * @deprecated Use an attribute instead. + */ + source?: TransactionSource; + + /** + * @deprecated Use attributes or set tags on the scope instead. + */ + tags?: { [key: string]: Primitive }; + + /** + * @deprecated Use attributes instead. + */ + // eslint-disable-next-line @typescript-eslint/no-explicit-any + data?: { [key: string]: any }; + + /** + * @deprecated Use `startTime` instead. + */ + startTimestamp?: number; + + /** + * @deprecated Use `span.end()` instead. + */ + endTimestamp?: number; + + /** + * @deprecated You cannot set the instrumenter manually anymore. + */ + instrumenter?: Instrumenter; +}