Make cookie writes async by default to improve tracker performance #1340
Conversation
BundleMonFiles added (6)
Total files change +104.68KB 0% Final result: ✅ View report in BundleMon website ➡️ |
| setCookieTimer = setTimeout(() => { | ||
| setCookieTimer = undefined; | ||
| cookie(name, value, ttl, path, domain, samesite, secure); | ||
|
|
||
| if (ttl !== undefined) { | ||
| scheduleClearCache(ttl); | ||
| } | ||
| }, debounceTimeout); |
There was a problem hiding this comment.
I am just thinking... Why aren't we using the requestAnimationFrame and setTimeout trick here?
requestAnimationFrame(() => {
setTimeout(() => {
cookie()...
}, 0);
});
Since the time is random it might again hit a blocking point when rendering would be critical and INP metrics might still be an issue. requestAnimationFrame is a bit safer on that aspect.
There was a problem hiding this comment.
I think the race risk is already pretty high with this change, adding indeterminate amounts of time before updating the value seems dangerous to values like eventIndex.
| setCookieTimer = setTimeout(() => { | ||
| setCookieTimer = undefined; | ||
| cookie(name, value, ttl, path, domain, samesite, secure); | ||
|
|
||
| if (ttl !== undefined) { | ||
| scheduleClearCache(ttl); | ||
| } | ||
| }, debounceTimeout); |
There was a problem hiding this comment.
I think the race risk is already pretty high with this change, adding indeterminate amounts of time before updating the value seems dangerous to values like eventIndex.
|
Thanks both for the reviews! @jethron I have added a It's a trade-off between performance and correctness that we have to make. In my opinion, the performance gains make it worth it in this case. Also I don't see another way to speed up the cookie writes without making them async, but happy to make changes if you have some other idea that would be safer! |
|
Sorry for the delay on this @matus-tomlein! I did some testing and did find a race, but then when I compared it to v3 the behaviour there had a similar problem anyway, so I guess it doesn't really count as a regression -- it's just a long-standing bug. So I don't think that should block this.
e.g. in below test v4 generates 20-30 requests but v3 generates more like 10-15 for similar numbers of events. To test I had the same page open in two on-screen browser windows (important, just different tabs would throttle intervals) and ran the following snippet at roughly the same time (it tries to give some leeway with an initial timeout) in each via browser console: let start = Date.now();
setTimeout(function(){
let i = setInterval(function(){
if ((Date.now() - start) > 3000) clearInterval(i);
spActivity('trackPageView')}, 100)
}, (+new Date()) % 1000);( Most events are OK, but when there's a race like this enforces, eventually you get collisions, where between the two pages events with different event IDs/payloads will generate with the same v3: v4: |
|
FML, that was testing |
…acking link clicks and form submit events in the plugins
matus-tomlein
force-pushed
the
issue/cookie_optimization
branch
from
d4ecc6a to
ae249a8
Compare
|
Thanks @jethron for testing that! That's a good test case since it's quite possible that users will open the same website in multiple windows. I think the problem is not so much in the async cookie writes but in the fact that we now cache the cookie values. Once a cookie value is cached, we don't check for the actual cookie value which may be modified by another window. I have now made a change to add a maximum lifetime of the cached cookie value to 100 ms. I have run the same test as you have and while I still get conflicts on the We can lower the max cache lifetime further, hard to say what should be the setting... Happy to go with a different value if you prefer. |
This PR makes cookie writes async by default on the browser tracker in order to improve performance of the tracker call (to block the app for a shorter period of time when an event is tracked).
It adds a wrapper around the calls to read and write cookies that caches the last written value and uses
setTimeoutwith 10ms to schedule the cookie write (it also debounces if there are multiple cookie writes in short sequences).There is also a new option to keep the previous behaviour and write cookies synchronously with the track calls – the
synchronousCookieWriteoption. This may be useful in tests and it also has the benefit of checking that the cookies were correctly set.I added an e2e test that compares the performance of the different storage strategies and the sync option, results below.