ref: Record dropped events in BaseClient (#5017)

Author: lforst

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,
       });
@@ -270,7 +276,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
   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<O extends ClientOptions> implements Client<O> {
   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<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;
     }
   }
 
@@ -565,20 +577,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.'));
     }
@@ -588,7 +588,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})`,
@@ -599,7 +599,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.');
         }
 
@@ -613,7 +613,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.');
         }
 
@@ -659,6 +659,35 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
     );
   }
 
+  /**
+   * @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
    */

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(loggerWarnSpy).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,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> = {}): TestClientOptions {
   return {
     integrations: [],
+    sendClientReports: true,
     transport: () =>
       createTransport(
         { recordDroppedEvent: () => undefined }, // noop
@@ -79,6 +76,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 {