Send events using POST #36

mattagra · 2021-04-20T18:05:37Z

Pivotal ticket(s)

https://www.pivotaltracker.com/story/show/177824064

Context

Missing WT events have been a recurring problem since WT events first launched. Adblockers can affect the efficacy of our pixels (and even prevent them from appearing in fullstory). There are some low-hanging-fruit steps we can take to reduce the impact of adblockers and have a higher integrity events dataset.

Changes

Send events to /wt/t with POST method using window.fetch()
Fallback to image pixel tracking if window.fetch() fails (plan is to remove in the future)
Send data in POST request body to avoid "URI Too Long" error:

Test Cases

Tested on macallan

Published an rc version to npm with @clutter/wt@1.2.0-rc.1
Pointed landing branch to rc version: https://github.com/clutter/landing/pull/2003
Updated wt gem for new wt endpoint: https://github.com/clutter/clutter-platform/pull/11408

See events on macallan:

select * from wt_events order by created_at desc limit 30

Chrome:

Firefox:

Safari:

Callouts

wt changes: https://github.com/clutter/wt/pull/62

Future

Removing pixel fallback (if we don't see many fallback=true requests after we deploy)
Use sendBeacon with polyfill? Not sure if we still want this if things work well with fetch/POST
Use prettier
(Optional) Update circleci config

bcharna

had just a couple comments. a couple other call-outs:

can we see if this seems to not get blocked by Adblock on folk's local machines? I coincidentally was just helping Michael with a WT thing and it was being blocked by his Adblock, so we could test it on his machine (or likely anyone else's)
can we ensure that we have the best view of pageloads -> pageviews possible ahead of launching this? i was working with Clayton on some of this, but I think we should be a bit more confident on those charts before shipping so we are confident we have a good way to track how well this change goes.

bcharna · 2021-04-22T21:46:11Z

src/index.js

@@ -144,7 +151,13 @@ export class WT {
      delete this.loaderImage.onload;
      reject();
    };
-    this.loaderImage.src = `${this.getUrl()}?${query}`;
+    const params = new this.context.URLSearchParams(query);
+    const sendBeaconResult = this.context.navigator.sendBeacon(`${this.getBeaconUrl()}`, params);


afaict, the only time this would return false is if the data limit is surpassed, see the note under Return Value here. given that, I'm a bit skeptical that we'd ever get false returned but I think we don't know for sure. that being said, can we just hit the new POST endpoint instead (via a standard AJAX call) so we don't have to maintain the track.gif endpoint? then, we also wouldn't need beaconUrl and trackerUrl, we can just have the latter.

additionally, not all browsers will have navigator.sendBeacon, so I think it would be wise to add a polyfill to avoid crashing in this case, or checking for the existence of the function and falling back to an AJAX call if it doesn't exist.

@bcharna i believe there's an example of hitting the data limit (via an overly long query string) in the PR description.

that said, i think the only reason that's likely to happen is due to the how we batch events together. i'm not sure if there's any reason to do that with the beacon API, but if we can remove that it would be a win in terms of complexity.

re: polyfilling, i think we can safely offload that to consumers of the library rather than make that decision for everybody.

yeah, except the data limits may be different for query string size limit vs beacon's limit.

also agree batching may not be necessary, but likely something we can punt on for now to keep change size smaller.

fair point on polyfilling – are you proposing we go with my latter suggestion (check for existence of function and hit AJAX if not present)?

proposing we leave as is + add a note in the readme that sendBeacon support is required and users may polyfill if neccesary.

ah, I see. sorry, I sort of combined two pieces of feedback in the original comment. so I think to summarize we're saying:

end users of wt (for us, landing, platform and today) will polyfill themselves.

we'll make a POST request instead of an image request in the case that this.context.navigator.sendBeacon returns false

Do we want to completely get rid of the image request (i.e. try sendBeacon then try POST)? or try all three (i.e. try sendBeacon then try POST then try image)?

I don't see a reason to keep the image request around – all browsers should support POST via standard AJAX. but to clarify, @nopelluhh was proposing only to fallback to the standard AJAX if sendBeacon returns false, thus we assume that the function is present (this means we'll want to add a polyfill in our apps that use wt).

What are your thoughts on a sendbeacon-polyfill? I see from the source code that it already handles sending as AJAX.

