From 6085b7214622132ebcc107d3ae64194bb4de5ee4 Mon Sep 17 00:00:00 2001 From: Tyler Brandt Date: Fri, 22 Jun 2018 10:18:24 -0700 Subject: [PATCH 1/6] Fix: add handler for http error event --- .../lib/plugins/event_dispatcher/index.node.js | 3 +++ .../plugins/event_dispatcher/index.node.tests.js | 13 +++++++++++++ 2 files changed, 16 insertions(+) diff --git a/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.js b/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.js index 378fe51a8..62675f86d 100644 --- a/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.js +++ b/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.js @@ -59,6 +59,9 @@ module.exports = { }; var req = (parsedUrl.protocol === 'http:' ? http : https).request(requestOptions, requestCallback); + req.on('error', function(err) { + callback(err); + }); req.write(dataString); req.end(); return req; diff --git a/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.tests.js b/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.tests.js index 971199040..4c70a3d9c 100644 --- a/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.tests.js +++ b/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.tests.js @@ -98,5 +98,18 @@ describe('lib/plugins/event_dispatcher/node', function() { eventDispatcher.dispatchEvent(eventObj, callback); }); }); + + it('calls callback with err when one is emitted', function(done) { + var eventObj = { + url: 'https://example', + params: {}, + httpVerb: 'POST', + }; + + eventDispatcher.dispatchEvent(eventObj, function(err) { + assert.isNotNull(err); + done(); + }); + }); }); }); From e964ecf0dbda54e129e2f846ffd2bd6279366de9 Mon Sep 17 00:00:00 2001 From: Tyler Brandt Date: Fri, 22 Jun 2018 10:36:29 -0700 Subject: [PATCH 2/6] also fix request callback --- .../lib/plugins/event_dispatcher/index.node.js | 8 +++----- .../lib/plugins/event_dispatcher/index.node.tests.js | 9 +-------- 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.js b/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.js index 62675f86d..183f99c56 100644 --- a/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.js +++ b/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.js @@ -52,16 +52,14 @@ module.exports = { } }; - var requestCallback = function(error, response, body) { - if (response && response.statusCode && response.statusCode >= 200 && response.statusCode < 400 && callback && typeof callback === 'function') { + var requestCallback = function(response) { + if (response && response.statusCode && response.statusCode >= 200 && response.statusCode < 400) { callback(); } }; var req = (parsedUrl.protocol === 'http:' ? http : https).request(requestOptions, requestCallback); - req.on('error', function(err) { - callback(err); - }); + req.on('error', callback); req.write(dataString); req.end(); return req; diff --git a/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.tests.js b/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.tests.js index 4c70a3d9c..d4d653e47 100644 --- a/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.tests.js +++ b/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.tests.js @@ -49,14 +49,7 @@ describe('lib/plugins/event_dispatcher/node', function() { httpVerb: 'POST', }; - eventDispatcher.dispatchEvent(eventObj) - .on('response', function(response) { - assert.isTrue(response.statusCode === 200); - done(); - }) - .on('error', function(error) { - assert.fail('status code okay', 'status code not okay', ''); - }); + eventDispatcher.dispatchEvent(eventObj, done); }); it('should execute the callback passed to event dispatcher', function(done) { From a60c31df9233319895d43c8644be83e036278e54 Mon Sep 17 00:00:00 2001 From: Tyler Brandt Date: Fri, 22 Jun 2018 10:47:27 -0700 Subject: [PATCH 3/6] Expect error in first arg, log appropriate message --- packages/optimizely-sdk/lib/optimizely/index.js | 12 ++++++++++-- .../lib/plugins/event_dispatcher/index.browser.js | 2 +- .../lib/plugins/event_dispatcher/index.node.js | 2 +- packages/optimizely-sdk/lib/utils/enums/index.js | 1 + 4 files changed, 13 insertions(+), 4 deletions(-) diff --git a/packages/optimizely-sdk/lib/optimizely/index.js b/packages/optimizely-sdk/lib/optimizely/index.js index 44c00dcf5..e9632f342 100644 --- a/packages/optimizely-sdk/lib/optimizely/index.js +++ b/packages/optimizely-sdk/lib/optimizely/index.js @@ -189,7 +189,11 @@ Optimizely.prototype._sendImpressionEvent = function(experimentKey, variationKey impressionEvent.url, JSON.stringify(impressionEvent.params)); this.logger.log(LOG_LEVEL.DEBUG, dispatchedImpressionEventLogMessage); - var eventDispatcherCallback = function() { + var eventDispatcherCallback = function(err, response) { + if (err) { + this.logger.log(LOG_LEVEL.ERROR, sprintf(LOG_MESSAGES.FAILED_TO_TRACK_EVENT, MODULE_NAME, 'IMPRESSION', experimentKey, err)); + return; + } var activatedLogMessage = sprintf(LOG_MESSAGES.ACTIVATE_USER, MODULE_NAME, userId, experimentKey); this.logger.log(LOG_LEVEL.INFO, activatedLogMessage); }.bind(this); @@ -263,7 +267,11 @@ Optimizely.prototype.track = function(eventKey, userId, attributes, eventTags) { JSON.stringify(conversionEvent.params)); this.logger.log(LOG_LEVEL.DEBUG, dispatchedConversionEventLogMessage); - var eventDispatcherCallback = function() { + var eventDispatcherCallback = function(err) { + if (err) { + this.logger.log(LOG_LEVEL.ERROR, sprintf(LOG_MESSAGES.FAILED_TO_TRACK_EVENT, MODULE_NAME, 'CONVERSION', experimentKey, err)); + return; + } var trackedLogMessage = sprintf(LOG_MESSAGES.TRACK_EVENT, MODULE_NAME, eventKey, userId); this.logger.log(LOG_LEVEL.INFO, trackedLogMessage); }.bind(this); diff --git a/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.browser.js b/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.browser.js index 7d5ba4090..1bf46626e 100644 --- a/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.browser.js +++ b/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.browser.js @@ -35,7 +35,7 @@ module.exports = { req.setRequestHeader('Content-Type', 'application/json'); req.onreadystatechange = function() { if (req.readyState === READYSTATE_COMPLETE && callback && typeof callback === 'function') { - callback(params); + callback(null, params); } }; req.send(JSON.stringify(params)); diff --git a/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.js b/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.js index 183f99c56..a4fc85f7f 100644 --- a/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.js +++ b/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.js @@ -54,7 +54,7 @@ module.exports = { var requestCallback = function(response) { if (response && response.statusCode && response.statusCode >= 200 && response.statusCode < 400) { - callback(); + callback(null, response); } }; diff --git a/packages/optimizely-sdk/lib/utils/enums/index.js b/packages/optimizely-sdk/lib/utils/enums/index.js index a5ac00e57..10b5b407c 100644 --- a/packages/optimizely-sdk/lib/utils/enums/index.js +++ b/packages/optimizely-sdk/lib/utils/enums/index.js @@ -73,6 +73,7 @@ exports.LOG_MESSAGES = { FEATURE_HAS_NO_EXPERIMENTS: '%s: Feature %s is not attached to any experiments.', FAILED_TO_PARSE_VALUE: '%s: Failed to parse event value "%s" from event tags.', FAILED_TO_PARSE_REVENUE: '%s: Failed to parse revenue value "%s" from event tags.', + FAILED_TO_TRACK_EVENT: '%s: Failed to track %s event for experiment %s: %s', FORCED_BUCKETING_FAILED: '%s: Variation key %s is not in datafile. Not activating user %s.', INVALID_OBJECT: '%s: Optimizely object is not valid. Failing %s.', INVALID_CLIENT_ENGINE: '%s: Invalid client engine passed: %s. Defaulting to node-sdk.', From 868beba2d1eb1cbda4ee9bf1f18cbea18edc7b94 Mon Sep 17 00:00:00 2001 From: Tyler Brandt Date: Fri, 22 Jun 2018 10:49:00 -0700 Subject: [PATCH 4/6] fix typo --- packages/optimizely-sdk/lib/optimizely/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/optimizely-sdk/lib/optimizely/index.js b/packages/optimizely-sdk/lib/optimizely/index.js index e9632f342..14306a25d 100644 --- a/packages/optimizely-sdk/lib/optimizely/index.js +++ b/packages/optimizely-sdk/lib/optimizely/index.js @@ -269,7 +269,7 @@ Optimizely.prototype.track = function(eventKey, userId, attributes, eventTags) { var eventDispatcherCallback = function(err) { if (err) { - this.logger.log(LOG_LEVEL.ERROR, sprintf(LOG_MESSAGES.FAILED_TO_TRACK_EVENT, MODULE_NAME, 'CONVERSION', experimentKey, err)); + this.logger.log(LOG_LEVEL.ERROR, sprintf(LOG_MESSAGES.FAILED_TO_TRACK_EVENT, MODULE_NAME, 'CONVERSION', eventKey, err)); return; } var trackedLogMessage = sprintf(LOG_MESSAGES.TRACK_EVENT, MODULE_NAME, eventKey, userId); From 977a3caf81f30ca8af679cb7fccf2f77c35ac3d4 Mon Sep 17 00:00:00 2001 From: Tyler Brandt Date: Fri, 22 Jun 2018 10:50:26 -0700 Subject: [PATCH 5/6] just use 2 messages --- packages/optimizely-sdk/lib/optimizely/index.js | 4 ++-- packages/optimizely-sdk/lib/utils/enums/index.js | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/optimizely-sdk/lib/optimizely/index.js b/packages/optimizely-sdk/lib/optimizely/index.js index 14306a25d..f55b8b05d 100644 --- a/packages/optimizely-sdk/lib/optimizely/index.js +++ b/packages/optimizely-sdk/lib/optimizely/index.js @@ -191,7 +191,7 @@ Optimizely.prototype._sendImpressionEvent = function(experimentKey, variationKey this.logger.log(LOG_LEVEL.DEBUG, dispatchedImpressionEventLogMessage); var eventDispatcherCallback = function(err, response) { if (err) { - this.logger.log(LOG_LEVEL.ERROR, sprintf(LOG_MESSAGES.FAILED_TO_TRACK_EVENT, MODULE_NAME, 'IMPRESSION', experimentKey, err)); + this.logger.log(LOG_LEVEL.ERROR, sprintf(LOG_MESSAGES.FAILED_TO_TRACK_IMPRESSION_EVENT, MODULE_NAME, experimentKey, err)); return; } var activatedLogMessage = sprintf(LOG_MESSAGES.ACTIVATE_USER, MODULE_NAME, userId, experimentKey); @@ -269,7 +269,7 @@ Optimizely.prototype.track = function(eventKey, userId, attributes, eventTags) { var eventDispatcherCallback = function(err) { if (err) { - this.logger.log(LOG_LEVEL.ERROR, sprintf(LOG_MESSAGES.FAILED_TO_TRACK_EVENT, MODULE_NAME, 'CONVERSION', eventKey, err)); + this.logger.log(LOG_LEVEL.ERROR, sprintf(LOG_MESSAGES.FAILED_TO_TRACK_CONVERSION_EVENT, MODULE_NAME, eventKey, err)); return; } var trackedLogMessage = sprintf(LOG_MESSAGES.TRACK_EVENT, MODULE_NAME, eventKey, userId); diff --git a/packages/optimizely-sdk/lib/utils/enums/index.js b/packages/optimizely-sdk/lib/utils/enums/index.js index 10b5b407c..f9571f7cb 100644 --- a/packages/optimizely-sdk/lib/utils/enums/index.js +++ b/packages/optimizely-sdk/lib/utils/enums/index.js @@ -73,7 +73,8 @@ exports.LOG_MESSAGES = { FEATURE_HAS_NO_EXPERIMENTS: '%s: Feature %s is not attached to any experiments.', FAILED_TO_PARSE_VALUE: '%s: Failed to parse event value "%s" from event tags.', FAILED_TO_PARSE_REVENUE: '%s: Failed to parse revenue value "%s" from event tags.', - FAILED_TO_TRACK_EVENT: '%s: Failed to track %s event for experiment %s: %s', + FAILED_TO_TRACK_CONVERSION_EVENT: '%s: Failed to track conversion event for %s: %s', + FAILED_TO_TRACK_IMPRESSION_EVENT: '%s: Failed to track impression event for experiment %s: %s', FORCED_BUCKETING_FAILED: '%s: Variation key %s is not in datafile. Not activating user %s.', INVALID_OBJECT: '%s: Optimizely object is not valid. Failing %s.', INVALID_CLIENT_ENGINE: '%s: Invalid client engine passed: %s. Defaulting to node-sdk.', From bb7598b1889c8c3df400d9b6696e670fb0c98ae0 Mon Sep 17 00:00:00 2001 From: Tyler Brandt Date: Fri, 22 Jun 2018 11:05:30 -0700 Subject: [PATCH 6/6] revert error-logging changes --- .../optimizely-sdk/lib/optimizely/index.js | 12 ++------ .../plugins/event_dispatcher/index.browser.js | 2 +- .../plugins/event_dispatcher/index.node.js | 6 ++-- .../event_dispatcher/index.node.tests.js | 28 ++++++++----------- .../optimizely-sdk/lib/utils/enums/index.js | 2 -- 5 files changed, 18 insertions(+), 32 deletions(-) diff --git a/packages/optimizely-sdk/lib/optimizely/index.js b/packages/optimizely-sdk/lib/optimizely/index.js index f55b8b05d..44c00dcf5 100644 --- a/packages/optimizely-sdk/lib/optimizely/index.js +++ b/packages/optimizely-sdk/lib/optimizely/index.js @@ -189,11 +189,7 @@ Optimizely.prototype._sendImpressionEvent = function(experimentKey, variationKey impressionEvent.url, JSON.stringify(impressionEvent.params)); this.logger.log(LOG_LEVEL.DEBUG, dispatchedImpressionEventLogMessage); - var eventDispatcherCallback = function(err, response) { - if (err) { - this.logger.log(LOG_LEVEL.ERROR, sprintf(LOG_MESSAGES.FAILED_TO_TRACK_IMPRESSION_EVENT, MODULE_NAME, experimentKey, err)); - return; - } + var eventDispatcherCallback = function() { var activatedLogMessage = sprintf(LOG_MESSAGES.ACTIVATE_USER, MODULE_NAME, userId, experimentKey); this.logger.log(LOG_LEVEL.INFO, activatedLogMessage); }.bind(this); @@ -267,11 +263,7 @@ Optimizely.prototype.track = function(eventKey, userId, attributes, eventTags) { JSON.stringify(conversionEvent.params)); this.logger.log(LOG_LEVEL.DEBUG, dispatchedConversionEventLogMessage); - var eventDispatcherCallback = function(err) { - if (err) { - this.logger.log(LOG_LEVEL.ERROR, sprintf(LOG_MESSAGES.FAILED_TO_TRACK_CONVERSION_EVENT, MODULE_NAME, eventKey, err)); - return; - } + var eventDispatcherCallback = function() { var trackedLogMessage = sprintf(LOG_MESSAGES.TRACK_EVENT, MODULE_NAME, eventKey, userId); this.logger.log(LOG_LEVEL.INFO, trackedLogMessage); }.bind(this); diff --git a/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.browser.js b/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.browser.js index 1bf46626e..7d5ba4090 100644 --- a/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.browser.js +++ b/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.browser.js @@ -35,7 +35,7 @@ module.exports = { req.setRequestHeader('Content-Type', 'application/json'); req.onreadystatechange = function() { if (req.readyState === READYSTATE_COMPLETE && callback && typeof callback === 'function') { - callback(null, params); + callback(params); } }; req.send(JSON.stringify(params)); diff --git a/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.js b/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.js index a4fc85f7f..0b6e2b9cf 100644 --- a/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.js +++ b/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.js @@ -30,7 +30,6 @@ module.exports = { dispatchEvent: function(eventObj, callback) { // Non-POST requests not supported if (eventObj.httpVerb !== 'POST') { - callback(new Error('httpVerb not supported: ' + eventObj.httpVerb)); return; } @@ -54,12 +53,13 @@ module.exports = { var requestCallback = function(response) { if (response && response.statusCode && response.statusCode >= 200 && response.statusCode < 400) { - callback(null, response); + callback(response); } }; var req = (parsedUrl.protocol === 'http:' ? http : https).request(requestOptions, requestCallback); - req.on('error', callback); + // Add no-op error listener to prevent this from throwing + req.on('error', function() {}); req.write(dataString); req.end(); return req; diff --git a/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.tests.js b/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.tests.js index d4d653e47..82ec57ee5 100644 --- a/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.tests.js +++ b/packages/optimizely-sdk/lib/plugins/event_dispatcher/index.node.tests.js @@ -49,7 +49,10 @@ describe('lib/plugins/event_dispatcher/node', function() { httpVerb: 'POST', }; - eventDispatcher.dispatchEvent(eventObj, done); + eventDispatcher.dispatchEvent(eventObj, function(resp) { + assert.equal(200, resp.statusCode); + done(); + }); }); it('should execute the callback passed to event dispatcher', function(done) { @@ -63,15 +66,15 @@ describe('lib/plugins/event_dispatcher/node', function() { eventDispatcher.dispatchEvent(eventObj, stubCallback.callback) .on('response', function(response) { - done(); sinon.assert.calledOnce(stubCallback.callback); + done(); }) .on('error', function(error) { assert.fail('status code okay', 'status code not okay', ''); }); }); - it('rejects GET httpVerb', function(done) { + it('rejects GET httpVerb', function() { var eventObj = { url: 'https://cdn.com/event', params: { @@ -80,29 +83,22 @@ describe('lib/plugins/event_dispatcher/node', function() { httpVerb: 'GET', }; - var callback = function(err) { - if (err) { - done(); - } else { - done(new Error('expected error not thrown')); - } - }; - + var callback = sinon.spy(); eventDispatcher.dispatchEvent(eventObj, callback); + sinon.assert.notCalled(callback); }); }); - it('calls callback with err when one is emitted', function(done) { + it('does not throw in the event of an error', function() { var eventObj = { url: 'https://example', params: {}, httpVerb: 'POST', }; - eventDispatcher.dispatchEvent(eventObj, function(err) { - assert.isNotNull(err); - done(); - }); + var callback = sinon.spy(); + eventDispatcher.dispatchEvent(eventObj, callback); + sinon.assert.notCalled(callback); }); }); }); diff --git a/packages/optimizely-sdk/lib/utils/enums/index.js b/packages/optimizely-sdk/lib/utils/enums/index.js index f9571f7cb..a5ac00e57 100644 --- a/packages/optimizely-sdk/lib/utils/enums/index.js +++ b/packages/optimizely-sdk/lib/utils/enums/index.js @@ -73,8 +73,6 @@ exports.LOG_MESSAGES = { FEATURE_HAS_NO_EXPERIMENTS: '%s: Feature %s is not attached to any experiments.', FAILED_TO_PARSE_VALUE: '%s: Failed to parse event value "%s" from event tags.', FAILED_TO_PARSE_REVENUE: '%s: Failed to parse revenue value "%s" from event tags.', - FAILED_TO_TRACK_CONVERSION_EVENT: '%s: Failed to track conversion event for %s: %s', - FAILED_TO_TRACK_IMPRESSION_EVENT: '%s: Failed to track impression event for experiment %s: %s', FORCED_BUCKETING_FAILED: '%s: Variation key %s is not in datafile. Not activating user %s.', INVALID_OBJECT: '%s: Optimizely object is not valid. Failing %s.', INVALID_CLIENT_ENGINE: '%s: Invalid client engine passed: %s. Defaulting to node-sdk.',