User Sync Module #1229

grevory · 2017-05-24T16:59:30Z

Type of change

Feature

Description of change

Added a user sync module for aggregating and handling all syncs for all adapters. This first phase of user sync only uses image pixels. The next phase will include iframes and ajax which will be configurable by the publisher.

Other information

Expands on the cookie sync changes recently added for prebid server

snapwich · 2017-05-24T22:53:49Z

this is related to #1088

mkendall07 · 2017-05-24T22:57:17Z

src/userSync.js

+ * @private
+ */
+function fireSyncs() {
+  let bodyElem = document.getElementsByTagName('body')[0];


We should make sure this isn't undefined, and fall back to head as the insertion point (common in prebid.js setups).

Cool. Will implement.

mkendall07 · 2017-05-24T22:58:01Z

src/userSync.js

+  try {
+    // Fire image pixels
+    queue.image.forEach((sync) => {
+      let bidderName = sync[0];


these can all be const

mkendall07 · 2017-05-25T01:35:20Z

src/userSync.js

+  if (!url) {
+    return;
+  }
+  const img = hideAndIdElem(new Image());


this function seems a bit... odd. Is there reason to have all these smaller functions so you can compose other things in the future?

I was trying to keep everything small and functional. But yea, I do not expect this will be reused.

mkendall07 · 2017-05-25T01:35:54Z

src/userSync.js

+    return;
+  }
+  const img = hideAndIdElem(new Image());
+  img.src = encodeURI(url);


Should we make encoding the URL an option (default true)?

I was thinking we would just watch to see if adapters have their URLs encoded already in their PRs rather than checking programmatically.

mkendall07 · 2017-05-25T01:36:30Z

src/userSync.js

+  }
+  const img = hideAndIdElem(new Image());
+  img.src = encodeURI(url);
+  if (removeOnLoad) {


why would you not remove it?

There were tests that ran without passing that value but I removed them.

mkendall07 · 2017-05-25T01:36:48Z

src/userSync.js

+        thisImg.parentNode.removeChild(thisImg);
+      }
+      catch (e) {
+        utils.logWarn('Could not remove image pixel element', e);


don't think we need to warn about this... not actionable.

Ok. Thought it would be useful for debugging.

mkendall07 · 2017-05-25T01:38:48Z

src/userSync.js

+ * // registerSync(type, adapter, pixelUrl)
+ * userSync.registerSync('image', 'rubicon', 'http://example.com/pixel')
+ */
+userSync.registerSync = function(type, bidder, ...data) {


Should we make the 3rd argument just string[] for simplicity?

I made it spread for a future implementation, specially ajax. It is in the spec.

Will it not be cleaner to make this API accept an object as the sole parameter? It's easily extended, but is easier to read and easier to implement an API call for. Additionally, lines of code like line 18 and 19 will benefit in terms of readability, too.

Also - how about surfacing some constants that folks can use for the supported types?

userSync.registerSync({
type: userSync.TYPE_IMAGE,
bidder: 'foo',
url: 'http://pixerserver.com/bar/baz'
});

Strong +1 with @slimkrazy here. Attaching semantics to list elements is both error-prone (for callers) and unclear (for readers).

If this API were extended with 5-6 new values, it'd only be a matter of time before someone felt like they could improve the code with const BIDDER_NAME_INDEX = 1; constants, because those list accesses were in fact magic numbers.

Better to just cut that cycle short and start out with the map and named keys.

mkendall07

looks good! Final question, do we want to limit the syncUsers function to once per page load? (don't fire for refresh bids?)

grevory · 2017-05-26T13:58:53Z

I would presume they wouldn't need the user sync once they have it, but it is also not blocking the auction. It could block the adserver call I guess. @bretg What do you think?

bretg · 2017-05-30T23:33:07Z

do we want to limit the syncUsers function to once per page load?

agreed

grevory · 2017-05-31T03:04:39Z

So each sync will only fire once, but what about syncs that are added late for whatever reason. They should be available for future auctions.

bretg · 2017-05-31T12:02:57Z

Proposing that userSync on refresh is a possible future feature, but not 1.0. Syncs may be registered after the initial auction and still be part of the sync-drop, but after that drop, it's too late.

dbemiller · 2017-06-01T18:17:37Z

src/userSync.js

+ */
+userSync.syncUsers = function(timeout = 0) {
+  setTimeout(fireSyncs, timeout);
+};


Checking my understanding here: isn't the goal here is to prevent the user sync from interfering with the auction? If so, making the auction methods async is a much more reliable way to do it.

Imagine yourself as a publisher here. How would you choose an appropriate timeout? What if half your users are in countries with extremely slow machines? The problem is fundamentally impossible to solve reliably... and some publishers might not even have much of a tech department to discuss the tradeoffs with.

This is what callbacks (or Promises) were made for. Both reliably execute some code after a long-running task is done... and since they wouldn't touch the external API, none of our users have to worry about how to configure it properly.

Not really. Most publishers have an average page load time before which they will not want the user sync to execute. So if a publisher knows their average page load is 5s, they may pass in a timeout of 5100 to delay the execution so the user sync doesn't slow down the page load.

We have already identified a default of 0 for the timeout delay as too low and will be updating before this merges.

Thanks.

I think you hit the nail on the head. If their average page load time is 5s, then a timeout of 5100s is a reasonable choice.

The timeout is just an indirect means of accomplishing the real goal. In your example, it's "after page loads"

I'm just having a hard time imagining when it's the appropriate choice.

For simple pages, they should userSync on the load event.
For pages running scripts with a setTimeout call, they should userSync at the end of the callback.
For complex pages which lots of js which needs to be executed in small bits to avoid page jank, then they should be adding the userSync calls to whatever async-queue they're using.

There may be pages out there where a fixed timeout is actually the best implementation choice... but I feel like I must raise the question: is it really a wise decision to hardcode that choice into prebid?

what about using:

document.addEventListener("DOMContentLoaded", function(event) { //do work });

prebid is loaded async, so DOMContentLoaded wouldn't be reliable. If anything you'd want load...

...but as I mentioned, that might still interfere with the page load experience if the pub has javascript inside an unfortunately-timed setTimeout that does a bunch of processing at the same time the load event fires.

It seems like any default behavior chosen here will negatively impact some sites. Letting the pubs initiate (and prevent the default behavior) is at least safe because they can decide when best to run the sync on their site.

How about just defaulting to load event?

bretg · 2017-06-05T14:37:31Z

thanks for the good feedback @dbemiller - we'll support an option for pubs to initiate the sync.

…into improvement/user-sync

slimkrazy · 2017-06-08T10:51:50Z

src/userSync.js

+  if (!url) {
+    return;
+  }
+  let img = hideAndIdElem(new Image());


Using the hideAndIdElem function for an in-memory image is really not required, right? I'm guessing that hideAndIdElem was created for iframe support? Which leads me nicely onto my next question: Can iframe support go into this initial implementation? It would make sense if the goal is for bidders to adopt using this util over creating their own routines dealing with user syncing.

You are correct. Iframe and ajax support will be nipping on the heals of our initial release of user sync. Right now this is on hold while we implement some enhancements we discussed internally.

slimkrazy · 2017-06-08T11:15:57Z

src/userSync.js

+      // insertAdjacentHTML expects HTML string - convert DOM object to string
+      let img = userSync.createImgObject(trackingPixelUrl);
+      if (img) {
+        utils.insertElement(img);


Do you need to insert the image element into the DOM? Can't in-memory with the src set, do?

I have not used srcset for triggering tracking pixels. How does it work? Each URL in the list gets fired? I will need to look into it.

…into improvement/user-sync

mkendall07 · 2017-08-15T19:30:49Z

src/utils.js

  const img = new Image();
-  img.id = _getUniqueIdentifierStr();
  img.src = url;


Why was this code dropped? I realize that chrome requests this URL anyway even if the image is not inserted into the DOM but is this universal? Is a spec on it?

I think it was @dbemiller who made the suggestion to not add elements to the DOM which we don't need to. I believe my research suggested that simply creating the JS image object with a src was enough for all browsers.

Not I... but I do agree with whoever said it, as long as you're sure it works cross-browser :).

If you don't into the DOM, it shouldn't cause a re-paint of the page, which can have huge performance impact on pub pages.

my experience is that just creating the image with a src works on all major browsers, but don't have an authoritative source.

I think it's fine being OK for this use case, I'm thinking about when this is used to count an impression we need to make sure it works. I'm ok as long as this was tested in all the browsers we care about.

mkendall07 · 2017-08-15T19:57:39Z

src/userSync.js

+   * @param {boolean} enableOverride Tells this module to expose the syncAll method to the public
+   * @public
+   */
+  publicApi.overrideSync = (enableOverride) => {


Ideally the only file that mutates the pbjs global would be prebid.js. I discussed this with @dbemiller and he suggested just putting this code into src/prebid.js - example:

import { userSync } from 'src/userSync'; if ($$PREBID_GLOBAL$$.userSync.enableOverride) { $$PREBID_GLOBAL$$.userSync.syncAll = userSync.syncUsers; }

mkendall07 · 2017-08-15T19:59:45Z

src/userSync.js

@@ -0,0 +1,175 @@
+import * as utils from 'src/utils';
+
+export function newUserSync() {


edit: nevermind

This follows the pattern in config (https://github.com/prebid/Prebid.js/blob/master/src/config.js#L28). It allows for resetting the state of this module in between tests.

mkendall07 · 2017-08-15T20:00:56Z

src/userSync.js

+  let queue = getDefaultQueue();
+
+  // Since user syncs require cookie access we want to prevent sending syncs if cookies are not supported
+  let cookiesAreSupported = !utils.isSafariBrowser() && utils.cookiesAreEnabled();


do we want to block this or leave it to the caller? Talking about safari specifically.

This was a decision made by the group. @bretg what is your opinion here?

mkendall07 · 2017-08-15T20:06:23Z

src/userSync.js

+
+  // Merge the defaults with the user-defined config
+  let userSyncConfig = Object.assign($$PREBID_GLOBAL$$.userSync || {},
+    userSyncDefaultConfig);


Should we pass in the userSync object or have it set in the constructor? This is a basically undocumented dependency.

That's fair. I thinking passing it in is fine. I think it would make testing easier too.

mkendall07 · 2017-08-15T20:08:51Z

src/userSync.js

+   * userSync.registerSync('image', 'rubicon', 'http://example.com/pixel')
+   */
+  publicApi.registerSync = (type, bidder, ...data) => {
+    if (!userSyncConfig.syncEnabled || !utils.isArray(queue[type])) {


this should protect again undefined bidder being sent.

…oject/Prebid.js into improvement/user-sync

harpere · 2017-08-16T01:05:35Z

test/spec/userSync_spec.js

+
+  it('should register and fire a pixel URL', () => {
+    userSync.registerSync('image', 'testBidder', 'http://example.com');
+    userSync.syncUsers();


are there any tests that verify that syncUsers() is called when an auction finishes, rather than calling it directly?

Sounds like an integration test.

dbemiller · 2017-08-21T15:57:57Z

src/userSync.js

+   * // registerSync(type, adapter, pixelUrl)
+   * userSync.registerSync('image', 'rubicon', 'http://example.com/pixel')
+   */
+  publicApi.registerSync = (type, bidder, ...data) => {


Would strongly prefer a @param {SyncData} data, with a @typedef {object} SyncData (and associated @param tags) here.

As someone who wants to call this method, this is not self-documenting. Rules like "the first arg of this ...data is the URL" are invisible, and the only way to figure them out is to read the code.

Since this method is adapter-facing, I think it's very important to make this as clear as possible.

Since we are no longer support AJAX we can make this fix without causing a mess. I will do it now. AJAX support made it complicated.

We are accepting the proposed change as is.

mkendall07 · 2017-08-31T01:40:54Z

modules/rubiconBidAdapter.js

+ *    pixelEnabled: true,
+ *    syncsPerBidder: 5,
+ *    syncDelay: 3000,
+ *    iframeEnabled: true


So by turning on the Rubicon adapter, you automatically enable iframe user sync?

mkendall07 · 2017-08-31T01:48:07Z

src/userSync.js

+   * @summary A `syncUsers` wrapper for determining if enableOverride has been turned on
+   * @public
+   */
+  publicApi.syncUsersOverride = () => {


prefer this name to match what's in prebid.js so triggerUserSyncs for consistency.

mkendall07 · 2017-08-31T01:49:02Z

src/cookie.js

-  queue.length = 0;
-}
-
-function setBidderSynced(bidder) {


I'd like to preserve this functionality if we can. How about a callback on the pixel queue?

mkendall07

Need to move changes out of cookie.js into prebid adapter or into cookieSync

grevory added 9 commits May 15, 2017 15:22

Add new user sync module

89515a0

Rename user sync file

3adaee4

Reset image pixel queue after firing pixels

e233465

Resolved conflicts from master

9d20a9a

Added unit test for user sync

b7d1455

Merge conflict from master

efa23e2

Conflict with master

9bef360

Fixed typo

ca8cb54

Overlooked conflict from master

b28f15b

mkendall07 added the in progress label May 24, 2017

grevory changed the title ~~Improvement/user sync~~ User Sync Module May 24, 2017

mkendall07 requested changes May 25, 2017

View reviewed changes

Improvements based on peer feedback for user sync

7840f88

bretg mentioned this pull request May 26, 2017

Add drop pixels once feature to AOL Adapter #1224

Merged

1 task

mkendall07 previously approved these changes May 26, 2017

View reviewed changes

bretg mentioned this pull request May 30, 2017

User sync updates prebid/prebid.github.io#260

Merged

dbemiller reviewed Jun 1, 2017

View reviewed changes

grevory added the on hold label Jun 5, 2017

Merge branch 'master' of https://github.com/rubicon-project/Prebid.js …

7450ee5

…into improvement/user-sync

slimkrazy reviewed Jun 8, 2017

View reviewed changes

protonate self-assigned this Jun 20, 2017

grevory added 2 commits June 23, 2017 11:52

Added global user sync config

f981c20

Allow auto-syncing override

66abfef

grevory added 3 commits August 6, 2017 15:19

Remove unused functions for user sync

82d1ce6

Merge branch 'master' of https://github.com/rubicon-project/Prebid.js …

9f6d3fd

…into improvement/user-sync

Clean merge markers

7c71de1

ndhimehta assigned mkendall07 Aug 7, 2017

mkendall07 previously requested changes Aug 15, 2017

View reviewed changes

Eric Harper added 3 commits August 15, 2017 17:08

Merge branch 'improvement/user-sync' of https://github.com/rubicon-pr…

d0185d6

…oject/Prebid.js into improvement/user-sync

Merge branch 'master' into improvement/user-sync

88f8197

fix usersync test problems

bd9acb4

harpere reviewed Aug 16, 2017

View reviewed changes

dbemiller mentioned this pull request Aug 17, 2017

BaseAdapter for the Prebid 0.x -> 1.x transition #1494

Merged

mkendall07 added the needs update label Aug 17, 2017

Eric Harper added 3 commits August 17, 2017 16:56

added setConfig to userSync

6766102

Merge branch 'master' into improvement/user-sync

0e67a72

fixed lint problem

2213fa5

dbemiller reviewed Aug 21, 2017

View reviewed changes

dbemiller mentioned this pull request Aug 21, 2017

Intent to implement: Prebid Architecture #1510

Closed

grevory added 2 commits August 29, 2017 17:36

Change override user sync API and how user sync uses config

e79760b

Fixed registerSync API to avoid spread operator

f00a8b5

grevory removed the needs update label Aug 29, 2017

Merged from upstream

f49a1e4

Fixed issues from last conflict resolution

9a197ab

mkendall07 reviewed Aug 31, 2017

View reviewed changes

mkendall07 requested changes Aug 31, 2017

View reviewed changes

mkendall07 mentioned this pull request Aug 31, 2017

Rubicon project improvement/user sync #1549

Merged

mkendall07 closed this Aug 31, 2017

jaiminpanchal27 mentioned this pull request Oct 4, 2017

Unit tests failing in IE 10 #1656

Closed

robertrmartinez deleted the improvement/user-sync branch July 5, 2023 19:45

		@@ -0,0 +1,175 @@
		import * as utils from 'src/utils';

		export function newUserSync() {

User Sync Module #1229

User Sync Module #1229

Conversation

grevory commented May 24, 2017

Type of change

Description of change

Other information

snapwich commented May 24, 2017

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

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

Choose a reason for hiding this comment

slimkrazy Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

dbemiller Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

mkendall07 left a comment

Choose a reason for hiding this comment

grevory commented May 26, 2017

bretg commented May 30, 2017

grevory commented May 31, 2017

bretg commented May 31, 2017

dbemiller Jun 1, 2017 • 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

bretg commented Jun 5, 2017

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

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

mkendall07 Aug 15, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbemiller Aug 21, 2017 • 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

mkendall07 left a comment

Choose a reason for hiding this comment

slimkrazy Jun 8, 2017 •

edited

Loading

dbemiller Jun 8, 2017 •

edited

Loading

dbemiller Jun 1, 2017 •

edited

Loading

mkendall07 Aug 15, 2017 •

edited

Loading

dbemiller Aug 21, 2017 •

edited

Loading