From d599fd116bba804d616aaff13e79a6f625601b5f Mon Sep 17 00:00:00 2001 From: Krystof Woldrich Date: Fri, 6 Oct 2023 16:58:49 +0200 Subject: [PATCH 1/3] fix(tracing): Ignore defaults when warning about duplicate definition of trace propagation targets --- CHANGELOG.md | 1 + src/js/tracing/reactnativetracing.ts | 13 ++++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cbe9d81770..9c10a3c57c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,7 @@ ### Fixes - Change log output to show what paths are considered when collecting modules ([#3316](https://github.com/getsentry/sentry-react-native/pull/3316)) +- Ignore defaults when warning about duplicate definition of trace propagation targets ([#3327](https://github.com/getsentry/sentry-react-native/pull/3327)) ### Dependencies diff --git a/src/js/tracing/reactnativetracing.ts b/src/js/tracing/reactnativetracing.ts index e05efe5dbe..b69763a7f7 100644 --- a/src/js/tracing/reactnativetracing.ts +++ b/src/js/tracing/reactnativetracing.ts @@ -136,8 +136,19 @@ export class ReactNativeTracing implements Integration { private _awaitingAppStartData?: NativeAppStartResponse; private _appStartFinishTimestamp?: number; private _currentRoute?: string; + private _hasSetTracePropagationTargets: boolean; public constructor(options: Partial = {}) { + this._hasSetTracePropagationTargets = false; + + if (__DEV__) { + this._hasSetTracePropagationTargets = !!( + options && + // eslint-disable-next-line deprecation/deprecation + (options.tracePropagationTargets || options.tracingOrigins) + ); + } + this.options = { ...defaultReactNativeTracingOptions, ...options, @@ -193,7 +204,7 @@ export class ReactNativeTracing implements Integration { // // If both 1 and either one of 2 or 3 are set (from above), we log out a warning. const tracePropagationTargets = clientOptionsTracePropagationTargets || thisOptionsTracePropagationTargets; - if (__DEV__ && (thisOptionsTracePropagationTargets || tracingOrigins) && clientOptionsTracePropagationTargets) { + if (__DEV__ && this._hasSetTracePropagationTargets && clientOptionsTracePropagationTargets) { logger.warn( '[ReactNativeTracing] The `tracePropagationTargets` option was set in the ReactNativeTracing integration and top level `Sentry.init`. The top level `Sentry.init` value is being used.', ); From 0c5faffd9ff207a81544be38124a05a1d51f91e5 Mon Sep 17 00:00:00 2001 From: Krystof Woldrich Date: Mon, 9 Oct 2023 10:46:12 +0200 Subject: [PATCH 2/3] fix(tracing): Use deprecated tracing origins option if set --- CHANGELOG.md | 1 + src/js/tracing/reactnativetracing.ts | 35 +++++---- test/tracing/reactnativetracing.test.ts | 96 ++++++++++++++++++++++++- 3 files changed, 119 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c10a3c57c..092c32c00b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,7 @@ - Change log output to show what paths are considered when collecting modules ([#3316](https://github.com/getsentry/sentry-react-native/pull/3316)) - Ignore defaults when warning about duplicate definition of trace propagation targets ([#3327](https://github.com/getsentry/sentry-react-native/pull/3327)) +- Use deprecated `ReactNativeTracingOptions.tracingOrigins` if set in the options ([#3331](https://github.com/getsentry/sentry-react-native/pull/3331)) ### Dependencies diff --git a/src/js/tracing/reactnativetracing.ts b/src/js/tracing/reactnativetracing.ts index b69763a7f7..a4d22f7604 100644 --- a/src/js/tracing/reactnativetracing.ts +++ b/src/js/tracing/reactnativetracing.ts @@ -95,6 +95,8 @@ export interface ReactNativeTracingOptions extends RequestInstrumentationOptions enableUserInteractionTracing: boolean; } +const DEFAULT_TRACE_PROPAGATION_TARGETS = ['localhost', /^\/(?!\/)/]; + const defaultReactNativeTracingOptions: ReactNativeTracingOptions = { ...defaultRequestInstrumentationOptions, idleTimeout: 1000, @@ -137,17 +139,19 @@ export class ReactNativeTracing implements Integration { private _appStartFinishTimestamp?: number; private _currentRoute?: string; private _hasSetTracePropagationTargets: boolean; + private _hasSetTracingOrigins: boolean; public constructor(options: Partial = {}) { - this._hasSetTracePropagationTargets = false; - - if (__DEV__) { - this._hasSetTracePropagationTargets = !!( - options && - // eslint-disable-next-line deprecation/deprecation - (options.tracePropagationTargets || options.tracingOrigins) - ); - } + this._hasSetTracePropagationTargets = !!( + options && + // eslint-disable-next-line deprecation/deprecation + options.tracePropagationTargets + ); + this._hasSetTracingOrigins = !!( + options && + // eslint-disable-next-line deprecation/deprecation + options.tracingOrigins + ); this.options = { ...defaultReactNativeTracingOptions, @@ -203,8 +207,16 @@ export class ReactNativeTracing implements Integration { // ReactNativeTracing option `tracePropagationTargets` and then `tracingOrigins` (deprecated). // // If both 1 and either one of 2 or 3 are set (from above), we log out a warning. - const tracePropagationTargets = clientOptionsTracePropagationTargets || thisOptionsTracePropagationTargets; - if (__DEV__ && this._hasSetTracePropagationTargets && clientOptionsTracePropagationTargets) { + const tracePropagationTargets = + clientOptionsTracePropagationTargets || + (this._hasSetTracePropagationTargets && thisOptionsTracePropagationTargets) || + (this._hasSetTracingOrigins && tracingOrigins) || + DEFAULT_TRACE_PROPAGATION_TARGETS; + if ( + __DEV__ && + (this._hasSetTracePropagationTargets || this._hasSetTracingOrigins) && + clientOptionsTracePropagationTargets + ) { logger.warn( '[ReactNativeTracing] The `tracePropagationTargets` option was set in the ReactNativeTracing integration and top level `Sentry.init`. The top level `Sentry.init` value is being used.', ); @@ -246,7 +258,6 @@ export class ReactNativeTracing implements Integration { instrumentOutgoingRequests({ traceFetch, traceXHR, - tracingOrigins, shouldCreateSpanForRequest, tracePropagationTargets, }); diff --git a/test/tracing/reactnativetracing.test.ts b/test/tracing/reactnativetracing.test.ts index 47fa0e8e6f..4d0f931ca1 100644 --- a/test/tracing/reactnativetracing.test.ts +++ b/test/tracing/reactnativetracing.test.ts @@ -1,12 +1,14 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ import type { SpanStatusType, User } from '@sentry/browser'; -import { BrowserClient } from '@sentry/browser'; +import * as SentryBrowser from '@sentry/browser'; import type { IdleTransaction } from '@sentry/core'; import { addGlobalEventProcessor, Hub, Transaction } from '@sentry/core'; import type { NativeAppStartResponse } from '../../src/js/NativeRNSentry'; import { RoutingInstrumentation } from '../../src/js/tracing/routingInstrumentation'; +const BrowserClient = SentryBrowser.BrowserClient; + jest.mock('../../src/js/wrapper', () => { return { NATIVE: { @@ -117,6 +119,98 @@ describe('ReactNativeTracing', () => { jest.clearAllMocks(); }); + describe('trace propagation targets', () => { + it('uses tracingOrigins', () => { + const instrumentOutgoingRequests = jest.spyOn(SentryBrowser, 'instrumentOutgoingRequests'); + const mockedHub = { + getClient: () => ({ + getOptions: () => ({}), + }), + }; + + const integration = new ReactNativeTracing({ + tracingOrigins: ['test1', 'test2'], + }); + integration.setupOnce( + () => {}, + () => mockedHub as unknown as Hub, + ); + + expect(instrumentOutgoingRequests).toBeCalledWith( + expect.objectContaining({ + tracePropagationTargets: ['test1', 'test2'], + }), + ); + }); + + it('uses tracePropagationTargets', () => { + const instrumentOutgoingRequests = jest.spyOn(SentryBrowser, 'instrumentOutgoingRequests'); + const mockedHub = { + getClient: () => ({ + getOptions: () => ({}), + }), + }; + + const integration = new ReactNativeTracing({ + tracePropagationTargets: ['test1', 'test2'], + }); + integration.setupOnce( + () => {}, + () => mockedHub as unknown as Hub, + ); + + expect(instrumentOutgoingRequests).toBeCalledWith( + expect.objectContaining({ + tracePropagationTargets: ['test1', 'test2'], + }), + ); + }); + + it('uses tracePropagationTargets from client options', () => { + const instrumentOutgoingRequests = jest.spyOn(SentryBrowser, 'instrumentOutgoingRequests'); + const mockedHub = { + getClient: () => ({ + getOptions: () => ({ + tracePropagationTargets: ['test1', 'test2'], + }), + }), + }; + + const integration = new ReactNativeTracing({}); + integration.setupOnce( + () => {}, + () => mockedHub as unknown as Hub, + ); + + expect(instrumentOutgoingRequests).toBeCalledWith( + expect.objectContaining({ + tracePropagationTargets: ['test1', 'test2'], + }), + ); + }); + + it('uses defaults', () => { + const instrumentOutgoingRequests = jest.spyOn(SentryBrowser, 'instrumentOutgoingRequests'); + const mockedHub = { + getClient: () => ({ + getOptions: () => ({}), + }), + }; + + const integration = new ReactNativeTracing({}); + integration.setupOnce( + () => {}, + () => mockedHub as unknown as Hub, + ); + + expect(instrumentOutgoingRequests).toBeCalledWith( + expect.objectContaining({ + tracePropagationTargets: ['localhost', /^\/(?!\/)/], + }), + ); + }); + }); + describe('App Start', () => { describe('Without routing instrumentation', () => { it('Starts route transaction (cold)', async () => { From f28690cbbdd4678c766117c32c7671f54485d8fd Mon Sep 17 00:00:00 2001 From: Krystof Woldrich Date: Mon, 9 Oct 2023 15:14:55 +0200 Subject: [PATCH 3/3] Add more tests --- test/tracing/reactnativetracing.test.ts | 50 +++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/test/tracing/reactnativetracing.test.ts b/test/tracing/reactnativetracing.test.ts index 4d0f931ca1..545ec0352c 100644 --- a/test/tracing/reactnativetracing.test.ts +++ b/test/tracing/reactnativetracing.test.ts @@ -209,6 +209,56 @@ describe('ReactNativeTracing', () => { }), ); }); + + it('client tracePropagationTargets takes priority over integration options', () => { + const instrumentOutgoingRequests = jest.spyOn(SentryBrowser, 'instrumentOutgoingRequests'); + const mockedHub = { + getClient: () => ({ + getOptions: () => ({ + tracePropagationTargets: ['test1', 'test2'], + }), + }), + }; + + const integration = new ReactNativeTracing({ + tracePropagationTargets: ['test3', 'test4'], + tracingOrigins: ['test5', 'test6'], + }); + integration.setupOnce( + () => {}, + () => mockedHub as unknown as Hub, + ); + + expect(instrumentOutgoingRequests).toBeCalledWith( + expect.objectContaining({ + tracePropagationTargets: ['test1', 'test2'], + }), + ); + }); + + it('integration tracePropagationTargets takes priority over tracingOrigins', () => { + const instrumentOutgoingRequests = jest.spyOn(SentryBrowser, 'instrumentOutgoingRequests'); + const mockedHub = { + getClient: () => ({ + getOptions: () => ({}), + }), + }; + + const integration = new ReactNativeTracing({ + tracePropagationTargets: ['test3', 'test4'], + tracingOrigins: ['test5', 'test6'], + }); + integration.setupOnce( + () => {}, + () => mockedHub as unknown as Hub, + ); + + expect(instrumentOutgoingRequests).toBeCalledWith( + expect.objectContaining({ + tracePropagationTargets: ['test3', 'test4'], + }), + ); + }); }); describe('App Start', () => {