From 865695a3dffbbf4f354f2406f74390c44687b66e Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 7 Mar 2024 10:02:13 +0000 Subject: [PATCH] ref: Remove `spanRecorder` and all related code This is now not used anymore, we use `getSpanDescendants()` internally now everywhere. --- packages/core/src/tracing/hubextensions.ts | 3 -- packages/core/src/tracing/sentrySpan.ts | 47 ------------------- packages/core/src/tracing/transaction.ts | 16 +------ packages/node/test/handlers.test.ts | 1 - packages/node/test/integrations/http.test.ts | 32 ++++++------- .../node/test/integrations/undici.test.ts | 26 ++++------ packages/node/test/performance.test.ts | 1 + .../opentelemetry/src/custom/transaction.ts | 3 -- .../test/custom/transaction.test.ts | 42 +++++++++++++++-- packages/sveltekit/test/server/handle.test.ts | 15 +++--- 10 files changed, 74 insertions(+), 112 deletions(-) diff --git a/packages/core/src/tracing/hubextensions.ts b/packages/core/src/tracing/hubextensions.ts index d3674b61f341..6702a213549a 100644 --- a/packages/core/src/tracing/hubextensions.ts +++ b/packages/core/src/tracing/hubextensions.ts @@ -42,9 +42,6 @@ function _startTransaction( }, ...customSamplingContext, }); - if (transaction.isRecording()) { - transaction.initSpanRecorder(); - } if (client) { client.emit('startTransaction', transaction); client.emit('spanStart', transaction); diff --git a/packages/core/src/tracing/sentrySpan.ts b/packages/core/src/tracing/sentrySpan.ts index d4da47231c51..d125f0cde6b3 100644 --- a/packages/core/src/tracing/sentrySpan.ts +++ b/packages/core/src/tracing/sentrySpan.ts @@ -28,38 +28,6 @@ import { spanToTraceContext, } from '../utils/spanUtils'; -/** - * Keeps track of finished spans for a given transaction - * @internal - * @hideconstructor - * @hidden - */ -export class SpanRecorder { - public spans: SentrySpan[]; - - private readonly _maxlen: number; - - public constructor(maxlen: number = 1000) { - this._maxlen = maxlen; - this.spans = []; - } - - /** - * This is just so that we don't run out of memory while recording a lot - * of spans. At some point we just stop and flush out the start of the - * trace tree (i.e.the first n spans with the smallest - * start_timestamp). - */ - public add(span: SentrySpan): void { - if (this.spans.length > this._maxlen) { - // eslint-disable-next-line deprecation/deprecation - span.spanRecorder = undefined; - } else { - this.spans.push(span); - } - } -} - /** * Span contains all data about a span */ @@ -71,13 +39,6 @@ export class SentrySpan implements Span { // eslint-disable-next-line @typescript-eslint/no-explicit-any public data: { [key: string]: any }; - /** - * List of spans that were finalized - * - * @deprecated This property will no longer be public. Span recording will be handled internally. - */ - public spanRecorder?: SpanRecorder; - /** * @inheritDoc * @deprecated Use top level `Sentry.getRootSpan()` instead @@ -278,14 +239,6 @@ export class SentrySpan implements Span { traceId: this._traceId, }); - // eslint-disable-next-line deprecation/deprecation - childSpan.spanRecorder = this.spanRecorder; - // eslint-disable-next-line deprecation/deprecation - if (childSpan.spanRecorder) { - // eslint-disable-next-line deprecation/deprecation - childSpan.spanRecorder.add(childSpan); - } - // To allow for interoperability we track the children of a span twice: Once with the span recorder (old) once with // the `addChildSpanToSpan`. Eventually we will only use `addChildSpanToSpan` and drop the span recorder. // To ensure interoperability with the `startSpan` API, `addChildSpanToSpan` is also called here. diff --git a/packages/core/src/tracing/transaction.ts b/packages/core/src/tracing/transaction.ts index 31d50253d2e0..847cffeaa37d 100644 --- a/packages/core/src/tracing/transaction.ts +++ b/packages/core/src/tracing/transaction.ts @@ -21,7 +21,7 @@ import { getMetricSummaryJsonForSpan } from '../metrics/metric-summary'; import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '../semanticAttributes'; import { getSpanDescendants, spanTimeInputToSeconds, spanToJSON, spanToTraceContext } from '../utils/spanUtils'; import { getDynamicSamplingContextFromSpan } from './dynamicSamplingContext'; -import { SentrySpan, SpanRecorder } from './sentrySpan'; +import { SentrySpan } from './sentrySpan'; import { getCapturedScopesOnSpan } from './utils'; /** JSDoc */ @@ -128,20 +128,6 @@ export class Transaction extends SentrySpan implements TransactionInterface { return this; } - /** - * Attaches SpanRecorder to the span itself - * @param maxlen maximum number of spans that can be recorded - */ - public initSpanRecorder(maxlen: number = 1000): void { - // eslint-disable-next-line deprecation/deprecation - if (!this.spanRecorder) { - // eslint-disable-next-line deprecation/deprecation - this.spanRecorder = new SpanRecorder(maxlen); - } - // eslint-disable-next-line deprecation/deprecation - this.spanRecorder.add(this); - } - /** * Set the context of a transaction event. * @deprecated Use either `.setAttribute()`, or set the context on the scope before creating the transaction. diff --git a/packages/node/test/handlers.test.ts b/packages/node/test/handlers.test.ts index 861cfdf13259..eff077f2b332 100644 --- a/packages/node/test/handlers.test.ts +++ b/packages/node/test/handlers.test.ts @@ -461,7 +461,6 @@ describe('tracingHandler', () => { it('waits to finish transaction until all spans are finished, even though `transaction.end()` is registered on `res.finish` event first', done => { // eslint-disable-next-line deprecation/deprecation const transaction = new Transaction({ name: 'mockTransaction', sampled: true }); - transaction.initSpanRecorder(); // eslint-disable-next-line deprecation/deprecation const span = transaction.startChild({ name: 'reallyCoolHandler', diff --git a/packages/node/test/integrations/http.test.ts b/packages/node/test/integrations/http.test.ts index 0a0f8f16645b..62f7aaed1edc 100644 --- a/packages/node/test/integrations/http.test.ts +++ b/packages/node/test/integrations/http.test.ts @@ -1,7 +1,7 @@ import * as http from 'http'; import * as https from 'https'; -import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/core'; -import type { Hub, SentrySpan } from '@sentry/core'; +import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, getSpanDescendants } from '@sentry/core'; +import type { Hub } from '@sentry/core'; import { getCurrentHub, getIsolationScope, setCurrentClient } from '@sentry/core'; import { Transaction } from '@sentry/core'; import { getCurrentScope, setUser, spanToJSON, startInactiveSpan } from '@sentry/core'; @@ -81,12 +81,10 @@ describe('tracing', () => { nock('http://dogs.are.great').get('/').reply(200); const transaction = createTransactionOnScope(); - // eslint-disable-next-line deprecation/deprecation - const spans = (transaction as unknown as SentrySpan).spanRecorder?.spans as SentrySpan[]; http.get('http://dogs.are.great/'); - expect(spans.length).toEqual(2); + const spans = getSpanDescendants(transaction); // our span is at index 1 because the transaction itself is at index 0 expect(spanToJSON(spans[1]).description).toEqual('GET http://dogs.are.great/'); @@ -97,11 +95,11 @@ describe('tracing', () => { nock('http://squirrelchasers.ingest.sentry.io').get('/api/12312012/store/').reply(200); const transaction = createTransactionOnScope(); - // eslint-disable-next-line deprecation/deprecation - const spans = (transaction as unknown as SentrySpan).spanRecorder?.spans as SentrySpan[]; http.get('http://squirrelchasers.ingest.sentry.io/api/12312012/store/'); + const spans = getSpanDescendants(transaction); + // only the transaction itself should be there expect(spans.length).toEqual(1); expect(spanToJSON(spans[0]).description).toEqual('dogpark'); @@ -263,11 +261,11 @@ describe('tracing', () => { nock('http://dogs.are.great').get('/spaniel?tail=wag&cute=true#learn-more').reply(200); const transaction = createTransactionOnScope(); - // eslint-disable-next-line deprecation/deprecation - const spans = (transaction as unknown as SentrySpan).spanRecorder?.spans as SentrySpan[]; http.get('http://dogs.are.great/spaniel?tail=wag&cute=true#learn-more'); + const spans = getSpanDescendants(transaction); + expect(spans.length).toEqual(2); // our span is at index 1 because the transaction itself is at index 0 @@ -286,11 +284,11 @@ describe('tracing', () => { nock('http://dogs.are.great').get('/spaniel?tail=wag&cute=true#learn-more').reply(200); const transaction = createTransactionOnScope(); - // eslint-disable-next-line deprecation/deprecation - const spans = (transaction as unknown as SentrySpan).spanRecorder?.spans as SentrySpan[]; http.request({ method: 'GET', host: 'dogs.are.great', path: '/spaniel?tail=wag&cute=true#learn-more' }); + const spans = getSpanDescendants(transaction); + expect(spans.length).toEqual(2); const spanAttributes = spanToJSON(spans[1]).data || {}; @@ -314,11 +312,11 @@ describe('tracing', () => { nock(`http://${auth}@dogs.are.great`).get('/').reply(200); const transaction = createTransactionOnScope(); - // eslint-disable-next-line deprecation/deprecation - const spans = (transaction as unknown as SentrySpan).spanRecorder?.spans as SentrySpan[]; http.get(`http://${auth}@dogs.are.great/`); + const spans = getSpanDescendants(transaction); + expect(spans.length).toEqual(2); // our span is at index 1 because the transaction itself is at index 0 @@ -374,11 +372,11 @@ describe('tracing', () => { ); const transaction = createTransactionAndPutOnScope(); - // eslint-disable-next-line deprecation/deprecation - const spans = (transaction as unknown as SentrySpan).spanRecorder?.spans as SentrySpan[]; const request = http.get(url); + const spans = getSpanDescendants(transaction); + // There should be no http spans const httpSpans = spans.filter(span => spanToJSON(span).op?.startsWith('http')); expect(httpSpans.length).toBe(0); @@ -483,11 +481,11 @@ describe('tracing', () => { ); const transaction = createTransactionAndPutOnScope(); - // eslint-disable-next-line deprecation/deprecation - const spans = (transaction as unknown as SentrySpan).spanRecorder?.spans as SentrySpan[]; const request = http.get(url); + const spans = getSpanDescendants(transaction); + // There should be no http spans const httpSpans = spans.filter(span => spanToJSON(span).op?.startsWith('http')); expect(httpSpans.length).toBe(0); diff --git a/packages/node/test/integrations/undici.test.ts b/packages/node/test/integrations/undici.test.ts index 86bfb98a6802..adc466021a6b 100644 --- a/packages/node/test/integrations/undici.test.ts +++ b/packages/node/test/integrations/undici.test.ts @@ -6,6 +6,7 @@ import { getCurrentScope, getIsolationScope, getMainCarrier, + getSpanDescendants, setCurrentClient, spanToJSON, startSpan, @@ -125,8 +126,7 @@ conditionalTest({ min: 16 })('Undici integration', () => { await fetch(request, requestInit); expect(outerSpan).toBeInstanceOf(Transaction); - // eslint-disable-next-line deprecation/deprecation - const spans = (outerSpan as Transaction).spanRecorder?.spans || []; + const spans = getSpanDescendants(outerSpan); expect(spans.length).toBe(2); @@ -144,8 +144,7 @@ conditionalTest({ min: 16 })('Undici integration', () => { } expect(outerSpan).toBeInstanceOf(Transaction); - // eslint-disable-next-line deprecation/deprecation - const spans = (outerSpan as Transaction).spanRecorder?.spans || []; + const spans = getSpanDescendants(outerSpan); expect(spans.length).toBe(2); @@ -165,8 +164,7 @@ conditionalTest({ min: 16 })('Undici integration', () => { } expect(outerSpan).toBeInstanceOf(Transaction); - // eslint-disable-next-line deprecation/deprecation - const spans = (outerSpan as Transaction).spanRecorder?.spans || []; + const spans = getSpanDescendants(outerSpan); expect(spans.length).toBe(2); @@ -187,8 +185,7 @@ conditionalTest({ min: 16 })('Undici integration', () => { } expect(outerSpan).toBeInstanceOf(Transaction); - // eslint-disable-next-line deprecation/deprecation - const spans = (outerSpan as Transaction).spanRecorder?.spans || []; + const spans = getSpanDescendants(outerSpan); expect(spans.length).toBe(1); }); @@ -207,18 +204,17 @@ conditionalTest({ min: 16 })('Undici integration', () => { it('does create a span if `shouldCreateSpanForRequest` is defined', async () => { await startSpan({ name: 'outer-span' }, async outerSpan => { expect(outerSpan).toBeInstanceOf(Transaction); - // eslint-disable-next-line deprecation/deprecation - const spans = (outerSpan as Transaction).spanRecorder?.spans || []; + expect(getSpanDescendants(outerSpan).length).toBe(1); const undoPatch = patchUndici({ shouldCreateSpanForRequest: url => url.includes('yes') }); await fetch('http://localhost:18100/no', { method: 'POST' }); - expect(spans.length).toBe(1); + expect(getSpanDescendants(outerSpan).length).toBe(1); await fetch('http://localhost:18100/yes', { method: 'POST' }); - expect(spans.length).toBe(2); + expect(getSpanDescendants(outerSpan).length).toBe(2); undoPatch(); }); @@ -232,8 +228,7 @@ conditionalTest({ min: 16 })('Undici integration', () => { await withIsolationScope(async () => { await startSpan({ name: 'outer-span' }, async outerSpan => { expect(outerSpan).toBeInstanceOf(Transaction); - // eslint-disable-next-line deprecation/deprecation - const spans = (outerSpan as Transaction).spanRecorder?.spans || []; + const spans = getSpanDescendants(outerSpan); await fetch('http://localhost:18100', { method: 'POST' }); @@ -307,8 +302,7 @@ conditionalTest({ min: 16 })('Undici integration', () => { await startSpan({ name: 'outer-span' }, async outerSpan => { expect(outerSpan).toBeInstanceOf(Transaction); - // eslint-disable-next-line deprecation/deprecation - const spans = (outerSpan as Transaction).spanRecorder?.spans || []; + const spans = getSpanDescendants(outerSpan); expect(spans.length).toBe(1); diff --git a/packages/node/test/performance.test.ts b/packages/node/test/performance.test.ts index 86907e408746..b8a0d34cb054 100644 --- a/packages/node/test/performance.test.ts +++ b/packages/node/test/performance.test.ts @@ -81,6 +81,7 @@ describe('startSpan()', () => { const transactionEvent = await transactionEventPromise; + expect(transactionEvent.spans).toHaveLength(1); expect(transactionEvent.spans?.[0].description).toBe('second'); }); }); diff --git a/packages/opentelemetry/src/custom/transaction.ts b/packages/opentelemetry/src/custom/transaction.ts index 6ebb8aa63f36..35028f450a4c 100644 --- a/packages/opentelemetry/src/custom/transaction.ts +++ b/packages/opentelemetry/src/custom/transaction.ts @@ -12,9 +12,6 @@ export function startTransaction(hub: HubInterface, transactionContext: Transact // eslint-disable-next-line deprecation/deprecation const transaction = new Transaction(transactionContext, hub as Hub); - // Since we do not do sampling here, we assume that this is _always_ sampled - // Any sampling decision happens in OpenTelemetry's sampler - transaction.initSpanRecorder(); if (client) { client.emit('startTransaction', transaction); diff --git a/packages/opentelemetry/test/custom/transaction.test.ts b/packages/opentelemetry/test/custom/transaction.test.ts index f56350abeb8e..8d6a7ef6d8cf 100644 --- a/packages/opentelemetry/test/custom/transaction.test.ts +++ b/packages/opentelemetry/test/custom/transaction.test.ts @@ -1,4 +1,11 @@ -import { Transaction, getCurrentHub, setCurrentClient, spanToJSON } from '@sentry/core'; +import { + Transaction, + getCurrentHub, + getSpanDescendants, + setCurrentClient, + spanIsSampled, + spanToJSON, +} from '@sentry/core'; import { startTransaction } from '../../src/custom/transaction'; import { TestClient, getDefaultTestClientOptions } from '../helpers/TestClient'; @@ -7,7 +14,7 @@ describe('startTranscation', () => { jest.resetAllMocks(); }); - it('creates a Transaction', () => { + it('creates an unsampled transaction', () => { const client = new TestClient(getDefaultTestClientOptions()); // eslint-disable-next-line deprecation/deprecation const hub = getCurrentHub(); @@ -18,10 +25,37 @@ describe('startTranscation', () => { expect(transaction).toBeInstanceOf(Transaction); expect(transaction['_sampled']).toBe(undefined); + expect(spanIsSampled(transaction)).toBe(false); + // unsampled span is filtered out here + expect(getSpanDescendants(transaction)).toHaveLength(0); // eslint-disable-next-line deprecation/deprecation - expect(transaction.spanRecorder).toBeDefined(); + expect(transaction.metadata).toEqual({ + spanMetadata: {}, + }); + + expect(spanToJSON(transaction)).toEqual( + expect.objectContaining({ + origin: 'manual', + span_id: expect.any(String), + start_timestamp: expect.any(Number), + trace_id: expect.any(String), + }), + ); + }); + + it('creates a sampled transaction', () => { + const client = new TestClient(getDefaultTestClientOptions()); // eslint-disable-next-line deprecation/deprecation - expect(transaction.spanRecorder?.spans).toHaveLength(1); + const hub = getCurrentHub(); + setCurrentClient(client); + client.init(); + + const transaction = startTransaction(hub, { name: 'test', sampled: true }); + + expect(transaction).toBeInstanceOf(Transaction); + expect(transaction['_sampled']).toBe(true); + expect(spanIsSampled(transaction)).toBe(true); + expect(getSpanDescendants(transaction)).toHaveLength(1); // eslint-disable-next-line deprecation/deprecation expect(transaction.metadata).toEqual({ spanMetadata: {}, diff --git a/packages/sveltekit/test/server/handle.test.ts b/packages/sveltekit/test/server/handle.test.ts index 5653623c31ad..9dfbaa765dfc 100644 --- a/packages/sveltekit/test/server/handle.test.ts +++ b/packages/sveltekit/test/server/handle.test.ts @@ -1,5 +1,10 @@ -import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, addTracingExtensions, spanIsSampled, spanToJSON } from '@sentry/core'; -import type { Transaction as TransactionClass } from '@sentry/core'; +import { + SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, + addTracingExtensions, + getSpanDescendants, + spanIsSampled, + spanToJSON, +} from '@sentry/core'; import { NodeClient, setCurrentClient } from '@sentry/node-experimental'; import * as SentryNode from '@sentry/node-experimental'; import type { Span, Transaction } from '@sentry/types'; @@ -138,8 +143,7 @@ describe('handleSentry', () => { expect(spanToJSON(_span!).timestamp).toBeDefined(); - // eslint-disable-next-line deprecation/deprecation - const spans = (_span! as TransactionClass).spanRecorder?.spans; + const spans = getSpanDescendants(_span!); expect(spans).toHaveLength(1); }); @@ -177,8 +181,7 @@ describe('handleSentry', () => { expect(spanToJSON(_span!).timestamp).toBeDefined(); - // eslint-disable-next-line deprecation/deprecation - const spans = (_span! as TransactionClass).spanRecorder?.spans?.map(spanToJSON); + const spans = getSpanDescendants(_span!).map(spanToJSON); expect(spans).toHaveLength(2); expect(spans).toEqual(