diff --git a/packages/core/src/utils/spanOnScope.ts b/packages/core/src/utils/spanOnScope.ts index 0e95d63984fa..ff1e5983895d 100644 --- a/packages/core/src/utils/spanOnScope.ts +++ b/packages/core/src/utils/spanOnScope.ts @@ -1,11 +1,12 @@ import type { Scope } from '../scope'; import type { Span } from '../types/span'; import { addNonEnumerableProperty } from '../utils/object'; +import { derefWeakRef, makeWeakRef, type MaybeWeakRef } from './weakRef'; const SCOPE_SPAN_FIELD = '_sentrySpan'; type ScopeWithMaybeSpan = Scope & { - [SCOPE_SPAN_FIELD]?: Span; + [SCOPE_SPAN_FIELD]?: MaybeWeakRef; }; /** @@ -14,7 +15,8 @@ type ScopeWithMaybeSpan = Scope & { */ export function _setSpanForScope(scope: Scope, span: Span | undefined): void { if (span) { - addNonEnumerableProperty(scope as ScopeWithMaybeSpan, SCOPE_SPAN_FIELD, span); + // Use WeakRef to avoid circular reference with span holding scope + addNonEnumerableProperty(scope as ScopeWithMaybeSpan, SCOPE_SPAN_FIELD, makeWeakRef(span)); } else { // eslint-disable-next-line @typescript-eslint/no-dynamic-delete delete (scope as ScopeWithMaybeSpan)[SCOPE_SPAN_FIELD]; @@ -26,5 +28,5 @@ export function _setSpanForScope(scope: Scope, span: Span | undefined): void { * NOTE: This should NOT be used directly, but is only used internally by the trace methods. */ export function _getSpanForScope(scope: ScopeWithMaybeSpan): Span | undefined { - return scope[SCOPE_SPAN_FIELD]; + return derefWeakRef(scope[SCOPE_SPAN_FIELD]); } diff --git a/packages/core/test/lib/tracing/utils.test.ts b/packages/core/test/lib/tracing/utils.test.ts index 3f65bd89ef9a..c72c7a09109f 100644 --- a/packages/core/test/lib/tracing/utils.test.ts +++ b/packages/core/test/lib/tracing/utils.test.ts @@ -44,17 +44,16 @@ describe('tracing utils', () => { expect(retrieved.isolationScope).toBeUndefined(); }); - it('uses WeakRef only for isolation scopes', () => { + it('uses WeakRef only for isolation scope, stores scope directly', () => { const span = createMockSpan(); const scope = new Scope(); const isolationScope = new Scope(); setCapturedScopesOnSpan(span, scope, isolationScope); - // Check that only isolation scope is wrapped with WeakRef const spanWithScopes = span as any; - expect(spanWithScopes._sentryScope).toBe(scope); // Regular scope stored directly - expect(spanWithScopes._sentryIsolationScope).toBeInstanceOf(WeakRef); // Isolation scope wrapped + expect(spanWithScopes._sentryScope).toBe(scope); + expect(spanWithScopes._sentryIsolationScope).toBeInstanceOf(WeakRef); // Verify we can still retrieve the scopes const retrieved = getCapturedScopesOnSpan(span); @@ -77,14 +76,8 @@ describe('tracing utils', () => { // Check that both scopes are stored directly when WeakRef is not available const spanWithScopes = span as any; - expect(spanWithScopes._sentryScope).toBe(scope); // Regular scope always stored directly - expect(spanWithScopes._sentryIsolationScope).toBe(isolationScope); // Isolation scope falls back to direct storage - - // When WeakRef is available, ensure regular scope is not wrapped but isolation scope would be - if (originalWeakRef) { - expect(spanWithScopes._sentryScope).not.toBeInstanceOf(originalWeakRef); - expect(spanWithScopes._sentryIsolationScope).not.toBeInstanceOf(originalWeakRef); - } + expect(spanWithScopes._sentryScope).toBe(scope); + expect(spanWithScopes._sentryIsolationScope).toBe(isolationScope); // Verify we can still retrieve the scopes const retrieved = getCapturedScopesOnSpan(span); @@ -96,37 +89,33 @@ describe('tracing utils', () => { } }); - it('handles WeakRef deref returning undefined gracefully', () => { + it('handles WeakRef deref returning undefined gracefully for isolation scope', () => { const span = createMockSpan(); const scope = new Scope(); const isolationScope = new Scope(); setCapturedScopesOnSpan(span, scope, isolationScope); - // Mock WeakRef.deref to return undefined for isolation scope (simulating garbage collection) - // Regular scope is stored directly, so it should always be available + // Mock isolation scope WeakRef to return undefined (simulating garbage collection) const spanWithScopes = span as any; const mockIsolationScopeWeakRef = { deref: vi.fn().mockReturnValue(undefined), }; - // Keep the regular scope as is (stored directly) - // Only replace the isolation scope with a mock WeakRef spanWithScopes._sentryIsolationScope = mockIsolationScopeWeakRef; const retrieved = getCapturedScopesOnSpan(span); - expect(retrieved.scope).toBe(scope); // Regular scope should still be available - expect(retrieved.isolationScope).toBeUndefined(); // Isolation scope should be undefined due to GC + expect(retrieved.scope).toBe(scope); + expect(retrieved.isolationScope).toBeUndefined(); expect(mockIsolationScopeWeakRef.deref).toHaveBeenCalled(); }); - it('handles corrupted WeakRef objects gracefully', () => { + it('handles corrupted WeakRef objects gracefully for isolation scope', () => { const span = createMockSpan(); const scope = new Scope(); - // Set up a regular scope (stored directly) and a corrupted isolation scope WeakRef const spanWithScopes = span as any; - spanWithScopes._sentryScope = scope; // Regular scope stored directly + spanWithScopes._sentryScope = scope; spanWithScopes._sentryIsolationScope = { deref: vi.fn().mockImplementation(() => { throw new Error('WeakRef deref failed'); @@ -134,8 +123,8 @@ describe('tracing utils', () => { }; const retrieved = getCapturedScopesOnSpan(span); - expect(retrieved.scope).toBe(scope); // Regular scope should still be available - expect(retrieved.isolationScope).toBeUndefined(); // Isolation scope should be undefined due to error + expect(retrieved.scope).toBe(scope); + expect(retrieved.isolationScope).toBeUndefined(); }); it('preserves scope data when using WeakRef', () => { diff --git a/packages/opentelemetry/src/spanExporter.ts b/packages/opentelemetry/src/spanExporter.ts index aed57b52e58e..b050c92a583d 100644 --- a/packages/opentelemetry/src/spanExporter.ts +++ b/packages/opentelemetry/src/spanExporter.ts @@ -20,6 +20,7 @@ import { getCapturedScopesOnSpan, getDynamicSamplingContextFromSpan, getStatusMessage, + LRUMap, SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME, SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, @@ -41,6 +42,7 @@ type SpanNodeCompleted = SpanNode & { span: ReadableSpan }; const MAX_SPAN_COUNT = 1000; const DEFAULT_TIMEOUT = 300; // 5 min +const SENT_SPANS_MAX_SIZE = 10000; interface FinishedSpanBucket { timestampInS: number; @@ -71,9 +73,7 @@ export class SentrySpanExporter { private _finishedSpanBucketSize: number; private _spansToBucketEntry: WeakMap; private _lastCleanupTimestampInS: number; - // Essentially a a set of span ids that are already sent. The values are expiration - // times in this cache so we don't hold onto them indefinitely. - private _sentSpans: Map; + private _sentSpans: LRUMap; /* Internally, we use a debounced flush to give some wiggle room to the span processor to accumulate more spans. */ private _debouncedFlush: ReturnType; @@ -85,7 +85,7 @@ export class SentrySpanExporter { this._finishedSpanBuckets = new Array(this._finishedSpanBucketSize).fill(undefined); this._lastCleanupTimestampInS = Math.floor(_INTERNAL_safeDateNow() / 1000); this._spansToBucketEntry = new WeakMap(); - this._sentSpans = new Map(); + this._sentSpans = new LRUMap(SENT_SPANS_MAX_SIZE); this._debouncedFlush = debounce(this.flush.bind(this), 1, { maxWait: 100 }); } @@ -124,7 +124,7 @@ export class SentrySpanExporter { // If the span doesn't have a local parent ID (it's a root span), we're gonna flush all the ended spans const localParentId = getLocalParentId(span); - if (!localParentId || this._sentSpans.has(localParentId)) { + if (!localParentId || this._sentSpans.get(localParentId)) { this._debouncedFlush(); } } @@ -137,7 +137,6 @@ export class SentrySpanExporter { public flush(): void { const finishedSpans = this._finishedSpanBuckets.flatMap(bucket => (bucket ? Array.from(bucket.spans) : [])); - this._flushSentSpanCache(); const sentSpans = this._maybeSend(finishedSpans); const sentSpanCount = sentSpans.size; @@ -147,15 +146,14 @@ export class SentrySpanExporter { `SpanExporter exported ${sentSpanCount} spans, ${remainingOpenSpanCount} spans are waiting for their parent spans to finish`, ); - const expirationDate = _INTERNAL_safeDateNow() + DEFAULT_TIMEOUT * 1000; - for (const span of sentSpans) { - this._sentSpans.set(span.spanContext().spanId, expirationDate); + this._sentSpans.set(span.spanContext().spanId, 1); const bucketEntry = this._spansToBucketEntry.get(span); if (bucketEntry) { bucketEntry.spans.delete(span); } } + // Cancel a pending debounced flush, if there is one // This can be relevant if we directly flush, circumventing the debounce // in that case, we want to cancel any pending debounced flush @@ -193,7 +191,7 @@ export class SentrySpanExporter { const transactionEvent = createTransactionForOtelSpan(span); // Add an attribute to the transaction event to indicate that this transaction is an orphaned transaction - if (root.parentNode && this._sentSpans.has(root.parentNode.id)) { + if (root.parentNode && this._sentSpans.get(root.parentNode.id)) { const traceData = transactionEvent.contexts?.trace?.data; if (traceData) { traceData['sentry.parent_span_already_sent'] = true; @@ -235,20 +233,9 @@ export class SentrySpanExporter { return sentSpans; } - /** Remove "expired" span id entries from the _sentSpans cache. */ - private _flushSentSpanCache(): void { - const currentTimestamp = _INTERNAL_safeDateNow(); - // Note, it is safe to delete items from the map as we go: https://stackoverflow.com/a/35943995/90297 - for (const [spanId, expirationTime] of this._sentSpans.entries()) { - if (expirationTime <= currentTimestamp) { - this._sentSpans.delete(spanId); - } - } - } - /** Check if a node is a completed root node or a node whose parent has already been sent */ private _nodeIsCompletedRootNodeOrHasSentParent(node: SpanNode): node is SpanNodeCompleted { - return !!node.span && (!node.parentNode || this._sentSpans.has(node.parentNode.id)); + return !!node.span && (!node.parentNode || !!this._sentSpans.get(node.parentNode.id)); } /** Get all completed root nodes from a list of nodes */ diff --git a/packages/opentelemetry/test/integration/transactions.test.ts b/packages/opentelemetry/test/integration/transactions.test.ts index aae975d8d8a9..7280bc4d9b0c 100644 --- a/packages/opentelemetry/test/integration/transactions.test.ts +++ b/packages/opentelemetry/test/integration/transactions.test.ts @@ -612,7 +612,7 @@ describe('Integration | Transactions', () => { expect(finishedSpans.length).toBe(0); }); - it('discards child spans that are finished after 5 minutes their parent span has been sent', async () => { + it('sends child spans that are finished after 5 minutes as orphaned transactions', async () => { const timeout = 5 * 60 * 1000; const now = Date.now(); vi.useFakeTimers(); @@ -631,14 +631,6 @@ describe('Integration | Transactions', () => { }, }); - const spanProcessor = getSpanProcessor(); - - const exporter = spanProcessor ? spanProcessor['_exporter'] : undefined; - - if (!exporter) { - throw new Error('No exporter found, aborting test...'); - } - startSpanManual({ name: 'test name' }, async span => { const subSpan = startInactiveSpan({ name: 'inner span 1' }); subSpan.end(); @@ -654,18 +646,15 @@ describe('Integration | Transactions', () => { vi.advanceTimersByTime(timeout + 2); - expect(transactions).toHaveLength(1); + expect(transactions).toHaveLength(2); expect(transactions[0]?.spans).toHaveLength(1); - // subSpan2 is pending (and will eventually be cleaned up) - const finishedSpans: any = []; - exporter['_finishedSpanBuckets'].forEach(bucket => { - if (bucket) { - finishedSpans.push(...bucket.spans); - } + expect(transactions[1]?.transaction).toBe('inner span 2'); + expect(transactions[1]?.contexts?.trace?.data).toEqual({ + 'sentry.parent_span_already_sent': true, + 'sentry.origin': 'manual', + 'sentry.source': 'custom', }); - expect(finishedSpans.length).toBe(1); - expect(finishedSpans[0]?.name).toBe('inner span 2'); }); it('only considers sent spans, not finished spans, for flushing orphaned spans of sent spans', async () => {