Skip to content

Commit

Permalink
backport(tracing): Report dropped spans for transactions (#13343)
Browse files Browse the repository at this point in the history
Co-authored-by: Francesco Novy <francesco.novy@sentry.io>
  • Loading branch information
krystofwoldrich and mydea authored Aug 13, 2024
1 parent eeae3cf commit f54c97a
Show file tree
Hide file tree
Showing 4 changed files with 87 additions and 10 deletions.
4 changes: 2 additions & 2 deletions .size-limit.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)',
Expand All @@ -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)',
Expand Down
41 changes: 34 additions & 7 deletions packages/core/src/baseclient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -419,21 +419,21 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
/**
* @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.
// A correct type for map-based implementation if we want to go that route
// would be `Partial<Record<SentryRequestType, Partial<Record<Outcome, number>>>>`
// 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;
}
}

Expand Down Expand Up @@ -778,6 +778,12 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
.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');
}

Expand All @@ -786,6 +792,18 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
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
Expand Down Expand Up @@ -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);
}

Expand Down
51 changes: 50 additions & 1 deletion packages/core/test/lib/base.test.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ describe('Integration | Transactions', () => {
}),
sampleRate: 1,
source: 'task',
spanCountBeforeProcessing: 2,
spanMetadata: expect.any(Object),
requestPath: 'test-path',
},
Expand Down

0 comments on commit f54c97a

Please sign in to comment.