From 02b06b29a4541795b02922bdbfec95c7d29baef0 Mon Sep 17 00:00:00 2001 From: Christopher Willis-Ford Date: Tue, 9 Jul 2019 13:59:39 -0700 Subject: [PATCH] fix telemetry init and enforce queue length limit --- src/main/telemetry/TelemetryClient.js | 76 ++++++++++++++++++++------- 1 file changed, 58 insertions(+), 18 deletions(-) diff --git a/src/main/telemetry/TelemetryClient.js b/src/main/telemetry/TelemetryClient.js index 23480ac..16fe3b5 100644 --- a/src/main/telemetry/TelemetryClient.js +++ b/src/main/telemetry/TelemetryClient.js @@ -20,6 +20,14 @@ const TelemetryServerURL = Object.freeze({ staging: 'http://scratch-telemetry-s.us-east-1.elasticbeanstalk.com/', production: 'https://telemetry.scratch.mit.edu/' }); +const DefaultServerURL = ( + process.env.NODE_ENV === 'production' ? TelemetryServerURL.production : TelemetryServerURL.staging +); + +/** + * Default name for persistent configuration & queue storage + */ +const DefaultStoreName = 'telemetry'; /** * Default interval, in seconds, between delivery attempts @@ -31,6 +39,17 @@ const DefaultDeliveryInterval = 60; */ const DefaultNetworkCheckInterval = 300; +/** + * Default limit on the number of queued events + */ +const DefaultQueueLimit = 100; + +/** + * Default limit on the number of delivery attempts for each event + */ +const DeliveryAttemptLimit = 3; + + /** * Client interface for the Scratch telemetry service. * @@ -45,32 +64,39 @@ class TelemetryClient { * @param {object} [options] - optional configuration settings for this client * @property {string} [storeName] - optional name for persistent config/queue storage (default: 'telemetry') * @property {string} [clientId] - optional UUID for this client (default: automatically determine a UUID) - * @property {string} [url] - optional telemetry service endpoint URL (default: automatically choose a server) + * @property {string} [serverURL] - optional telemetry service endpoint URL (default: automatically choose a server) * @property {boolean} [didOptIn] - optional flag for whether the user opted into telemetry service (default: false) * @property {int} [deliveryInterval] - optional number of seconds between delivery attempts (default: 60) - * @property {int} [networkInterval] - optional number of seconds between connectivity checks (default: 300) + * @property {int} [networkCheckInterval] - optional number of seconds between connectivity checks (default: 300) * @property {int} [queueLimit] - optional limit on the number of queued events (default: 100) - * @property {int} [attemptLimit] - optional limit on the number of delivery attempts for each event (default: 3) + * @property {int} [deliveryAttemptLimit] - optional limit on delivery attempts for each event (default: 3) */ - constructor (options = null) { - options = options || {}; - + constructor ({ + storeName = DefaultStoreName, + clientID, // undefined = load or create + serverURL, // undefined = automatic + didOptIn, // undefined = show prompt + deliveryInterval = DefaultDeliveryInterval, + networkCheckInterval = DefaultNetworkCheckInterval, + queueLimit = DefaultQueueLimit, + deliveryAttemptLimit = DeliveryAttemptLimit + } = {}) { /** * Persistent storage for the client ID, opt in flag, and packet queue. */ this._store = new ElectronStore({ - name: options.storeName || 'telemetry' + name: storeName }); console.log(`Telemetry configuration storage path: ${this._store.path}`); - if (options.hasOwnProperty('clientID')) { - this.clientID = options.clientID; + if (clientID) { + this.clientID = clientID; } else if (!this._store.has('clientID')) { this.clientID = uuidv1(); } - if (options.hasOwnProperty('optIn')) { - this.didOptIn = options.didOptIn; + if (typeof didOptIn !== 'undefined') { + this.didOptIn = didOptIn; } /** @@ -81,7 +107,7 @@ class TelemetryClient { /** * Server URL */ - this._serverURL = options.url || TelemetryServerURL.staging; + this._serverURL = serverURL || DefaultServerURL; /** * Can we currently reach the telemetry service? @@ -91,13 +117,22 @@ class TelemetryClient { /** * Try to deliver telemetry packets at this interval */ - this._deliveryInterval = (options.deliveryInterval > 0) ? options.deliveryInterval : DefaultDeliveryInterval; + this._deliveryInterval = (deliveryInterval > 0) ? deliveryInterval : DefaultDeliveryInterval; /** * Check for connectivity at this interval */ - this._networkCheckInterval = - (options.networkCheckInterval > 0) ? options.networkCheckInterval : DefaultNetworkCheckInterval; + this._networkCheckInterval = (networkCheckInterval > 0) ? networkCheckInterval : DefaultNetworkCheckInterval; + + /** + * Queue at most this many events + */ + this._queueLimit = (queueLimit > 0) ? queueLimit : DefaultQueueLimit; + + /** + * Attempt to deliver an event at most this many times + */ + this._deliveryAttemptLimit = (deliveryAttemptLimit > 0) ? deliveryAttemptLimit : DeliveryAttemptLimit; /** * Bind event handlers @@ -121,9 +156,13 @@ class TelemetryClient { * Stop this client. Do not use this object after disposal. */ dispose () { - if (this._deliveryInterval !== null) { + if (this._networkTimer !== null) { + clearInterval(this._networkTimer); + this._networkTimer = null; + } + if (this._deliveryTimer !== null) { clearInterval(this._deliveryTimer); - this._deliveryInterval = null; + this._deliveryTimer = null; } } @@ -183,6 +222,7 @@ class TelemetryClient { packet }; this._packetQueue.push(packetInfo); + this._packetQueue.splice(0, this._packetQueue.length - this._queueLimit); // enforce queue length limit this.saveQueue(); } @@ -220,7 +260,7 @@ class TelemetryClient { // TODO: check if the failure is because there's no Internet connection and if so refund the attempt const packetFailed = err || (res.statusCode !== 200); if (packetFailed) { - if (packetInfo.attempts < this._attemptsLimit) { + if (packetInfo.attempts < this._deliveryAttemptLimit) { this._packetQueue.push(packetInfo); } else { console.warn('Dropping packet which exceeded retry limit', packet);