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

Improve event flushing options and remove pageUnloadTimer #719

Closed
yalisassoon opened this issue Apr 16, 2019 · 5 comments
Closed

Improve event flushing options and remove pageUnloadTimer #719

yalisassoon opened this issue Apr 16, 2019 · 5 comments
Assignees
Labels
category:browser About the browser-specific code. priority:medium On the roadmap. status:in_progress Maintainers are working on this. status:needs_triage Needs maintainer triage. type:defect Bugs or weaknesses. The issue has to contain steps to reproduce.
Milestone

Comments

@yalisassoon
Copy link
Member

Current value used in combination with automatic link click tracking can slow the loading of subsequent pages on a website, as the timer is enabled after each link click.

@paulboocock paulboocock added category:browser About the browser-specific code. priority:medium On the roadmap. type:defect Bugs or weaknesses. The issue has to contain steps to reproduce. labels Feb 6, 2020
@paulboocock paulboocock added this to the Version 3.0.0 milestone Feb 6, 2020
@paulboocock
Copy link
Contributor

With improvements to Beacon and the local storage queue, it seems reasonable to reduce this.
Perhaps we should have a set of defaults depending on tracker config:
Using GET and Cookies - stay at 500ms
Using POST and Local Storage - drop to 100ms(?)
Using Beacon - drop to 0ms(?)

Needs considerable testing for event loss.

@paulboocock paulboocock added the status:needs_triage Needs maintainer triage. label Feb 6, 2020
@alperakgun
Copy link

Using Beacon - drop to 0ms(?)

@paulboocock Thinking about the question mark at the end, what risks could there be using Beacon with a 0ms pageUnloadTimer?

PS: We have a Post/Localstorage based implementation, where we want to keep the link_click events, and have no delay before unloading a page.

@paulboocock
Copy link
Contributor

Hey @alperakgun I totally missed this notification, apologies!

In an ideal world, using 0ms with beacon would be the perfect scenario, and it should technically work, but beacon behaves inconsistently across browsers, which is the main cause for me to add the (?). I think you'll be fine but you're trusting the browsers to send events so you might find some browsers don't behave as you would expect (particularly older Safari (10-11ish) and Chrome < v81 (falls back to POST)). I think this is a balancing act, are you happy to lose some events for improved website performance? If so, give it a try and monitor your results over a couple of weeks - I'd strongly advise looking at results grouped by browser to see if you notice any significant changes in events from certain browsers.

@alperakgun
Copy link

Thanks @paulboocock - it makes sense.

@paulboocock paulboocock added the status:in_progress Maintainers are working on this. label Feb 12, 2021
@paulboocock paulboocock self-assigned this Feb 12, 2021
@paulboocock paulboocock changed the title Reduce default pageUnloadTimer value Improve event flushing options and remove pageUnloadTimer Mar 10, 2021
@paulboocock
Copy link
Contributor

I've taken a multiple pronged strategy here which allows for the removal of the pageUnloadTimer.

I'm now flushing the buffers on page vivisilibty -> hidden events. This means we flush more frequently but works on mobile and desktop. I'm also leaving in the beforeUnload flush but with no delay timer (since it doesn't work in modern browsers anyway). To keep the "old browser" functionality, when flushing I'm sending the XHR with the sync flag as true. Modern browsers ignore this but old browsers likely respect it so should still see beforeUnload flushing work.

I've also extended the flushing options too. You can now set the buffer size using setBufferSize meaning if you want to start with a higher buffer size but drop it in your own page unload handling you can do. You can also set an optional property on flushBuffer which will lower the buffer size post flushing (handy in scenarios such as those described in #890 and #894).

I've also updated beacon so that the beacon preflight is only required on Safari. This should increase the effectiveness of using eventMethod: beacon on all browsers except for Safari <= 13 where beacon is a bit janky (see #716). We'll now always use post in those older Safari versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:browser About the browser-specific code. priority:medium On the roadmap. status:in_progress Maintainers are working on this. status:needs_triage Needs maintainer triage. type:defect Bugs or weaknesses. The issue has to contain steps to reproduce.
Projects
None yet
Development

No branches or pull requests

3 participants