Add possibility to queue tracking requests so they are sent in bulk #13616
Conversation
| clearTimeout(this.timeout); | ||
| this.timeout = null; | ||
| } | ||
| // we always extend by another 1.75 seconds after receiving a tracking request |
There was a problem hiding this comment.
couldn't decide between 1.5 and 2 seconds :) the earlier we send it, the more likely the request won't fail I suppose.
There was a problem hiding this comment.
fyi #13444 will be useful to have to ensure the bulk request won't be too large in case tracking requests keep getting queued
There was a problem hiding this comment.
Could make the time configurable. At least that way it won't require a commit to core to experiment with it.
There was a problem hiding this comment.
I'd prefer to not make it configurable for now to keep it simple but could add it later if needed. For now only premium features will use it anyway I think. Although could maybe also start using it for content impressions eventually. The defined interval shouldn't make too much of a difference.
| sendRequest(requestsToTrack[0]); | ||
| } else { | ||
| sendBulkRequest(requestsToTrack); | ||
| } |
There was a problem hiding this comment.
Does sending a single request vs bulk request w/ single entry make a big difference?
There was a problem hiding this comment.
shouldn't make too much of a big difference. Although... for bulk requests we currently don't fallback to GET requests and always do a POST request compared to usually GET request, and it will be wrapped with {"requests":["?' + requests.join('","?') + '"]} so not sure if it can be replayed easily using log analytics etc.
I would say it is better when possible to use sendRequest when there is only one request and keep differentiating.
|
Left some comments, otherwise looks good. |
|
@diosmosis I'll merge for now so I can merge it into #13596 and resolve merge conflicts etc. Let me know if I should change anything further. |
No description provided.