From 305f4f461459b4a415dc634b14a1ad9c69386c9f Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 29 Jul 2024 09:00:28 +0200 Subject: [PATCH 1/4] feat(core): Capture # of dropped spans through `beforeSendTransaction` (#13022) Fixes https://github.com/getsentry/sentry-javascript/issues/12849. This is a bit tricky because `beforeSendTransaction` can return a promise, so in order to avoid dealing with this I put the # of spans on the sdk metadata, and then compare that afterwards. --- packages/core/src/baseclient.ts | 32 +++++++++++---- packages/core/test/lib/base.test.ts | 63 +++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 7 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 5a5c748b4282..66c34b8a65d6 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -419,10 +419,12 @@ export abstract class BaseClient implements Client { /** * @inheritDoc */ - public recordDroppedEvent(reason: EventDropReason, category: DataCategory, _event?: Event): void { - // Note: we use `event` in replay, where we overwrite this hook. - + public recordDroppedEvent(reason: EventDropReason, category: DataCategory, eventOrCount?: Event | number): void { if (this._options.sendClientReports) { + // TODO v9: We do not need the `event` passed as third argument anymore, and can possibly remove this overload + // If event is passed as third argument, we assume this is a count of 1 + const count = typeof eventOrCount === 'number' ? eventOrCount : 1; + // We want to track each category (error, transaction, session, replay_event) separately // but still keep the distinction between different type of outcomes. // We could use nested maps, but it's much easier to read and type this way. @@ -430,10 +432,8 @@ export abstract class BaseClient implements Client { // would be `Partial>>>` // With typescript 4.1 we could even use template literal types const key = `${reason}:${category}`; - DEBUG_BUILD && logger.log(`Adding outcome: "${key}"`); - - // The following works because undefined + 1 === NaN and NaN is falsy - this._outcomes[key] = this._outcomes[key] + 1 || 1; + DEBUG_BUILD && logger.log(`Recording outcome: "${key}"${count > 1 ? ` (${count} times)` : ''}`); + this._outcomes[key] = (this._outcomes[key] || 0) + count; } } @@ -778,6 +778,12 @@ export abstract class BaseClient implements Client { .then(processedEvent => { if (processedEvent === null) { this.recordDroppedEvent('before_send', dataCategory, event); + if (isTransaction) { + const spans = event.spans || []; + // the transaction itself counts as one span, plus all the child spans that are added + const spanCount = 1 + spans.length; + this.recordDroppedEvent('before_send', 'span', spanCount); + } throw new SentryError(`${beforeSendLabel} returned \`null\`, will not send event.`, 'log'); } @@ -786,6 +792,18 @@ export abstract class BaseClient implements Client { this._updateSessionFromEvent(session, processedEvent); } + if (isTransaction) { + const spanCountBefore = + (processedEvent.sdkProcessingMetadata && processedEvent.sdkProcessingMetadata.spanCountBeforeProcessing) || + 0; + const spanCountAfter = processedEvent.spans ? processedEvent.spans.length : 0; + + const droppedSpanCount = spanCountBefore - spanCountAfter; + if (droppedSpanCount > 0) { + this.recordDroppedEvent('before_send', 'span', droppedSpanCount); + } + } + // None of the Sentry built event processor will update transaction name, // so if the transaction name has been changed by an event processor, we know // it has to come from custom event processor added by a user diff --git a/packages/core/test/lib/base.test.ts b/packages/core/test/lib/base.test.ts index 855ea73da056..bbe7e706395c 100644 --- a/packages/core/test/lib/base.test.ts +++ b/packages/core/test/lib/base.test.ts @@ -1004,6 +1004,69 @@ describe('BaseClient', () => { expect(TestClient.instance!.event!.transaction).toBe('/adopt/dont/shop'); }); + test('calls `beforeSendTransaction` and drops spans', () => { + const beforeSendTransaction = jest.fn(event => { + event.spans = [{ span_id: 'span5', trace_id: 'trace1', start_timestamp: 1234 }]; + return event; + }); + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSendTransaction }); + const client = new TestClient(options); + + client.captureEvent({ + transaction: '/dogs/are/great', + type: 'transaction', + spans: [ + { span_id: 'span1', trace_id: 'trace1', start_timestamp: 1234 }, + { span_id: 'span2', trace_id: 'trace1', start_timestamp: 1234 }, + { span_id: 'span3', trace_id: 'trace1', start_timestamp: 1234 }, + ], + }); + + expect(beforeSendTransaction).toHaveBeenCalled(); + expect(TestClient.instance!.event!.spans?.length).toBe(1); + + expect(client['_outcomes']).toEqual({ 'before_send:span': 2 }); + }); + + test('calls `beforeSendSpan` and uses the modified spans', () => { + expect.assertions(3); + + const beforeSendSpan = jest.fn(span => { + span.data = { version: 'bravo' }; + return span; + }); + + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSendSpan }); + const client = new TestClient(options); + const transaction: Event = { + transaction: '/cats/are/great', + type: 'transaction', + spans: [ + { + description: 'first span', + span_id: '9e15bf99fbe4bc80', + start_timestamp: 1591603196.637835, + trace_id: '86f39e84263a4de99c326acab3bfe3bd', + }, + { + description: 'second span', + span_id: 'aa554c1f506b0783', + start_timestamp: 1591603196.637835, + trace_id: '86f39e84263a4de99c326acab3bfe3bd', + }, + ], + }; + + client.captureEvent(transaction); + + expect(beforeSendSpan).toHaveBeenCalledTimes(2); + const capturedEvent = TestClient.instance!.event!; + for (const [idx, span] of capturedEvent.spans!.entries()) { + const originalSpan = transaction.spans![idx]; + expect(span).toEqual({ ...originalSpan, data: { version: 'bravo' } }); + } + }); + test('calls `beforeSend` and discards the event', () => { expect.assertions(4); From 72052a49ba4a8dfe8b09dbd61968c0c4c5107d91 Mon Sep 17 00:00:00 2001 From: Krystof Woldrich Date: Tue, 13 Aug 2024 11:23:24 +0200 Subject: [PATCH 2/4] fix test --- packages/core/test/lib/base.test.ts | 39 ----------------------------- 1 file changed, 39 deletions(-) diff --git a/packages/core/test/lib/base.test.ts b/packages/core/test/lib/base.test.ts index bbe7e706395c..a8e37e8e2864 100644 --- a/packages/core/test/lib/base.test.ts +++ b/packages/core/test/lib/base.test.ts @@ -1028,45 +1028,6 @@ describe('BaseClient', () => { expect(client['_outcomes']).toEqual({ 'before_send:span': 2 }); }); - test('calls `beforeSendSpan` and uses the modified spans', () => { - expect.assertions(3); - - const beforeSendSpan = jest.fn(span => { - span.data = { version: 'bravo' }; - return span; - }); - - const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSendSpan }); - const client = new TestClient(options); - const transaction: Event = { - transaction: '/cats/are/great', - type: 'transaction', - spans: [ - { - description: 'first span', - span_id: '9e15bf99fbe4bc80', - start_timestamp: 1591603196.637835, - trace_id: '86f39e84263a4de99c326acab3bfe3bd', - }, - { - description: 'second span', - span_id: 'aa554c1f506b0783', - start_timestamp: 1591603196.637835, - trace_id: '86f39e84263a4de99c326acab3bfe3bd', - }, - ], - }; - - client.captureEvent(transaction); - - expect(beforeSendSpan).toHaveBeenCalledTimes(2); - const capturedEvent = TestClient.instance!.event!; - for (const [idx, span] of capturedEvent.spans!.entries()) { - const originalSpan = transaction.spans![idx]; - expect(span).toEqual({ ...originalSpan, data: { version: 'bravo' } }); - } - }); - test('calls `beforeSend` and discards the event', () => { expect.assertions(4); From 2bd36c8311a24dd4c1f165ea23e7fa63fb383904 Mon Sep 17 00:00:00 2001 From: Krystof Woldrich Date: Tue, 13 Aug 2024 13:01:16 +0200 Subject: [PATCH 3/4] fix tests --- packages/core/src/baseclient.ts | 9 ++++++++ packages/core/test/lib/base.test.ts | 35 ++++++++++++++++++++++++----- 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 66c34b8a65d6..5f10bfb2ff02 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -942,6 +942,15 @@ function processBeforeSend( } if (isTransactionEvent(event) && beforeSendTransaction) { + if (event.spans) { + // We store the # of spans before processing in SDK metadata, + // so we can compare it afterwards to determine how many spans were dropped + const spanCountBefore = event.spans.length; + event.sdkProcessingMetadata = { + ...event.sdkProcessingMetadata, + spanCountBeforeProcessing: spanCountBefore, + }; + } return beforeSendTransaction(event, hint); } diff --git a/packages/core/test/lib/base.test.ts b/packages/core/test/lib/base.test.ts index a8e37e8e2864..edbace365c25 100644 --- a/packages/core/test/lib/base.test.ts +++ b/packages/core/test/lib/base.test.ts @@ -1,7 +1,7 @@ import type { Client, Envelope, Event, Span, Transaction } from '@sentry/types'; import { SentryError, SyncPromise, dsnToString, logger } from '@sentry/utils'; -import { Hub, Scope, makeSession, setGlobalScope } from '../../src'; +import { Hub, Scope, Span as CoreSpan, makeSession, setGlobalScope } from '../../src'; import * as integrationModule from '../../src/integration'; import { TestClient, getDefaultTestClientOptions } from '../mocks/client'; import { AdHocIntegration, TestIntegration } from '../mocks/integration'; @@ -1006,7 +1006,7 @@ describe('BaseClient', () => { test('calls `beforeSendTransaction` and drops spans', () => { const beforeSendTransaction = jest.fn(event => { - event.spans = [{ span_id: 'span5', trace_id: 'trace1', start_timestamp: 1234 }]; + event.spans = [new CoreSpan({ spanId: 'span5', traceId: 'trace1', startTimestamp: 1234 })]; return event; }); const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSendTransaction }); @@ -1016,9 +1016,9 @@ describe('BaseClient', () => { transaction: '/dogs/are/great', type: 'transaction', spans: [ - { span_id: 'span1', trace_id: 'trace1', start_timestamp: 1234 }, - { span_id: 'span2', trace_id: 'trace1', start_timestamp: 1234 }, - { span_id: 'span3', trace_id: 'trace1', start_timestamp: 1234 }, + new CoreSpan({ spanId: 'span1', traceId: 'trace1', startTimestamp: 1234 }), + new CoreSpan({ spanId: 'span2', traceId: 'trace1', startTimestamp: 1234 }), + new CoreSpan({ spanId: 'span3', traceId: 'trace1', startTimestamp: 1234 }), ], }); @@ -1028,6 +1028,31 @@ describe('BaseClient', () => { expect(client['_outcomes']).toEqual({ 'before_send:span': 2 }); }); + test('calls `beforeSendTransaction` and drops spans + 1 if tx null', () => { + const beforeSendTransaction = jest.fn(() => { + return null; + }); + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, beforeSendTransaction }); + const client = new TestClient(options); + + client.captureEvent({ + transaction: '/dogs/are/great', + type: 'transaction', + spans: [ + new CoreSpan({ spanId: 'span1', traceId: 'trace1', startTimestamp: 1234 }), + new CoreSpan({ spanId: 'span2', traceId: 'trace1', startTimestamp: 1234 }), + new CoreSpan({ spanId: 'span3', traceId: 'trace1', startTimestamp: 1234 }), + ], + }); + + expect(beforeSendTransaction).toHaveBeenCalled(); + + expect(client['_outcomes']).toEqual({ + 'before_send:span': 4, + 'before_send:transaction': 1, + }); + }); + test('calls `beforeSend` and discards the event', () => { expect.assertions(4); From e5c519d18c49d4772b0a05b744c7dd48da9b35d6 Mon Sep 17 00:00:00 2001 From: Krystof Woldrich Date: Tue, 13 Aug 2024 13:17:34 +0200 Subject: [PATCH 4/4] fix tests --- .size-limit.js | 4 ++-- .../node-experimental/test/integration/transactions.test.ts | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.size-limit.js b/.size-limit.js index 7794d74d771d..92e7cd7bce58 100644 --- a/.size-limit.js +++ b/.size-limit.js @@ -93,7 +93,7 @@ module.exports = [ name: '@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped)', path: 'packages/browser/build/bundles/bundle.tracing.min.js', gzip: true, - limit: '37 KB', + limit: '38 KB', }, { name: '@sentry/browser - ES6 CDN Bundle (gzipped)', @@ -115,7 +115,7 @@ module.exports = [ path: 'packages/browser/build/bundles/bundle.tracing.min.js', gzip: false, brotli: false, - limit: '112 KB', + limit: '113 KB', }, { name: '@sentry/browser - ES6 CDN Bundle (minified & uncompressed)', diff --git a/packages/node-experimental/test/integration/transactions.test.ts b/packages/node-experimental/test/integration/transactions.test.ts index d379070c4ee1..eabb11df3b62 100644 --- a/packages/node-experimental/test/integration/transactions.test.ts +++ b/packages/node-experimental/test/integration/transactions.test.ts @@ -110,6 +110,7 @@ describe('Integration | Transactions', () => { }), sampleRate: 1, source: 'task', + spanCountBeforeProcessing: 2, spanMetadata: expect.any(Object), requestPath: 'test-path', },