ref: Record dropped events in BaseClient

Author: Luca Forstner

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<O extends ClientOptions> implements Client<O> {
   /** 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<O extends ClientOptions> implements Client<O> {
       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,
       });
@@ -284,6 +290,23 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
     }
   }
 
+  /**
+   * @inheritDoc
+   */
+  public recordDroppedEvent(reason: EventDropReason, category: DataCategory): void {
+    // 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<Record<SentryRequestType, Partial<Record<Outcome, number>>>>`
+    // 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;
+  }
+
   /** Updates existing session based on the provided event */
   protected _updateSessionFromEvent(session: Session, event: Event): void {
     let crashed = false;
@@ -552,20 +575,8 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
    * @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<Event> {
-    // eslint-disable-next-line @typescript-eslint/unbound-method
     const { beforeSend, sampleRate } = this.getOptions();
 
-    // type RecordLostEvent = NonNullable<Transport['recordLostEvent']>;
-    type RecordLostEventParams = unknown[]; // Parameters<RecordLostEvent>;
-
-    // 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.'));
     }
@@ -575,7 +586,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
     // 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})`,
@@ -586,7 +597,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
     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.');
         }
 
@@ -600,7 +611,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
       })
       .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.');
         }
 
@@ -649,7 +660,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
   /**
    * @inheritdoc
    */
-  private _sendEnvelope(envelope: Envelope): void {
+  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);
@@ -659,6 +670,22 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
     }
   }
 
+  /**
+   * 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
    */

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);
+    });
+  });
 });

packages/core/test/mocks/client.ts

@@ -1,15 +1,11 @@
 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> = {}): TestClientOptions {
   return {
     integrations: [],
@@ -79,6 +75,11 @@ export class TestClient extends BaseClient<TestClientOptions> {
   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 {

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;
     }