From f54c97aff0b0df6de0872b01c24a5e8afef2e2db Mon Sep 17 00:00:00 2001 From: Krystof Woldrich <31292499+krystofwoldrich@users.noreply.github.com> Date: Tue, 13 Aug 2024 14:06:17 +0200 Subject: [PATCH] backport(tracing): Report dropped spans for transactions (#13343) Co-authored-by: Francesco Novy --- .size-limit.js | 4 +- packages/core/src/baseclient.ts | 41 ++++++++++++--- packages/core/test/lib/base.test.ts | 51 ++++++++++++++++++- .../test/integration/transactions.test.ts | 1 + 4 files changed, 87 insertions(+), 10 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/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 5a5c748b4282..5f10bfb2ff02 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 @@ -924,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 855ea73da056..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'; @@ -1004,6 +1004,55 @@ describe('BaseClient', () => { expect(TestClient.instance!.event!.transaction).toBe('/adopt/dont/shop'); }); + test('calls `beforeSendTransaction` and drops spans', () => { + const beforeSendTransaction = jest.fn(event => { + event.spans = [new CoreSpan({ spanId: 'span5', traceId: 'trace1', startTimestamp: 1234 })]; + return event; + }); + 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(TestClient.instance!.event!.spans?.length).toBe(1); + + 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); 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', },