If we take all the suggestions into consideration, seems like we can clean up a lot of code (remove image fallback, remove batching, use polyfill to handle fallback)

it looks like that's a fork of https://github.com/miguelmota/Navigator.sendBeacon, can we see what the difference is between those? it looks like the original repo has more stars/has been updated more recently but have never worked w/ either.

and yep, a lot of cleanup is possible here!

bcharna · 2021-04-22T21:49:42Z

src/index.js

@@ -52,7 +53,9 @@ export class WT {
    this.emitter = new EventEmitter();
    this.wtConfig = { cookies: { expires: EXPIRES_IN_DAYS } };
    this.context = context;
-    this.paramDefaults = {};
+    this.paramDefaults = {
+      wt_version: WT_VERSION,


I like this! will it go into event.metadata?

i'm not sure that i like this (as is); recording this for every event seems excessive, especially if it's going into an already unstructured metadata column. what's the motivation for tracking this/do we need it on a data level/do we need it for every event?

if we do keep this i'd suggest renaming to wt_client_version for accuracy.

I don't think we have to worry about storage size, but I hear you. I actually think having app version info would be more useful (so not super opinionated on whether we leave wt version info in or not), but that's outside of the scope here.

yeah, this will be saved in event.metadata. this was to help me QA that the new events were saving properly on macallan so I don't feel too strongly about keeping it. i feel ok with removing it because it doesn't add much value outside of the QA use case.

bcharna · 2021-04-22T22:04:11Z

also, going to pass along to @nopelluhh to do a pass as well, since this is such critical code.

nopelluhh · 2021-04-22T22:12:50Z

src/index.js

@@ -52,7 +53,9 @@ export class WT {
    this.emitter = new EventEmitter();
    this.wtConfig = { cookies: { expires: EXPIRES_IN_DAYS } };
    this.context = context;
-    this.paramDefaults = {};
+    this.paramDefaults = {
+      wt_version: WT_VERSION,


i'm not sure that i like this (as is); recording this for every event seems excessive, especially if it's going into an already unstructured metadata column. what's the motivation for tracking this/do we need it on a data level/do we need it for every event?

if we do keep this i'd suggest renaming to wt_client_version for accuracy.

nopelluhh · 2021-04-22T22:21:29Z

src/index.js

@@ -144,7 +151,13 @@ export class WT {
      delete this.loaderImage.onload;
      reject();
    };
-    this.loaderImage.src = `${this.getUrl()}?${query}`;
+    const params = new this.context.URLSearchParams(query);
+    const sendBeaconResult = this.context.navigator.sendBeacon(`${this.getBeaconUrl()}`, params);


@bcharna i believe there's an example of hitting the data limit (via an overly long query string) in the PR description.

that said, i think the only reason that's likely to happen is due to the how we batch events together. i'm not sure if there's any reason to do that with the beacon API, but if we can remove that it would be a win in terms of complexity.

re: polyfilling, i think we can safely offload that to consumers of the library rather than make that decision for everybody.

nopelluhh · 2021-04-22T22:24:51Z

src/index.js

@@ -144,7 +151,13 @@ export class WT {
      delete this.loaderImage.onload;
      reject();
    };
-    this.loaderImage.src = `${this.getUrl()}?${query}`;
+    const params = new this.context.URLSearchParams(query);


along the lines of brad's browser support callout, this is a newer API. we already use the qs library so i'd suggest using that (or fully migrating to URLSearchParams if you have time for a bigger change).

What's our cutoff for browser support for both sendBeacon and URLSearchParams? Their browser compatibility is pretty similar with full support on latest versions except no support on IE. For sendBeacon's data parameter, I'm taking advantage of the URLSearchParams object so they kind of go hand-in-hand so I don't think I could use qs with sendBeacon. I also tried the Blob object with sendBeacon to send the data as JSON but that led to CORS issues.

i selfishly like the idea that noah suggested to leave as is, add note in the readme that sendBeacon and URLSearchParams are required and let users polyfill if necessary.

this also leads back to the fallback question: if we're requiring sendBeacon support, should we just get rid of all fallbacks (POST and image)? i left the image code in there to keep the change small

nopelluhh · 2021-04-22T22:31:29Z

src/index.js

+    if (sendBeaconResult) {
+      resolve();
+    } else {
+      this.loaderImage.src = `${this.getUrl()}?${query}`;


in line with the above comment on when we'd fall back to this, i'm concerned we don't have visibility into when/how often this fallback is happening. it's liable to happen frequently in areas where we track text input (putting aside how silly some of our tracking is on that front).

yeah, great call-out, was thinking something similar. we could add an extra param in the body (that doesn't need to go into the wt event), something like fallback: true, and then see frequency by querying request logs.

Motivations: 1) Change the tracking endpoint so that it doesn't contain "track" in its path, i.e. move away from "/track.gif" 2) Use the POST HTTP method for more reliability and avoiding errors from URI length limits Other Notes: - Uses whatwg-fetch to polyfill window.fetch() in older browsers. - Sends requests to /wt/t at the configured trackerDomain, otherwise the same host as the current page. - Cross-origin tracking may require CORS changes on the server

mattagra · 2021-04-23T14:34:51Z

I made the following changes, repushed to Macallan, and re-tested on Chrome, Safari, and Firefox.

Changes:

Removed adding wt_version to all events since it doesn't add much value
Changed sendBeacon to polyfilled window.fetch() for increased browser compatibility
Added ?fallback=true param to the image pixel fallback so we can measure POST errors

Going to fast-follow these changes so we can unblock marketing:

Removing pixel fallback (if we don't see many fallback=true requests after we deploy)
Use sendBeacon with polyfill? Not sure if we still want this if things work well with fetch/POST
Use prettier
(Optional) Update circleci config

can we see if this seems to not get blocked by Adblock on folk's local machines? I coincidentally was just helping Michael with a WT thing and it was being blocked by his Adblock, so we could test it on his machine (or likely anyone else's)

I got a few people on the team to QA but didn't explicitly call out using AdBlock. I did verify that the fix was not blocked by uBlock on my own machine. I'll get a few more people to test with ad blockers today.

can we ensure that we have the best view of pageloads -> pageviews possible ahead of launching this? i was working with Clayton on some of this, but I think we should be a bit more confident on those charts before shipping so we are confident we have a good way to track how well this change goes.

Yup, Clayton sent over this dashboard. Hoping that we see a noticeable jump up after the release.

mattagra · 2021-04-23T16:55:34Z

Update: Got Deergha to confirm that events are coming through, even with Adblock

bcharna · 2021-04-23T17:19:01Z

great on the QA front, also yeah that's the dashboard we were looking at. I don't think it's perfect, but should provide us at least directional insights.

Use sendBeacon with polyfill? Not sure if we still want this if things work well with fetch/POST

I still think we should do this as Beacon provides more of a guarantee that requests will go through (if browser window is closed). let's also consider removing batching in the next go.

bcharna · 2021-04-23T17:21:25Z

src/index.js

+    fetch(`${this.getRoot()}/wt/t`, {
+      method: 'POST',
+      headers: {
+        'Content-Type': 'application/x-www-form-urlencoded',


can we switch this to json? suppose can wait until the follow-up release

Yeah good question. I initially thought json wouldn't work because I was getting a CORS error, but the change in platform seems to have addressed it now. I can switch to json in the follow-up.

Edit: Actually, if we go with sendBeacon this may not be relevant

bcharna · 2021-04-23T17:28:16Z

also @mattagra, can you update the PR description now that stuff has changed?

mattagra · 2021-04-23T17:36:06Z

can you update the PR description now that stuff has changed?

Done. Also added fast-follows to the PR description.

mattagra · 2021-04-23T18:50:26Z

Interesting, we're seeing a few events fall back to the pixel image with ?fallback=true: https://app.logz.io/#/goto/8260e1c0ae2376cf9f37cea32bdb3eb2?switchToAccountId=54176.

Not seeing non-2xx on the /wt/t endpoint though so it might be a problem receiving the response from the server. Also makes me wonder if this would double record events. Seems like sendBeacon would fix this since it's not relying on the response to be returned.

mattagra force-pushed the mattagra/send-events-using-send-beacon branch 2 times, most recently from 680e022 to cac9020 Compare April 20, 2021 22:31

mattagra marked this pull request as ready for review April 20, 2021 22:35

mattagra force-pushed the mattagra/send-events-using-send-beacon branch from cac9020 to 2e99415 Compare April 21, 2021 18:14

mattagra changed the title ~~Send events using send beacon~~ Send events using sendBeacon Apr 21, 2021

mattagra force-pushed the mattagra/send-events-using-send-beacon branch from 2e99415 to 1f6df61 Compare April 21, 2021 20:48

mattagra requested a review from bcharna April 21, 2021 22:14

mattagra force-pushed the mattagra/send-events-using-send-beacon branch from 1f6df61 to dccaac1 Compare April 21, 2021 22:15

mattagra removed the request for review from bcharna April 21, 2021 22:18

mattagra force-pushed the mattagra/send-events-using-send-beacon branch 5 times, most recently from 32f81cb to d549746 Compare April 22, 2021 17:52

mattagra requested review from bcharna and a team April 22, 2021 18:03

bcharna reviewed Apr 22, 2021

View reviewed changes

nopelluhh reviewed Apr 22, 2021

View reviewed changes

Matt Agra added 5 commits April 23, 2021 07:20

Update README for new tracking changes

acaf166

Add fallback query param to identify failed POST tracking

6fb5ab6

v1.2.0

64664a8

Fix CircleCI failure on npm install

db26301

mattagra force-pushed the mattagra/send-events-using-send-beacon branch from d549746 to db26301 Compare April 23, 2021 14:20

mattagra changed the title ~~Send events using sendBeacon~~ Send events using POST Apr 23, 2021

mattagra requested review from bcharna and nopelluhh April 23, 2021 14:35

bcharna approved these changes Apr 23, 2021

View reviewed changes

mattagra merged commit 16feac3 into master Apr 23, 2021

mattagra deleted the mattagra/send-events-using-send-beacon branch April 23, 2021 17:41

mattagra mentioned this pull request Apr 24, 2021

Send cookies when making track request #38

Merged

bcharna mentioned this pull request Oct 21, 2021

Port to TS + Jest + esbuild + use more developer friendly API #46

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send events using POST #36

Send events using POST #36

mattagra commented Apr 20, 2021 •

edited

Loading

bcharna left a comment

bcharna Apr 22, 2021

nopelluhh Apr 22, 2021

bcharna Apr 22, 2021 •

edited

Loading

nopelluhh Apr 22, 2021

bcharna Apr 22, 2021

mattagra Apr 23, 2021

bcharna Apr 23, 2021

mattagra Apr 23, 2021 •

edited

Loading

bcharna Apr 23, 2021

bcharna Apr 22, 2021

nopelluhh Apr 22, 2021

bcharna Apr 22, 2021

mattagra Apr 23, 2021

bcharna commented Apr 22, 2021

nopelluhh Apr 22, 2021

nopelluhh Apr 22, 2021

nopelluhh Apr 22, 2021

mattagra Apr 23, 2021

nopelluhh Apr 22, 2021

bcharna Apr 22, 2021

mattagra commented Apr 23, 2021

mattagra commented Apr 23, 2021

bcharna commented Apr 23, 2021

bcharna Apr 23, 2021

mattagra Apr 23, 2021 •

edited

Loading

bcharna commented Apr 23, 2021

mattagra commented Apr 23, 2021

mattagra commented Apr 23, 2021 •

edited

Loading

Send events using POST #36

Send events using POST #36

Conversation

mattagra commented Apr 20, 2021 • edited Loading

Pivotal ticket(s)

Context

Changes

Test Cases

Callouts

Future

bcharna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcharna Apr 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattagra Apr 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcharna commented Apr 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattagra commented Apr 23, 2021

mattagra commented Apr 23, 2021

bcharna commented Apr 23, 2021

Choose a reason for hiding this comment

mattagra Apr 23, 2021 • edited Loading

Choose a reason for hiding this comment

bcharna commented Apr 23, 2021

mattagra commented Apr 23, 2021

mattagra commented Apr 23, 2021 • edited Loading

mattagra commented Apr 20, 2021 •

edited

Loading

bcharna Apr 22, 2021 •

edited

Loading

mattagra Apr 23, 2021 •

edited

Loading

mattagra Apr 23, 2021 •

edited

Loading

mattagra commented Apr 23, 2021 •

edited

Loading