Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Bug with logEvent callbacks not being called when unsent events are dropped #342

Merged
merged 7 commits into from
Jan 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/amplitude-client.js
Original file line number Diff line number Diff line change
Expand Up @@ -1384,7 +1384,14 @@ var _generateApiPropertiesTrackingConfig = function _generateApiPropertiesTracki
*/
AmplitudeClient.prototype._limitEventsQueued = function _limitEventsQueued(queue) {
if (queue.length > this.options.savedMaxCount) {
queue.splice(0, queue.length - this.options.savedMaxCount);
const deletedEvents = queue.splice(0, queue.length - this.options.savedMaxCount);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this.options.savedMaxCount guaranteed to be set?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, the default config for it is 1000

deletedEvents.forEach((event) => {
if (type(event.callback) === 'function') {
event.callback(0, 'No request sent', {
reason: 'Event dropped because options.savedMaxCount exceeded. User may be offline or have a content blocker',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this is relatively long compared to our other reason strings. I appreciate the helpful direction though. How would you feel if this was in the util logs instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not necessarily saying it should be in the util logs since I don't know how visible that might be) - and leaving this as-is might be a good idea as well.

Copy link
Contributor Author

@jooohhn jooohhn Jan 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that if it's only in the util logs, users won't be able to have failed upload event handling via callback

});
}
});
}
};

Expand All @@ -1394,6 +1401,7 @@ AmplitudeClient.prototype._limitEventsQueued = function _limitEventsQueued(queue
* @callback Amplitude~eventCallback
* @param {number} responseCode - Server response code for the event / identify upload request.
* @param {string} responseBody - Server response body for the event / identify upload request.
* @param {object} details - (optional) Additional information associated with sending event.
*/

/**
Expand Down Expand Up @@ -1684,7 +1692,7 @@ AmplitudeClient.prototype.sendEvents = function sendEvents() {
// here.
// }
} catch (e) {
// utils.log('failed upload');
// utils.log.error('failed upload');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's this line for if commented out?

Copy link
Contributor Author

@jooohhn jooohhn Jan 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was commented out before, didn't want to touch it 😂

I just made a change to uncomment it

}
});
};
Expand Down
27 changes: 27 additions & 0 deletions test/amplitude-client.js
Original file line number Diff line number Diff line change
Expand Up @@ -1945,6 +1945,33 @@ describe('AmplitudeClient', function () {
assert.equal(message, 'success');
});

it.only('should run the callback even with a dropped unsent event', function () {
amplitude.init(apiKey, null, { savedMaxCount: 1 });
var counter = 0;
var value = null;
var message = null;
var reason = null;
var callback = function (status, response, details) {
counter++;
value = status;
message = response;
reason = details.reason;
};
amplitude.logEvent('DroppedEvent', {}, callback);
amplitude.logEvent('SavedEvent', {}, callback);
server.respondWith([0, {}, '']);
server.respond();

// verify callback fired
assert.equal(counter, 1);
assert.equal(value, 0);
assert.equal(message, 'No request sent');
assert.equal(
reason,
'Event dropped because options.savedMaxCount exceeded. User may be offline or have a content blocker',
);
});

it('should run callback once and only after 413 resolved', function () {
var counter = 0;
var value = -1;
Expand Down