From 9398d3e7566c99be7d740d81a1a658db47cdb8e5 Mon Sep 17 00:00:00 2001 From: k-fish Date: Mon, 17 Jul 2023 13:11:05 -0400 Subject: [PATCH 1/3] feat(tracing): Bring http timings out of experiment This brings HTTP timings out of experiment, with an option to turn them off still. For now we'll be hiding them in the UI until we've figure out which ones we want to display. --- .../tracing-internal/src/browser/browsertracing.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/tracing-internal/src/browser/browsertracing.ts b/packages/tracing-internal/src/browser/browsertracing.ts index 4be633821a1a..3c9de58e1f1f 100644 --- a/packages/tracing-internal/src/browser/browsertracing.ts +++ b/packages/tracing-internal/src/browser/browsertracing.ts @@ -80,6 +80,13 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions { */ enableLongTask: boolean; + /** + * If true, Sentry will capture http timings and add them to the corresponding http spans. + * + * Default: true + */ + enableHTTPTimings: boolean; + /** * _metricOptions allows the user to send options to change how metrics are collected. * @@ -105,7 +112,6 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions { _experiments: Partial<{ enableLongTask: boolean; enableInteractions: boolean; - enableHTTPTimings: boolean; onStartRouteTransaction: (t: Transaction | undefined, ctx: TransactionContext, getCurrentHub: () => Hub) => void; }>; @@ -140,6 +146,7 @@ const DEFAULT_BROWSER_TRACING_OPTIONS: BrowserTracingOptions = { startTransactionOnLocationChange: true, startTransactionOnPageLoad: true, enableLongTask: true, + enableHTTPTimings: true, ...defaultRequestInstrumentationOptions, }; @@ -230,6 +237,7 @@ export class BrowserTracing implements Integration { traceFetch, traceXHR, shouldCreateSpanForRequest, + enableHTTPTimings, _experiments, } = this.options; @@ -278,7 +286,7 @@ export class BrowserTracing implements Integration { tracePropagationTargets, shouldCreateSpanForRequest, _experiments: { - enableHTTPTimings: _experiments.enableHTTPTimings, + enableHTTPTimings, }, }); } From 92249f33542a4aba02ca40702ea22fd922fa538a Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 17 Jul 2023 13:41:46 -0400 Subject: [PATCH 2/3] elevate enableHTTPTimings --- .../src/browser/browsertracing.ts | 13 ++------ .../tracing-internal/src/browser/request.ts | 31 ++++++++++++------- 2 files changed, 21 insertions(+), 23 deletions(-) diff --git a/packages/tracing-internal/src/browser/browsertracing.ts b/packages/tracing-internal/src/browser/browsertracing.ts index 3c9de58e1f1f..13cde107074f 100644 --- a/packages/tracing-internal/src/browser/browsertracing.ts +++ b/packages/tracing-internal/src/browser/browsertracing.ts @@ -80,13 +80,6 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions { */ enableLongTask: boolean; - /** - * If true, Sentry will capture http timings and add them to the corresponding http spans. - * - * Default: true - */ - enableHTTPTimings: boolean; - /** * _metricOptions allows the user to send options to change how metrics are collected. * @@ -146,7 +139,7 @@ const DEFAULT_BROWSER_TRACING_OPTIONS: BrowserTracingOptions = { startTransactionOnLocationChange: true, startTransactionOnPageLoad: true, enableLongTask: true, - enableHTTPTimings: true, + _experiments: {}, ...defaultRequestInstrumentationOptions, }; @@ -285,9 +278,7 @@ export class BrowserTracing implements Integration { traceXHR, tracePropagationTargets, shouldCreateSpanForRequest, - _experiments: { - enableHTTPTimings, - }, + enableHTTPTimings, }); } diff --git a/packages/tracing-internal/src/browser/request.ts b/packages/tracing-internal/src/browser/request.ts index 071b2146bb16..f7332b9d2957 100644 --- a/packages/tracing-internal/src/browser/request.ts +++ b/packages/tracing-internal/src/browser/request.ts @@ -16,13 +16,6 @@ export const DEFAULT_TRACE_PROPAGATION_TARGETS = ['localhost', /^\/(?!\/)/]; /** Options for Request Instrumentation */ export interface RequestInstrumentationOptions { - /** - * Allow experiments for the request instrumentation. - */ - _experiments: Partial<{ - enableHTTPTimings: boolean; - }>; - /** * @deprecated Will be removed in v8. * Use `shouldCreateSpanForRequest` to control span creation and `tracePropagationTargets` to control @@ -52,6 +45,13 @@ export interface RequestInstrumentationOptions { */ traceXHR: boolean; + /** + * If true, Sentry will capture http timings and add them to the corresponding http spans. + * + * Default: true + */ + enableHTTPTimings: boolean; + /** * This function will be called before creating a span for a request with the given url. * Return false if you don't want a span for the given url. @@ -114,16 +114,23 @@ type PolymorphicRequestHeaders = export const defaultRequestInstrumentationOptions: RequestInstrumentationOptions = { traceFetch: true, traceXHR: true, + enableHTTPTimings: true, // TODO (v8): Remove this property tracingOrigins: DEFAULT_TRACE_PROPAGATION_TARGETS, tracePropagationTargets: DEFAULT_TRACE_PROPAGATION_TARGETS, - _experiments: {}, }; /** Registers span creators for xhr and fetch requests */ export function instrumentOutgoingRequests(_options?: Partial): void { - // eslint-disable-next-line deprecation/deprecation - const { traceFetch, traceXHR, tracePropagationTargets, tracingOrigins, shouldCreateSpanForRequest, _experiments } = { + const { + traceFetch, + traceXHR, + tracePropagationTargets, + // eslint-disable-next-line deprecation/deprecation + tracingOrigins, + shouldCreateSpanForRequest, + enableHTTPTimings, + } = { traceFetch: defaultRequestInstrumentationOptions.traceFetch, traceXHR: defaultRequestInstrumentationOptions.traceXHR, ..._options, @@ -143,7 +150,7 @@ export function instrumentOutgoingRequests(_options?: Partial { const createdSpan = fetchCallback(handlerData, shouldCreateSpan, shouldAttachHeadersWithTargets, spans); - if (_experiments?.enableHTTPTimings && createdSpan) { + if (enableHTTPTimings && createdSpan) { addHTTPTimings(createdSpan); } }); @@ -152,7 +159,7 @@ export function instrumentOutgoingRequests(_options?: Partial { const createdSpan = xhrCallback(handlerData, shouldCreateSpan, shouldAttachHeadersWithTargets, spans); - if (_experiments?.enableHTTPTimings && createdSpan) { + if (enableHTTPTimings && createdSpan) { addHTTPTimings(createdSpan); } }); From 1c617d22840a7dbed4bef04ac83883b002ffb2c5 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 17 Jul 2023 13:57:02 -0400 Subject: [PATCH 3/3] fix tests --- .../tracing-internal/test/browser/browsertracing.test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/tracing-internal/test/browser/browsertracing.test.ts b/packages/tracing-internal/test/browser/browsertracing.test.ts index 0754afd65fc8..e6a9eff3fb82 100644 --- a/packages/tracing-internal/test/browser/browsertracing.test.ts +++ b/packages/tracing-internal/test/browser/browsertracing.test.ts @@ -95,6 +95,7 @@ conditionalTest({ min: 10 })('BrowserTracing', () => { expect(browserTracing.options).toEqual({ enableLongTask: true, + _experiments: {}, ...TRACING_DEFAULTS, markBackgroundTransactions: true, routingInstrumentation: instrumentRoutingWithDefaults, @@ -132,6 +133,7 @@ conditionalTest({ min: 10 })('BrowserTracing', () => { expect(browserTracing.options).toEqual({ enableLongTask: false, + _experiments: {}, ...TRACING_DEFAULTS, markBackgroundTransactions: true, routingInstrumentation: instrumentRoutingWithDefaults, @@ -246,7 +248,7 @@ conditionalTest({ min: 10 })('BrowserTracing', () => { traceFetch: true, traceXHR: true, tracePropagationTargets: ['something'], - _experiments: {}, + enableHTTPTimings: true, }); }); @@ -260,7 +262,7 @@ conditionalTest({ min: 10 })('BrowserTracing', () => { }); expect(instrumentOutgoingRequestsMock).toHaveBeenCalledWith({ - _experiments: {}, + enableHTTPTimings: true, traceFetch: true, traceXHR: true, tracePropagationTargets: ['something-else'],