diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index c50403ab3903..15da128703d0 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -3,12 +3,15 @@ import { Scope, Session } from '@sentry/hub'; import { Client, ClientOptions, + DataCategory, DsnComponents, Envelope, Event, + EventDropReason, EventHint, Integration, IntegrationClass, + Outcome, SessionAggregates, Severity, SeverityLevel, @@ -87,6 +90,9 @@ export abstract class BaseClient implements Client { /** Number of calls being processed */ protected _numProcessing: number = 0; + /** Holds flushable */ + private _outcomes: { [key: string]: number } = {}; + /** * Initializes this client instance. * @@ -98,7 +104,7 @@ export abstract class BaseClient implements Client { this._dsn = makeDsn(options.dsn); const url = getEnvelopeEndpointWithUrlEncodedAuth(this._dsn, options.tunnel); this._transport = options.transport({ - recordDroppedEvent: () => undefined, // TODO(v7): Provide a proper function instead of noop + recordDroppedEvent: this.recordDroppedEvent.bind(this), ...options.transportOptions, url, }); @@ -270,7 +276,7 @@ export abstract class BaseClient implements Client { public sendEvent(event: Event): void { if (this._dsn) { const env = createEventEnvelope(event, this._dsn, this._options._metadata, this._options.tunnel); - this.sendEnvelope(env); + this._sendEnvelope(env); } } @@ -280,20 +286,26 @@ export abstract class BaseClient implements Client { public sendSession(session: Session | SessionAggregates): void { if (this._dsn) { const env = createSessionEnvelope(session, this._dsn, this._options._metadata, this._options.tunnel); - this.sendEnvelope(env); + this._sendEnvelope(env); } } /** - * @inheritdoc + * @inheritDoc */ - public sendEnvelope(env: Envelope): void { - if (this._transport && this._dsn) { - this._transport.send(env).then(null, reason => { - IS_DEBUG_BUILD && logger.error('Error while sending event:', reason); - }); - } else { - IS_DEBUG_BUILD && logger.error('Transport disabled'); + public recordDroppedEvent(reason: EventDropReason, category: DataCategory): void { + if (this._options.sendClientReports) { + // We want to track each category (error, transaction, session) 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>>>` + // With typescript 4.1 we could even use template literal types + const key = `${reason}:${category}`; + IS_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; } } @@ -565,20 +577,8 @@ export abstract class BaseClient implements Client { * @returns A SyncPromise that resolves with the event or rejects in case event was/will not be send. */ protected _processEvent(event: Event, hint?: EventHint, scope?: Scope): PromiseLike { - // eslint-disable-next-line @typescript-eslint/unbound-method const { beforeSend, sampleRate } = this.getOptions(); - // type RecordLostEvent = NonNullable; - type RecordLostEventParams = unknown[]; // Parameters; - - // no-op as new transports don't have client outcomes - // TODO(v7): Re-add this functionality - function recordLostEvent(_outcome: RecordLostEventParams[0], _category: RecordLostEventParams[1]): void { - // if (transport.recordLostEvent) { - // transport.recordLostEvent(outcome, category); - // } - } - if (!this._isEnabled()) { return rejectedSyncPromise(new SentryError('SDK not enabled, will not capture event.')); } @@ -588,7 +588,7 @@ export abstract class BaseClient implements Client { // 0.0 === 0% events are sent // Sampling for transaction happens somewhere else if (!isTransaction && typeof sampleRate === 'number' && Math.random() > sampleRate) { - recordLostEvent('sample_rate', 'event'); + this.recordDroppedEvent('sample_rate', 'error'); return rejectedSyncPromise( new SentryError( `Discarding event because it's not included in the random sample (sampling rate = ${sampleRate})`, @@ -599,7 +599,7 @@ export abstract class BaseClient implements Client { return this._prepareEvent(event, scope, hint) .then(prepared => { if (prepared === null) { - recordLostEvent('event_processor', event.type || 'event'); + this.recordDroppedEvent('event_processor', event.type || 'error'); throw new SentryError('An event processor returned null, will not send event.'); } @@ -613,7 +613,7 @@ export abstract class BaseClient implements Client { }) .then(processedEvent => { if (processedEvent === null) { - recordLostEvent('before_send', event.type || 'event'); + this.recordDroppedEvent('before_send', event.type || 'error'); throw new SentryError('`beforeSend` returned `null`, will not send event.'); } @@ -659,6 +659,35 @@ export abstract class BaseClient implements Client { ); } + /** + * @inheritdoc + */ + protected _sendEnvelope(envelope: Envelope): void { + if (this._transport && this._dsn) { + this._transport.send(envelope).then(null, reason => { + IS_DEBUG_BUILD && logger.error('Error while sending event:', reason); + }); + } else { + IS_DEBUG_BUILD && logger.error('Transport disabled'); + } + } + + /** + * Clears outcomes on this client and returns them. + */ + protected _clearOutcomes(): Outcome[] { + const outcomes = this._outcomes; + this._outcomes = {}; + return Object.keys(outcomes).map(key => { + const [reason, category] = key.split(':') as [EventDropReason, DataCategory]; + return { + reason, + category, + quantity: outcomes[key], + }; + }); + } + /** * @inheritDoc */ diff --git a/packages/core/test/lib/base.test.ts b/packages/core/test/lib/base.test.ts index feb18390c3e8..ee873f504d43 100644 --- a/packages/core/test/lib/base.test.ts +++ b/packages/core/test/lib/base.test.ts @@ -1061,30 +1061,24 @@ describe('BaseClient', () => { expect((TestClient.instance!.event! as any).data).toBe('someRandomThing'); }); - // TODO(v7): Add back test with client reports - // test('beforeSend records dropped events', () => { - // expect.assertions(1); - - // const client = new TestClient( - // getDefaultTestClientOptions({ - // dsn: PUBLIC_DSN, - // beforeSend() { - // return null; - // }, - // }), - // ); - // const recordLostEventSpy = jest.fn(); - // jest.spyOn(client, 'getTransport').mockImplementationOnce( - // () => - // ({ - // recordLostEvent: recordLostEventSpy, - // } as any as Transport), - // ); - - // client.captureEvent({ message: 'hello' }, {}); - - // expect(recordLostEventSpy).toHaveBeenCalledWith('before_send', 'event'); - // }); + test('beforeSend records dropped events', () => { + expect.assertions(1); + + const client = new TestClient( + getDefaultTestClientOptions({ + dsn: PUBLIC_DSN, + beforeSend() { + return null; + }, + }), + ); + + const recordLostEventSpy = jest.spyOn(client, 'recordDroppedEvent'); + + client.captureEvent({ message: 'hello' }, {}); + + expect(recordLostEventSpy).toHaveBeenCalledWith('before_send', 'error'); + }); test('eventProcessor can drop the even when it returns null', () => { expect.assertions(3); @@ -1102,28 +1096,21 @@ describe('BaseClient', () => { expect(loggerErrorSpy).toBeCalledWith(new SentryError('An event processor returned null, will not send event.')); }); - // TODO(v7): Add back tests with client reports - // test('eventProcessor records dropped events', () => { - // expect.assertions(1); + test('eventProcessor records dropped events', () => { + expect.assertions(1); - // const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN }); - // const client = new TestClient(options); + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN }); + const client = new TestClient(options); - // const recordLostEventSpy = jest.fn(); - // jest.spyOn(client, 'getTransport').mockImplementationOnce( - // () => - // ({ - // recordLostEvent: recordLostEventSpy, - // } as any as Transport), - // ); + const recordLostEventSpy = jest.spyOn(client, 'recordDroppedEvent'); - // const scope = new Scope(); - // scope.addEventProcessor(() => null); + const scope = new Scope(); + scope.addEventProcessor(() => null); - // client.captureEvent({ message: 'hello' }, {}, scope); + client.captureEvent({ message: 'hello' }, {}, scope); - // expect(recordLostEventSpy).toHaveBeenCalledWith('event_processor', 'event'); - // }); + expect(recordLostEventSpy).toHaveBeenCalledWith('event_processor', 'error'); + }); test('eventProcessor sends an event and logs when it crashes', () => { expect.assertions(3); @@ -1154,24 +1141,17 @@ describe('BaseClient', () => { ); }); - // TODO(v7): Add back test with client reports - // test('records events dropped due to sampleRate', () => { - // expect.assertions(1); + test('records events dropped due to sampleRate', () => { + expect.assertions(1); - // const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, sampleRate: 0 }); - // const client = new TestClient(options); + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, sampleRate: 0 }); + const client = new TestClient(options); - // const recordLostEventSpy = jest.fn(); - // jest.spyOn(client, 'getTransport').mockImplementationOnce( - // () => - // ({ - // recordLostEvent: recordLostEventSpy, - // } as any as Transport), - // ); + const recordLostEventSpy = jest.spyOn(client, 'recordDroppedEvent'); - // client.captureEvent({ message: 'hello' }, {}); - // expect(recordLostEventSpy).toHaveBeenCalledWith('sample_rate', 'event'); - // }); + client.captureEvent({ message: 'hello' }, {}); + expect(recordLostEventSpy).toHaveBeenCalledWith('sample_rate', 'error'); + }); }); describe('integrations', () => { @@ -1366,4 +1346,69 @@ describe('BaseClient', () => { expect(TestClient.instance!.session).toBeUndefined(); }); }); + + describe('recordDroppedEvent()/_clearOutcomes()', () => { + test('record and return outcomes', () => { + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN }); + const client = new TestClient(options); + + client.recordDroppedEvent('ratelimit_backoff', 'error'); + client.recordDroppedEvent('ratelimit_backoff', 'error'); + client.recordDroppedEvent('network_error', 'transaction'); + client.recordDroppedEvent('network_error', 'transaction'); + client.recordDroppedEvent('before_send', 'session'); + client.recordDroppedEvent('event_processor', 'attachment'); + client.recordDroppedEvent('network_error', 'transaction'); + + const clearedOutcomes = client._clearOutcomes(); + + expect(clearedOutcomes).toEqual( + expect.arrayContaining([ + { + reason: 'ratelimit_backoff', + category: 'error', + quantity: 2, + }, + { + reason: 'network_error', + category: 'transaction', + quantity: 3, + }, + { + reason: 'before_send', + category: 'session', + quantity: 1, + }, + { + reason: 'event_processor', + category: 'attachment', + quantity: 1, + }, + ]), + ); + }); + + test('to clear outcomes', () => { + const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN }); + const client = new TestClient(options); + + client.recordDroppedEvent('ratelimit_backoff', 'error'); + client.recordDroppedEvent('ratelimit_backoff', 'error'); + client.recordDroppedEvent('event_processor', 'attachment'); + + const clearedOutcomes1 = client._clearOutcomes(); + expect(clearedOutcomes1.length).toEqual(2); + + const clearedOutcomes2 = client._clearOutcomes(); + expect(clearedOutcomes2.length).toEqual(0); + + client.recordDroppedEvent('network_error', 'attachment'); + + const clearedOutcomes3 = client._clearOutcomes(); + expect(clearedOutcomes3.length).toEqual(1); + + const clearedOutcomes4 = client._clearOutcomes(); + expect(clearedOutcomes4.length).toEqual(0); + }); + }); }); diff --git a/packages/core/test/mocks/client.ts b/packages/core/test/mocks/client.ts index 6878a649bf17..d46eb000d480 100644 --- a/packages/core/test/mocks/client.ts +++ b/packages/core/test/mocks/client.ts @@ -1,18 +1,15 @@ import { Session } from '@sentry/hub'; -import { ClientOptions, Event, Integration, Severity, SeverityLevel } from '@sentry/types'; +import { ClientOptions, Event, Integration, Outcome, Severity, SeverityLevel } from '@sentry/types'; import { resolvedSyncPromise } from '@sentry/utils'; import { BaseClient } from '../../src/baseclient'; import { initAndBind } from '../../src/sdk'; import { createTransport } from '../../src/transports/base'; -// TODO(v7): Add client reports tests to this file -// See old tests in packages/browser/test/unit/transports/base.test.ts -// from https://github.com/getsentry/sentry-javascript/pull/4967 - export function getDefaultTestClientOptions(options: Partial = {}): TestClientOptions { return { integrations: [], + sendClientReports: true, transport: () => createTransport( { recordDroppedEvent: () => undefined }, // noop @@ -79,6 +76,11 @@ export class TestClient extends BaseClient { public sendSession(session: Session): void { this.session = session; } + + // Public proxy for protected method + public _clearOutcomes(): Outcome[] { + return super._clearOutcomes(); + } } export function init(options: TestClientOptions): void { diff --git a/packages/tracing/src/transaction.ts b/packages/tracing/src/transaction.ts index 3e3c148fd5d3..0001589349ca 100644 --- a/packages/tracing/src/transaction.ts +++ b/packages/tracing/src/transaction.ts @@ -103,12 +103,11 @@ export class Transaction extends SpanClass implements TransactionInterface { // At this point if `sampled !== true` we want to discard the transaction. IS_DEBUG_BUILD && logger.log('[Tracing] Discarding transaction because its trace was not chosen to be sampled.'); - // TODO(v7): Add back client reports functionality - // const client = this._hub.getClient(); - // const transport = client && client.getTransport && client.getTransport(); - // if (transport && transport.recordLostEvent) { - // transport.recordLostEvent('sample_rate', 'transaction'); - // } + const client = this._hub.getClient(); + if (client) { + client.recordDroppedEvent('sample_rate', 'transaction'); + } + return undefined; } diff --git a/packages/tracing/test/idletransaction.test.ts b/packages/tracing/test/idletransaction.test.ts index e6035ad491a1..bf4d5d51af41 100644 --- a/packages/tracing/test/idletransaction.test.ts +++ b/packages/tracing/test/idletransaction.test.ts @@ -163,19 +163,18 @@ describe('IdleTransaction', () => { } }); - // TODO(v7): Add with client reports - // it('should record dropped transactions', async () => { - // const transaction = new IdleTransaction({ name: 'foo', startTimestamp: 1234, sampled: false }, hub, 1000); + it('should record dropped transactions', async () => { + const transaction = new IdleTransaction({ name: 'foo', startTimestamp: 1234, sampled: false }, hub, 1000); - // const transport = hub.getClient()!.getTransport!(); + const client = hub.getClient()!; - // const spy = jest.spyOn(transport, 'recordLostEvent'); + const recordDroppedEventSpy = jest.spyOn(client, 'recordDroppedEvent'); - // transaction.initSpanRecorder(10); - // transaction.finish(transaction.startTimestamp + 10); + transaction.initSpanRecorder(10); + transaction.finish(transaction.startTimestamp + 10); - // expect(spy).toHaveBeenCalledWith('sample_rate', 'transaction'); - // }); + expect(recordDroppedEventSpy).toHaveBeenCalledWith('sample_rate', 'transaction'); + }); describe('_initTimeout', () => { it('finishes if no activities are added to the transaction', () => { diff --git a/packages/types/src/client.ts b/packages/types/src/client.ts index 9b91c6712e06..3425006e645d 100644 --- a/packages/types/src/client.ts +++ b/packages/types/src/client.ts @@ -1,3 +1,5 @@ +import { EventDropReason } from './clientreport'; +import { DataCategory } from './datacategory'; import { DsnComponents } from './dsn'; import { Event, EventHint } from './event'; import { Integration, IntegrationClass } from './integration'; @@ -54,7 +56,8 @@ export interface Client { */ captureEvent(event: Event, hint?: EventHint, scope?: Scope): string | undefined; - /** Captures a session + /** + * Captures a session * * @param session Session to be delivered */ @@ -117,4 +120,12 @@ export interface Client { /** Submits the session to Sentry */ sendSession(session: Session | SessionAggregates): void; + + /** + * Record on the client that an event got dropped (ie, an event that will not be sent to sentry). + * + * @param reason The reason why the event got dropped. + * @param category The data category of the dropped event. + */ + recordDroppedEvent(reason: EventDropReason, category: DataCategory): void; } diff --git a/packages/types/src/clientreport.ts b/packages/types/src/clientreport.ts index 1f59ad4910a3..79b3c2a4454c 100644 --- a/packages/types/src/clientreport.ts +++ b/packages/types/src/clientreport.ts @@ -8,7 +8,13 @@ export type EventDropReason = | 'ratelimit_backoff' | 'sample_rate'; +export type Outcome = { + reason: EventDropReason; + category: DataCategory; + quantity: number; +}; + export type ClientReport = { timestamp: number; - discarded_events: Array<{ reason: EventDropReason; category: DataCategory; quantity: number }>; + discarded_events: Outcome[]; }; diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 545a894a81e7..2fbcf4bbed6b 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -1,6 +1,6 @@ export type { Breadcrumb, BreadcrumbHint } from './breadcrumb'; export type { Client } from './client'; -export type { ClientReport, EventDropReason } from './clientreport'; +export type { ClientReport, Outcome, EventDropReason } from './clientreport'; export type { Context, Contexts } from './context'; export type { DataCategory } from './datacategory'; export type { DsnComponents, DsnLike, DsnProtocol } from './dsn';