feat: Add simple request queue with close function (#1504)

HazAT · kamilogorek · commit 551021fff157 · 2018-09-03T15:45:06.000+02:00
* ref: Rename send on transport to captureEvent
* feat: Add basic queue implementation
* feat: Add queue to backends and client + reason of skip
* fix: Tests
* fix: Typo
* fix: Integration tests
* Fixed integration tests
* fix: Rename queue to transportbuffer
diff --git a/packages/browser/src/backend.ts b/packages/browser/src/backend.ts
@@ -1,4 +1,4 @@
-import { Backend, logger, Options, SentryError } from '@sentry/core';
+import { Backend, logger, Options, SentryError, TransportBuffer } from '@sentry/core';
 import { SentryEvent, SentryEventHint, SentryResponse, Severity, Status, Transport } from '@sentry/types';
 import { isDOMError, isDOMException, isError, isErrorEvent, isPlainObject } from '@sentry/utils/is';
 import { supportsBeacon, supportsFetch } from '@sentry/utils/supports';
@@ -40,6 +40,9 @@ export class BrowserBackend implements Backend {
   /** Cached transport used internally. */
   private transport?: Transport;
 
+  /** A simple buffer holding all requests. */
+  private readonly buffer: TransportBuffer<SentryResponse> = new TransportBuffer();
+
   /**
    * @inheritDoc
    */
@@ -141,8 +144,8 @@ export class BrowserBackend implements Backend {
   public async sendEvent(event: SentryEvent): Promise<SentryResponse> {
     if (!this.options.dsn) {
       logger.warn(`Event has been skipped because no Dsn is configured.`);
-      // We do nothing in case there is no Dsn
-      return { status: Status.Skipped };
+      // We do nothing in case there is no DSN
+      return { status: Status.Skipped, reason: `Event has been skipped because no Dsn is configured.` };
     }
 
     if (!this.transport) {
@@ -161,7 +164,7 @@ export class BrowserBackend implements Backend {
       }
     }
 
-    return this.transport.send(event);
+    return this.buffer.add(this.transport.captureEvent(event));
   }
 
   /**
@@ -177,4 +180,11 @@ export class BrowserBackend implements Backend {
   public storeScope(): void {
     // Noop
   }
+
+  /**
+   * @inheritDoc
+   */
+  public async close(timeout?: number): Promise<boolean> {
+    return this.buffer.drain(timeout);
+  }
 }
diff --git a/packages/browser/src/transports/base.ts b/packages/browser/src/transports/base.ts
@@ -15,7 +15,7 @@ export abstract class BaseTransport implements Transport {
   /**
    * @inheritDoc
    */
-  public async send(_: SentryEvent): Promise<SentryResponse> {
-    throw new SentryError('Transport Class has to implement `send` method');
+  public async captureEvent(_: SentryEvent): Promise<SentryResponse> {
+    throw new SentryError('Transport Class has to implement `captureEvent` method');
   }
 }
diff --git a/packages/browser/src/transports/beacon.ts b/packages/browser/src/transports/beacon.ts
@@ -10,7 +10,7 @@ export class BeaconTransport extends BaseTransport {
   /**
    * @inheritDoc
    */
-  public async send(event: SentryEvent): Promise<SentryResponse> {
+  public async captureEvent(event: SentryEvent): Promise<SentryResponse> {
     const data = serialize(event);
 
     const result = global.navigator.sendBeacon(this.url, data);
diff --git a/packages/browser/src/transports/fetch.ts b/packages/browser/src/transports/fetch.ts
@@ -11,7 +11,7 @@ export class FetchTransport extends BaseTransport {
   /**
    * @inheritDoc
    */
-  public async send(event: SentryEvent): Promise<SentryResponse> {
+  public async captureEvent(event: SentryEvent): Promise<SentryResponse> {
     const defaultOptions: RequestInit = {
       body: serialize(event),
       method: 'POST',
diff --git a/packages/browser/src/transports/xhr.ts b/packages/browser/src/transports/xhr.ts
@@ -7,7 +7,7 @@ export class XHRTransport extends BaseTransport {
   /**
    * @inheritDoc
    */
-  public async send(event: SentryEvent): Promise<SentryResponse> {
+  public async captureEvent(event: SentryEvent): Promise<SentryResponse> {
     return new Promise<SentryResponse>((resolve, reject) => {
       const request = new XMLHttpRequest();
 
diff --git a/packages/browser/test/backend.test.ts b/packages/browser/test/backend.test.ts
@@ -4,7 +4,7 @@ import { BrowserBackend } from '../src/backend';
 import { BaseTransport } from '../src/transports';
 
 class SimpleTransport extends BaseTransport {
-  public async send(_: SentryEvent): Promise<SentryResponse> {
+  public async captureEvent(_: SentryEvent): Promise<SentryResponse> {
     return {
       status: Status.fromHttpCode(200),
     };
@@ -34,7 +34,7 @@ describe('BrowserBackend', () => {
       }
     });
 
-    it('should call send() on provided transport', async () => {
+    it('should call captureEvent() on provided transport', async () => {
       backend = new BrowserBackend({ dsn, transport: SimpleTransport });
       const status = await backend.sendEvent(testEvent);
       expect(status.status).equal(Status.Success);
diff --git a/packages/browser/test/integration/frame.html b/packages/browser/test/integration/frame.html
@@ -110,19 +110,25 @@
 
     // stub transport so we don't actually transmit any data
     function DummyTransport() { }
-    DummyTransport.prototype.send = function (event) {
+    DummyTransport.prototype.captureEvent = function (event) {
       // console.log(JSON.stringify(event, null, 2));
       sentryData.push(event);
-      return {
+      return Promise.resolve({
         status: 'success'
-      }
+      })
     }
 
     Sentry.init({
       dsn: 'https://public@example.com/1',
       attachStacktrace: true,
       transport: DummyTransport,
       beforeBreadcrumb: function (breadcrumb) {
+        // Filter console logs as we use them for debugging *a lot* and they are not *that* important
+        if (breadcrumb.category === 'console') { return null }
+
+        // overlyComplicatedDebuggingMechanism 'aka' console.log driven debugging
+        // console.log(JSON.stringify(breadcrumb, null, 2));
+
         // Filter internal Karma requests
         if (
           breadcrumb.type === 'http' &&
diff --git a/packages/browser/test/integration/test.js b/packages/browser/test/integration/test.js
@@ -766,7 +766,7 @@ describe('integration', function() {
         done,
         function() {
           var xhr = new XMLHttpRequest();
-          xhr.open('GET', 'https://public@example.com/1/store');
+          xhr.open('GET', 'https://example.com/api/1/store/');
           xhr.send('{"message":"someMessage","level":"warning"}');
           setTimeout(done);
         },
diff --git a/packages/browser/test/transports/base.test.ts b/packages/browser/test/transports/base.test.ts
@@ -6,13 +6,13 @@ const testDsn = 'https://123@sentry.io/42';
 class SimpleTransport extends BaseTransport {}
 
 describe('BaseTransport', () => {
-  it('doesnt provide send() implementation', async () => {
+  it('doesnt provide captureEvent() implementation', async () => {
     const transport = new SimpleTransport({ dsn: testDsn });
 
     try {
-      await transport.send({});
+      await transport.captureEvent({});
     } catch (e) {
-      expect(e.message).equal('Transport Class has to implement `send` method');
+      expect(e.message).equal('Transport Class has to implement `captureEvent` method');
     }
   });
 
diff --git a/packages/browser/test/transports/beacon.test.ts b/packages/browser/test/transports/beacon.test.ts
@@ -29,11 +29,11 @@ describe('BeaconTransport', () => {
     expect(transport.url).equal(transportUrl);
   });
 
-  describe('send()', async () => {
+  describe('captureEvent()', async () => {
     it('sends a request to Sentry servers', async () => {
       sendBeacon.returns(true);
 
-      return transport.send(payload).then(res => {
+      return transport.captureEvent(payload).then(res => {
         expect(res.status).equal(Status.Success);
         expect(sendBeacon.calledOnce).equal(true);
         expect(sendBeacon.calledWith(transportUrl, JSON.stringify(payload))).equal(true);
@@ -43,7 +43,7 @@ describe('BeaconTransport', () => {
     it('rejects with failed status', async () => {
       sendBeacon.returns(false);
 
-      return transport.send(payload).catch(res => {
+      return transport.captureEvent(payload).catch(res => {
         expect(res.status).equal(Status.Failed);
         expect(sendBeacon.calledOnce).equal(true);
         expect(sendBeacon.calledWith(transportUrl, JSON.stringify(payload))).equal(true);
diff --git a/packages/browser/test/transports/fetch.test.ts b/packages/browser/test/transports/fetch.test.ts
@@ -29,13 +29,13 @@ describe('FetchTransport', () => {
     expect(transport.url).equal(transportUrl);
   });
 
-  describe('send()', async () => {
+  describe('captureEvent()', async () => {
     it('sends a request to Sentry servers', async () => {
       const response = { status: 200 };
 
       fetch.returns(Promise.resolve(response));
 
-      return transport.send(payload).then(res => {
+      return transport.captureEvent(payload).then(res => {
         expect(res.status).equal(Status.Success);
         expect(fetch.calledOnce).equal(true);
         expect(
@@ -53,7 +53,7 @@ describe('FetchTransport', () => {
 
       fetch.returns(Promise.reject(response));
 
-      return transport.send(payload).catch(res => {
+      return transport.captureEvent(payload).catch(res => {
         expect(res.status).equal(403);
         expect(fetch.calledOnce).equal(true);
         expect(
diff --git a/packages/browser/test/transports/xhr.test.ts b/packages/browser/test/transports/xhr.test.ts
@@ -30,11 +30,11 @@ describe('XHRTransport', () => {
     expect(transport.url).equal(transportUrl);
   });
 
-  describe('send()', async () => {
+  describe('captureEvent()', async () => {
     it('sends a request to Sentry servers', async () => {
       server.respondWith('POST', transportUrl, [200, {}, '']);
 
-      return transport.send(payload).then(res => {
+      return transport.captureEvent(payload).then(res => {
         expect(res.status).equal(Status.Success);
         const request = server.requests[0];
         expect(server.requests.length).equal(1);
@@ -46,7 +46,7 @@ describe('XHRTransport', () => {
     it('rejects with non-200 status code', done => {
       server.respondWith('POST', transportUrl, [403, {}, '']);
 
-      transport.send(payload).catch(res => {
+      transport.captureEvent(payload).catch(res => {
         expect(res.status).equal(403);
 
         const request = server.requests[0];
diff --git a/packages/core/src/base.ts b/packages/core/src/base.ts
@@ -318,4 +318,11 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
 
     return response;
   }
+
+  /**
+   * @inheritDoc
+   */
+  public async close(timeout?: number): Promise<boolean> {
+    return this.backend.close(timeout);
+  }
 }
diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts
@@ -5,5 +5,6 @@ export { API } from './api';
 export { BackendClass, BaseClient } from './base';
 export { Dsn } from './dsn';
 export { SentryError } from './error';
+export { TransportBuffer } from './transportbuffer';
 export { Backend, Client, LogLevel, Options } from './interfaces';
 export { initAndBind, ClientClass } from './sdk';
diff --git a/packages/core/src/interfaces.ts b/packages/core/src/interfaces.ts
@@ -196,6 +196,15 @@ export interface Client<O extends Options = Options> {
 
   /** Returns the current options. */
   getOptions(): O;
+
+  /**
+   * A promise that resolves whenever the transport buffer is empty.
+   * If you provide a timeout and the buffer takes longer to drain the promise returns false.
+   * Calls {@link Backend.close}.
+   *
+   * @param timeout Maximum time in ms the client should wait.
+   */
+  close(timeout?: number): Promise<boolean>;
 }
 
 /**
@@ -256,4 +265,12 @@ export interface Backend {
    * @param scope The scope to store.
    */
   storeScope(scope: Scope): void;
+
+  /**
+   * A promise that resolves whenever the transport buffer is empty.
+   * If you provide a timeout and the buffer takes longer to drain the promise returns false.
+   *
+   * @param timeout Maximum time in ms the client should wait.
+   */
+  close(timeout?: number): Promise<boolean>;
 }
diff --git a/packages/core/src/transportbuffer.ts b/packages/core/src/transportbuffer.ts
@@ -0,0 +1,48 @@
+/** A simple queue that holds promises. */
+export class TransportBuffer<T> {
+  /** Internal set of queued Promises */
+  private readonly buffer: Set<Promise<T>> = new Set();
+
+  /**
+   * Add a promise to the queue.
+   *
+   * @param task Can be any Promise<T>
+   * @returns The original promise.
+   */
+  public async add(task: Promise<T>): Promise<T> {
+    this.buffer.add(task);
+    task.then(() => this.buffer.delete(task)).catch(() => this.buffer.delete(task));
+    return task;
+  }
+
+  /**
+   * This function returns the number of unresolved promises in the queue.
+   */
+  public length(): number {
+    return this.buffer.size;
+  }
+
+  /**
+   * This will drain the whole queue, returns true if queue is empty or drained.
+   * If timeout is provided and the queue takes longer to drain, the promise still resolves but with false.
+   *
+   * @param timeout Number in ms to wait until it resolves with false.
+   */
+  public async drain(timeout?: number): Promise<boolean> {
+    return new Promise<boolean>(resolve => {
+      const capturedSetTimeout = setTimeout(() => {
+        if (timeout && timeout > 0) {
+          resolve(false);
+        }
+      }, timeout);
+      Promise.all(this.buffer.values())
+        .then(() => {
+          clearTimeout(capturedSetTimeout);
+          resolve(true);
+        })
+        .catch(() => {
+          resolve(true);
+        });
+    });
+  }
+}
diff --git a/packages/core/test/lib/transportbuffer.test.ts b/packages/core/test/lib/transportbuffer.test.ts
diff --git a/packages/node/src/backend.ts b/packages/node/src/backend.ts
diff --git a/packages/node/src/transports/base.ts b/packages/node/src/transports/base.ts
diff --git a/packages/node/src/transports/http.ts b/packages/node/src/transports/http.ts
diff --git a/packages/node/src/transports/https.ts b/packages/node/src/transports/https.ts
diff --git a/packages/node/test/index.test.ts b/packages/node/test/index.test.ts
diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts

Original file line number	Diff line number	Diff line change
`@@ -15,7 +15,7 @@ export abstract class BaseTransport implements Transport {`
`15`	`15`	`/**`
`16`	`16`	`* @inheritDoc`
`17`	`17`	`*/`
`18`		`- public async send(_: SentryEvent): Promise<SentryResponse> {`
`19`		- throw new SentryError('Transport Class has to implement `send` method');
	`18`	`+ public async captureEvent(_: SentryEvent): Promise<SentryResponse> {`
	`19`	+ throw new SentryError('Transport Class has to implement `captureEvent` method');
`20`	`20`	`}`
`21`	`21`	`}`