From 53f6986bb42f41e1bd17e3ebcf5eb55f479d56b4 Mon Sep 17 00:00:00 2001 From: Antonis Lilis Date: Wed, 13 Nov 2024 15:49:07 +0200 Subject: [PATCH 1/8] Drop dev server spans --- packages/core/src/js/tracing/reactnativetracing.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/packages/core/src/js/tracing/reactnativetracing.ts b/packages/core/src/js/tracing/reactnativetracing.ts index b6ee5e1787..5c3eedfa76 100644 --- a/packages/core/src/js/tracing/reactnativetracing.ts +++ b/packages/core/src/js/tracing/reactnativetracing.ts @@ -4,6 +4,7 @@ import { getClient } from '@sentry/core'; import type { Client, Event, Integration, StartSpanOptions } from '@sentry/types'; import { isWeb } from '../utils/environment'; +import { getDevServer } from './../integrations/debugsymbolicatorutils'; import { addDefaultOpForSpanFrom, defaultIdleOptions } from './span'; export const INTEGRATION_NAME = 'ReactNativeTracing'; @@ -97,13 +98,22 @@ export const reactNativeTracingIntegration = ( idleTimeoutMs: options.idleTimeoutMs ?? defaultIdleOptions.idleTimeout, }; + // Drop Dev Server Spans + const devServerUrl = getDevServer()?.url || ''; + const finalShouldCreateSpanForRequest = (url: string): boolean => { + if (url.includes(devServerUrl)) { + return false; + } + return finalOptions.shouldCreateSpanForRequest(url); + }; + const setup = (client: Client): void => { addDefaultOpForSpanFrom(client); instrumentOutgoingRequests(client, { traceFetch: finalOptions.traceFetch, traceXHR: finalOptions.traceXHR, - shouldCreateSpanForRequest: finalOptions.shouldCreateSpanForRequest, + shouldCreateSpanForRequest: finalShouldCreateSpanForRequest, tracePropagationTargets: client.getOptions().tracePropagationTargets || getDefaultTracePropagationTargets(), }); }; From 36b6282f2f1d66030f4e6462d8ea958102370275 Mon Sep 17 00:00:00 2001 From: Antonis Lilis Date: Wed, 13 Nov 2024 15:52:29 +0200 Subject: [PATCH 2/8] Adds changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4439c0806a..931702a011 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,7 @@ ### Fixes +- Skips development server spans ([#4271](https://github.com/getsentry/sentry-react-native/pull/4271)) - Ignore JavascriptException to filter out obfuscated duplicate JS Errors on Android ([#4232](https://github.com/getsentry/sentry-react-native/pull/4232)) - Skips ignoring require cycle logs for RN 0.70 or newer ([#4214](https://github.com/getsentry/sentry-react-native/pull/4214)) - Enhanced accuracy of time-to-display spans. ([#4189](https://github.com/getsentry/sentry-react-native/pull/4189)) From db32d09a3901d1367f2b2d31cfd3111694c48756 Mon Sep 17 00:00:00 2001 From: Antonis Lilis Date: Wed, 13 Nov 2024 16:35:08 +0200 Subject: [PATCH 3/8] Adds tests --- .../core/src/js/tracing/reactnativetracing.ts | 9 +++- .../core/test/tracing/reactnavigation.test.ts | 43 +++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/packages/core/src/js/tracing/reactnativetracing.ts b/packages/core/src/js/tracing/reactnativetracing.ts index 5c3eedfa76..e4571731da 100644 --- a/packages/core/src/js/tracing/reactnativetracing.ts +++ b/packages/core/src/js/tracing/reactnativetracing.ts @@ -98,15 +98,22 @@ export const reactNativeTracingIntegration = ( idleTimeoutMs: options.idleTimeoutMs ?? defaultIdleOptions.idleTimeout, }; + const userShouldCreateSpanForRequest = finalOptions.shouldCreateSpanForRequest; + // Drop Dev Server Spans const devServerUrl = getDevServer()?.url || ''; const finalShouldCreateSpanForRequest = (url: string): boolean => { if (url.includes(devServerUrl)) { return false; } - return finalOptions.shouldCreateSpanForRequest(url); + if (userShouldCreateSpanForRequest) { + return userShouldCreateSpanForRequest(url); + } + return true; }; + finalOptions.shouldCreateSpanForRequest = finalShouldCreateSpanForRequest; + const setup = (client: Client): void => { addDefaultOpForSpanFrom(client); diff --git a/packages/core/test/tracing/reactnavigation.test.ts b/packages/core/test/tracing/reactnavigation.test.ts index 6d8a234d63..72343b3946 100644 --- a/packages/core/test/tracing/reactnavigation.test.ts +++ b/packages/core/test/tracing/reactnavigation.test.ts @@ -24,6 +24,7 @@ import { DEFAULT_NAVIGATION_SPAN_NAME } from '../../src/js/tracing/span'; import { RN_GLOBAL_OBJ } from '../../src/js/utils/worldwide'; import { getDefaultTestClientOptions, TestClient } from '../mocks/client'; import { NATIVE } from '../mockWrapper'; +import { getDevServer } from './../../src/js/integrations/debugsymbolicatorutils'; import { createMockNavigationAndAttachTo } from './reactnavigationutils'; const dummyRoute = { @@ -32,6 +33,9 @@ const dummyRoute = { }; jest.mock('../../src/js/wrapper.ts', () => jest.requireActual('../mockWrapper.ts')); +jest.mock('./../../src/js/integrations/debugsymbolicatorutils', () => ({ + getDevServer: jest.fn(), +})); jest.useFakeTimers({ advanceTimers: true }); class MockNavigationContainer { @@ -392,6 +396,45 @@ describe('ReactNavigationInstrumentation', () => { }); }); + describe('shouldCreateSpanForRequest', () => { + it('should return false for Dev Server URLs', () => { + const devServerUrl = 'http://localhost:8081'; + (getDevServer as jest.Mock).mockReturnValue({ url: devServerUrl }); + + const rnTracing = reactNativeTracingIntegration(); + + const result = rnTracing.options.shouldCreateSpanForRequest(devServerUrl); + + expect(result).toBe(false); + }); + + it('should return true for non Dev Server URLs', () => { + const devServerUrl = 'http://localhost:8081'; + (getDevServer as jest.Mock).mockReturnValue({ url: devServerUrl }); + + const rnTracing = reactNativeTracingIntegration(); + + const result = rnTracing.options.shouldCreateSpanForRequest('http://some-other-url.com'); + + expect(result).toBe(true); + }); + + it('should chain the user defined shouldCreateSpanForRequest if defined', () => { + const devServerUrl = 'http://localhost:8081'; + (getDevServer as jest.Mock).mockReturnValue({ url: devServerUrl }); + + const userShouldCreateSpanForRequest = (_url: string): boolean => { + return false; + }; + + const rnTracing = reactNativeTracingIntegration({ shouldCreateSpanForRequest: userShouldCreateSpanForRequest }); + + const result = rnTracing.options.shouldCreateSpanForRequest('http://some-other-url.com'); + + expect(result).toBe(false); + }); + }); + function setupTestClient( setupOptions: { beforeSpanStart?: (options: StartSpanOptions) => StartSpanOptions; From ec23f7e4f0a4da32b9bfa8ec461e94e01b3231de Mon Sep 17 00:00:00 2001 From: Antonis Lilis Date: Wed, 13 Nov 2024 17:11:41 +0200 Subject: [PATCH 4/8] Revert unneeded change --- packages/core/src/js/tracing/reactnativetracing.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/js/tracing/reactnativetracing.ts b/packages/core/src/js/tracing/reactnativetracing.ts index e4571731da..f8f9c73caf 100644 --- a/packages/core/src/js/tracing/reactnativetracing.ts +++ b/packages/core/src/js/tracing/reactnativetracing.ts @@ -120,7 +120,7 @@ export const reactNativeTracingIntegration = ( instrumentOutgoingRequests(client, { traceFetch: finalOptions.traceFetch, traceXHR: finalOptions.traceXHR, - shouldCreateSpanForRequest: finalShouldCreateSpanForRequest, + shouldCreateSpanForRequest: finalOptions.shouldCreateSpanForRequest, tracePropagationTargets: client.getOptions().tracePropagationTargets || getDefaultTracePropagationTargets(), }); }; From ffcf870a457301e47271bc13154604bd56f6154b Mon Sep 17 00:00:00 2001 From: Antonis Lilis Date: Thu, 14 Nov 2024 10:21:05 +0200 Subject: [PATCH 5/8] Handle undefined dev server urls Co-authored-by: LucasZF --- .../core/src/js/tracing/reactnativetracing.ts | 23 +++++++++++-------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/packages/core/src/js/tracing/reactnativetracing.ts b/packages/core/src/js/tracing/reactnativetracing.ts index f8f9c73caf..5544731619 100644 --- a/packages/core/src/js/tracing/reactnativetracing.ts +++ b/packages/core/src/js/tracing/reactnativetracing.ts @@ -101,16 +101,19 @@ export const reactNativeTracingIntegration = ( const userShouldCreateSpanForRequest = finalOptions.shouldCreateSpanForRequest; // Drop Dev Server Spans - const devServerUrl = getDevServer()?.url || ''; - const finalShouldCreateSpanForRequest = (url: string): boolean => { - if (url.includes(devServerUrl)) { - return false; - } - if (userShouldCreateSpanForRequest) { - return userShouldCreateSpanForRequest(url); - } - return true; - }; + const devServerUrl = getDevServer()?.url; + const finalShouldCreateSpanForRequest = + devServerUrl === undefined + ? userShouldCreateSpanForRequest + : (url: string): boolean => { + if (url.includes(devServerUrl)) { + return false; + } + if (userShouldCreateSpanForRequest) { + return userShouldCreateSpanForRequest(url); + } + return true; + }; finalOptions.shouldCreateSpanForRequest = finalShouldCreateSpanForRequest; From 5291fb1d01fc801bd09d4b2061fda65a5c5121df Mon Sep 17 00:00:00 2001 From: Antonis Lilis Date: Thu, 14 Nov 2024 10:35:51 +0200 Subject: [PATCH 6/8] Adds tests for the case when the dev server url is undefined --- .../core/test/tracing/reactnavigation.test.ts | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/packages/core/test/tracing/reactnavigation.test.ts b/packages/core/test/tracing/reactnavigation.test.ts index 72343b3946..1e702c1e7c 100644 --- a/packages/core/test/tracing/reactnavigation.test.ts +++ b/packages/core/test/tracing/reactnavigation.test.ts @@ -433,6 +433,28 @@ describe('ReactNavigationInstrumentation', () => { expect(result).toBe(false); }); + + it('should handle undefined devServerUrls by using only the user defined shouldCreateSpanForRequest', () => { + (getDevServer as jest.Mock).mockReturnValue({ url: undefined }); + + const userShouldCreateSpanForRequest = (_url: string): boolean => { + return true; + }; + + const rnTracing = reactNativeTracingIntegration({ shouldCreateSpanForRequest: userShouldCreateSpanForRequest }); + + const result = rnTracing.options.shouldCreateSpanForRequest('http://any-url.com'); + + expect(result).toBe(true); + }); + + it('should not set the shouldCreateSpanForRequest if not user provided and the devServerUrl is undefined', () => { + (getDevServer as jest.Mock).mockReturnValue({ url: undefined }); + + const rnTracing = reactNativeTracingIntegration(); + + expect(rnTracing.options.shouldCreateSpanForRequest).toBe(undefined); + }); }); function setupTestClient( From 30a702e4cc6d3e6cc343fe2ed7a06932cc9dd2a8 Mon Sep 17 00:00:00 2001 From: Antonis Lilis Date: Fri, 15 Nov 2024 14:55:15 +0200 Subject: [PATCH 7/8] Use startsWith to check url matching --- packages/core/src/js/tracing/reactnativetracing.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/js/tracing/reactnativetracing.ts b/packages/core/src/js/tracing/reactnativetracing.ts index 5544731619..a90abe1caf 100644 --- a/packages/core/src/js/tracing/reactnativetracing.ts +++ b/packages/core/src/js/tracing/reactnativetracing.ts @@ -106,7 +106,7 @@ export const reactNativeTracingIntegration = ( devServerUrl === undefined ? userShouldCreateSpanForRequest : (url: string): boolean => { - if (url.includes(devServerUrl)) { + if (url.startsWith(devServerUrl)) { return false; } if (userShouldCreateSpanForRequest) { From feaab4383d32733628027851d966fd0f41a4f250 Mon Sep 17 00:00:00 2001 From: Antonis Lilis Date: Mon, 18 Nov 2024 13:58:14 +0200 Subject: [PATCH 8/8] Moves changelog to the unreleased section --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f989f48d0a..4fff01d245 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ ### Fixes - Prevents exception capture context from being overwritten by native scope sync ([#4124](https://github.com/getsentry/sentry-react-native/pull/4124)) +- Skips development server spans ([#4271](https://github.com/getsentry/sentry-react-native/pull/4271)) ## 6.2.0 @@ -39,7 +40,6 @@ ### Fixes -- Skips development server spans ([#4271](https://github.com/getsentry/sentry-react-native/pull/4271)) - Ignore JavascriptException to filter out obfuscated duplicate JS Errors on Android ([#4232](https://github.com/getsentry/sentry-react-native/pull/4232)) - Skips ignoring require cycle logs for RN 0.70 or newer ([#4214](https://github.com/getsentry/sentry-react-native/pull/4214)) - Enhanced accuracy of time-to-display spans. ([#4189](https://github.com/getsentry/sentry-react-native/pull/4189))