From 24483738dffda0c900d52825fb93897e5804aece Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 10 Jul 2024 17:05:40 +0200 Subject: [PATCH 1/4] fix: Cleanup hooks when they are not used anymore Small optimization using the new hook cleanup capabilities to remove unused hooks. --- packages/core/src/tracing/idleSpan.ts | 66 +++++++++++++--------- packages/feedback/src/core/sendFeedback.ts | 3 +- packages/react/src/errorboundary.tsx | 8 ++- 3 files changed, 47 insertions(+), 30 deletions(-) diff --git a/packages/core/src/tracing/idleSpan.ts b/packages/core/src/tracing/idleSpan.ts index 67093076f443..552b935a2e1f 100644 --- a/packages/core/src/tracing/idleSpan.ts +++ b/packages/core/src/tracing/idleSpan.ts @@ -96,6 +96,8 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti let _autoFinishAllowed: boolean = !options.disableAutoFinish; + const _cleanupHooks: (() => void)[] = []; + const { idleTimeout = TRACING_DEFAULTS.idleTimeout, finalTimeout = TRACING_DEFAULTS.finalTimeout, @@ -237,6 +239,8 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti } function onIdleSpanEnded(endTimestamp: number): void { + _cleanupHooks.forEach(cleanup => cleanup()); + _finished = true; activities.clear(); @@ -298,41 +302,47 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti } } - client.on('spanStart', startedSpan => { - // If we already finished the idle span, - // or if this is the idle span itself being started, - // or if the started span has already been closed, - // we don't care about it for activity - if (_finished || startedSpan === span || !!spanToJSON(startedSpan).timestamp) { - return; - } + _cleanupHooks.push( + client.on('spanStart', startedSpan => { + // If we already finished the idle span, + // or if this is the idle span itself being started, + // or if the started span has already been closed, + // we don't care about it for activity + if (_finished || startedSpan === span || !!spanToJSON(startedSpan).timestamp) { + return; + } - const allSpans = getSpanDescendants(span); + const allSpans = getSpanDescendants(span); - // If the span that was just started is a child of the idle span, we should track it - if (allSpans.includes(startedSpan)) { - _pushActivity(startedSpan.spanContext().spanId); - } - }); + // If the span that was just started is a child of the idle span, we should track it + if (allSpans.includes(startedSpan)) { + _pushActivity(startedSpan.spanContext().spanId); + } + }), + ); - client.on('spanEnd', endedSpan => { - if (_finished) { - return; - } + _cleanupHooks.push( + client.on('spanEnd', endedSpan => { + if (_finished) { + return; + } - _popActivity(endedSpan.spanContext().spanId); - }); + _popActivity(endedSpan.spanContext().spanId); + }), + ); - client.on('idleSpanEnableAutoFinish', spanToAllowAutoFinish => { - if (spanToAllowAutoFinish === span) { - _autoFinishAllowed = true; - _restartIdleTimeout(); + _cleanupHooks.push( + client.on('idleSpanEnableAutoFinish', spanToAllowAutoFinish => { + if (spanToAllowAutoFinish === span) { + _autoFinishAllowed = true; + _restartIdleTimeout(); - if (activities.size) { - _restartChildSpanTimeout(); + if (activities.size) { + _restartChildSpanTimeout(); + } } - } - }); + }), + ); // We only start the initial idle timeout if we are not delaying the auto finish if (!options.disableAutoFinish) { diff --git a/packages/feedback/src/core/sendFeedback.ts b/packages/feedback/src/core/sendFeedback.ts index 3f8c08a51ee3..ca9875284c6e 100644 --- a/packages/feedback/src/core/sendFeedback.ts +++ b/packages/feedback/src/core/sendFeedback.ts @@ -40,12 +40,13 @@ export const sendFeedback: SendFeedback = ( // After 5s, we want to clear anyhow const timeout = setTimeout(() => reject('Unable to determine if Feedback was correctly sent.'), 5_000); - client.on('afterSendEvent', (event: Event, response: TransportMakeRequestResponse) => { + const cleanup = client.on('afterSendEvent', (event: Event, response: TransportMakeRequestResponse) => { if (event.event_id !== eventId) { return; } clearTimeout(timeout); + cleanup(); // Require valid status codes, otherwise can assume feedback was not sent successfully if ( diff --git a/packages/react/src/errorboundary.tsx b/packages/react/src/errorboundary.tsx index e12ca9f44d79..abd532c29a53 100644 --- a/packages/react/src/errorboundary.tsx +++ b/packages/react/src/errorboundary.tsx @@ -77,6 +77,7 @@ class ErrorBoundary extends React.Component void; public constructor(props: ErrorBoundaryProps) { super(props); @@ -87,7 +88,7 @@ class ErrorBoundary extends React.Component { + this._cleanupHook = client.on('afterSendEvent', event => { if (!event.type && this._lastEventId && event.event_id === this._lastEventId) { showReportDialog({ ...props.dialogOptions, eventId: this._lastEventId }); } @@ -137,6 +138,11 @@ class ErrorBoundary extends React.Component void = () => { From b5f2408ffeb0ad44deee8de9bd7a225123fda355 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 11 Jul 2024 14:57:30 +0200 Subject: [PATCH 2/4] WIP debug --- .../suites/tracing/trace-lifetime/init.js | 1 + .../suites/tracing/trace-lifetime/navigation/test.ts | 12 +++++++++++- packages/core/src/baseclient.ts | 2 ++ packages/core/src/envelope.ts | 4 ++++ packages/core/src/tracing/idleSpan.ts | 4 ++-- 5 files changed, 20 insertions(+), 3 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/init.js b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/init.js index 9c34f4d99f69..c838f92ad161 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/init.js +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/init.js @@ -7,4 +7,5 @@ Sentry.init({ integrations: [Sentry.browserTracingIntegration(), Sentry.feedbackIntegration()], tracePropagationTargets: ['http://example.com'], tracesSampleRate: 1, + debug: true, }); diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts index b62923be0e9b..a6bc48f6f198 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts @@ -69,11 +69,21 @@ sentryTest('creates a new trace on each navigation', async ({ getLocalTestUrl, p expect(navigation1TraceContext?.trace_id).not.toEqual(navigation2TraceContext?.trace_id); }); -sentryTest('error after navigation has navigation traceId', async ({ getLocalTestUrl, page }) => { +sentryTest.only('error after navigation has navigation traceId', async ({ getLocalTestUrl, page }) => { if (shouldSkipTracingTest()) { sentryTest.skip(); } + await page.route('https://dsn.ingest.sentry.io/**/*', route => { + return route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ id: 'test-id' }), + }); + }); + + page.on('console', msg => console.log(msg.text())); + const url = await getLocalTestUrl({ testDir: __dirname }); // ensure pageload transaction is finished diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 1ad35e291013..7bf81e704b54 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -709,6 +709,8 @@ export abstract class BaseClient implements Client { dynamicSamplingContext, ...evt.sdkProcessingMetadata, }; + + logger.log('xxx', JSON.stringify(evt.sdkProcessingMetadata, null, 2)); } return evt; }); diff --git a/packages/core/src/envelope.ts b/packages/core/src/envelope.ts index d8b8c03e888b..b90572107248 100644 --- a/packages/core/src/envelope.ts +++ b/packages/core/src/envelope.ts @@ -20,6 +20,7 @@ import { createEventEnvelopeHeaders, dsnToString, getSdkMetadataForEnvelopeHeader, + logger, } from '@sentry/utils'; import { createSpanEnvelopeItem } from '@sentry/utils'; import { getDynamicSamplingContextFromSpan } from './tracing/dynamicSamplingContext'; @@ -86,6 +87,9 @@ export function createEventEnvelope( const envelopeHeaders = createEventEnvelopeHeaders(event, sdkInfo, tunnel, dsn); + logger.log(JSON.stringify(envelopeHeaders, null, 2)); + logger.log(JSON.stringify(event.sdkProcessingMetadata, null, 2)); + // Prevent this data (which, if it exists, was used in earlier steps in the processing pipeline) from being sent to // sentry. (Note: Our use of this property comes and goes with whatever we might be debugging, whatever hacks we may // have temporarily added, etc. Even if we don't happen to be using it at some point in the future, let's not get rid diff --git a/packages/core/src/tracing/idleSpan.ts b/packages/core/src/tracing/idleSpan.ts index 552b935a2e1f..c3d66a4b7593 100644 --- a/packages/core/src/tracing/idleSpan.ts +++ b/packages/core/src/tracing/idleSpan.ts @@ -239,11 +239,11 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti } function onIdleSpanEnded(endTimestamp: number): void { - _cleanupHooks.forEach(cleanup => cleanup()); - _finished = true; activities.clear(); + _cleanupHooks.forEach(cleanup => cleanup()); + _setSpanForScope(scope, previousActiveSpan); const spanJSON = spanToJSON(span); From eebe1b50cea53adc39bf4144f94e28885a158fcc Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 11 Jul 2024 15:17:35 +0200 Subject: [PATCH 3/4] Revert "WIP debug" This reverts commit b5f2408ffeb0ad44deee8de9bd7a225123fda355. --- .../suites/tracing/trace-lifetime/init.js | 1 - .../suites/tracing/trace-lifetime/navigation/test.ts | 12 +----------- packages/core/src/baseclient.ts | 2 -- packages/core/src/envelope.ts | 4 ---- packages/core/src/tracing/idleSpan.ts | 4 ++-- 5 files changed, 3 insertions(+), 20 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/init.js b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/init.js index c838f92ad161..9c34f4d99f69 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/init.js +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/init.js @@ -7,5 +7,4 @@ Sentry.init({ integrations: [Sentry.browserTracingIntegration(), Sentry.feedbackIntegration()], tracePropagationTargets: ['http://example.com'], tracesSampleRate: 1, - debug: true, }); diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts index a6bc48f6f198..b62923be0e9b 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts @@ -69,21 +69,11 @@ sentryTest('creates a new trace on each navigation', async ({ getLocalTestUrl, p expect(navigation1TraceContext?.trace_id).not.toEqual(navigation2TraceContext?.trace_id); }); -sentryTest.only('error after navigation has navigation traceId', async ({ getLocalTestUrl, page }) => { +sentryTest('error after navigation has navigation traceId', async ({ getLocalTestUrl, page }) => { if (shouldSkipTracingTest()) { sentryTest.skip(); } - await page.route('https://dsn.ingest.sentry.io/**/*', route => { - return route.fulfill({ - status: 200, - contentType: 'application/json', - body: JSON.stringify({ id: 'test-id' }), - }); - }); - - page.on('console', msg => console.log(msg.text())); - const url = await getLocalTestUrl({ testDir: __dirname }); // ensure pageload transaction is finished diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 7bf81e704b54..1ad35e291013 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -709,8 +709,6 @@ export abstract class BaseClient implements Client { dynamicSamplingContext, ...evt.sdkProcessingMetadata, }; - - logger.log('xxx', JSON.stringify(evt.sdkProcessingMetadata, null, 2)); } return evt; }); diff --git a/packages/core/src/envelope.ts b/packages/core/src/envelope.ts index b90572107248..d8b8c03e888b 100644 --- a/packages/core/src/envelope.ts +++ b/packages/core/src/envelope.ts @@ -20,7 +20,6 @@ import { createEventEnvelopeHeaders, dsnToString, getSdkMetadataForEnvelopeHeader, - logger, } from '@sentry/utils'; import { createSpanEnvelopeItem } from '@sentry/utils'; import { getDynamicSamplingContextFromSpan } from './tracing/dynamicSamplingContext'; @@ -87,9 +86,6 @@ export function createEventEnvelope( const envelopeHeaders = createEventEnvelopeHeaders(event, sdkInfo, tunnel, dsn); - logger.log(JSON.stringify(envelopeHeaders, null, 2)); - logger.log(JSON.stringify(event.sdkProcessingMetadata, null, 2)); - // Prevent this data (which, if it exists, was used in earlier steps in the processing pipeline) from being sent to // sentry. (Note: Our use of this property comes and goes with whatever we might be debugging, whatever hacks we may // have temporarily added, etc. Even if we don't happen to be using it at some point in the future, let's not get rid diff --git a/packages/core/src/tracing/idleSpan.ts b/packages/core/src/tracing/idleSpan.ts index c3d66a4b7593..552b935a2e1f 100644 --- a/packages/core/src/tracing/idleSpan.ts +++ b/packages/core/src/tracing/idleSpan.ts @@ -239,11 +239,11 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti } function onIdleSpanEnded(endTimestamp: number): void { + _cleanupHooks.forEach(cleanup => cleanup()); + _finished = true; activities.clear(); - _cleanupHooks.forEach(cleanup => cleanup()); - _setSpanForScope(scope, previousActiveSpan); const spanJSON = spanToJSON(span); From e5b41be56a3699a08170fbbb4818f869a66a660c Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 11 Jul 2024 15:39:02 +0200 Subject: [PATCH 4/4] fix stuff --- .../tracing/trace-lifetime/navigation/test.ts | 8 ++++++++ .../src/tracing/browserTracingIntegration.ts | 4 ++-- packages/core/src/baseclient.ts | 18 +++++------------- packages/core/src/tracing/idleSpan.ts | 4 ++-- 4 files changed, 17 insertions(+), 17 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts index b62923be0e9b..9d158ea5491e 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/trace-lifetime/navigation/test.ts @@ -74,6 +74,14 @@ sentryTest('error after navigation has navigation traceId', async ({ getLocalTes sentryTest.skip(); } + await page.route('https://dsn.ingest.sentry.io/**/*', route => { + return route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify({ id: 'test-id' }), + }); + }); + const url = await getLocalTestUrl({ testDir: __dirname }); // ensure pageload transaction is finished diff --git a/packages/browser/src/tracing/browserTracingIntegration.ts b/packages/browser/src/tracing/browserTracingIntegration.ts index 3deaa195abe3..9d5421f697cd 100644 --- a/packages/browser/src/tracing/browserTracingIntegration.ts +++ b/packages/browser/src/tracing/browserTracingIntegration.ts @@ -288,7 +288,7 @@ export const browserTracingIntegration = ((_options: Partial implements Client { /** @inheritdoc */ public on(hook: string, callback: unknown): () => void { - // Note that the code below, with nullish coalescing assignment, - // may reduce the code, so it may be switched to when Node 14 support - // is dropped (the `??=` operator is supported since Node 15). - // (this._hooks[hook] ??= []).push(callback); - if (!this._hooks[hook]) { - this._hooks[hook] = []; - } + const hooks = (this._hooks[hook] = this._hooks[hook] || []); // @ts-expect-error We assue the types are correct - this._hooks[hook].push(callback); + hooks.push(callback); // This function returns a callback execution handler that, when invoked, // deregisters a callback. This is crucial for managing instances where callbacks // need to be unregistered to prevent self-referencing in callback closures, // ensuring proper garbage collection. return () => { - const hooks = this._hooks[hook]; - - if (hooks) { - // @ts-expect-error We assue the types are correct - const cbIndex = hooks.indexOf(callback); + // @ts-expect-error We assue the types are correct + const cbIndex = hooks.indexOf(callback); + if (cbIndex > -1) { hooks.splice(cbIndex, 1); } }; diff --git a/packages/core/src/tracing/idleSpan.ts b/packages/core/src/tracing/idleSpan.ts index 552b935a2e1f..c3d66a4b7593 100644 --- a/packages/core/src/tracing/idleSpan.ts +++ b/packages/core/src/tracing/idleSpan.ts @@ -239,11 +239,11 @@ export function startIdleSpan(startSpanOptions: StartSpanOptions, options: Parti } function onIdleSpanEnded(endTimestamp: number): void { - _cleanupHooks.forEach(cleanup => cleanup()); - _finished = true; activities.clear(); + _cleanupHooks.forEach(cleanup => cleanup()); + _setSpanForScope(scope, previousActiveSpan); const spanJSON = spanToJSON(span);