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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 7 commits into from
Jan 8, 2021
Merged

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

merged 7 commits into from
Jan 8, 2021

Conversation

jooohhn
Copy link
Contributor

@jooohhn jooohhn commented Jan 7, 2021 •
edited
Loading

Summary

Checklist

  • Does your PR title have the correct title format?
  • Does your PR have a breaking change?: NO

@jooohhn jooohhn requested review from kelvin-lu and kelsonpw January 7, 2021 18:15
@jooohhn jooohhn mentioned this pull request Jan 7, 2021
Closed
1 task
@kelvin-lu kelvin-lu requested a review from theoschao January 7, 2021 21:13
theoschao
theoschao reviewed Jan 7, 2021
View reviewed changes
src/amplitude-client.js
@@ -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);
Copy link

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

src/amplitude-client.js
@@ -1684,7 +1692,7 @@ AmplitudeClient.prototype.sendEvents = function sendEvents() {
// here.
// }
} catch (e) {
// utils.log('failed upload');
// utils.log.error('failed upload');
Copy link

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 •
edited
Loading

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

kelvin-lu
kelvin-lu approved these changes Jan 7, 2021
View reviewed changes
Copy link
Contributor

@kelvin-lu kelvin-lu left a comment

Choose a reason for hiding this comment

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

lgtm! one nit and one conversation but overall makes sense.

src/amplitude-client.js Outdated
@@ -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);
deletedEvents.forEach((e) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: no single letter variable

src/amplitude-client.js
deletedEvents.forEach((e) => {
if (type(e.callback) === 'function') {
e.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 •
edited
Loading

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

@jooohhn jooohhn merged commit f243a92 into master Jan 8, 2021
@jooohhn jooohhn deleted the fix-logevent-callback-on-fail branch January 8, 2021 19:04
github-actions bot pushed a commit that referenced this pull request Jan 11, 2021
@amplitude-sdk-bot
chore(release): 7.4.1 [skip ci]
a252b87 
## [7.4.1](v7.4.0...v7.4.1) (2021-01-11)

### Bug Fixes

* Bug with logEvent callbacks not being called when unsent events are dropped  ([#342](#342)) ([f243a92](f243a92)), closes [#142](#142)
@jooohhn jooohhn mentioned this pull request Feb 22, 2021
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kelvin-lu kelvin-lu kelvin-lu approved these changes

@theoschao theoschao theoschao left review comments

@kelsonpw kelsonpw Awaiting requested review from kelsonpw

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Amplitude callback isn't fired if a user enables adblock
4 participants
@jooohhn @theoschao @kelvin-lu @donaldpipowitch