From 9ee03f36aa1a895a982052f568f80d119d1d473e Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Fri, 11 Oct 2024 11:04:11 +0200 Subject: [PATCH 1/7] add opt-in option for stream instrumentation --- .../src/tracing/browserTracingIntegration.ts | 9 +++++++ packages/browser/src/tracing/request.ts | 24 +++++++++++++------ packages/browser/test/tracing/request.test.ts | 10 +++++++- packages/utils/src/instrument/fetch.ts | 7 ++++-- 4 files changed, 40 insertions(+), 10 deletions(-) diff --git a/packages/browser/src/tracing/browserTracingIntegration.ts b/packages/browser/src/tracing/browserTracingIntegration.ts index 523edd7e4262..26bfc2f4fb3e 100644 --- a/packages/browser/src/tracing/browserTracingIntegration.ts +++ b/packages/browser/src/tracing/browserTracingIntegration.ts @@ -132,6 +132,13 @@ export interface BrowserTracingOptions { */ traceXHR: boolean; + /** + * Flag to disable tracing of long-lived streams, like server-sent events (SSE) via fetch. + * + * Default: false + */ + traceStreams: boolean; + /** * If true, Sentry will capture http timings and add them to the corresponding http spans. * @@ -200,6 +207,7 @@ export const browserTracingIntegration = ((_options: Partial): void { - const { traceFetch, traceXHR, shouldCreateSpanForRequest, enableHTTPTimings, tracePropagationTargets } = { - traceFetch: defaultRequestInstrumentationOptions.traceFetch, - traceXHR: defaultRequestInstrumentationOptions.traceXHR, - ..._options, - }; + const { traceFetch, traceXHR, traceStreams, shouldCreateSpanForRequest, enableHTTPTimings, tracePropagationTargets } = + { + traceFetch: defaultRequestInstrumentationOptions.traceFetch, + traceXHR: defaultRequestInstrumentationOptions.traceXHR, + traceStreams: defaultRequestInstrumentationOptions.traceStreams, + ..._options, + }; const shouldCreateSpan = typeof shouldCreateSpanForRequest === 'function' ? shouldCreateSpanForRequest : (_: string) => true; @@ -143,7 +153,7 @@ export function instrumentOutgoingRequests(client: Client, _options?: Partial { const createdSpan = instrumentFetchRequest(handlerData, shouldCreateSpan, shouldAttachHeadersWithTargets, spans); @@ -167,7 +177,7 @@ export function instrumentOutgoingRequests(client: Client, _options?: Partial { instrumentOutgoingRequests(client); - expect(addFetchSpy).toHaveBeenCalledWith(expect.any(Function)); + expect(addFetchSpy).toHaveBeenCalledWith(expect.any(Function), false); expect(addXhrSpy).toHaveBeenCalledWith(expect.any(Function)); }); @@ -54,6 +54,14 @@ describe('instrumentOutgoingRequests', () => { expect(addXhrSpy).not.toHaveBeenCalled(); }); + + it('does instrument streaming requests if traceStream is true', () => { + const addFetchSpy = vi.spyOn(utils, 'addFetchInstrumentationHandler'); + + instrumentOutgoingRequests(client, { traceStreams: true }); + + expect(addFetchSpy).toHaveBeenCalledWith(expect.any(Function), true); + }); }); interface ProtocolInfo { diff --git a/packages/utils/src/instrument/fetch.ts b/packages/utils/src/instrument/fetch.ts index ad28edf81e3f..37230fb6e1af 100644 --- a/packages/utils/src/instrument/fetch.ts +++ b/packages/utils/src/instrument/fetch.ts @@ -35,10 +35,13 @@ export function addFetchInstrumentationHandler( * Only used internally * @hidden */ -export function addFetchEndInstrumentationHandler(handler: (data: HandlerDataFetch) => void): void { +export function addFetchEndInstrumentationHandler( + handler: (data: HandlerDataFetch) => void, + traceStreams?: boolean, +): void { const type = 'fetch-body-resolved'; addHandler(type, handler); - maybeInstrument(type, () => instrumentFetch(streamHandler)); + maybeInstrument(type, () => instrumentFetch(traceStreams ? streamHandler : undefined)); } function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNativeFetchCheck: boolean = false): void { From dbc0843488612c1e47dac9892e9f0cbc1f20faad Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Fri, 11 Oct 2024 11:14:56 +0200 Subject: [PATCH 2/7] opt-in test for streams --- .../e2e-tests/test-applications/react-router-6/src/index.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/dev-packages/e2e-tests/test-applications/react-router-6/src/index.tsx b/dev-packages/e2e-tests/test-applications/react-router-6/src/index.tsx index 434c1677bf88..215cc411919d 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-6/src/index.tsx +++ b/dev-packages/e2e-tests/test-applications/react-router-6/src/index.tsx @@ -26,6 +26,7 @@ Sentry.init({ useNavigationType, createRoutesFromChildren, matchRoutes, + traceStreams: true, }), replay, ], From ec980166b3bb1aebd50f679b6f94ed385232c1f2 Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Fri, 11 Oct 2024 11:21:24 +0200 Subject: [PATCH 3/7] add changelog release notes --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f32d3f4387b8..29321de59f3f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,11 @@ - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott +- feat(core): Make stream instrumentation opt-in ([#13951](https://github.com/getsentry/sentry-javascript/pull/13951)) + +This change adds a new option `traceStreams` to the browser tracing integration. Only when set to `true`, Sentry will +instrument streams via fetch. + ## 8.34.0 ### Important Changes From 76b16a37940c88f497671e5ca6d39824e45d7ee3 Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Fri, 11 Oct 2024 12:24:09 +0200 Subject: [PATCH 4/7] rename option + fix --- .../src/tracing/browserTracingIntegration.ts | 8 ++--- packages/browser/src/tracing/request.ts | 30 +++++++++++-------- packages/browser/test/tracing/request.test.ts | 4 +-- packages/utils/src/instrument/fetch.ts | 4 +-- 4 files changed, 26 insertions(+), 20 deletions(-) diff --git a/packages/browser/src/tracing/browserTracingIntegration.ts b/packages/browser/src/tracing/browserTracingIntegration.ts index 26bfc2f4fb3e..123d9eda5b14 100644 --- a/packages/browser/src/tracing/browserTracingIntegration.ts +++ b/packages/browser/src/tracing/browserTracingIntegration.ts @@ -133,11 +133,11 @@ export interface BrowserTracingOptions { traceXHR: boolean; /** - * Flag to disable tracing of long-lived streams, like server-sent events (SSE) via fetch. + * Flag to disable tracking of long-lived streams, like server-sent events (SSE) via fetch. * * Default: false */ - traceStreams: boolean; + trackFetchStreamPerformance: boolean; /** * If true, Sentry will capture http timings and add them to the corresponding http spans. @@ -207,7 +207,7 @@ export const browserTracingIntegration = ((_options: Partial): void { - const { traceFetch, traceXHR, traceStreams, shouldCreateSpanForRequest, enableHTTPTimings, tracePropagationTargets } = - { - traceFetch: defaultRequestInstrumentationOptions.traceFetch, - traceXHR: defaultRequestInstrumentationOptions.traceXHR, - traceStreams: defaultRequestInstrumentationOptions.traceStreams, - ..._options, - }; + const { + traceFetch, + traceXHR, + trackFetchStreamPerformance, + shouldCreateSpanForRequest, + enableHTTPTimings, + tracePropagationTargets, + } = { + traceFetch: defaultRequestInstrumentationOptions.traceFetch, + traceXHR: defaultRequestInstrumentationOptions.traceXHR, + trackFetchStreamPerformance: defaultRequestInstrumentationOptions.trackFetchStreamPerformance, + ..._options, + }; const shouldCreateSpan = typeof shouldCreateSpanForRequest === 'function' ? shouldCreateSpanForRequest : (_: string) => true; @@ -153,7 +159,7 @@ export function instrumentOutgoingRequests(client: Client, _options?: Partial { const createdSpan = instrumentFetchRequest(handlerData, shouldCreateSpan, shouldAttachHeadersWithTargets, spans); @@ -177,7 +183,7 @@ export function instrumentOutgoingRequests(client: Client, _options?: Partial { expect(addXhrSpy).not.toHaveBeenCalled(); }); - it('does instrument streaming requests if traceStream is true', () => { + it('does instrument streaming requests if trackFetchStreamPerformance is true', () => { const addFetchSpy = vi.spyOn(utils, 'addFetchInstrumentationHandler'); - instrumentOutgoingRequests(client, { traceStreams: true }); + instrumentOutgoingRequests(client, { trackFetchStreamPerformance: true }); expect(addFetchSpy).toHaveBeenCalledWith(expect.any(Function), true); }); diff --git a/packages/utils/src/instrument/fetch.ts b/packages/utils/src/instrument/fetch.ts index 37230fb6e1af..9dc1bbfd6eaf 100644 --- a/packages/utils/src/instrument/fetch.ts +++ b/packages/utils/src/instrument/fetch.ts @@ -37,11 +37,11 @@ export function addFetchInstrumentationHandler( */ export function addFetchEndInstrumentationHandler( handler: (data: HandlerDataFetch) => void, - traceStreams?: boolean, + trackFetchStreamPerformance?: boolean, ): void { const type = 'fetch-body-resolved'; addHandler(type, handler); - maybeInstrument(type, () => instrumentFetch(traceStreams ? streamHandler : undefined)); + maybeInstrument(type, () => instrumentFetch(trackFetchStreamPerformance ? streamHandler : undefined)); } function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNativeFetchCheck: boolean = false): void { From b6d9905d16521e4f890c3c9bd081cef6bff3af4e Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Fri, 11 Oct 2024 12:52:27 +0200 Subject: [PATCH 5/7] skip adding the handler altogether when the option is not set --- CHANGELOG.md | 4 ++-- .../react-router-6/src/index.tsx | 2 +- packages/browser/src/tracing/request.ts | 17 ++++++++++------- packages/browser/test/tracing/request.test.ts | 6 +++--- packages/utils/src/instrument/fetch.ts | 7 ++----- 5 files changed, 18 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 29321de59f3f..c50c63e36488 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,8 +12,8 @@ - feat(core): Make stream instrumentation opt-in ([#13951](https://github.com/getsentry/sentry-javascript/pull/13951)) -This change adds a new option `traceStreams` to the browser tracing integration. Only when set to `true`, Sentry will -instrument streams via fetch. +This change adds a new option `trackFetchStreamPerformance` to the browser tracing integration. Only when set to `true`, +Sentry will instrument streams via fetch. ## 8.34.0 diff --git a/dev-packages/e2e-tests/test-applications/react-router-6/src/index.tsx b/dev-packages/e2e-tests/test-applications/react-router-6/src/index.tsx index 215cc411919d..8c219563e5a4 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-6/src/index.tsx +++ b/dev-packages/e2e-tests/test-applications/react-router-6/src/index.tsx @@ -26,7 +26,7 @@ Sentry.init({ useNavigationType, createRoutesFromChildren, matchRoutes, - traceStreams: true, + trackFetchStreamPerformance: true, }), replay, ], diff --git a/packages/browser/src/tracing/request.ts b/packages/browser/src/tracing/request.ts index 49f4b526b6c2..f13f0e82b3d6 100644 --- a/packages/browser/src/tracing/request.ts +++ b/packages/browser/src/tracing/request.ts @@ -1,3 +1,4 @@ +/* eslint-disable max-lines */ import { SENTRY_XHR_DATA_KEY, addPerformanceInstrumentationHandler, @@ -152,14 +153,16 @@ export function instrumentOutgoingRequests(client: Client, _options?: Partial { - if (handlerData.response) { - const span = responseToSpanId.get(handlerData.response); - if (span && handlerData.endTimestamp) { - spanIdToEndTimestamp.set(span, handlerData.endTimestamp); + if (trackFetchStreamPerformance) { + addFetchEndInstrumentationHandler(handlerData => { + if (handlerData.response) { + const span = responseToSpanId.get(handlerData.response); + if (span && handlerData.endTimestamp) { + spanIdToEndTimestamp.set(span, handlerData.endTimestamp); + } } - } - }, trackFetchStreamPerformance); + }); + } addFetchInstrumentationHandler(handlerData => { const createdSpan = instrumentFetchRequest(handlerData, shouldCreateSpan, shouldAttachHeadersWithTargets, spans); diff --git a/packages/browser/test/tracing/request.test.ts b/packages/browser/test/tracing/request.test.ts index a2d9c5f9d755..4f1c0bfdaf0a 100644 --- a/packages/browser/test/tracing/request.test.ts +++ b/packages/browser/test/tracing/request.test.ts @@ -35,7 +35,7 @@ describe('instrumentOutgoingRequests', () => { instrumentOutgoingRequests(client); - expect(addFetchSpy).toHaveBeenCalledWith(expect.any(Function), false); + expect(addFetchSpy).toHaveBeenCalledWith(expect.any(Function)); expect(addXhrSpy).toHaveBeenCalledWith(expect.any(Function)); }); @@ -56,11 +56,11 @@ describe('instrumentOutgoingRequests', () => { }); it('does instrument streaming requests if trackFetchStreamPerformance is true', () => { - const addFetchSpy = vi.spyOn(utils, 'addFetchInstrumentationHandler'); + const addFetchEndSpy = vi.spyOn(utils, 'addFetchEndInstrumentationHandler'); instrumentOutgoingRequests(client, { trackFetchStreamPerformance: true }); - expect(addFetchSpy).toHaveBeenCalledWith(expect.any(Function), true); + expect(addFetchEndSpy).toHaveBeenCalledWith(expect.any(Function)); }); }); diff --git a/packages/utils/src/instrument/fetch.ts b/packages/utils/src/instrument/fetch.ts index 9dc1bbfd6eaf..ad28edf81e3f 100644 --- a/packages/utils/src/instrument/fetch.ts +++ b/packages/utils/src/instrument/fetch.ts @@ -35,13 +35,10 @@ export function addFetchInstrumentationHandler( * Only used internally * @hidden */ -export function addFetchEndInstrumentationHandler( - handler: (data: HandlerDataFetch) => void, - trackFetchStreamPerformance?: boolean, -): void { +export function addFetchEndInstrumentationHandler(handler: (data: HandlerDataFetch) => void): void { const type = 'fetch-body-resolved'; addHandler(type, handler); - maybeInstrument(type, () => instrumentFetch(trackFetchStreamPerformance ? streamHandler : undefined)); + maybeInstrument(type, () => instrumentFetch(streamHandler)); } function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNativeFetchCheck: boolean = false): void { From 37a9b3f6dd1b1aac8d559a7f25a1dbc8cd849d03 Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Fri, 11 Oct 2024 13:08:00 +0200 Subject: [PATCH 6/7] add some comments --- .../browser/src/tracing/browserTracingIntegration.ts | 1 + packages/browser/src/tracing/request.ts | 10 ++++++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/browser/src/tracing/browserTracingIntegration.ts b/packages/browser/src/tracing/browserTracingIntegration.ts index 123d9eda5b14..b3f8d9abdba8 100644 --- a/packages/browser/src/tracing/browserTracingIntegration.ts +++ b/packages/browser/src/tracing/browserTracingIntegration.ts @@ -134,6 +134,7 @@ export interface BrowserTracingOptions { /** * Flag to disable tracking of long-lived streams, like server-sent events (SSE) via fetch. + * Do not enable this in case you have live streams or very long running streams. * * Default: false */ diff --git a/packages/browser/src/tracing/request.ts b/packages/browser/src/tracing/request.ts index f13f0e82b3d6..4efe76a637a5 100644 --- a/packages/browser/src/tracing/request.ts +++ b/packages/browser/src/tracing/request.ts @@ -77,13 +77,15 @@ export interface RequestInstrumentationOptions { * * Default: true */ - traceXHR: boolean; - - /** + traceXHR: boolean /** * Flag to disable tracking of long-lived streams, like server-sent events (SSE) via fetch. + * Do not enable this in case you have live streams or very long running streams. + * + * Disabled by default since it can lead to issues with streams using the `cancel()` api + * (https://github.com/getsentry/sentry-javascript/issues/13950) * * Default: false - */ + */; trackFetchStreamPerformance: boolean; /** From d02e78a61a5bad2d8003410d72b394f12ccae4bc Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Fri, 11 Oct 2024 14:24:58 +0200 Subject: [PATCH 7/7] wow --- CHANGELOG.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ac5b9ce94608..0a7cb97c8c4f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,15 +10,14 @@ - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott - -- **feat(core): Make stream instrumentation opt-in ([#13951](https://github.com/getsentry/sentry-javascript/pull/13951))** +- **feat(core): Make stream instrumentation opt-in + ([#13951](https://github.com/getsentry/sentry-javascript/pull/13951))** This change adds a new option `trackFetchStreamPerformance` to the browser tracing integration. Only when set to `true`, Sentry will instrument streams via fetch. Work in this release was contributed by @ZakrepaShe. Thank you for your contribution! - ## 8.34.0 ### Important Changes