fix: issue where the same logs would get sent multiple times (#8)

* fix: issue where the same logs would get sent multiple times

This is caused by us only clearing out the queue when the metrics
response came back

* Prettier

* fix: test coverage

* test: improving the tests for queue flushing

* test: delaying for 1s, not 1ms

* fix: removing an unused test callback that was put in place for mocha

* test: making requests slightly more unique to avoid test date flakiness

Co-authored-by: Dom Harrington <[email protected]>

Author: erunion

packages/node/__tests__/index.test.js

@@ -1,12 +1,14 @@
 const express = require('express');
 const request = require('supertest');
 const nock = require('nock');
+const crypto = require('crypto');
 const { isValidUUIDV4 } = require('is-valid-uuid-v4');
 const config = require('../config');
 
 const middleware = require('..');
 
-const apiKey = 'OUW3RlI4gUCwWGpO10srIo2ufdWmMhMH';
+const apiKey = 'fakeApiKey';
+const group = '5afa21b97011c63320226ef3';
 
 expect.extend({
   toHaveLogHeader(res) {
@@ -50,8 +52,6 @@ describe('#metrics', () => {
   });
 
   it('should send a request to the metrics server', async function test() {
-    const group = '5afa21b97011c63320226ef3';
-
     const mock = nock(config.host)
       .post('/v1/request', ([body]) => {
         expect(body.group).toBe(group);
@@ -75,8 +75,6 @@ describe('#metrics', () => {
 
   describe('#bufferLength', () => {
     it('should send requests when number hits `bufferLength` size', async function test() {
-      const group = '5afa21b97011c63320226ef3';
-
       const mock = nock(config.host)
         .post('/v1/request', body => {
           expect(body).toHaveLength(3);
@@ -123,6 +121,47 @@ describe('#metrics', () => {
       expect(mock.isDone()).toBe(true);
       mock.done();
     });
+
+    it('should clear out the queue when sent', () => {
+      const numberOfLogs = 20;
+      const numberOfMocks = 4;
+      const bufferLength = numberOfLogs / numberOfMocks;
+
+      const seenLogs = [];
+
+      const mocks = [...new Array(numberOfMocks).keys()].map(() =>
+        nock(config.host)
+          .post('/v1/request', body => {
+            expect(body).toHaveLength(bufferLength);
+
+            // Ensure that our executed requests and the buffered queue they're in remain unique.
+            body.forEach(req => {
+              const requestHash = crypto.createHash('md5').update(JSON.stringify(req)).digest('hex');
+              expect(seenLogs).not.toContain(requestHash);
+              seenLogs.push(requestHash);
+            });
+
+            return true;
+          })
+          // This is the important part of this test,
+          // the delay mimics the latency of a real
+          // HTTP request
+          .delay(1000)
+          .reply(200)
+      );
+
+      const app = express();
+      app.use(middleware.metrics(apiKey, () => group, { bufferLength }));
+      app.get('/test', (req, res) => res.sendStatus(200));
+
+      return Promise.all(
+        [...new Array(numberOfLogs).keys()].map(i => {
+          return request(app).get(`/test?log=${i}`).expect(200);
+        })
+      ).then(() => {
+        mocks.map(mock => mock.done());
+      });
+    });
   });
 
   describe('`res._body`', () => {
@@ -139,7 +178,7 @@ describe('#metrics', () => {
     it('should buffer up res.write() calls', async function test() {
       const mock = createMock();
       const app = express();
-      app.use(middleware.metrics(apiKey, () => '123'));
+      app.use(middleware.metrics(apiKey, () => group));
       app.get('/test', (req, res) => {
         res.write('{"a":1,');
         res.write('"b":2,');
@@ -155,7 +194,7 @@ describe('#metrics', () => {
     it('should buffer up res.end() calls', async function test() {
       const mock = createMock();
       const app = express();
-      app.use(middleware.metrics(apiKey, () => '123'));
+      app.use(middleware.metrics(apiKey, () => group));
       app.get('/test', (req, res) => res.end(JSON.stringify(responseBody)));
 
       await request(app)
@@ -169,7 +208,7 @@ describe('#metrics', () => {
     it('should work for res.send() calls', async function test() {
       const mock = createMock();
       const app = express();
-      app.use(middleware.metrics(apiKey, () => '123'));
+      app.use(middleware.metrics(apiKey, () => group));
       app.get('/test', (req, res) => res.send(responseBody));
 
       await request(app)

packages/node/config/default.json

@@ -1,5 +1,4 @@
 {
   "host": "https://metrics.readme.io",
-  "readmeUrl": "https://dash.readme.io",
   "bufferLength": 1
 }

packages/node/index.js

@@ -49,14 +49,12 @@ module.exports.metrics = (apiKey, group, options = {}) => {
       const payload = constructPayload(req, res, group, options, { logId, startedDateTime });
       queue.push(payload);
       if (queue.length >= bufferLength) {
-        request
-          .post(`${config.host}/v1/request`, {
-            headers: { authorization: `Basic ${encoded}` },
-            json: queue,
-          })
-          .response.then(() => {
-            queue = [];
-          });
+        const json = queue.slice();
+        queue = [];
+        request.post(`${config.host}/v1/request`, {
+          headers: { authorization: `Basic ${encoded}` },
+          json,
+        });
       }
 
       cleanup(); // eslint-disable-line no-use-before-define