Remove Unnecessary Delay (#25897)

This adds logic so that the code will wait until any previous
attempt to send telemetry causes follow-up attempts to wait until
it has completed sending (in the unlikely event that it takes
longer than a minute, we don't want to waste time / resources).

x-pack/plugins/xpack_main/public/hacks/__tests__/telemetry.js

@@ -4,122 +4,12 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import expect from 'expect.js';
-import sinon from 'sinon';
-import { Telemetry } from '../telemetry';
 import { uiModules } from 'ui/modules';
 
+// This overrides settings for other UI tests
 uiModules.get('kibana')
   // disable stat reporting while running tests,
   // MockInjector used in these tests is not impacted
   .constant('telemetryEnabled', false)
   .constant('telemetryOptedIn', null)
   .constant('telemetryUrl', 'not.a.valid.url.0');
-
-const getMockInjector = ({ allowReport, lastReport }) => {
-  const get = sinon.stub();
-  get.withArgs('localStorage').returns({
-    get: sinon.stub().returns({ lastReport: lastReport }),
-    set: sinon.stub()
-  });
-  get.withArgs('telemetryOptedIn').returns(allowReport);
-  const mockHttp = (req) => {
-    return req;
-  };
-  mockHttp.get = (url) => Promise.resolve({
-    data: [
-      url,
-      { info: true }
-    ]
-  });
-
-  get.withArgs('$http').returns(mockHttp);
-  get.withArgs('telemetryUrl').returns('https://testo.com/');
-
-  return { get };
-};
-const mockFetchTelemetry = () => Promise.resolve({
-  data: [
-    { cluster_uuid: 'fake-123', },
-    { cluster_uuid: 'fake-456' }
-  ]
-});
-
-describe('telemetry class', () => {
-  it('start method for beginning a timer', () => {
-    const sender = new Telemetry(getMockInjector({ allowReport: true }), mockFetchTelemetry);
-    expect(sender.start).to.be.a('function');
-  });
-
-  // call the private method
-  describe('should send a report', () => {
-    it('never reported before', () => {
-      const sender = new Telemetry(
-        getMockInjector({ allowReport: true }),
-        mockFetchTelemetry
-      );
-      return sender._sendIfDue()
-        .then(result => {
-          expect(result).to.eql([
-            {
-              method: 'POST',
-              url: 'https://testo.com/',
-              data: { cluster_uuid: 'fake-123' },
-              kbnXsrfToken: false
-            },
-            {
-              method: 'POST',
-              url: 'https://testo.com/',
-              data: { cluster_uuid: 'fake-456' },
-              kbnXsrfToken: false
-            }
-          ]);
-        });
-    });
-
-    it('interval check finds last report over a day ago', () => {
-      const sender = new Telemetry(
-        getMockInjector({
-          allowReport: true,
-          lastReport: (new Date()).getTime() - 86401000 // reported 1 day + 1 second ago
-        }),
-        mockFetchTelemetry
-      );
-      return sender._sendIfDue()
-        .then(result => expect(result).to.eql([
-          {
-            method: 'POST',
-            url: 'https://testo.com/',
-            data: { cluster_uuid: 'fake-123' },
-            kbnXsrfToken: false
-          },
-          {
-            method: 'POST',
-            url: 'https://testo.com/',
-            data: { cluster_uuid: 'fake-456' },
-            kbnXsrfToken: false
-          }
-        ]));
-    });
-  });
-
-  describe('should not send the report', () => {
-    it('config does not allow report', () => {
-      const sender = new Telemetry(getMockInjector({ allowReport: false }), mockFetchTelemetry);
-      return sender._sendIfDue()
-        .then(result => expect(result).to.be(null));
-    });
-
-    it('interval check finds last report less than a day ago', () => {
-      const sender = new Telemetry(
-        getMockInjector({
-          allowReport: true,
-          lastReport: (new Date()).getTime() - 82800000 // reported 23 hours ago
-        }),
-        mockFetchTelemetry
-      );
-      return sender._sendIfDue()
-        .then(result => expect(result).to.be(null));
-    });
-  });
-});

x-pack/plugins/xpack_main/public/hacks/telemetry.js

@@ -4,7 +4,6 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import Promise from 'bluebird';
 import {
   REPORT_INTERVAL_MS,
   LOCALSTORAGE_KEY,
@@ -21,35 +20,31 @@ export class Telemetry {
     this._$http = $injector.get('$http');
     this._telemetryUrl = $injector.get('telemetryUrl');
     this._telemetryOptedIn = $injector.get('telemetryOptedIn');
-    this._attributes = this._storage.get(LOCALSTORAGE_KEY) || {};
     this._fetchTelemetry = fetchTelemetry;
-  }
+    this._sending = false;
 
-  _set(key, value) {
-    this._attributes[key] = value;
-  }
-
-  _get(key) {
-    return this._attributes[key];
+    // try to load the local storage data
+    const attributes = this._storage.get(LOCALSTORAGE_KEY) || {};
+    this._lastReport = attributes.lastReport;
   }
 
   _saveToBrowser() {
-    this._storage.set(LOCALSTORAGE_KEY, this._attributes);
+    // we are the only code that manipulates this key, so it's safe to blindly overwrite the whole object
+    this._storage.set(LOCALSTORAGE_KEY, { lastReport: this._lastReport });
   }
 
-  /*
-   * Check time interval passage
+  /**
+   * Determine if we are due to send a new report.
+   *
+   * @returns {Boolean} true if a new report should be sent. false otherwise.
    */
   _checkReportStatus() {
     // check if opt-in for telemetry is enabled
     if (this._telemetryOptedIn) {
-      // If the last report is empty it means we've never sent telemetry and
-      // now is the time to send it.
-      if (!this._get('lastReport')) {
-        return true;
-      }
+      // returns NaN for any malformed or unset (null/undefined) value
+      const lastReport = parseInt(this._lastReport, 10);
       // If it's been a day since we last sent telemetry
-      if (Date.now() - parseInt(this._get('lastReport'), 10) > REPORT_INTERVAL_MS) {
+      if (isNaN(lastReport) || (Date.now() - lastReport) > REPORT_INTERVAL_MS) {
         return true;
       }
     }
@@ -57,15 +52,20 @@ export class Telemetry {
     return false;
   }
 
-  /*
+  /**
    * Check report permission and if passes, send the report
+   *
+   * @returns {Promise} Always.
    */
   _sendIfDue() {
-    if (!this._checkReportStatus()) { return Promise.resolve(null); }
+    if (this._sending || !this._checkReportStatus()) { return Promise.resolve(false); }
+
+    // mark that we are working so future requests are ignored until we're done
+    this._sending = true;
 
     return this._fetchTelemetry()
       .then(response => {
-        return response.data.map(cluster => {
+        return Promise.all(response.data.map(cluster => {
           const req = {
             method: 'POST',
             url: this._telemetryUrl,
@@ -74,25 +74,30 @@ export class Telemetry {
           // if passing data externally, then suppress kbnXsrfToken
           if (this._telemetryUrl.match(/^https/)) { req.kbnXsrfToken = false; }
           return this._$http(req);
-        });
+        }));
       })
-      .then(response => {
-        // we sent a report, so we need to record and store the current time stamp
-        this._set('lastReport', Date.now());
+      // the response object is ignored because we do not check it
+      .then(() => {
+        // we sent a report, so we need to record and store the current timestamp
+        this._lastReport = Date.now();
         this._saveToBrowser();
-        return response;
       })
-      .catch(() => {
-        // no ajaxErrorHandlers for telemetry
-        return Promise.resolve(null);
+      // no ajaxErrorHandlers for telemetry
+      .catch(() => null)
+      .then(() => {
+        this._sending = false;
+        return true; // sent, but not necessarilly successfully
       });
   }
 
-  /*
+  /**
    * Public method
+   *
+   * @returns {Number} `window.setInterval` response to allow cancelling the interval.
    */
   start() {
-    window.setInterval(() => this._sendIfDue(), 60000);
+    // continuously check if it's due time for a report
+    return window.setInterval(() => this._sendIfDue(), 60000);
   }
 
 } // end class

x-pack/plugins/xpack_main/public/hacks/telemetry.test.js

@@ -0,0 +1,307 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+import { Telemetry } from './telemetry';
+import {
+  REPORT_INTERVAL_MS,
+  LOCALSTORAGE_KEY,
+} from '../../common/constants';
+
+describe('telemetry class', () => {
+
+  const clusters = [
+    { cluster_uuid: 'fake-123' },
+    { cluster_uuid: 'fake-456' },
+  ];
+  const telemetryUrl = 'https://not.a.valid.url.0';
+  const mockFetchTelemetry = () => Promise.resolve({ data: clusters });
+  // returns a function that behaves like the injector by fetching the requested key from the object directly
+  // for example:
+  // { '$http': jest.fn() } would be how to mock the '$http' injector value
+  const mockInjectorFromObject = object => {
+    return { get: key => object[key] };
+  };
+
+  describe('constructor', () => {
+
+    test('defaults lastReport if unset', () => {
+      const injector = {
+        localStorage: {
+          get: jest.fn().mockReturnValueOnce(undefined),
+        },
+        '$http': jest.fn(),
+        telemetryOptedIn: true,
+        telemetryUrl,
+      };
+      const telemetry = new Telemetry(mockInjectorFromObject(injector), mockFetchTelemetry);
+
+      expect(telemetry._storage).toBe(injector.localStorage);
+      expect(telemetry._$http).toBe(injector.$http);
+      expect(telemetry._telemetryOptedIn).toBe(injector.telemetryOptedIn);
+      expect(telemetry._telemetryUrl).toBe(injector.telemetryUrl);
+      expect(telemetry._fetchTelemetry).toBe(mockFetchTelemetry);
+      expect(telemetry._sending).toBe(false);
+      expect(telemetry._lastReport).toBeUndefined();
+
+      expect(injector.localStorage.get).toHaveBeenCalledTimes(1);
+      expect(injector.localStorage.get).toHaveBeenCalledWith(LOCALSTORAGE_KEY);
+    });
+
+    test('uses lastReport if set', () => {
+      const lastReport = Date.now();
+      const injector = {
+        localStorage: {
+          get: jest.fn().mockReturnValueOnce({ lastReport }),
+        },
+        '$http': jest.fn(),
+        telemetryOptedIn: true,
+        telemetryUrl,
+      };
+      const telemetry = new Telemetry(mockInjectorFromObject(injector), mockFetchTelemetry);
+
+      expect(telemetry._storage).toBe(injector.localStorage);
+      expect(telemetry._$http).toBe(injector.$http);
+      expect(telemetry._telemetryOptedIn).toBe(injector.telemetryOptedIn);
+      expect(telemetry._telemetryUrl).toBe(injector.telemetryUrl);
+      expect(telemetry._fetchTelemetry).toBe(mockFetchTelemetry);
+      expect(telemetry._sending).toBe(false);
+      expect(telemetry._lastReport).toBe(lastReport);
+
+      expect(injector.localStorage.get).toHaveBeenCalledTimes(1);
+      expect(injector.localStorage.get).toHaveBeenCalledWith(LOCALSTORAGE_KEY);
+    });
+
+  });
+
+  test('_saveToBrowser uses _lastReport', () => {
+    const injector = {
+      localStorage: {
+        get: jest.fn().mockReturnValueOnce({ random: 'junk', gets: 'thrown away' }),
+        set: jest.fn(),
+      }
+    };
+    const lastReport = Date.now();
+    const telemetry = new Telemetry(mockInjectorFromObject(injector), mockFetchTelemetry);
+    telemetry._lastReport = lastReport;
+
+    telemetry._saveToBrowser();
+
+    expect(injector.localStorage.set).toHaveBeenCalledTimes(1);
+    expect(injector.localStorage.set).toHaveBeenCalledWith(LOCALSTORAGE_KEY, { lastReport });
+  });
+
+  describe('_checkReportStatus', () => {
+    // send the report if we get to check the time
+    const lastReportShouldSendNow = Date.now() - REPORT_INTERVAL_MS - 1;
+
+    test('returns false whenever telemetryOptedIn is null', () => {
+      const injector = {
+        localStorage: {
+          get: jest.fn().mockReturnValueOnce({ lastReport: lastReportShouldSendNow }),
+        },
+        telemetryOptedIn: null, // not yet opted in
+      };
+      const telemetry = new Telemetry(mockInjectorFromObject(injector), mockFetchTelemetry);
+
+      expect(telemetry._checkReportStatus()).toBe(false);
+    });
+
+    test('returns false whenever telemetryOptedIn is false', () => {
+      const injector = {
+        localStorage: {
+          get: jest.fn().mockReturnValueOnce({ lastReport: lastReportShouldSendNow }),
+        },
+        telemetryOptedIn: false, // opted out explicitly
+      };
+      const telemetry = new Telemetry(mockInjectorFromObject(injector), mockFetchTelemetry);
+
+      expect(telemetry._checkReportStatus()).toBe(false);
+    });
+
+    test('returns false if last report is too recent', () => {
+      const injector = {
+        localStorage: {
+          // we expect '>', not '>='
+          get: jest.fn().mockReturnValueOnce({ lastReport: Date.now() - REPORT_INTERVAL_MS }),
+        },
+        telemetryOptedIn: true,
+      };
+      const telemetry = new Telemetry(mockInjectorFromObject(injector), mockFetchTelemetry);
+
+      expect(telemetry._checkReportStatus()).toBe(false);
+    });
+
+    test('returns true if last report is not defined', () => {
+      const injector = {
+        localStorage: {
+          get: jest.fn().mockReturnValueOnce({ }),
+        },
+        telemetryOptedIn: true,
+      };
+      const telemetry = new Telemetry(mockInjectorFromObject(injector), mockFetchTelemetry);
+
+      expect(telemetry._checkReportStatus()).toBe(true);
+    });
+
+    test('returns true if last report is defined and old enough', () => {
+      const injector = {
+        localStorage: {
+          get: jest.fn().mockReturnValueOnce({ lastReport: lastReportShouldSendNow }),
+        },
+        telemetryOptedIn: true,
+      };
+      const telemetry = new Telemetry(mockInjectorFromObject(injector), mockFetchTelemetry);
+
+      expect(telemetry._checkReportStatus()).toBe(true);
+    });
+
+    test('returns true if last report is defined and old enough as a string', () => {
+      const injector = {
+        localStorage: {
+          get: jest.fn().mockReturnValueOnce({ lastReport: lastReportShouldSendNow.toString() }),
+        },
+        telemetryOptedIn: true,
+      };
+      const telemetry = new Telemetry(mockInjectorFromObject(injector), mockFetchTelemetry);
+
+      expect(telemetry._checkReportStatus()).toBe(true);
+    });
+
+    test('returns true if last report is defined and malformed', () => {
+      const injector = {
+        localStorage: {
+          get: jest.fn().mockReturnValueOnce({ lastReport: { not: { a: 'number' } } }),
+        },
+        telemetryOptedIn: true,
+      };
+      const telemetry = new Telemetry(mockInjectorFromObject(injector), mockFetchTelemetry);
+
+      expect(telemetry._checkReportStatus()).toBe(true);
+    });
+
+  });
+
+  describe('_sendIfDue', () => {
+
+    test('ignores and returns false if already sending', () => {
+      const injector = {
+        localStorage: {
+          get: jest.fn().mockReturnValueOnce(undefined), // never sent
+        },
+        telemetryOptedIn: true,
+      };
+      const telemetry = new Telemetry(mockInjectorFromObject(injector), mockFetchTelemetry);
+      telemetry._sending = true;
+
+      return expect(telemetry._sendIfDue()).resolves.toBe(false);
+    });
+
+    test('ignores and returns false if _checkReportStatus says so', () => {
+      const injector = {
+        localStorage: {
+          get: jest.fn().mockReturnValueOnce(undefined), // never sent, so it would try if opted in
+        },
+        telemetryOptedIn: false, // opted out
+      };
+      const telemetry = new Telemetry(mockInjectorFromObject(injector), mockFetchTelemetry);
+
+      return expect(telemetry._sendIfDue()).resolves.toBe(false);
+    });
+
+    test('sends telemetry when requested', () => {
+      const now = Date.now();
+      const injector = {
+        '$http': jest.fn().mockResolvedValue({ }), // ignored response
+        localStorage: {
+          get: jest.fn().mockReturnValueOnce({ lastReport: now - REPORT_INTERVAL_MS - 1 }),
+          set: jest.fn(),
+        },
+        telemetryOptedIn: true,
+        telemetryUrl,
+      };
+      const telemetry = new Telemetry(mockInjectorFromObject(injector), mockFetchTelemetry);
+
+      expect.hasAssertions();
+
+      return telemetry._sendIfDue()
+        .then(result => {
+          expect(result).toBe(true);
+          expect(telemetry._sending).toBe(false);
+
+          // should be updated
+          const lastReport = telemetry._lastReport;
+
+          // if the test runs fast enough it should be exactly equal, but probably a few ms greater
+          expect(lastReport).toBeGreaterThanOrEqual(now);
+
+          expect(injector.$http).toHaveBeenCalledTimes(2);
+          // assert that it sent every cluster's telemetry
+          clusters.forEach(cluster => {
+            expect(injector.$http).toHaveBeenCalledWith({
+              method: 'POST',
+              url: telemetryUrl,
+              data: cluster,
+              kbnXsrfToken: false,
+            });
+          });
+
+          expect(injector.localStorage.set).toHaveBeenCalledTimes(1);
+          expect(injector.localStorage.set).toHaveBeenCalledWith(LOCALSTORAGE_KEY, { lastReport });
+        });
+    });
+
+    test('sends telemetry when requested and catches exceptions', () => {
+      const lastReport = Date.now() - REPORT_INTERVAL_MS - 1;
+      const injector = {
+        '$http': jest.fn().mockRejectedValue(new Error('TEST - expected')), // caught failure
+        localStorage: {
+          get: jest.fn().mockReturnValueOnce({ lastReport }),
+          set: jest.fn(),
+        },
+        telemetryOptedIn: true,
+        telemetryUrl,
+      };
+      const telemetry = new Telemetry(mockInjectorFromObject(injector), mockFetchTelemetry);
+
+      expect.hasAssertions();
+
+      return telemetry._sendIfDue()
+        .then(result => {
+          expect(result).toBe(true); // attempted to send
+          expect(telemetry._sending).toBe(false);
+
+          // should be unchanged
+          expect(telemetry._lastReport).toBe(lastReport);
+          expect(injector.localStorage.set).toHaveBeenCalledTimes(0);
+
+          expect(injector.$http).toHaveBeenCalledTimes(2);
+          // assert that it sent every cluster's telemetry
+          clusters.forEach(cluster => {
+            expect(injector.$http).toHaveBeenCalledWith({
+              method: 'POST',
+              url: telemetryUrl,
+              data: cluster,
+              kbnXsrfToken: false,
+            });
+          });
+        });
+    });
+
+  });
+
+  test('start', () => {
+    const injector = {
+      localStorage: {
+        get: jest.fn().mockReturnValueOnce(undefined),
+      },
+      telemetryOptedIn: false, // opted out
+    };
+    const telemetry = new Telemetry(mockInjectorFromObject(injector), mockFetchTelemetry);
+
+    clearInterval(telemetry.start());
+  });
+
+});